refactor(platform): split governance settings into nested routes#1603
Merged
Conversation
Restructure the Settings → Governance page from a single-file Tabs layout into a nested-route layout with a persistent left sidebar and five groups: Content & Models, Policies & Limits, Security & Monitoring, Guardrails, and Usage. Each group is now its own route file under routes/dashboard/$id/settings/governance/. - Introduces a governance route layout with sidebar nav + <Outlet/> - Splits Guardrails (overview, content safety, PII, moderation) and Usage into first-class sidebar entries rather than sub-tabs - Moves the inline useUploadPolicy/usePiiConfig hooks into the shared queries module and removes the standalone use-upload-policy file - Small UI polish across analytics, chat selectors, and governance editors (tables, toaster, select/searchable-select) - Adds rules-table-empty-state shared component - i18n: adds "groups.usage" key in en/de/fr alongside existing guardrails group
📝 WalkthroughWalkthroughThis PR restructures the governance settings feature and updates related UI components. It moves the Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/platform/app/components/ui/feedback/toaster.tsx (1)
74-79:⚠️ Potential issue | 🟡 MinorHardcoded
aria-label="Close"should use translation hook.Per coding guidelines, every icon-only button's aria-label must go through the translation layer. The hardcoded "Close" string should be replaced with a translated value.
Proposed fix
+import { useT } from '@/lib/i18n/client'; + export function Toaster() { const { toasts } = useToast(); + const { t } = useT('common'); return ( ... <ToastPrimitives.Close className="..." - aria-label="Close" + aria-label={t('actions.close')} >As per coding guidelines: "Every icon-only button has an
aria-label. The label goes through the translation layer — never hardcode English in ARIA."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/components/ui/feedback/toaster.tsx` around lines 74 - 79, Replace the hardcoded aria-label on the ToastPrimitives.Close button with a translated string via the app's i18n hook (e.g., call the translation hook used in this codebase and pass a key like "close" or "common.close"); update the Toast component to import/use that hook (locate the ToastPrimitives.Close usage in toaster.tsx) and set aria-label to the translated value (ensure the translation key exists or add it to locale files and keep aria-label fallback safe).services/platform/messages/fr.json (1)
3932-4504:⚠️ Potential issue | 🔴 CriticalFrench locale missing 125 keys required for cross-locale parity.
The verification found that
fr.jsonis missing 125 keys present inen.json:
- 50+
analytics.usage.*keys- 12
governance.twoFactorPolicy.*andgovernance.groups.subtabs.twoFactorkeysGerman (
de.json) has correct parity with English. Every key inen.jsonmust exist infr.jsonon the same commit (per the i18n sync requirement). Add the missing translations to restore parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/messages/fr.json` around lines 3932 - 4504, fr.json is missing 125 keys required for i18n parity (notably analytics.usage.* and governance.twoFactorPolicy.* plus governance.groups.subtabs.twoFactor); restore parity by adding all missing keys from en.json into services/platform/messages/fr.json with French translations (or provisional placeholders) preserving the exact key names and JSON structure—particularly ensure analytics.usage.* entries and governance.twoFactorPolicy.* and governance.groups.subtabs.twoFactor are present and match the keys in en.json so the locale file structure matches de.json/en.json.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commitlint.config.mjs`:
- Line 20: The commitlint rule has added the 'design' scope (the string 'design'
in the commitlint config) but no commits in this PR use it; either remove
'design' from the allowed scopes to keep this PR focused, or leave it and add a
short note in the PR description (or a trailing comment in
commitlint.config.mjs) explaining that 'design' is being added intentionally as
preparatory scope for upcoming design-related work so reviewers understand the
rationale.
In `@services/platform/app/components/ui/forms/searchable-select.tsx`:
- Around line 373-391: The current render always emits Label/Description even
when a custom trigger is provided, causing a broken htmlFor association; update
the render logic in searchable-select.tsx to bypass the default field chrome
when the trigger prop is supplied by returning popover directly (i.e., only
render Label and Description when there is no custom trigger and when
label/description exist), using the existing identifiers trigger, triggerId,
Label, Description, descriptionId and popover to locate and adjust the
condition.
In `@services/platform/app/components/ui/forms/select.tsx`:
- Around line 43-50: The size union is defined as SelectTriggerSize but
SelectProps.size duplicates the union; update SelectProps (the prop
type/interface that defines size) to reuse SelectTriggerSize instead of
repeating 'default' | 'sm' | 'lg' so the types stay in sync (e.g., change the
size type annotation on SelectProps to SelectTriggerSize), and run a quick
typecheck to ensure no other places relied on the duplicated literal union.
In `@services/platform/app/components/ui/navigation/mobile-navigation.tsx`:
- Line 141: The mobile drawer container in the MobileNavigation component is
using the wrong background token (bg-background) which causes contrast issues in
dark theme; update the container element that currently has
className="bg-background flex h-full w-full max-w-none flex-col" to use the
sidebar surface token by replacing bg-background with bg-sidebar so the drawer
renders with the correct surface contrast.
In
`@services/platform/app/features/settings/governance/components/budget-editor.tsx`:
- Around line 555-557: The scope and period table cells render raw enum values
(rule.scope, rule.period) instead of translated labels; update these cells to
use the translation helper used elsewhere (e.g., the t hook or the existing
label-mapping used by the selects) — for example replace {rule.scope} and
{rule.period} with their translated variants (such as t('governance.scopes.' +
rule.scope) and t('governance.periods.' + rule.period) or by calling the same
helper that the Scope/Period select uses) so the table shows localized labels;
ensure you import/use the same translation hook (useTranslation t) or mapping
function used by the select components and keep resolveTarget(rule) as-is.
- Around line 361-383: The optimistic updates to the local `rules` state are
never rolled back when `upsertMutation.mutateAsync` inside saveConfig fails;
capture the previous rules before applying optimistic changes (in the handlers
that call saveConfig), pass that snapshot into saveConfig (or have the handlers
call saveConfig with a rollback callback), and in the catch block of saveConfig
restore the previous rules via the state setter (e.g., setRules(prevRules)) in
addition to showing the error toast so the UI stays consistent with the
persisted `budgets` policy.
- Around line 558-570: The code uses banned locale-aware methods and a local
formatCost(); replace calls to rule.maxTokens.toLocaleString() and
rule.maxRequests.toLocaleString() with formatNumber(rule.maxTokens) and
formatNumber(rule.maxRequests), replace formatCost(rule.maxCostCents) with
formatCostCents(rule.maxCostCents), import formatNumber and formatCostCents from
"@/lib/utils/format/number", and remove the local formatCost() implementation so
the component (budget-editor.tsx) consistently uses the shared utilities.
In
`@services/platform/app/features/settings/governance/components/feature-flags-editor.tsx`:
- Line 523: Replace the raw enum rendering of rule.scope with the localized
label from featureFlags.scopeLabels using the component's translation hook (the
same pattern used in the dialog). In the FeatureFlagsEditor component
(feature-flags-editor.tsx) where the cell currently renders rule.scope, call the
translation function (t) for the key under featureFlags.scopeLabels keyed by
rule.scope (and optionally fall back to rule.scope if translation is missing) so
no hardcoded enum values are shown.
- Around line 349-357: The optimistic updates in removeRule and handleDialogSave
currently mutate the local rules state before the persistence call and only show
a toast on error; capture the previous rules snapshot (e.g., const prevRules =
rules) before mutating, perform the optimistic update, then in each catch block
(the catch in handleDialogSave and the removal catch paths noted around the
other blocks) call setRules(prevRules) to revert the local state and then show
the existing toast; apply the same pattern to the other similar mutation
locations (the catch blocks around lines referenced 362-367 and 386-397) to
ensure local state stays in sync when the backend write fails.
In
`@services/platform/app/features/settings/governance/components/login-policy-editor.tsx`:
- Around line 186-207: The upsert in handleToggleEnabled currently uses
savedConfig which overwrites any unsaved form edits; change it to send the
current draft state instead of savedConfig when calling
upsertMutation.mutateAsync (i.e., include the live form/draft config object plus
enabled: next). Update the payload passed to upsertMutation.mutateAsync in
handleToggleEnabled to reference the form's draft variable (the component's
draft/config state) rather than savedConfig, keeping the same enabled toggle
logic and the existing error revert/setEnabled behavior.
- Around line 163-172: The toast calls in login-policy-editor.tsx are using
missing i18n keys t('loginPolicy.saved') and t('loginPolicy.saveFailed'); either
update the toast calls to use the existing keys (e.g. replace the
title/description uses with the defined keys like 'governance.toastSavedTitle'
and 'governance.toastSaveFailedTitle' where appropriate) or add the missing keys
('governance.loginPolicy.saved' and 'governance.loginPolicy.saveFailed') to the
messages bundle; locate the toast block in the save handler in the
LoginPolicyEditor component and make the key names consistent with the messages
bundle.
- Around line 274-279: Replace hardcoded English placeholders in the
LoginPolicyEditor JSX with translation strings via the i18n hook: change the
Input placeholder "1, 10, 60, 600" and any other literal placeholders (e.g.,
"loopback, uniquelocal, 10.0.0.0/8") to use t('...') keys (add new keys like
loginPolicy.placeholder.backoffSchedule and loginPolicy.placeholder.allowedCIDRs
or similar) and update any related Inputs in the same component
(login-policy-editor.tsx) so all user-facing literals are pulled from t instead
of hardcoded text.
In
`@services/platform/app/features/settings/governance/components/retention-editor.tsx`:
- Around line 232-263: Replace all hardcoded user-facing strings inside the Chat
History RetentionSection with the translation hook (t). In the RetentionSection
props use t('retentionPolicy.chatHistory.title') and
t('retentionPolicy.chatHistory.description'); for the Input component replace
"Retention Days" with t('retentionPolicy.chatHistory.retentionDaysLabel'), the
placeholder "e.g. 90" with t('retentionPolicy.chatHistory.placeholder'), and the
helper Text with t('retentionPolicy.chatHistory.helper'); ensure the component
has access to the translation function (e.g., call/use the same t hook used
elsewhere in this file) and pass translated strings into RetentionSection,
Input, and Text instead of literals.
- Around line 83-89: The Switch in the RetentionSection component currently uses
a hardcoded label "Enabled"; update RetentionSection to accept an enabledLabel
prop (or call the translation hook inside RetentionSection) and replace the
literal with that prop (used where the Switch is rendered), then update each
caller to pass enabledLabel={t('retentionPolicy.enabled')} so all user-facing
text uses the i18n translation key instead of a hardcoded string.
In
`@services/platform/app/features/settings/governance/components/upload-policy-editor.tsx`:
- Around line 161-205: The skeleton uses the component state variable enabled to
decide whether to render the full form skeleton, but during initial load enabled
is false so the skeleton only shows the header; update the rendering logic in
upload-policy-editor.tsx so the form skeleton is shown while isLoading (e.g.
change the conditional from {enabled && ...} to {enabled || isLoading && ...} or
introduce a separate skeletonEnabled flag set to true during loading) so the
complete skeleton UI (variable skeleton) matches the expected enabled layout
during isLoading; adjust any related state checks around enabled/isLoading to
avoid visual flicker when real data arrives.
In `@services/platform/app/features/settings/governance/hooks/queries.ts`:
- Around line 146-149: The allowedTypes return uses config.allowedMimeTypes
verbatim which can fail exact includes checks if entries have mixed case or
extra whitespace; update the logic in the allowedTypes computation (where
config.allowedMimeTypes and CHAT_UPLOAD_ALLOWED_TYPES are used) to normalize all
MIME types by trimming and lowercasing each string (and remove empty entries and
duplicates) before returning, ensuring both the config.allowedMimeTypes branch
and the fallback ([...CHAT_UPLOAD_ALLOWED_TYPES]) produce a cleaned, normalized
array.
In `@services/platform/app/routes/dashboard/`$id.tsx:
- Line 135: Replace the use of the background token on the dashboard nav shells
with the sidebar token: find the JSX elements whose className contains
"bg-background flex h-[--nav-size] items-center gap-2 p-2 md:hidden" (and the
similar sibling at the nearby nav container) and change their surface token from
bg-background to bg-sidebar so the nav containers use the lighter --sidebar
surface in dark mode; update both occurrences to keep all other utility classes
unchanged.
In `@services/platform/messages/de.json`:
- Around line 4015-4016: French translations file is missing 13 governance keys
present in en.json (restore parity): add the keys groups.subtabs.twoFactor and
the twoFactorPolicy subtree keys (title, description, enforced, exemptSsoUsers,
exemptSsoUsersHint, gracePeriodDays, gracePeriodDaysHint, invalidGrace,
policyDisabledHint, saved, saveFailed) into fr.json; locate the twoFactor
entries by the unique key names above (e.g., groups.subtabs.twoFactor and
twoFactorPolicy.*) and add them with appropriate French translations or
temporary English placeholders copied from en.json so fr.json matches en/de
structure.
---
Outside diff comments:
In `@services/platform/app/components/ui/feedback/toaster.tsx`:
- Around line 74-79: Replace the hardcoded aria-label on the
ToastPrimitives.Close button with a translated string via the app's i18n hook
(e.g., call the translation hook used in this codebase and pass a key like
"close" or "common.close"); update the Toast component to import/use that hook
(locate the ToastPrimitives.Close usage in toaster.tsx) and set aria-label to
the translated value (ensure the translation key exists or add it to locale
files and keep aria-label fallback safe).
In `@services/platform/messages/fr.json`:
- Around line 3932-4504: fr.json is missing 125 keys required for i18n parity
(notably analytics.usage.* and governance.twoFactorPolicy.* plus
governance.groups.subtabs.twoFactor); restore parity by adding all missing keys
from en.json into services/platform/messages/fr.json with French translations
(or provisional placeholders) preserving the exact key names and JSON
structure—particularly ensure analytics.usage.* entries and
governance.twoFactorPolicy.* and governance.groups.subtabs.twoFactor are present
and match the keys in en.json so the locale file structure matches
de.json/en.json.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 780bce46-9f81-46ff-ac1b-229e43a37e20
📒 Files selected for processing (49)
commitlint.config.mjsservices/platform/app/components/ui/feedback/toaster.tsxservices/platform/app/components/ui/forms/searchable-select.tsxservices/platform/app/components/ui/forms/select.tsxservices/platform/app/components/ui/navigation/mobile-navigation.stories.tsxservices/platform/app/components/ui/navigation/mobile-navigation.tsxservices/platform/app/components/ui/navigation/navigation.stories.tsxservices/platform/app/features/analytics/usage/top-agents-table.tsxservices/platform/app/features/analytics/usage/top-models-table.tsxservices/platform/app/features/analytics/usage/usage-metrics-page.tsxservices/platform/app/features/analytics/usage/usage-summary-cards.tsxservices/platform/app/features/analytics/usage/usage-trend-chart.tsxservices/platform/app/features/analytics/usage/users-table.tsxservices/platform/app/features/chat/components/agent-selector.tsxservices/platform/app/features/chat/components/chat-input.tsxservices/platform/app/features/chat/components/model-selector.tsxservices/platform/app/features/chat/hooks/use-convex-file-upload.tsservices/platform/app/features/documents/components/__tests__/document-upload-dialog.test.tsxservices/platform/app/features/documents/components/document-upload-dialog.tsxservices/platform/app/features/settings/components/settings-navigation.tsxservices/platform/app/features/settings/governance/components/budget-editor.tsxservices/platform/app/features/settings/governance/components/default-model-editor.tsxservices/platform/app/features/settings/governance/components/feature-flags-editor.test.tsxservices/platform/app/features/settings/governance/components/feature-flags-editor.tsxservices/platform/app/features/settings/governance/components/login-policy-editor.tsxservices/platform/app/features/settings/governance/components/model-access-editor.tsxservices/platform/app/features/settings/governance/components/password-policy-editor.tsxservices/platform/app/features/settings/governance/components/pii-config.tsxservices/platform/app/features/settings/governance/components/retention-editor.tsxservices/platform/app/features/settings/governance/components/rules-table-empty-state.tsxservices/platform/app/features/settings/governance/components/select-trigger-button.tsxservices/platform/app/features/settings/governance/components/system-prompt-editor.tsxservices/platform/app/features/settings/governance/components/upload-policy-editor.test.tsxservices/platform/app/features/settings/governance/components/upload-policy-editor.tsxservices/platform/app/features/settings/governance/hooks/queries.tsservices/platform/app/features/settings/governance/hooks/use-upload-policy.tsservices/platform/app/routeTree.gen.tsservices/platform/app/routes/dashboard/$id.tsxservices/platform/app/routes/dashboard/$id/settings/governance.tsxservices/platform/app/routes/dashboard/$id/settings/governance/content-models.tsxservices/platform/app/routes/dashboard/$id/settings/governance/guardrails.tsxservices/platform/app/routes/dashboard/$id/settings/governance/index.tsxservices/platform/app/routes/dashboard/$id/settings/governance/policies-limits.tsxservices/platform/app/routes/dashboard/$id/settings/governance/route.tsxservices/platform/app/routes/dashboard/$id/settings/governance/security-monitoring.tsxservices/platform/app/routes/dashboard/$id/settings/governance/usage.tsxservices/platform/messages/de.jsonservices/platform/messages/en.jsonservices/platform/messages/fr.json
💤 Files with no reviewable changes (4)
- services/platform/app/features/settings/governance/components/feature-flags-editor.test.tsx
- services/platform/app/features/settings/governance/components/select-trigger-button.tsx
- services/platform/app/routes/dashboard/$id/settings/governance.tsx
- services/platform/app/features/settings/governance/hooks/use-upload-policy.ts
Comment on lines
+373
to
+391
| if (!label && !description) { | ||
| return popover; | ||
| } | ||
|
|
||
| return ( | ||
| <div className="flex flex-col gap-1.5"> | ||
| {label && ( | ||
| <Label htmlFor={triggerId} required={required} error={error}> | ||
| {label} | ||
| </Label> | ||
| )} | ||
| {popover} | ||
| {description && ( | ||
| <Description id={descriptionId} className="text-xs"> | ||
| {description} | ||
| </Description> | ||
| )} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Custom-trigger path should bypass default field chrome.
When trigger is supplied, this branch still renders Label/Description, which contradicts the prop contract and can create a broken htmlFor association (label points to triggerId even if the custom trigger doesn’t use it).
🛠️ Suggested fix
- if (!label && !description) {
+ if (trigger || (!label && !description)) {
return popover;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/forms/searchable-select.tsx` around lines
373 - 391, The current render always emits Label/Description even when a custom
trigger is provided, causing a broken htmlFor association; update the render
logic in searchable-select.tsx to bypass the default field chrome when the
trigger prop is supplied by returning popover directly (i.e., only render Label
and Description when there is no custom trigger and when label/description
exist), using the existing identifiers trigger, triggerId, Label, Description,
descriptionId and popover to locate and adjust the condition.
Comment on lines
+43
to
+50
| export type SelectTriggerSize = 'default' | 'sm' | 'lg'; | ||
|
|
||
| export function selectTriggerClasses({ | ||
| size = 'default', | ||
| error, | ||
| }: { | ||
| size?: SelectTriggerSize; | ||
| error?: boolean; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unify trigger size typing to avoid drift.
SelectTriggerSize is now the canonical size type, but SelectProps.size still duplicates the union. Reuse the alias so both stay in sync.
♻️ Suggested diff
interface SelectProps extends Omit<
ComponentPropsWithoutRef<typeof SelectPrimitive.Root>,
'children'
> {
@@
/** Size of the select */
- size?: 'default' | 'sm' | 'lg';
+ size?: SelectTriggerSize;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/forms/select.tsx` around lines 43 - 50,
The size union is defined as SelectTriggerSize but SelectProps.size duplicates
the union; update SelectProps (the prop type/interface that defines size) to
reuse SelectTriggerSize instead of repeating 'default' | 'sm' | 'lg' so the
types stay in sync (e.g., change the size type annotation on SelectProps to
SelectTriggerSize), and run a quick typecheck to ensure no other places relied
on the duplicated literal union.
| <NavigationMenu | ||
| aria-label={t('aria.mainNavigation')} | ||
| className="bg-sidebar flex h-full w-full max-w-none flex-col" | ||
| className="bg-background flex h-full w-full max-w-none flex-col" |
There was a problem hiding this comment.
Revert mobile drawer container to bg-sidebar for correct surface contrast.
Line 141 uses bg-background, which collapses the drawer surface into page background in dark theme. The nav container should use the sidebar token.
Suggested fix
- className="bg-background flex h-full w-full max-w-none flex-col"
+ className="bg-sidebar flex h-full w-full max-w-none flex-col"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| className="bg-background flex h-full w-full max-w-none flex-col" | |
| className="bg-sidebar flex h-full w-full max-w-none flex-col" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/navigation/mobile-navigation.tsx` at line
141, The mobile drawer container in the MobileNavigation component is using the
wrong background token (bg-background) which causes contrast issues in dark
theme; update the container element that currently has className="bg-background
flex h-full w-full max-w-none flex-col" to use the sidebar surface token by
replacing bg-background with bg-sidebar so the drawer renders with the correct
surface contrast.
Comment on lines
361
to
+383
| const saveConfig = useCallback( | ||
| async (configToSave: { enabled: boolean; rules: BudgetRule[] }) => { | ||
| async (nextRules: BudgetRule[]) => { | ||
| try { | ||
| await upsertMutation.mutateAsync({ | ||
| organizationId, | ||
| policyType: 'budgets', | ||
| config: configToSave, | ||
| // `enabled` is always true from the UI — rules presence drives | ||
| // enforcement (server short-circuits on `!enabled || rules.length === 0`). | ||
| config: { enabled: true, rules: nextRules }, | ||
| }); | ||
| toast({ | ||
| title: t('toastSavedTitle'), | ||
| description: t('budgets.saved'), | ||
| variant: 'success', | ||
| }); | ||
| toast({ title: t('budgets.saved'), variant: 'success' }); | ||
| } catch (error: unknown) { | ||
| const message = | ||
| error instanceof Error ? error.message : 'Failed to save'; | ||
| toast({ title: message, variant: 'destructive' }); | ||
| toast({ | ||
| title: t('toastSaveFailedTitle'), | ||
| description: message, | ||
| variant: 'destructive', | ||
| }); |
There was a problem hiding this comment.
Roll back optimistic rule changes when the mutation fails.
Both rule handlers update rules immediately and then fire saveConfig, but saveConfig only toasts on error. A rejected write leaves this table out of sync with the persisted budgets policy until the page refetches.
Also applies to: 389-395, 413-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/governance/components/budget-editor.tsx`
around lines 361 - 383, The optimistic updates to the local `rules` state are
never rolled back when `upsertMutation.mutateAsync` inside saveConfig fails;
capture the previous rules before applying optimistic changes (in the handlers
that call saveConfig), pass that snapshot into saveConfig (or have the handlers
call saveConfig with a rollback callback), and in the catch block of saveConfig
restore the previous rules via the state setter (e.g., setRules(prevRules)) in
addition to showing the error toast so the UI stays consistent with the
persisted `budgets` policy.
Comment on lines
+232
to
+263
| <RetentionSection | ||
| title="Chat History" | ||
| description="Automatically delete chat threads (conversations with agents) older than the retention period. Message contents in the agent component are archived. Deletion is irreversible." | ||
| action={ | ||
| <Switch | ||
| label="Enabled" | ||
| checked={chatHistoryEnabled} | ||
| onCheckedChange={(checked) => { | ||
| setChatHistoryEnabled(checked); | ||
| void saveConfig({ chatHistoryEnabled: checked }); | ||
| }} | ||
| disabled={cannotManage || upsertMutation.isPending} | ||
| /> | ||
| } | ||
| enabled={chatHistoryEnabled} | ||
| toggleDisabled={toggleDisabled} | ||
| onToggle={(checked) => { | ||
| setChatHistoryEnabled(checked); | ||
| void saveConfig({ chatHistoryEnabled: checked }); | ||
| }} | ||
| > | ||
| <div | ||
| className={cn( | ||
| 'transition-opacity duration-200', | ||
| !chatHistoryEnabled && 'pointer-events-none opacity-50', | ||
| )} | ||
| > | ||
| <div className="max-w-xs"> | ||
| <Input | ||
| label="Retention Days" | ||
| type="number" | ||
| value={chatHistoryRetentionDays} | ||
| onChange={(e) => | ||
| setChatHistoryRetentionDays( | ||
| e.target.value ? Number(e.target.value) : 0, | ||
| ) | ||
| } | ||
| onBlur={() => void saveConfig({ chatHistoryRetentionDays })} | ||
| disabled={cannotManage || !chatHistoryEnabled} | ||
| size="sm" | ||
| placeholder="e.g. 90" | ||
| min={1} | ||
| max={3650} | ||
| /> | ||
| <Text className="text-muted-foreground mt-1 text-xs"> | ||
| Chat threads older than this (by last-updated time) will be | ||
| deleted. | ||
| </Text> | ||
| </div> | ||
| <div className="max-w-xs"> | ||
| <Input | ||
| label="Retention Days" | ||
| type="number" | ||
| value={chatHistoryRetentionDays} | ||
| onChange={(e) => | ||
| setChatHistoryRetentionDays( | ||
| e.target.value ? Number(e.target.value) : 0, | ||
| ) | ||
| } | ||
| onBlur={() => void saveConfig({ chatHistoryRetentionDays })} | ||
| disabled={cannotManage} | ||
| size="sm" | ||
| placeholder="e.g. 90" | ||
| min={1} | ||
| max={3650} | ||
| /> | ||
| <Text className="text-muted-foreground mt-1 text-xs"> | ||
| Chat threads older than this (by last-updated time) will be deleted. | ||
| </Text> | ||
| </div> | ||
| </PageSection> | ||
| </RetentionSection> |
There was a problem hiding this comment.
Multiple hardcoded English strings violate i18n guidelines.
Section titles, descriptions, and input labels are hardcoded English strings. For example:
- Line 233:
"Chat History" - Line 234:
"Automatically delete chat threads..." - Line 244:
"Retention Days" - Line 259-260:
"Chat threads older than this..."
These should all use the translation hook (e.g., t('retentionPolicy.chatHistory.title')).
As per coding guidelines: "No hardcoded user-facing strings. Always the translation hook. A stray English literal in JSX is a bug."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/governance/components/retention-editor.tsx`
around lines 232 - 263, Replace all hardcoded user-facing strings inside the
Chat History RetentionSection with the translation hook (t). In the
RetentionSection props use t('retentionPolicy.chatHistory.title') and
t('retentionPolicy.chatHistory.description'); for the Input component replace
"Retention Days" with t('retentionPolicy.chatHistory.retentionDaysLabel'), the
placeholder "e.g. 90" with t('retentionPolicy.chatHistory.placeholder'), and the
helper Text with t('retentionPolicy.chatHistory.helper'); ensure the component
has access to the translation function (e.g., call/use the same t hook used
elsewhere in this file) and pass translated strings into RetentionSection,
Input, and Text instead of literals.
Comment on lines
+161
to
205
| const skeleton = ( | ||
| <div className="flex flex-col gap-6"> | ||
| <div className="flex items-center justify-between gap-4"> | ||
| <div className="flex flex-col gap-1"> | ||
| <Skeleton className="h-6 w-32" /> | ||
| <Skeleton className="h-4 w-80 max-w-full" /> | ||
| </div> | ||
| <div className="flex shrink-0 items-center gap-3"> | ||
| <Skeleton className="h-3.5 w-14" /> | ||
| <Skeleton className="h-[1.15rem] w-8 rounded-full" /> | ||
| </div> | ||
| </div> | ||
| ); | ||
| {enabled && ( | ||
| <div className="flex max-w-2xl flex-col gap-6"> | ||
| <div className="flex flex-col gap-4"> | ||
| <div className="grid grid-cols-1 gap-4 md:grid-cols-2"> | ||
| {[0, 1].map((i) => ( | ||
| <div key={i} className="flex flex-col gap-2.5"> | ||
| <Skeleton className="h-4 w-32" /> | ||
| <Skeleton className="h-8 w-full rounded-md" /> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| <div className="flex flex-col gap-2.5"> | ||
| <Skeleton className="h-4 w-32" /> | ||
| <Skeleton className="h-8 w-full rounded-md" /> | ||
| </div> | ||
| <div className="grid grid-cols-1 gap-4 md:grid-cols-2"> | ||
| {[0, 1].map((i) => ( | ||
| <div key={i} className="flex flex-col gap-2.5"> | ||
| <Skeleton className="h-4 w-32" /> | ||
| <Skeleton className="h-8 w-full rounded-md" /> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| <Skeleton className="h-8 w-20 rounded-md" /> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ); | ||
|
|
||
| if (isLoading) { | ||
| return <div aria-busy="true">{skeleton}</div>; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Skeleton always shows minimal state during loading.
The skeleton references enabled state (line 173), but during loading enabled is always false (default from line 62), so the form-fields skeleton block never renders. This results in a minimal header-only skeleton during initial load.
If this is intentional (showing a compact skeleton before knowing the policy state), this is fine. If you want the skeleton to match the enabled state's visual structure, consider showing the full form skeleton unconditionally or adding a separate skeletonEnabled flag.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/governance/components/upload-policy-editor.tsx`
around lines 161 - 205, The skeleton uses the component state variable enabled
to decide whether to render the full form skeleton, but during initial load
enabled is false so the skeleton only shows the header; update the rendering
logic in upload-policy-editor.tsx so the form skeleton is shown while isLoading
(e.g. change the conditional from {enabled && ...} to {enabled || isLoading &&
...} or introduce a separate skeletonEnabled flag set to true during loading) so
the complete skeleton UI (variable skeleton) matches the expected enabled layout
during isLoading; adjust any related state checks around enabled/isLoading to
avoid visual flicker when real data arrives.
Comment on lines
+146
to
+149
| allowedTypes: | ||
| config.allowedMimeTypes && config.allowedMimeTypes.length > 0 | ||
| ? config.allowedMimeTypes | ||
| : [...CHAT_UPLOAD_ALLOWED_TYPES], |
There was a problem hiding this comment.
Normalize configured MIME types before returning allowedTypes.
allowedMimeTypes is returned verbatim, but upload checks are exact includes(...). Mixed/upper-case policy entries can incorrectly block valid files.
Suggested fix
allowedTypes:
config.allowedMimeTypes && config.allowedMimeTypes.length > 0
- ? config.allowedMimeTypes
+ ? config.allowedMimeTypes.map((type) => type.toLowerCase().trim())
: [...CHAT_UPLOAD_ALLOWED_TYPES],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| allowedTypes: | |
| config.allowedMimeTypes && config.allowedMimeTypes.length > 0 | |
| ? config.allowedMimeTypes | |
| : [...CHAT_UPLOAD_ALLOWED_TYPES], | |
| allowedTypes: | |
| config.allowedMimeTypes && config.allowedMimeTypes.length > 0 | |
| ? config.allowedMimeTypes.map((type) => type.toLowerCase().trim()) | |
| : [...CHAT_UPLOAD_ALLOWED_TYPES], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/settings/governance/hooks/queries.ts` around
lines 146 - 149, The allowedTypes return uses config.allowedMimeTypes verbatim
which can fail exact includes checks if entries have mixed case or extra
whitespace; update the logic in the allowedTypes computation (where
config.allowedMimeTypes and CHAT_UPLOAD_ALLOWED_TYPES are used) to normalize all
MIME types by trimming and lowercasing each string (and remove empty entries and
duplicates) before returning, ensuring both the config.allowedMimeTypes branch
and the fallback ([...CHAT_UPLOAD_ALLOWED_TYPES]) produce a cleaned, normalized
array.
| <AdaptiveHeaderProvider> | ||
| <div className="flex size-full flex-col overflow-hidden md:flex-row"> | ||
| <div className="bg-sidebar flex h-[--nav-size] items-center gap-2 p-2 md:hidden"> | ||
| <div className="bg-background flex h-[--nav-size] items-center gap-2 p-2 md:hidden"> |
There was a problem hiding this comment.
Use bg-sidebar for dashboard nav containers (dark theme regression).
Lines 135 and 140 changed sidebar shells to bg-background, but theme tokens define a dedicated --sidebar surface that is lighter than --background in dark mode. This makes nav and content blend and weakens layout affordance.
Suggested fix
- <div className="bg-background flex h-[--nav-size] items-center gap-2 p-2 md:hidden">
+ <div className="bg-sidebar flex h-[--nav-size] items-center gap-2 p-2 md:hidden">
...
- <div className="bg-background hidden h-full px-2 md:flex md:flex-[0_0_var(--nav-size)]">
+ <div className="bg-sidebar hidden h-full px-2 md:flex md:flex-[0_0_var(--nav-size)]">Also applies to: 140-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/routes/dashboard/`$id.tsx at line 135, Replace the use
of the background token on the dashboard nav shells with the sidebar token: find
the JSX elements whose className contains "bg-background flex h-[--nav-size]
items-center gap-2 p-2 md:hidden" (and the similar sibling at the nearby nav
container) and change their surface token from bg-background to bg-sidebar so
the nav containers use the lighter --sidebar surface in dark mode; update both
occurrences to keep all other utility classes unchanged.
Comment on lines
+4015
to
+4016
| "toastSavedTitle": "Änderungen erfolgreich gespeichert", | ||
| "toastSaveFailedTitle": "Änderungen konnten nicht gespeichert werden", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json
from pathlib import Path
base = Path("services/platform/messages")
paths = {
"en": base / "en.json",
"de": base / "de.json",
"fr": base / "fr.json",
}
def flatten(obj, prefix=""):
out = set()
if isinstance(obj, dict):
for k, v in obj.items():
p = f"{prefix}.{k}" if prefix else k
out.add(p)
out |= flatten(v, p)
return out
data = {}
for loc, p in paths.items():
with p.open("r", encoding="utf-8") as f:
root = json.load(f)
data[loc] = flatten(root.get("governance", {}))
en_keys = data["en"]
for loc in ("de", "fr"):
missing = sorted(en_keys - data[loc])
extra = sorted(data[loc] - en_keys)
print(f"\n== {loc} vs en (governance subtree) ==")
print(f"missing: {len(missing)}")
if missing:
print("\n".join(missing[:200]))
print(f"extra: {len(extra)}")
if extra:
print("\n".join(extra[:200]))
# fail if any required keys are missing
has_missing = any((en_keys - data[loc]) for loc in ("de", "fr"))
raise SystemExit(1 if has_missing else 0)
PYRepository: tale-project/tale
Length of output: 538
Add 13 missing French governance keys to restore en/de/fr parity.
French (fr.json) is missing 13 keys from the governance subtree that exist in en.json: groups.subtabs.twoFactor, twoFactorPolicy.title, twoFactorPolicy.description, twoFactorPolicy.enforced, twoFactorPolicy.exemptSsoUsers, twoFactorPolicy.exemptSsoUsersHint, twoFactorPolicy.gracePeriodDays, twoFactorPolicy.gracePeriodDaysHint, twoFactorPolicy.invalidGrace, twoFactorPolicy.policyDisabledHint, twoFactorPolicy.saved, twoFactorPolicy.saveFailed. German alignment is correct. Update fr.json to include these keys.
Also applies to: 4058-4059, 4098-4099, 4216-4217, 4549-4549, 4579-4580
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/messages/de.json` around lines 4015 - 4016, French
translations file is missing 13 governance keys present in en.json (restore
parity): add the keys groups.subtabs.twoFactor and the twoFactorPolicy subtree
keys (title, description, enforced, exemptSsoUsers, exemptSsoUsersHint,
gracePeriodDays, gracePeriodDaysHint, invalidGrace, policyDisabledHint, saved,
saveFailed) into fr.json; locate the twoFactor entries by the unique key names
above (e.g., groups.subtabs.twoFactor and twoFactorPolicy.*) and add them with
appropriate French translations or temporary English placeholders copied from
en.json so fr.json matches en/de structure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Settings → Governancefrom a single-file Tabs layout to a nested-route layout with a persistent left sidebar. Each group (Content & Models, Policies & Limits, Security & Monitoring, Guardrails, Usage) is now its own route file underroutes/dashboard/$id/settings/governance/.useUploadPolicy/usePiiConfiginto the sharedgovernance/hooks/queries.tsand removes the standaloneuse-upload-policy.ts.rules-table-empty-statecomponent.groups.usagekey inen/de/fralongside the existinggroups.guardrails.Test plan
bunx tsc --noEmitpasses/dashboard/<id>/settings/governance— should redirect to/governance/content-models__tests__) still covered by testsSummary by CodeRabbit
New Features
Improvements