Skip to content

Conversation

@speaker-ender
Copy link
Contributor

@speaker-ender speaker-ender commented Nov 24, 2025

Ticket []

Description Of Changes

Fixes a bugs when dealing with async data in the list and drawer view and starts to establish a pattern for normalizing data as nodes.

Code Changes

  • Creates a generic hook for normalizing data as "nodes"
  • Implements normalized field nodes into the list and details views

Steps to Confirm

Classifier steps

  1. Add datastore_monitor_action_center_enabled = true to the [detection_discovery] section of the .fides/fides.toml file in fidesplus
  2. Enable the Helios v2 feature flag by going to /settings/about and toggling the option
  3. Add a new datastore monitor (can follow the steps in src/fidesplus/api/service/discovery/configurable-test-datastore-monitor.md)
  4. Visit the Action Center Page
  5. Click on a monitor to navigate to a datastore monitor (eg http://localhost:3000/data-discovery/action-center/datastore/Default_Test_Datastore_Monitor)

Normalized data steps

  1. Confirm that general functionality within the monitor screen has not regressed
  • Paginating
  • Individual Field Actions
  • Bulk Actions
  • Opening Details
  • Filtering
  1. Perform an action on multiple fields using a checkbox + dropdown
  2. Confirm the fields are no longer selected
  3. Select the same fields used before
  4. Confirm that the available actions have correctly updated

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 24, 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 Dec 3, 2025 8:18pm
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
fides-privacy-center Ignored Ignored Dec 3, 2025 8:18pm

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 1, 2025

Greptile Overview

Greptile Summary

This PR introduces a data normalization pattern for managing async data in the Action Center list and drawer views. The changes create a generic useNodeMap hook for normalizing data as nodes with Map-based state management, and implement this pattern through the new useNormalizedResources hook that combines list and details queries.

Key changes:

  • Added generic useNodeMap hook in clients/admin-ui/src/features/common/hooks/ for normalized state management with partial updates and merging
  • Created useNormalizedResources hook to combine list/details queries with the node map
  • Refactored useBulkListSelect to work with keys instead of full item objects, simplifying state management
  • Updated MonitorResource type union to include paginated API response items
  • Added defensive property access in MonitorFieldListItem and ResourceDetailsDrawer components

Issues found:

  • Test filename typo: useNoodeMap.test.ts should be useNodeMap.test.ts
  • Potential issue with encodeURIComponent(stagedResourceUrn ?? false) which could encode "false" as a string if skip logic is bypassed

Confidence Score: 4/5

  • This PR is generally safe to merge with minor issues that should be addressed
  • The code changes are well-structured with good test coverage for the new hooks. The normalization pattern is sound and the refactoring simplifies state management. However, there are two issues: a filename typo and a potentially problematic fallback value in the API slice that could cause unexpected behavior in edge cases.
  • Pay attention to clients/admin-ui/src/features/data-discovery-and-detection/action-center/action-center.slice.ts for the ?? false fallback, and rename useNoodeMap.test.ts to fix the typo

Important Files Changed

File Analysis

Filename Score Overview
clients/admin-ui/src/features/common/hooks/useNodeMap.ts 5/5 New generic hook for managing normalized data as nodes with Map-based state, supporting partial updates and merging
clients/admin-ui/src/features/common/hooks/useNoodeMap.test.ts 4/5 Test file for useNodeMap hook with good coverage, though filename contains typo ("Noode" instead of "Node")
clients/admin-ui/src/features/data-discovery-and-detection/action-center/action-center.slice.ts 3/5 Changed getStagedResourceDetails to accept optional stagedResourceUrn, but uses problematic fallback to false
clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/page.tsx 5/5 Major refactor to use useNormalizedResources hook, removes useCallback wrapping and simplifies state management with key-based selection
clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/useBulkListSelect.ts 5/5 Refactored to use Key-based selection instead of full item objects, simplifying state management
clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/useNormalizedResources.ts 5/5 New hook that combines list and details queries with normalized node state, properly using skip option for details query

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.

12 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@gilluminate gilluminate left a comment

Choose a reason for hiding this comment

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

approved with a couple of questions. nothing blocking. Looks good.

stagedResourceUrn,
)}`,
url: `/plus/discovery-monitor/staged_resource/${
stagedResourceUrn ? encodeURIComponent(stagedResourceUrn) : ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this works? I tried it in the Swagger docs and it threw an error because the staged_resource_urn is required. Would it be better to use a skip or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an unfortunate type issue with RTK.
I did use skip, but to use the hook i would still be required to pass in a value (essentially passing in an empty string if i don't have a value).
You can't conditionally choose to use a hook unless you decide to add conditional components that contain the hook (not exactly ergonomic).
I could type cast and pretend it's always there but that's prone to bugs.
This seemed like the most honest workaround.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, been there. Makes sense. Maybe just a comment about it?, but no big deal.

fix: bulk select normalization

wip: nearly there

chore: fixing naming update

test: adding normalization test

refactor: lots of small refinements

wip: some linting and adding new hook

refactors

chore: linting

refactor: names and interfaces

chore: linting

test: bulk list select

chore: linting

fix: rendering issues

fix: tests

chore: update changelog

fix: rename

refactor: url builder

chore: update apis

refactor: api update changes

fix: new field mapping

refactor: removing old code
@speaker-ender speaker-ender force-pushed the fix/bulk-select-normalization--squashed branch from 3449fe5 to 6f3a584 Compare December 3, 2025 20:14
@speaker-ender speaker-ender added this pull request to the merge queue Dec 3, 2025
Merged via the queue into main with commit 7691256 Dec 3, 2025
46 of 47 checks passed
@speaker-ender speaker-ender deleted the fix/bulk-select-normalization--squashed branch December 3, 2025 21:19
jjdaurora pushed a commit that referenced this pull request Dec 5, 2025
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