Conversation
…lis-agent-chat-for-policy-building
…lis-agent-chat-for-policy-building
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Dependency ReviewThe following issues were found:
Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned Files
|
|
/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 #7993 — Agent chat for policy builder UI
This is a frontend-only PR adding an AgentChatPanel component with @ant-design/x to the access policy editor. The core feature approach is solid, but there are two bugs to fix before merging and two architectural issues worth addressing.
Bugs (must fix)
1. .panel missing flex container (AgentChatPanel.module.scss:4)
The .body { flex: 1 }, .header { flex-shrink: 0 }, and .footer { flex-shrink: 0 } rules are flex-child properties that have no effect without a flex container parent. .panel needs display: flex; flex-direction: column. Without it, the body won't fill available height and the layout will break.
2. Wrong RTKErrorResult cast (AgentChatPanel.tsx:105)
(error as RTKErrorResult).error passes the outer wrapper object to getErrorMessage. The correct pattern used elsewhere in the codebase is error as RTKErrorResult["error"]. As written, error toasts will show a bad or empty message.
Architecture issues
3. Nested XProvider (AgentChatPanel.tsx:156)
The app already has a root FidesUIProvider / ConfigProvider. A per-component <XProvider> creates a second CSS-in-JS token context that overrides rather than merges. If @ant-design/x needs a provider, it should be added once in fidesui's FidesUIProvider.
4. @ant-design/cssinjs version split (package.json:19)
@ant-design/x requires @ant-design/cssinjs ^2.x but the root dependency is ^1.x. npm resolves this with a private nested copy (node_modules/@ant-design/x/node_modules/@ant-design/cssinjs@2.1.2), so two different CSS-in-JS engine versions coexist. This can cause duplicate style injection, token hash mismatches, and SSR hydration errors that are hard to diagnose. Consider moving @ant-design/x into fidesui to share the same dependency resolution. Also note that @ant-design/x unconditionally depends on mermaid and react-syntax-highlighter — bundle impact should be verified.
Minor / suggestions
canvasPanelreuse in two tab slots (AccessPolicyEditor.tsx): BothBuilderandSplittabs reference the same JSX element under a sharedReactFlowProvider, meaning twoPolicyCanvasPanelinstances are mounted simultaneously with shared node/edge state. Low risk since Split is dev-only, but worth a comment.- Feature flag coupling (
AccessPolicyEditor.tsx):detection_discovery.llm_classifier_enabledis a data-discovery capability flag, not a policy-agent flag. Works fine for now but worth a follow-up when this feature matures to give operators finer-grained control. - MSW handlers use
restfrom msw v1 API (agent-chat-handlers.ts): Consistent with the rest of the mock layer — no action needed, just noting it for the eventual msw v2 migration.
Overall: the feature structure is clean and the API slice is minimal and well-typed. Fix the two bugs above and consider the XProvider / cssinjs issues before merging to avoid hard-to-debug styling regressions.
🔬 Codegraph: connected (47178 nodes)
💡 Write /code-review in a comment to re-run this review.
|
/code-review |
…lis-agent-chat-for-policy-building
There was a problem hiding this comment.
Code Review: Agent Chat for Access Policy Builder
Overall this is a clean, well-scoped addition. The architecture is sound — fidesui re-exports Bubble/Sender from @ant-design/x/lib (CJS) to maintain a single ConfigProvider/cssinjs context identity, the webpack/Turbopack antd$ alias enforces that at the bundler level, and AgentChatPanel itself is simple and readable. The RTK slice is minimal and correct. A few things to address before merge:
Must Fix
Feature flag reuse (AccessPolicyEditor.tsx:799-800) — detection_discovery.llm_classifier_enabled gates a different feature (the discovery LLM classifier). Repurposing it couples two unrelated features to one config key. See inline comment.
handleYamlProposed state divergence (AccessPolicyEditor.tsx:852-859) — when the agent returns invalid YAML, controls is left at its previous value rather than being cleared, causing the canvas and code editor to disagree. Fix: setControls(parsed?.controls ?? []) unconditionally.
Shared SVG viewBox change (logomark-ethyca.svg) — the viewBox was changed from a 3:2 rectangle to a 1:1 square to fit the chat avatar. This is a shared asset; please verify all other consumers (nav header, etc.) still render correctly. A separate avatar-specific copy may be safer.
Suggestions
chatHistoryId not reset on external YAML changes (AgentChatPanel.tsx:56) — if the user manually edits the YAML outside the chat, the conversation thread continues referencing the old policy context. A "Clear conversation" button (resetting both messages and chatHistoryId) would address this.
isLoading in handleSend dep array (AgentChatPanel.tsx:116-120) — causes handleSend to be recreated on every request state change. The Sender's loading prop already prevents double-submission; isLoading can be removed from deps.
@ant-design/cssinjs major version bump (package.json:37) — confirm no visual/functional regressions in existing antd components after the 1.x → 2.x upgrade.
Nice to Have
- No unit tests for
AgentChatPanelor the state logic inhandleYamlProposed/handleSend. The 3-turn scripted mock handler is a great foundation for integration tests. - The
"Policy builder agent"header string is hardcoded — consider a named constant if it needs to be reused or localized.
🔬 Codegraph: connected (47276 nodes)
💡 Write /code-review in a comment to re-run this review.
| const handleYamlProposed = useCallback((newYaml: string) => { | ||
| setYamlValue(newYaml); | ||
| setSyncKey((k) => k + 1); | ||
| const parsed = parseYaml(newYaml); | ||
| if (parsed?.controls) { | ||
| setControls(parsed.controls); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
clients/admin-ui/src/features/access-policies/AccessPolicyEditor.tsx:852-859
If parseYaml(newYaml) returns null (invalid YAML from the agent), yamlValue and syncKey are still updated but controls is left at its previous value — state diverges silently. Compare with handleSave which already uses parsed?.controls ?? [].
Suggest:
const parsed = parseYaml(newYaml);
setControls(parsed?.controls ?? []);| }: AgentChatPanelProps) => { | ||
| const messageApi = useMessage(); | ||
| const [messages, setMessages] = useState<ChatMessage[]>([]); | ||
| const [chatHistoryId, setChatHistoryId] = useState<string>(); |
There was a problem hiding this comment.
clients/admin-ui/src/features/access-policies/AgentChatPanel.tsx:56
chatHistoryId is never reset when currentYaml changes externally (e.g. the user manually edits the Code tab, imports a new policy, or resets the form). After an external YAML edit, the next chat turn will continue the existing server-side conversation thread, which was built around the old YAML context. The agent will receive the updated current_policy_yaml but may make references to earlier turns that no longer apply.
At minimum, consider adding a "Clear conversation" button that resets both messages and chatHistoryId, so users can start fresh after making manual edits.
| [ | ||
| isLoading, | ||
| nextKey, | ||
| chatHistoryId, | ||
| currentYaml, |
There was a problem hiding this comment.
clients/admin-ui/src/features/access-policies/AgentChatPanel.tsx:116-120
isLoading in the dep array means handleSend is recreated every time a request starts or finishes, causing Sender's onSubmit prop to change on each state transition. The guard if (!trimmed || isLoading) return works regardless of when the callback was created, so isLoading is not needed as a dep here — the Sender's own loading prop already disables the submit button while in-flight.
Removing isLoading from the dep array reduces unnecessary renders.
| <div style={{ flex: "0 0 60%" }}> | ||
| <ReactFlowProvider>{canvasPanel}</ReactFlowProvider> | ||
| </div> | ||
| <div style={{ flex: "0 0 calc(40% - 8px)" }}> |
There was a problem hiding this comment.
This could probably go into a css module, but not critical.
There was a problem hiding this comment.
Nice catch, I'll move those to the modules file.
lucanovera
requested review from
speaker-ender
and removed request for
a team and
speaker-ender
| // Force all imports of "antd" to resolve to the CJS build. fidesui uses | ||
| // "antd/lib" (CJS), but third-party packages like @ant-design/x import plain | ||
| // "antd", which the bundler resolves to ESM — producing two separate antd | ||
| // modules with their own ConfigProvider contexts. useToken() then reads | ||
| // unthemed defaults inside those components. The alias keeps everything on | ||
| // one antd instance. | ||
| turbopack: { | ||
| resolveAlias: { | ||
| antd: "antd/lib", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This makes sense, but we have to be certain that everything using ant imports exactly the same way. ant and ant/lib will resolve to different themes and not respect any overrides.
Storybook and Chromatic can be a good place to check.
There was a problem hiding this comment.
Yes, that is the issue that the resolveAlias solves. When I added the Ant Design X library, it was importing from "antd" and we were using "antd/lib", so we had that issue of the themes not matching. Adding the resolveAlias ensures the bundler will use "antd/lib" even if it sees an "antd" import.
The unfortunate part, is that fidesui is not a full library that compiles on it's own. That means this resolveAlias had to be added at the admin-ui level instead of fidesui. That's one more reason to make fidesui a standalone library in the future.
lucanovera
deleted the
ENG-2959-FE-Astralis-agent-chat-for-policy-building
branch
2 participants
Ticket ENG-2959
Description Of Changes
Adds an AI agent chat panel next to the Access Policy builder. When enabled, users describe a policy in natural language and the agent proposes YAML (and a set of controls) that re-hydrates into the canvas and code editor. First UI surface for the Astralis policy-building assistant.
The chat UI is built on
@ant-design/xand sits inside a resizableSplitterpanel beside the existing Builder / Split / Code tabs. The panel is gated on the existing server-wide LLM capability flag — no new config key was introduced.Along with the feature itself, this PR picks up two dependency-hygiene changes that were required to make
@ant-design/xcomponents inherit the app's antd theme. Both are also fixes for pre-existingdual antd module instanceanddual cssinjs module instancepitfalls that fidesui has a comment about, but that weren't enforced end-to-end:@ant-design/cssinjsfrom^1.21.0→^2.1.2in admin-ui. The v1 pin was holding the top-levelnode_modulesslot, forcing antd and@ant-design/xto each install their own nested v2 copy. Different module identities meant differentStyleContextReact contexts and broken theme inheritance. The three APIs used in_document.tsx(createCache,extractStyle,StyleProvider) are backward-compatible in v2, so no code change was needed there."antd"→"antd/lib"in both Turbopack and webpack. fidesui re-exports antd from"antd/lib"(CJS) deliberately (see the existing comment infidesui/src/index.ts), but third-party packages like@ant-design/ximport plain"antd", which the bundler resolves to the ESM build. That produced twoConfigContextidentities anduseToken()inside Bubble/Sender fell back to antd defaults (#1677ff). The alias ensures every"antd"import resolves to the single CJS instance.Other notable details:
detection_discovery.llm_classifier_enabled, which is already exposed on/api/v1/configand already used byLlmModelSelectoras the "LLM is available on this server" signal. Parity with existing LLM features (classifier, privacy assessment chat, IDP/website monitor).@ant-design/xis now afidesuidependency.Bubble,Sender, andBubbleItemTypeare re-exported from fidesui using the@ant-design/x/libCJS path, mirroring the antd re-export pattern.new_policy_yamlflow throughhandleYamlProposed, which bumpssyncKeysoPolicyCanvasPanelre-hydrates from YAML and thecontrolsstate stays consistent with the parsed policy.useMemoaround the tab items with a plain array literal — the deps list was omittingcontrols,controlOptions, andsyncKeywith a disabled lint rule.dev:mock) drives a deterministic 3-turn demo conversation against/api/v1/plus/llm/access-policy-chatso the UI is iterable without the backend.Code Changes
clients/admin-ui/src/features/access-policies/AgentChatPanel.tsx— new panel usingBubble,Sender(via fidesui) anduseMessage-based error surfacing.clients/admin-ui/src/features/access-policies/AgentChatPanel.module.scss— panel layout styles.clients/admin-ui/src/features/access-policies/agent-chat.slice.ts— RTK Query mutation forPOST /api/v1/plus/llm/access-policy-chat, injected viabaseApi.injectEndpoints.clients/admin-ui/src/features/access-policies/AccessPolicyEditor.tsxdetection_discovery.llm_classifier_enabledand render the chat inside a<Splitter>when enabled.useMemo(tabItems, …)+ eslint-disable with a plain array literal.calc(100vh - 220px)→100%.handleYamlProposed→setYamlValue+setSyncKey+setControlsso agent-proposed YAML re-hydrates the canvas.clients/admin-ui/src/features/access-policies/AccessPolicyEditor.module.scss— make the editor body fill available height.clients/admin-ui/src/mocks/access-policies/agent-chat-handlers.ts— scripted 3-turn handler for/api/v1/plus/llm/access-policy-chat, keyed bychat_history_id.clients/admin-ui/src/mocks/handlers.ts— registeragentChatHandlers().clients/admin-ui/package.json— remove@ant-design/x; bump@ant-design/cssinjsfrom^1.21.0to^2.1.2.clients/admin-ui/next.config.js— Turbopack + webpack alias forcing"antd"→"antd/lib".clients/fidesui/package.json— add@ant-design/x: ^2.5.0.clients/fidesui/src/index.ts— re-exportBubble,Sender, andBubbleItemTypefrom@ant-design/x/lib.clients/fidesui/src/icons/carbon.ts— export the CarbonAiicon for the agent avatar.clients/package-lock.json— regenerated; nested cssinjs copies collapsed to a single root v2.1.2, one antd module instance.changelog/7993-astralis-agent-chat-policy-builder.yaml— "Added" entry.Steps to Confirm
http://localhost:3000detection_discovery.llm_classifier_enabled, enable that flag for your session. The simplest way is to set it in.fides/fides.tomlon your local backend and restart it; alternatively, temporarily stub the flag totrueinconfig-settings.slice.tswhile manually testing.Splitter.<Bubble>or the<Sender>input:--ant-color-primaryshould be#2b2e35(Fides brand), not#1677ff(antd default). Compare with a regular<Button>in the header — values must match. This verifies the antd theme actually reaches@ant-design/xcomponents.consent"unless" clause should now appear).llm_classifier_enabledoff (or run against a backend that doesn't set it) and reload. The chat panel must not render; the editor falls back to the original full-width tabs layout.npm run build && npm run startonce to confirm SSR style extraction still works with the cssinjs v2 bump. First paint in production mode should not show a flash of unstyled antd components.@ant-design/xcomponents because they share the same antd/cssinjs instance as the rest of the app.Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works