New policies builder

[lucanovera]

ENG-2963
ENG-2957

Summary

Implements the access policy create/edit pages with a visual node-based builder (React Flow) and a YAML code editor, following the Policy Engine v2 PRD schema.

New pages:

• /access-policies/new — create a new policy from scratch
• /access-policies/edit/[id] — edit an existing policy

Three editor modes:

• Builder (default) — visual directed graph with Policy → Decision → Match → Constraint nodes
• Code — full Monaco YAML editor for direct authoring
• Split — builder + read-only YAML preview side-by-side (dev only)

Visual builder nodes:

• Policy node — name, description, controls (multi-select), enabled toggle, advanced section (fides_key, priority)
• Decision node — ALLOW/DENY toggle with optional denial message
• Match node — taxonomy dimension selector with any/all operator and taxonomy-specific value pickers
• Constraint node — consent (privacy notice + requirement), geo location (field + operator + ISO locations), or data flow (direction + operator + systems)

Key behaviors:

• Bidirectional sync: node edits re-derive YAML; switching from Code→Builder re-parses YAML into nodes
• Add/delete nodes with chain surgery to maintain graph integrity
• Auto-layout via dagre with labeled edges (when, and, unless)
• RTK Query integration for policy CRUD and control group fetching
• MSW mock handlers for local development
• Unit tests for YAML↔node round-tripping

Test plan

• Enable PBAC alpha flag
• Create a new policy using the visual builder — verify all node types render and YAML output matches
• Delete match/constraint nodes — verify chain reconnects properly
• Edit an existing policy — verify pre-populated state
• Export a policy — verify .yaml file downloads
• Run npm run test -- policy-yaml — all unit tests pass

Tickets

• ENG-2957 — FE: Policy add/edit view with yaml editor
• ENG-2963 — FE: Policy visual editor/view

[greptile-apps]

Node ID collision after delete-then-add

When a condition node is deleted and then a new one is added, the new ID is derived from the current count of condition nodes. This can collide with an existing node's ID.

Example: Add condition-1, condition-2, condition-3 → delete condition-1 (remaining: condition-2, condition-3) → add new condition: conditionCount = 2, conditionId = "condition-3" → duplicate ID with the existing condition-3.

This causes React key conflicts and silently drops one condition from the serialized YAML (the BFS walk in collectConditionNodes stops on first visited match).

The same issue exists in handleAddConstraint (line 551–552).

Use a monotonically increasing counter (e.g., useRef) instead of deriving the ID from the current count:

// At the top of PolicyCanvasPanel const conditionCounterRef = useRef(0); // In handleAddCondition: conditionCounterRef.current += 1; const conditionId = `condition-${conditionCounterRef.current}`;

Apply the same pattern for constraintCounterRef in handleAddConstraint.

[greptile-apps]

Same node ID collision issue for constraint nodes

constraintId is derived from the current constraint count. After deleting a constraint and adding a new one, this will produce a duplicate ID (e.g., delete constraint-1, add → constraint-2 collides with the existing constraint-2).

See the comment on handleAddCondition (line 472) for the fix pattern — use a useRef counter.

[greptile-apps]

Missing DOM append may break download in Firefox

a.click() works without appending the element to the DOM in Chrome and most browsers, but older Firefox versions require the element to be part of the document for the programmatic click to trigger a download. The standard cross-browser pattern is:

const handleExport = () => { const blob = new Blob([yamlValue], { type: "text/yaml" }); const url = URL.createObjectURL(blob); const a = document.createElement("a"); a.href = url; a.download = `${displayName || "policy"}.yaml`; document.body.appendChild(a); a.click(); document.body.removeChild(a); URL.revokeObjectURL(url); };

[greptile-apps]

Component file exceeds 700-line limit

AccessPolicyEditor.tsx is 954 lines, which exceeds the project's 700-line limit per component file. Consider extracting:

• PolicyCanvasPanel into its own file (e.g., PolicyCanvasPanel.tsx) along with its helper utilities (createPolicyNode, findLastInChain, findFirstOfType, CenterOnInitialLoad, CenterOnNewNode).
• The add/delete node callback logic could move to a custom hook (usePolicyCanvasNodes.ts).

This would also make each piece easier to test in isolation.

Rule Used: Keep React component files under 700 lines and fol... (source)

Learnt From
ethyca/fides#6639

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

[greptile-apps]

Short variable names throughout callbacks

Several callback parameters use 1–2 character names (e, n, p, acc) throughout this file and AccessPolicyEditor.tsx. Per the project's naming convention, variable names should be descriptive. For example:

• edges.filter((e) => ...) → edges.filter((edge) => ...)
• nodes.find((n) => ...) → nodes.find((node) => ...)
• conditionNodes.reduce<MatchBlock>((acc, condNode) => ...) is fine since condNode is descriptive, but acc could be matchAcc.

This pattern also appears in AccessPolicyEditor.tsx in setNodes((nds) => ...), setEdges((eds) => ...), etc.

Rule Used: Use full names for variables, not 1 to 2 character... (source)

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

[claude]

