feat(studio): replace guardrails side panel with dedicated detail route - #610
Conversation
Promotes the guardrails config detail from a slide-out SidePanel into a full-page route (/guardrails/:guardrailConfigName), removing the GUARDRAILS_ENABLED feature flag gate and aligning with the pattern used by other resource types in Studio. - Delete GuardrailsDetailPanel (side panel component + test) - Add GuardrailDetailRoute (full-page detail view with breadcrumbs, delete) - Add guardrailDetail route constant and getGuardrailDetailRoute helper - Register both routes in guardrailsRoutes (list + detail) - Add mock GET /configs/:name handler for Storybook/test MSW - Fix optional chaining crash in GuardrailsDataView empty-state check Signed-off-by: Alex Ray <alray@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a standalone guardrail config detail route, updates list navigation to use it, removes the in-page detail panel, and fixes one empty-state null check. ChangesGuardrail detail flow
Sequence Diagram(s)sequenceDiagram
participant User
participant GuardrailsRoute
participant GuardrailDetailRoute
participant API
participant QueryClient
User->>GuardrailsRoute: click guardrail row
GuardrailsRoute->>QueryClient: cache selected config
GuardrailsRoute->>GuardrailDetailRoute: navigate to detail route
GuardrailDetailRoute->>API: fetch config by name
API-->>GuardrailDetailRoute: config data or error
GuardrailDetailRoute-->>User: render loading, error, or config details
User->>GuardrailDetailRoute: confirm delete
GuardrailDetailRoute->>API: delete config
API-->>GuardrailDetailRoute: delete result
GuardrailDetailRoute->>QueryClient: invalidate guardrail config queries
GuardrailDetailRoute-->>User: navigate back to list
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/packages/studio/src/routes/guardrails/GuardrailDetailRoute/index.tsx (2)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
countRailsreused from a sibling component's private utils file.Importing
countRailsfrom@studio/components/dataViews/GuardrailsDataView/guardrailUtilsinto an unrelated route module couples this page toGuardrailsDataView's internals. Since the util is now shared across a component and a route, consider moving it to a shared location (e.g.@studio/utils/guardrails) to keep component-local utils decoupled from route consumers.Also applies to: 82-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/guardrails/GuardrailDetailRoute/index.tsx` at line 14, `countRails` is being imported from `GuardrailsDataView`’s private utils, which tightly couples `GuardrailDetailRoute` to a sibling component’s internals. Move `countRails` into a shared guardrails utility location and update both `GuardrailDetailRoute` and any other consumers to import it from that shared module instead of `guardrailUtils`.
52-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelete-and-invalidate logic duplicated with
GuardrailsRoute.handleDelete.This callback (delete → invalidate list query → handle success/failure) mirrors the logic in
GuardrailsRoute/index.tsxalmost exactly (differing only in the post-delete navigation). Consider extracting a shared hook (e.g.useDeleteGuardrailConfig) to avoid drift between the two call sites.♻️ Sketch of shared hook
export function useDeleteGuardrailConfig(workspace: string) { const queryClient = useQueryClient(); const { mutateAsync: deleteConfig } = useGuardrailsDeleteConfig(); return useCallback( async (name: string): Promise<boolean> => { try { await deleteConfig({ workspace, name }); await queryClient.invalidateQueries({ queryKey: [`/apis/guardrails/v2/workspaces/${workspace}/configs`], }); return true; } catch { return false; } }, [deleteConfig, queryClient, workspace] ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/guardrails/GuardrailDetailRoute/index.tsx` around lines 52 - 64, The delete flow in handleDelete is duplicated with GuardrailsRoute.handleDelete, so extract the shared delete/invalidate/error-handling logic into a reusable hook such as useDeleteGuardrailConfig. Move the deleteConfig call and queryClient.invalidateQueries behavior into that hook, returning a boolean success value, and keep only the route-specific navigation (navigate(getGuardrailsRoute(workspace))) in GuardrailDetailRoute. Update both call sites to use the shared hook so the guardrail deletion behavior stays consistent and easier to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/packages/studio/src/routes/guardrails/GuardrailDetailRoute/index.tsx`:
- Line 14: `countRails` is being imported from `GuardrailsDataView`’s private
utils, which tightly couples `GuardrailDetailRoute` to a sibling component’s
internals. Move `countRails` into a shared guardrails utility location and
update both `GuardrailDetailRoute` and any other consumers to import it from
that shared module instead of `guardrailUtils`.
- Around line 52-64: The delete flow in handleDelete is duplicated with
GuardrailsRoute.handleDelete, so extract the shared
delete/invalidate/error-handling logic into a reusable hook such as
useDeleteGuardrailConfig. Move the deleteConfig call and
queryClient.invalidateQueries behavior into that hook, returning a boolean
success value, and keep only the route-specific navigation
(navigate(getGuardrailsRoute(workspace))) in GuardrailDetailRoute. Update both
call sites to use the shared hook so the guardrail deletion behavior stays
consistent and easier to maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 30bdc846-240e-4b02-b462-bf83bd45afbe
📒 Files selected for processing (12)
web/packages/studio/src/components/dataViews/GuardrailsDataView/index.tsxweb/packages/studio/src/constants/routes.tsweb/packages/studio/src/mocks/handlers/guardrails.tsweb/packages/studio/src/routes/groups/guardrailsRoutes.tsxweb/packages/studio/src/routes/guardrails/GuardrailDetailRoute/index.test.tsxweb/packages/studio/src/routes/guardrails/GuardrailDetailRoute/index.tsxweb/packages/studio/src/routes/guardrails/GuardrailsDetailPanel/index.test.tsxweb/packages/studio/src/routes/guardrails/GuardrailsDetailPanel/index.tsxweb/packages/studio/src/routes/guardrails/GuardrailsRoute/index.test.tsxweb/packages/studio/src/routes/guardrails/GuardrailsRoute/index.tsxweb/packages/studio/src/routes/utils.tsweb/packages/studio/src/tests/title-change.test.tsx
💤 Files with no reviewable changes (2)
- web/packages/studio/src/routes/guardrails/GuardrailsDetailPanel/index.tsx
- web/packages/studio/src/routes/guardrails/GuardrailsDetailPanel/index.test.tsx
|
…ant navigation Signed-off-by: Alex Ray <alray@nvidia.com>
…te (#610) * feat(studio): replace guardrails side panel with dedicated detail route Promotes the guardrails config detail from a slide-out SidePanel into a full-page route (/guardrails/:guardrailConfigName), removing the GUARDRAILS_ENABLED feature flag gate and aligning with the pattern used by other resource types in Studio. - Delete GuardrailsDetailPanel (side panel component + test) - Add GuardrailDetailRoute (full-page detail view with breadcrumbs, delete) - Add guardrailDetail route constant and getGuardrailDetailRoute helper - Register both routes in guardrailsRoutes (list + detail) - Add mock GET /configs/:name handler for Storybook/test MSW - Fix optional chaining crash in GuardrailsDataView empty-state check Signed-off-by: Alex Ray <alray@nvidia.com> * feat(studio): seed detail query cache on guardrail row click for instant navigation Signed-off-by: Alex Ray <alray@nvidia.com> --------- Signed-off-by: Alex Ray <alray@nvidia.com>
Summary
GuardrailsDetailPanelslide-out with a full-pageGuardrailDetailRouteat/workspaces/:workspace/guardrails/:guardrailConfigName, consistent with how other Studio resource types handle detail viewsGUARDRAILS_ENABLEDfeature flag gate — guardrails routes are now always registered (gating is handled bygateGuardrailsRoutes)guardrailDetailroute constant,getGuardrailDetailRoutehelper, and a MSW mock forGET /configs/:nameGuardrailsDataViewwhendata.datais undefinedTest plan
GuardrailDetailRouteunit tests passGuardrailsRouteunit tests passtitle-change.test.tsxpassesSummary by CodeRabbit