Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
lucanovera
requested review from
jpople and
kruulik
and removed request for
a team and
jpople
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — Access Policies List View
Good overall structure. The feature is well-decomposed: hooks own their data/mutation concerns, the container wires things together, and the table/grid split is clean. The drag-and-drop implementation follows the standard react-dnd pattern correctly, including the visual-only local state during a drag and the single API call on drop. The nuqs integration for URL-synced filters is a nice touch. Tests for formatRelativeTime, extractPolicyFields, and updateYamlField are thorough.
A few things to address before merging:
Bug
EditablePriorityCell— Escape does not resetinputValue(inline comment on line 160 ofPoliciesTable.tsx). After pressing Escape, re-opening the edit input shows the previously typed value instead of the current priority. One-line fix: resetinputValuebefore callingsetIsEditing(false).
Correctness concerns
updateYamlFielddrops YAML formatting (policy-yaml.ts). Theload+dumpround-trip strips comments, re-orders keys, and may reformat multi-line strings. Worth addressing before the policy YAML schema gets more complex.- Multi-control policies appear in multiple card groups (
useAccessPolicyGroups.ts). Same card renders once per matching control. Intentional or not, it should be explicit. - Policies missing
priorityin YAML sort to the top (useAccessPoliciesList.ts). Default of0may produce a confusing ordering for legacy policies.
Minor / nits
- Misleading doc comment in
extractPolicyFields(says it avoids a full parse but delegates toparseYaml). ViewModere-exported throughPoliciesToolbarrather than imported directly fromtypes.- Unused
typefield in theDragIteminterface. - No range validation on the priority
<Input>— backend errors surface via toast but the cell shows no inline error state.
| size="small" | ||
| value={inputValue} | ||
| onChange={(e) => setInputValue(e.target.value)} | ||
| onBlur={commit} |
There was a problem hiding this comment.
Bug: Escape key doesn't reset inputValue to the current value.
When the user types a new value and then presses Escape, setIsEditing(false) is called but inputValue is left as whatever the user typed. The next time they click the edit button, the stale typed value reappears instead of the current priority.
} else if (e.key === "Escape") {
setInputValue(String(value)); // reset first
setIsEditing(false);
}There was a problem hiding this comment.
good catch, fixed!
| setIsEditing(false); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
The priority input has no range validation — a user can submit -1, 0, or an arbitrarily large number. If the backend enforces constraints (e.g. priority >= 1), the API call will fail silently (the error is caught and surfaced via message.error in useUpdatePolicyPriority, but the cell shows no error state). Consider adding a min and/or max attribute on the <Input> and validating before calling onEdit:
const parsed = parseInt(inputValue, 10);
if (!Number.isNaN(parsed) && parsed > 0 && parsed !== value) {
onEdit(parsed);
}| ): string => { | ||
| try { | ||
| const parsed = yaml.load(yamlString) as Record<string, unknown> | null; | ||
| if (!parsed || typeof parsed !== "object") { |
There was a problem hiding this comment.
updateYamlField does not preserve YAML formatting or key order.
yaml.load + yaml.dump round-trips the document through a plain JS object, which means:
- Key insertion order is not preserved across JS engines
- Comments are stripped
- Multi-line string scalars may be reformatted
- Anchors/aliases are lost
For the current use-cases (toggling enabled, updating priority) this is likely fine today, but as policy YAML grows more complex this could silently corrupt the document shape. A safer approach is a targeted regex/string replacement for scalar fields, or using a YAML AST library that preserves document structure (e.g. yaml's Document API with doc.set()).
| * Extract top-level fields from a policy YAML string for list display. | ||
| * Avoids building the full node graph — just reads the scalar fields. | ||
| */ | ||
| export const extractPolicyFields = ( |
There was a problem hiding this comment.
The doc comment says "Avoids building the full node graph — just reads the scalar fields", but the implementation calls parseYaml which does a full yaml.load of the document. The comment is misleading and should be updated or removed.
|
|
||
| import { ControlGroup } from "./access-policies.slice"; | ||
| import { ViewMode } from "./types"; | ||
|
|
There was a problem hiding this comment.
ViewMode is defined in types.ts but re-exported from PoliciesToolbar.tsx, and then PoliciesContainer.tsx imports it from PoliciesToolbar. This indirect chain (types → Toolbar → Container) is confusing for future readers. Consider importing ViewMode directly from ./types in PoliciesContainer and removing the re-export here.
| if (!controlGroups) { | ||
| return []; | ||
| } | ||
|
|
There was a problem hiding this comment.
A policy with multiple controls (e.g. controls: ["eea_uk_gdpr", "us_glba_ccpa"]) will appear in every group whose key it matches. In the card view this means the same policy card renders multiple times — once per matching control group. Is that intentional? If so, it's worth a comment to make it explicit. If not, you'll want to de-duplicate, e.g. by assigning each policy to only its first matching group or showing it only in the group the user is currently viewing.
| if (!data?.items) { | ||
| return []; | ||
| } | ||
| return data.items.map(toListItem).sort((a, b) => a.priority - b.priority); |
There was a problem hiding this comment.
Sorting by priority client-side is fine, but policies without an explicit priority in their YAML all default to 0 and end up clustered at the top of the list in an arbitrary order. If there are many legacy policies without the field, this could produce a confusing default ordering. It may be worth treating a missing priority as Infinity (sorted to the end) rather than 0, or documenting the intent if placing unprioritized policies first is deliberate.
| id: string; | ||
| type: string; | ||
| } | ||
|
|
There was a problem hiding this comment.
The type field on DragItem is declared but never read in the implementation — useDrag handles the type internally via its type option. This can be removed to avoid confusion:
interface DragItem {
index: number;
originalIndex: number;
id: string;
}
kruulik
left a comment
kruulik
left a comment
There was a problem hiding this comment.
Checked out the branch and fiddled around with the table - super performant! Couple nits in the code but nothing critical. I think the table component itself could be split up a little bit, but that's up to your discretion.
| <Segmented | ||
| value={viewMode} | ||
| onChange={(value) => onViewModeChange(value as ViewMode)} | ||
| options={[ |
There was a problem hiding this comment.
Not totally related to your PR, but the these icons look reversed in the context of this page. The left one looks like a list of items or a table, the right one looks like a card. But it's the opposite.
There was a problem hiding this comment.
Good callout! I've updated the icons, hopefully they're more recognizable now.
lucanovera
force-pushed
the
ENG-2956-FE-Policies-list
branch
from
44f6f4d to
299259e
Compare
2 participants
Ticket ENG-2956
Description Of Changes
Implements the access policies list page with full CRUD-adjacent interactions. Users can browse policies in a table or card view, search and filter by control group or enabled state, reorder policies via drag-and-drop (which updates priority), and toggle a policy enabled/disabled inline. Policies are grouped by control group in the card view. The page is gated behind the
alphaPurposeBasedAccessControlfeature flag and requires a Fides Plus license.Code Changes
PoliciesContainer— top-level orchestrator: fetches policies and control groups, applies search/filter state, and delegates to table or card viewPoliciesToolbar— search input, control group select, enabled/disabled filter, and table/card view toggle (persisted as?view=query param)PoliciesTable— Ant Design table with drag-and-drop row reordering, inline editable priority cell, and an enabled/disabled toggle per rowPoliciesGridandPolicyCard— card view with policies grouped by control group category, each card showing name, description, enabled state, and associated controlsPolicyCategoryGroup— section header component for grouping cards by control group in grid viewpolicy-yaml.ts— utilities for parsing and patching YAML fields (enabled,priority) without a round-trip to the backend schemauseAccessPoliciesList,useAccessPolicyGroups,usePoliciesFilters,useReorderPolicies,useTogglePolicyEnabled,useUpdatePolicyPriorityaccess-policies.slice.tswithreorderAccessPolicyandupdateAccessPolicyRTK Query mutationsCustomTypographyinfidesuito support aLinkTextvariantSteps to Confirm
Start the admin UI with mock API:
cd clients/admin-ui npm run dev:mockNavigate to
http://localhost:3000and log inEnable the feature flag: Go to Settings → About and enable
alphaPurposeBasedAccessControl. The Access policies nav item should appear in the sidebar.Navigate to
/access-policiesand confirm the list loads with mock policies.Card view (default):
Table view:
Search and filtering:
Empty state:
"New policy" button:
/access-policies/newPre-Merge Checklist
CHANGELOG.mdupdated