Improve the editing experience with policy agent#8164
Conversation
…te-and-transition-the-editing-experience-with-agent-policy-builder
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
|
/code-review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #8164 — Animated transition & "Policy agent is working" indicator
This is a well-structured, well-tested implementation. The three-phase state machine (ghost-hold → settling → cleanup), the YAML diff tagging helpers, and the new PolicyAgentWorking component all follow clear patterns. Test coverage for the new policy-yaml.ts helpers is thorough.
Summary of findings
Medium
proOptions={{ hideAttribution: true }}(AccessPolicyEditor.tsx:852) — This is a React Flow Pro-only prop. Using it without a valid Pro license is a ToS violation. Confirm the license or remove it before merge.
Minor
- Hardcoded
#faad14(AccessPolicyEditor.module.scss:80) — ThediffStatus-modifiedrule uses a raw hex instead of a design token, unlike theadded/removedpeers. Should reference--fidesui-color-warningor equivalent. - Accessibility gap in
PolicyAgentWorking(PolicyAgentWorking.tsx:10) — The component has norole="status"/aria-live, so screen readers won't be notified when the indicator appears or when it swaps from the green check. Addingrole="status"to the wrapper is a one-liner fix. - Ghost constraint edge topology (policy-yaml.ts:641) — Constraint ghost edges always attach to
newFirstCondition, which is structurally wrong in multi-condition policies. This is acceptable for a transient visual, but deserves a comment explaining the known simplification. - Empty discriminator in
nodeContentId(policy-yaml.ts:503) — Unconfigured constraint nodes produceconstraint:consent:(empty trailing segment), which causes id collisions between multiple unconfigured constraints. Consider returningnullwhen the discriminator field is empty.
What looks good
- Timer cleanup on unmount and on mode switch is handled correctly.
- React 18 automatic batching means the two state updates inside each timeout (
setPendingTransition+setSyncKey) render atomically, so thesyncKeyeffect always sees the intendedpendingTransitionphase — no race. DiffViewportControllercorrectly guards against re-firingfitViewfor the same{phase, epoch}vialastSeenRef, and defers until all nodes are measured.- The YAML serialization guard (
if (!onYamlChange || pendingTransition) return) correctly prevents ghost nodes from being written back intoyamlValueduring the hold phase. - Unit tests for
nodeContentId,tagNodesWithDiff, andbuildUnionGraphcover the main paths including ghost node propagation to incident edges.
🔬 Codegraph: connected (49858 nodes)
💡 Write /code-review in a comment to re-run this review.
| id: `e-ghost-${newActionNode.id}-${ghostId}`, | ||
| source: newActionNode.id, | ||
| target: ghostId, | ||
| type: "labeledEdge", |
There was a problem hiding this comment.
clients/admin-ui/src/features/access-policies/policy-yaml.ts:641
Ghost constraint edges always wire to newFirstCondition (the first condition node found in the new graph), regardless of which condition actually owned the constraint in the old graph. In a policy that has multiple condition nodes this produces structurally incorrect ghost edges.
For the brief ghost-hold phase this is probably acceptable (the user just needs a visual hint that something is disappearing), but it's worth a comment here explaining the intentional simplification and its limitation:
// Simplified: always attach the ghost constraint to the first condition in
// the new graph. Multi-condition topologies may show structurally incorrect
// edges during the ghost-hold phase, but this is a transient visual only.| const { property } = node.data as ConditionNodeData; | ||
| return property ? `condition:${property}` : null; | ||
| } | ||
| if (node.type === "constraintNode") { |
There was a problem hiding this comment.
clients/admin-ui/src/features/access-policies/policy-yaml.ts:503
When privacyNoticeKey (or geoField / dataFlowDirection) is undefined, the discriminator becomes an empty string, producing ids like constraint:consent:. If a user has dragged in two unconfigured consent constraints, both will map to the same content id and both will receive the same diff highlight — even if only one was actually changed. This is an edge case, but worth noting in the JSDoc or guarding with a null return:
if (data.constraintType === ConstraintType.CONSENT) {
return data.privacyNoticeKey
? `constraint:consent:${data.privacyNoticeKey}`
: null; // not identifiable until configured
}
lucanovera
requested review from
gilluminate and
kruulik
and removed request for
a team and
gilluminate
…te-and-transition-the-editing-experience-with-agent-policy-builder
kruulik
left a comment
kruulik
left a comment
There was a problem hiding this comment.
Looks good - code comments are nits and can be addressed later.
…te-and-transition-the-editing-experience-with-agent-policy-builder
lucanovera
deleted the
ENG-3573-FE-Animate-and-transition-the-editing-experience-with-agent-policy-builder
branch
2 participants
Ticket ENG-3573
Description Of Changes
Animates the access-policy editing experience when the policy agent proposes a YAML diff. The React Flow canvas now plays a three-phase transition that shows the previous and proposed states side-by-side, fades the old state out, and pulse-highlights added / modified / removed nodes. A new "Policy agent is working" indicator surfaces this state in two places so the user can tell something is happening even when the relevant nodes are off-screen.
Tested alongside fidesplus#3558 — that PR backs the policy-update payload (
added/changed/removedIDs) consumed by this transition. Reviewers should check out both branches together.Code Changes
ghost-hold→settling→ cleanup) in AccessPolicyEditor.tsx that drives the diff transition when the agent returns aPolicyUpdate.tagNodesWithDiff/buildUnionGraphhelpers in policy-yaml.ts to tag React Flow nodes withdiffStatus-added/-modified/-removedclasses and merge old + new graphs during ghost-hold.DiffViewportController(insideAccessPolicyEditor.tsx) to fit the viewport on the affected sub-graph at each phase boundary.DIFF_HOLD_MS = 3000,DIFF_HIGHLIGHT_MS = 6500,DIFF_FIT_DURATION_MS = 800in constants.ts.defaultandsmallsize variants.<Panel position="top-center">while a transition is in flight.<PolicyAgentWorking size="small" />during the transition; it flips back to the green check + "The policy was updated" when the transition ends. Older applied messages always show the check.agent-chat.slicePolicyUpdatetype withadded/changed/removedID arrays and updated MSW handlers in agent-chat-handlers.ts.Steps to Confirm
PolicyUpdate.detection_discovery.llm_classifier_enabled).clients/admin-uirunnpm run devand log inpolicy_updatein the response) — no indicator should appear and no footer should be added to the chat bubble.npm run typecheckandnpm run test -- access-policiesinclients/admin-ui— both should pass.Pre-Merge Checklist
CHANGELOG.mdupdated