feat(platform): sticky save bars and conditional save buttons in settings#1555
Merged
Conversation
…ings forms Show save/submit buttons only when the form is dirty, reducing visual noise. Add sticky positioning to save bars in account, branding, and org settings so they remain visible while scrolling. Remove redundant pt-2 padding from dialog footers. Update org settings i18n: friendlier locale label, add detailsTitle/detailsDescription/organizationIdHint keys, and improve the organization page skeleton to match the new layout.
The save button is now only rendered when the form is dirty. Update tests to verify it's hidden when clean and shown when dirty.
Contributor
📝 WalkthroughWalkthroughThe PR refactors form and settings components to improve UX patterns. Key changes include: removing top padding utility ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/features/settings/account/components/account-form.tsx`:
- Around line 134-137: The submit Button's disabled predicate redundantly checks
!isDirty even though rendering is already gated by isDirty; update the Button
(component instance using props isSubmitting, isDirty, isValid) to remove the
redundant !isDirty check and use a simplified disabled expression (e.g.,
disabled when isSubmitting OR !isValid) so the prop becomes based only on
isSubmitting and isValid.
In
`@services/platform/app/features/settings/branding/components/__tests__/branding-form.test.tsx`:
- Around line 88-99: Add a test in branding-form.test.tsx that after changing
the BrandingForm input (use the same getByLabelText call used in the existing
test for 'branding.appName' and fireEvent.change) you revert the field back to
its original value and then assert the save button (role 'button', name
'actions.saveChanges') is no longer present; use queryByRole and wrap assertions
in waitFor to avoid timing flakes and reference the BrandingForm render and
isDirty behavior so the test verifies the dirty->clean transition.
In
`@services/platform/app/features/settings/organization/components/organization-settings.tsx`:
- Around line 177-183: The submit Button in organization-settings.tsx is
rendered only when isDirty is true, so the disabled prop should drop the
redundant check (!isDirty); update the Button (the JSX block that uses isDirty
and isSubmitting) to set disabled based solely on isSubmitting (e.g.,
disabled={isSubmitting}) to simplify the logic and avoid unreachable conditions.
In `@services/platform/app/routes/dashboard/`$id/settings/organization.tsx:
- Around line 42-49: The loading skeleton includes UI elements that don't appear
in the real OrganizationSettings (extra Separator components and a persistent
save-button placeholder), causing layout shift; update the loading markup in the
same component so it matches OrganizationSettings: remove the unnecessary
Separator elements and replace the persistent save-button Skeleton (the element
with className "h-9 w-28 shrink-0") with either no placeholder or a conditional
skeleton that only renders when the real form shows a save button (i.e., mimic
the same conditional/save-when-dirty behavior used by OrganizationSettings),
locating these changes around the HStack/Stack skeleton nodes in the
organization settings route.
In `@services/platform/messages/de.json`:
- Line 610: The German locale entry for the "defaultLocale" key uses the word
"Agenten" which conflicts with the repo's guideline to keep "Agent/Agents" in
English; update the string value for defaultLocale to replace "Agenten" with
"Agent" so it reads using the English term while preserving the rest of the
German text (locate the "defaultLocale" key in
services/platform/messages/de.json and change only the word "Agenten" to
"Agent").
🪄 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: d6873fb9-1ae7-4463-8ec3-320e3d7026b7
📒 Files selected for processing (11)
services/platform/app/components/ui/dialog/form-dialog.tsxservices/platform/app/features/automations/components/automation-create-dialog.tsxservices/platform/app/features/settings/account/components/account-form.tsxservices/platform/app/features/settings/branding/components/__tests__/branding-form.test.tsxservices/platform/app/features/settings/branding/components/branding-form.tsxservices/platform/app/features/settings/governance/components/login-policy-editor.tsxservices/platform/app/features/settings/governance/components/system-prompt-editor.tsxservices/platform/app/features/settings/organization/components/organization-settings.tsxservices/platform/app/routes/dashboard/$id/settings/organization.tsxservices/platform/messages/de.jsonservices/platform/messages/en.json
Comment on lines
+134
to
+137
| <Button | ||
| type="submit" | ||
| disabled={isSubmitting || !isDirty || !isValid} | ||
| > |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify the submit button disabled predicate.
Since rendering is already gated by isDirty, the !isDirty condition is redundant.
♻️ Proposed simplification
- disabled={isSubmitting || !isDirty || !isValid}
+ disabled={isSubmitting || !isValid}📝 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
| <Button | |
| type="submit" | |
| disabled={isSubmitting || !isDirty || !isValid} | |
| > | |
| <Button | |
| type="submit" | |
| disabled={isSubmitting || !isValid} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/settings/account/components/account-form.tsx`
around lines 134 - 137, The submit Button's disabled predicate redundantly
checks !isDirty even though rendering is already gated by isDirty; update the
Button (component instance using props isSubmitting, isDirty, isValid) to remove
the redundant !isDirty check and use a simplified disabled expression (e.g.,
disabled when isSubmitting OR !isValid) so the prop becomes based only on
isSubmitting and isValid.
Comment on lines
+88
to
99
| it('shows save button when form is dirty', async () => { | ||
| render(<BrandingForm {...defaultProps} />); | ||
|
|
||
| const input = screen.getByLabelText('branding.appName', { exact: false }); | ||
| fireEvent.change(input, { target: { value: 'Acme Corp' } }); | ||
|
|
||
| const button = screen.getByRole('button', { name: 'actions.saveChanges' }); | ||
| await waitFor(() => { | ||
| expect(button).not.toBeDisabled(); | ||
| expect( | ||
| screen.getByRole('button', { name: 'actions.saveChanges' }), | ||
| ).toBeInTheDocument(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add an edge-case test for reverting back to clean state.
After editing, revert the field to its original value and assert the save button disappears again. This protects isDirty regression behavior.
🧪 Suggested test addition
+ it('hides save button again when changes are reverted', async () => {
+ render(<BrandingForm {...defaultProps} />);
+
+ const input = screen.getByLabelText('branding.appName', { exact: false });
+ fireEvent.change(input, { target: { value: 'Acme Corp' } });
+ await screen.findByRole('button', { name: 'actions.saveChanges' });
+
+ fireEvent.change(input, { target: { value: '' } });
+ await waitFor(() => {
+ expect(
+ screen.queryByRole('button', { name: 'actions.saveChanges' }),
+ ).not.toBeInTheDocument();
+ });
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/branding/components/__tests__/branding-form.test.tsx`
around lines 88 - 99, Add a test in branding-form.test.tsx that after changing
the BrandingForm input (use the same getByLabelText call used in the existing
test for 'branding.appName' and fireEvent.change) you revert the field back to
its original value and then assert the save button (role 'button', name
'actions.saveChanges') is no longer present; use queryByRole and wrap assertions
in waitFor to avoid timing flakes and reference the BrandingForm render and
isDirty behavior so the test verifies the dirty->clean transition.
Comment on lines
+177
to
+183
| {isDirty && ( | ||
| <Button type="submit" disabled={isSubmitting || !isDirty}> | ||
| {isSubmitting | ||
| ? tCommon('actions.saving') | ||
| : tCommon('actions.saveChanges')} | ||
| </Button> | ||
| )} |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove redundant dirty check in submit button disable state.
!isDirty is unnecessary here because the button only renders when isDirty is true.
♻️ Proposed simplification
- <Button type="submit" disabled={isSubmitting || !isDirty}>
+ <Button type="submit" disabled={isSubmitting}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/organization/components/organization-settings.tsx`
around lines 177 - 183, The submit Button in organization-settings.tsx is
rendered only when isDirty is true, so the disabled prop should drop the
redundant check (!isDirty); update the Button (the JSX block that uses isDirty
and isSubmitting) to set disabled based solely on isSubmitting (e.g.,
disabled={isSubmitting}) to simplify the logic and avoid unreachable conditions.
Comment on lines
+42
to
+49
| <Separator /> | ||
| <HStack gap={3} align="end" justify="between"> | ||
| <Stack gap={1} className="max-w-sm flex-1"> | ||
| <Skeleton className="h-4 w-32" /> | ||
| <Skeleton className="h-9 w-full" /> | ||
| </Stack> | ||
| <Skeleton className="h-9 w-28 shrink-0" /> | ||
| </HStack> |
Contributor
There was a problem hiding this comment.
Skeleton layout does not match the loaded organization settings screen.
Line 42 and Line 72 add separators not present in OrganizationSettings, and Line 48 shows a save-button placeholder even though the loaded form only shows save when dirty. This causes avoidable layout shift after loading.
💡 Suggested fix
<PageSection
title={<Skeleton className="h-5 w-40" />}
description={<Skeleton className="h-4 w-64" />}
gap={5}
>
- <Separator />
<HStack gap={3} align="end" justify="between">
<Stack gap={1} className="max-w-sm flex-1">
<Skeleton className="h-4 w-32" />
<Skeleton className="h-9 w-full" />
</Stack>
- <Skeleton className="h-9 w-28 shrink-0" />
</HStack>
@@
<PageSection
title={<Skeleton className="h-5 w-24" />}
description={<Skeleton className="h-4 w-52" />}
>
- <Separator />
<HStack justify="between">
<Skeleton className="h-8 max-w-sm flex-1" />
</HStack>Also applies to: 72-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/routes/dashboard/`$id/settings/organization.tsx around
lines 42 - 49, The loading skeleton includes UI elements that don't appear in
the real OrganizationSettings (extra Separator components and a persistent
save-button placeholder), causing layout shift; update the loading markup in the
same component so it matches OrganizationSettings: remove the unnecessary
Separator elements and replace the persistent save-button Skeleton (the element
with className "h-9 w-28 shrink-0") with either no placeholder or a conditional
skeleton that only renders when the real form shows a save button (i.e., mimic
the same conditional/save-when-dirty behavior used by OrganizationSettings),
locating these changes around the HStack/Stack skeleton nodes in the
organization settings route.
| "manageAccess": "Zugriff auf die Organisation verwalten", | ||
| "defaultLocale": "Standardsprache", | ||
| "defaultLocaleDescription": "Die Basissprache für Agentennamen, Beschreibungen und Gesprächseinstiege.", | ||
| "defaultLocale": "Welche Sprache sollen deine Agenten verwenden?", |
Contributor
There was a problem hiding this comment.
Use repo-standard “Agents” terminology in German locale.
Line 610 uses Agenten, but this codebase’s de locale standard keeps Agent/Agents in English for consistency.
💡 Suggested fix
- "defaultLocale": "Welche Sprache sollen deine Agenten verwenden?",
+ "defaultLocale": "Welche Sprache sollen deine Agents verwenden?",Based on learnings: German (de) terminology guideline says to use “Agent” for Agent and keep it in English.
📝 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
| "defaultLocale": "Welche Sprache sollen deine Agenten verwenden?", | |
| "defaultLocale": "Welche Sprache sollen deine Agents verwenden?", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/messages/de.json` at line 610, The German locale entry for
the "defaultLocale" key uses the word "Agenten" which conflicts with the repo's
guideline to keep "Agent/Agents" in English; update the string value for
defaultLocale to replace "Agenten" with "Agent" so it reads using the English
term while preserving the rest of the German text (locate the "defaultLocale"
key in services/platform/messages/de.json and change only the word "Agenten" to
"Agent").
Since the save buttons are now only rendered when isDirty is true, the !isDirty condition in the disabled prop is always false and can be removed.
This was referenced Apr 27, 2026
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
pt-2padding from dialog footers (form-dialog, automation-create)detailsTitle,detailsDescription,organizationIdHintkeys (en + de)Test plan
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes & Improvements