Skip to content

Conversation

@gilluminate
Copy link
Contributor

@gilluminate gilluminate commented Nov 5, 2025

Ticket ENG-1669

Description Of Changes

Refactored pluralization logic across the Admin UI to use a centralized pluralize utility function instead of inline ternary operators. This improves consistency, readability, and maintainability.

Code Changes

  • Updated MonitorResult to use pluralize() for asset/field/system counts
  • Updated MonitorResultDescription to handle plural forms for all monitor update types
  • Updated InProgressMonitorTaskItem to use pluralize() for field counts
  • Updated MONITOR_UPDATE_NAMES constant to store [singular, plural] tuples
  • Updated DatahubDataSyncTab to use pluralize() for dataset counts
  • Updated useMonitorConfigTable to use pluralize() for project counts
  • Updated PrivacyRequestDetailsManualTaskTab to use pluralize() for task counts

Steps to Confirm

  1. Verify monitor results display correct singular/plural text for counts of 1 and multiple items
  2. Check monitor result descriptions properly pluralize all update types
  3. Confirm in-progress monitor tasks show correct pluralization
  4. Test DataHub sync success messages with 1 and multiple datasets
  5. Verify privacy request manual task tab pluralizes task counts correctly

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@vercel
Copy link

vercel bot commented Nov 5, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
fides-plus-nightly Ready Ready Preview Comment Nov 6, 2025 0:41am
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
fides-privacy-center Ignored Ignored Nov 6, 2025 0:41am

@gilluminate gilluminate marked this pull request as ready for review November 5, 2025 22:57
@gilluminate gilluminate requested a review from a team as a code owner November 5, 2025 22:57
@gilluminate gilluminate requested review from speaker-ender and removed request for a team November 5, 2025 22:57
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Greptile Summary

This PR successfully centralizes pluralization logic across the Admin UI by introducing a reusable pluralize utility function. The refactoring replaces numerous inline ternary operators with consistent function calls, improving code maintainability and readability.

Key changes:

  • Added pluralize(count, singular, plural) utility function in utils.ts
  • Updated MONITOR_UPDATE_NAMES to store [singular, plural] tuples, properly handling non-countable terms (e.g., "In review") by using identical forms
  • Refactored 7 components/hooks to use the centralized utility instead of inline ternary logic
  • Simplified message construction in DataHub sync by removing complex "is/are" logic

Impact:

  • Improves code consistency across the codebase
  • Makes pluralization logic more testable and maintainable
  • No functional changes to user-facing text (behavior preserved)
  • Minor cleanup opportunity: isAssetList prop in MonitorResultDescription is now unused

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The refactoring is straightforward and well-executed. All pluralization logic has been properly preserved through the tuple approach, with non-countable terms correctly using identical singular/plural forms. The changes are purely structural improvements with no functional impact on user-facing behavior. The only minor issue is an unused prop that can be cleaned up.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
clients/admin-ui/src/features/common/utils.ts 5/5 Added simple pluralize utility function that returns singular or plural form based on count
clients/admin-ui/src/features/data-discovery-and-detection/action-center/constants.ts 5/5 Updated MONITOR_UPDATE_NAMES to store [singular, plural] tuples instead of single strings; properly handles non-pluralizable terms by using identical forms
clients/admin-ui/src/features/data-discovery-and-detection/action-center/MonitorResultDescription.tsx 4/5 Removed isAssetList-dependent pluralization logic in favor of centralized pluralize() utility; isAssetList prop now unused in logic but still accepted

Additional Comments (1)

  1. clients/admin-ui/src/features/data-discovery-and-detection/action-center/MonitorResult.tsx, line 114-117 (link)

    style: isAssetList prop is no longer used by MonitorResultDescription and can be removed

8 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@gilluminate gilluminate added this pull request to the merge queue Nov 7, 2025
Merged via the queue into main with commit 47eb013 Nov 7, 2025
47 checks passed
@gilluminate gilluminate deleted the gill/ENG-1669/better-pluralize-handling branch November 7, 2025 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants