fix: normalization merging [ENG-2171] #7078

speaker-ender · 2025-12-05T04:34:30Z

Ticket ENG-2171

Description Of Changes

Partial rollback of data normalization to fix data updates.

Code Changes

Uses non-normalized details data
Temporary fix for mutable map objects

Steps to Confirm

Visit a monitor in the action center.
Open the details drawer for a field
Edit the data categories
Confirm that data categories are updated in the details view and the list view
Verify there are no regressions with existing functionality of the screen

Pre-Merge Checklist

vercel · 2025-12-05T04:34:35Z

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

2 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
fides-plus-nightly	Ignored	Preview		Dec 5, 2025 4:51pm
fides-privacy-center	Ignored			Dec 5, 2025 4:51pm

greptile-apps · 2025-12-05T15:17:39Z

Greptile Overview

Greptile Summary

This PR partially rolls back the data normalization feature to fix data update issues in the Action Center fields view. The changes address a mutable Map object mutation bug by adding cloneDeep to the merge logic, but intentionally bypass the merge result when setting state as a temporary workaround.

Key changes:

Fixed mutation bug in mergeNodes by adding _.cloneDeep() before merge
Removed initialData parameter from useNodeMap hook
Disabled details query normalization in useNormalizedResources - details now return raw API data instead of normalized node
Switched from listQuery.data to listQuery.currentData (RTK Query property that only updates when the current query succeeds)

Trade-offs:

The partialUpdates parameter now only affects change detection, not the actual state merge - this is acknowledged in a code comment as a temporary fix
Test expectations were updated to reflect this changed behavior (nodes now overwrite rather than merge)

Confidence Score: 4/5

This PR is safe to merge as a temporary fix, with the behavior changes being intentional and documented.
The changes are well-tested, the mutation bug fix is correct, and the temporary workaround is clearly documented in comments. The behavior change to normalization is intentional and test expectations were updated accordingly. However, a follow-up issue should track restoring full merge functionality.
Pay attention to useNodeMap.ts - the partialUpdates parameter behavior has changed and may need follow-up to restore full merge functionality.

Important Files Changed

File Analysis

Filename	Score	Overview
clients/admin-ui/src/features/common/hooks/useNodeMap.ts	3/5	Fixes mutation bug with cloneDeep but introduces inconsistency: `draft` is computed but `next` is used for state update when partialUpdates=true. The comment explains this is intentional as a temporary fix.
clients/admin-ui/src/features/common/hooks/useNodeMap.test.ts	4/5	Test expectations updated to reflect changed behavior. Removed duplicate test. Added new tests for `mergeNodes` including a test verifying no data mutation.
clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/useNormalizedResources.ts	3/5	Switched from `data` to `currentData` (RTK Query), disabled details query normalization, and returns raw `data` instead of normalized `node` for details query.
clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/page.tsx	5/5	Updated destructuring to match new return type: `data` instead of `node` for details query resource.

greptile-apps