Code Review: New policies builder (#7725)

This is a substantial, well-structured feature with good test coverage for the YAML↔node round-trip logic. The bidirectional sync, chain surgery on deletion, and dagre layout integration are all implemented thoughtfully. A few things need attention before merging:

---

Critical (must fix)

1. Node ID collision after delete + add (AccessPolicyEditor.tsx)

handleAddCondition, handleAddConstraint, and handleAddActionFromNode all derive the new node ID from the current count of that node type (.filter(...).length). After deleting a middle node (e.g. condition-2 of 3), the count drops and the next "add" generates an ID that already exists in the graph. React Flow deduplicates by ID, so the old and new node silently merge. Fix: use a useRef counter that only ever increments, or use a UUID.

2. Stale dagre graph corrupts layout after node deletion (layout-utils.ts)

The module-level dagreGraph singleton is never cleared of nodes/edges between calls — setGraph({...}) only resets config. Deleted nodes linger in the graph and affect the layout of subsequent renders. For the existing datamap this was low-risk (nodes are rarely removed), but the policy builder deletes nodes interactively. The fix is to instantiate new dagre.graphlib.Graph() inside getLayoutedElements on each call rather than reusing the singleton.

---

Suggestions

3. Infinite spinner on API error (edit/[id]/index.tsx)

If useGetAccessPolicyQuery rejects, isLoading goes false and data stays undefined, so the page shows <Spin /> forever with no error message. Also, the bare <Spin /> has no <Layout> wrapper so the nav disappears. Worth adding an isError branch.

4. URL.revokeObjectURL races the download (AccessPolicyEditor.tsx, handleExport)

a.click() is asynchronous — the browser queues the download but hasn't fetched the blob yet. Revoking the URL synchronously on the next line can abort the download on Firefox. Wrapping the revoke in a setTimeout(..., 100) is the standard fix.

5. PrivacyNoticeSelect silently truncates at 100 (PrivacyNoticeSelect.tsx)

The hard-coded size: 100 means orgs with more than 100 privacy notices will have an incomplete picker with no indication that results are missing. Either bump the limit or switch to a server-side search.

6. AccessPolicyYaml type marks decision/match as required but they aren't (types.ts)

parseYaml accepts objects with only name set (no decision, no match), so the return type doesn't match the interface. Every call-site already uses optional chaining, so it's not causing bugs today, but it's a type-level lie. Making those fields optional would match reality.

[claude]

Suggestion: Infinite spinner when the API call fails

When getAccessPolicy errors, isLoading becomes false but data stays undefined, so the page renders <Spin /> forever with no error message and no way out for the user.

Also, <Spin /> is returned without a <Layout> wrapper, so the nav/chrome disappears.

Consider adding an error state:

const { data, isLoading, isError } = useGetAccessPolicyQuery(policyId, { skip: !policyId }); if (isLoading) return <Layout title="Edit access policy"><Spin /></Layout>; if (isError || !data) return <Layout title="Edit access policy"><ErrorState /></Layout>;

[claude]

Suggestion: URL.revokeObjectURL called before the browser processes the download

a.click() is asynchronous — the browser schedules the download event but hasn't started reading the blob yet. Calling URL.revokeObjectURL immediately after can cause the download to fail on Firefox and some Chromium versions (the object URL is invalidated before the browser fetches it).

Standard fix:

a.click(); setTimeout(() => URL.revokeObjectURL(url), 100);

[claude]

Suggestion: Hard-coded page size of 100 will silently truncate results

If an org has more than 100 privacy notices, notices beyond the first 100 won't appear in the picker and the user won't be notified. Consider either paginating/searching server-side or at least bumping this to a safely large value and adding a showSearch filter that hits the server (the showSearch prop is already set, but it filters locally against the already-fetched list).

[claude]

Suggestion: AccessPolicyYaml declares decision and match as required but parseYaml returns objects without them

parseYaml accepts any object that has either name or decision, so the return type can legally be { name: "foo" } — no decision, no match. But AccessPolicyYaml marks both as required (decision: "ALLOW" | "DENY" and match: MatchBlock), which is a lie to the type system.

Every call-site does parsed?.decision anyway, so the type assertion is harmless today, but it invites future callers to skip null checks. Consider splitting into two types:

export type AccessPolicyYaml = Partial<AccessPolicyYamlFull> & { name?: string }; export interface AccessPolicyYamlFull { decision: "ALLOW" | "DENY"; match: MatchBlock; ... }

Or just make decision and match optional here.

[lucanovera]

Not sure if it's intended, but it looks like we're missing some borders and backgrounds.

Oh wow, yeah, that's not intended. I wonder if it's from an AI code review suggestion I approved. I'll take a look now. I'll DM you, it's looking fine locally.

[kruulik]

Nit: clicking the plus/add button doesn't do anything, but clicking the lock icon adds a new node, which feels backwards. This doesn't need to be solved now, but we may want to consider adding text labels for each CTA for clarity.

[kruulik]

Have we thought about saving the user state anywhere, as the user is building? Perhaps just to localstore? This could be a good follow-up.

[kruulik]

Looking good!

[lucanovera]

Nit: clicking the plus/add button doesn't do anything, but clicking the lock icon adds a new node, which feels backwards. This doesn't need to be solved now, but we may want to consider adding text labels for each CTA for clarity.

I will try the options we discussed and open a follow up PR for it.