_{4 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

...-ui/src/features/data-discovery-and-detection/action-center/fields/useNormalizedResources.ts

clients/admin-ui/src/features/common/hooks/useNodeMap.ts

speaker-ender · 2025-12-05T15:17:48Z

clients/admin-ui/src/features/common/hooks/useNodeMap.ts

-      _.chain(prev.get(key)).defaultTo(node).merge(node).value(),
+      _.chain(prev.get(key))
+        .defaultTo(node)
+        .cloneDeep() /** This is needed since we aren't using immutable Maps (yet), and would otherwise mutate referenced objects */


Maps use mutable refs.
This can cause lots of issues and is why I typically avoid them but here we are.
Cloning the referenced data before merging leaves the initial value as it should be.

speaker-ender · 2025-12-05T15:20:05Z

clients/admin-ui/src/features/common/hooks/useNodeMap.ts

+      _.chain(prev.get(key))
+        .defaultTo(node)
+        .cloneDeep() /** This is needed since we aren't using immutable Maps (yet), and would otherwise mutate referenced objects */
+        .merge(node)


Lodash merge does not do exactly what we want and would need to be replaced with a mergeWith fn in the future.
This works fine for the current implementation.

...-ui/src/features/data-discovery-and-detection/action-center/fields/useNormalizedResources.ts

fix: normalization merging fix: tests and expected behavior revert: changing from data to currentData

speaker-ender · 2025-12-05T15:25:04Z

clients/admin-ui/src/features/common/hooks/useNodeMap.test.ts

+    ]);
+  });
+
+  it("should not mutate data references", () => {


This only tests that we are not mutating the refs internally.
Ideally, we should have immutable values when dealing with maps externally but this will most likely require a library.

speaker-ender · 2025-12-05T15:26:25Z

clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/page.tsx

  const {
    listQuery: { nodes: listNodes, ...listQueryMeta },
-    detailsQuery: { node: resource },
+    detailsQuery: { data: resource },


Relying on the query directly since we aren't updating the node values with the details query.

speaker-ender · 2025-12-05T15:27:32Z

clients/admin-ui/src/features/common/hooks/useNodeMap.ts

    .value();

-const useNodeMap = <Data>(
-  initialData?: NodeMap<Data>,


Removing initial data since it's not necessary at the moment + introduces another vector for mutability issues

...-ui/src/features/data-discovery-and-detection/action-center/fields/useNormalizedResources.ts

…ion-center/fields/useNormalizedResources.ts Co-authored-by: Jason Gill <jason.gill@ethyca.com>

Co-authored-by: Jason Gill <jason.gill@ethyca.com>

speaker-ender force-pushed the fix/normalization-merging branch from 96c8406 to 648ea66 Compare December 5, 2025 04:36

vercel bot deployed to Preview – fides-plus-nightly December 5, 2025 04:40 View deployment

speaker-ender force-pushed the fix/normalization-merging branch from 648ea66 to d243498 Compare December 5, 2025 15:12

speaker-ender marked this pull request as ready for review December 5, 2025 15:14

speaker-ender requested a review from a team as a code owner December 5, 2025 15:14

speaker-ender requested review from jpople and removed request for a team December 5, 2025 15:14

vercel bot deployed to Preview – fides-plus-nightly December 5, 2025 15:16 View deployment

greptile-apps bot reviewed Dec 5, 2025

View reviewed changes

...-ui/src/features/data-discovery-and-detection/action-center/fields/useNormalizedResources.ts Outdated Show resolved Hide resolved

clients/admin-ui/src/features/common/hooks/useNodeMap.ts Show resolved Hide resolved

speaker-ender commented Dec 5, 2025

View reviewed changes

...-ui/src/features/data-discovery-and-detection/action-center/fields/useNormalizedResources.ts Outdated Show resolved Hide resolved

fix: normalization merging

15164b1

fix: normalization merging fix: tests and expected behavior revert: changing from data to currentData

speaker-ender force-pushed the fix/normalization-merging branch from d243498 to 15164b1 Compare December 5, 2025 15:23

speaker-ender commented Dec 5, 2025

View reviewed changes

vercel bot deployed to Preview – fides-plus-nightly December 5, 2025 15:27 View deployment

speaker-ender commented Dec 5, 2025

View reviewed changes

speaker-ender changed the title ~~fix: normalization merging~~ fix: normalization merging [ENG-2171] Dec 5, 2025

speaker-ender requested a review from gilluminate December 5, 2025 15:51

gilluminate approved these changes Dec 5, 2025

View reviewed changes

...-ui/src/features/data-discovery-and-detection/action-center/fields/useNormalizedResources.ts Outdated Show resolved Hide resolved

Update clients/admin-ui/src/features/data-discovery-and-detection/act…

174ca78

…ion-center/fields/useNormalizedResources.ts Co-authored-by: Jason Gill <jason.gill@ethyca.com>

speaker-ender enabled auto-merge December 5, 2025 16:42

vercel bot deployed to Preview – fides-plus-nightly December 5, 2025 16:45 View deployment

speaker-ender disabled auto-merge December 5, 2025 16:49

chore: update changelog

dd9f386

speaker-ender enabled auto-merge December 5, 2025 16:51

speaker-ender added this pull request to the merge queue Dec 5, 2025

Merged via the queue into main with commit f7ad962 Dec 5, 2025
47 checks passed

speaker-ender deleted the fix/normalization-merging branch December 5, 2025 17:15

speaker-ender added a commit that referenced this pull request Dec 5, 2025

fix: normalization merging [ENG-2171] (#7078)

5d4b1e4

Co-authored-by: Jason Gill <jason.gill@ethyca.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: normalization merging [ENG-2171] #7078

fix: normalization merging [ENG-2171] #7078

speaker-ender commented Dec 5, 2025 •

edited

Loading

Uh oh!

vercel bot commented Dec 5, 2025 •

edited

Loading

Uh oh!

greptile-apps bot commented Dec 5, 2025

Uh oh!

greptile-apps bot left a comment

Uh oh!

Uh oh!

Uh oh!

speaker-ender Dec 5, 2025

Uh oh!

speaker-ender Dec 5, 2025

Uh oh!

Uh oh!

speaker-ender Dec 5, 2025

Uh oh!

speaker-ender Dec 5, 2025

Uh oh!

speaker-ender Dec 5, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix: normalization merging [ENG-2171] #7078

fix: normalization merging [ENG-2171] #7078

Conversation

speaker-ender commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

Uh oh!

vercel bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps bot commented Dec 5, 2025

Greptile Overview

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

speaker-ender Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

speaker-ender Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

speaker-ender Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

speaker-ender Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

speaker-ender Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

speaker-ender commented Dec 5, 2025 •

edited

Loading

vercel bot commented Dec 5, 2025 •

edited

Loading