feat: warn before saving key names with leading/trailing whitespace - #3756
Merged
Conversation
Show a confirmation dialog when saving a key whose name has outer whitespace. On create it triggers whenever the name has leading/trailing spaces; on edit only when such whitespace is newly introduced, so keys that already had it are not flagged.
The highlight used a single hardcoded light-blue that washed out on the light theme. Use the theme's label.lightBlue token so it adapts to both modes.
Mirror the project settings behaviour so a new project's name has leading/trailing whitespace stripped before saving, and validation runs against the trimmed value.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds UI-level detection and visualization of leading/trailing whitespace in translation key names. A new ChangesKey Name Whitespace Warning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/views/projects/translations/KeyCreateForm/FormBody.tsx (1)
311-319:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent native submit from bypassing the whitespace guard.
LoadingButtonis stilltype="submit"while the check lives inonClick. When outer whitespace is detected,submitWithWhitespaceCheck()returns early, but the form submit can still fire and persist immediately. Make this buttontype="button"(or prevent default) and route submission only throughsubmitWithWhitespaceCheck().Suggested fix
<LoadingButton data-cy="global-form-save-button" loading={form.isSubmitting} color="primary" variant="contained" disabled={!form.isValid || isBaseOverCharLimit} - type="submit" - onClick={() => submitWithWhitespaceCheck()} + type="button" + onClick={submitWithWhitespaceCheck} >🤖 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 `@webapp/src/views/projects/translations/KeyCreateForm/FormBody.tsx` around lines 311 - 319, The LoadingButton component has type="submit" which triggers native form submission independent of the onClick handler, allowing the form to persist even when submitWithWhitespaceCheck() returns early. Change the button type from "submit" to "button" so that form submission is routed exclusively through the onClick handler that invokes submitWithWhitespaceCheck(), ensuring the whitespace guard cannot be bypassed.
🤖 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.
Inline comments:
In `@e2e/cypress/e2e/translations/keyNameWhitespace.cy.ts`:
- Around line 18-20: Replace the `translationSingleTestData.generate()` call
with the standardized `generateStandard()` API. Update the test data generation
pattern to follow the shared test-data object pattern as required for E2E tests
in the e2e/cypress directory. This means replacing the outdated generate()
method with the generateStandard() method to align with current coding
guidelines.
- Line 67: The assertion at line 67 uses `cy.contains('A key edited')` which is
a brittle text-based selector that violates the data-cy selector guidelines.
Replace this assertion with a stable selector using a `data-cy` attribute and
the `gcy()` or `cy.gcy()` helper function. First ensure the element being tested
has an appropriate `data-cy` attribute (e.g., `data-cy="key-edited"` or
similar), then replace the `cy.contains('A key edited')` call with
`cy.gcy('appropriate-data-cy-value').should('be.visible')` to follow the
required coding guidelines for E2E selectors.
In `@webapp/src/views/projects/translations/KeyEdit/KeyEditModal.tsx`:
- Around line 288-290: The onConfirm callback in the whitespace confirmation
dialog calls submitForm() but does not close the warning dialog first. This
causes the dialog to remain open if the form submission fails. Modify the
onConfirm callback to call setWhitespaceWarningOpen(false) before invoking
submitForm() to ensure the dialog is properly closed before attempting to
resubmit the form.
---
Outside diff comments:
In `@webapp/src/views/projects/translations/KeyCreateForm/FormBody.tsx`:
- Around line 311-319: The LoadingButton component has type="submit" which
triggers native form submission independent of the onClick handler, allowing the
form to persist even when submitWithWhitespaceCheck() returns early. Change the
button type from "submit" to "button" so that form submission is routed
exclusively through the onClick handler that invokes
submitWithWhitespaceCheck(), ensuring the whitespace guard cannot be bypassed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47c50cc5-6db3-43fe-9059-179f9b84cac3
📒 Files selected for processing (9)
e2e/cypress/e2e/translations/keyNameWhitespace.cy.tswebapp/src/component/editor/Editor.tsxwebapp/src/component/editor/utils/codemirrorVisibleWhitespace.tswebapp/src/constants/GlobalValidationSchema.tsxwebapp/src/fixtures/keyName.tswebapp/src/views/projects/project/ProjectCreateView.tsxwebapp/src/views/projects/project/ProjectSettingsGeneral.tsxwebapp/src/views/projects/translations/KeyCreateForm/FormBody.tsxwebapp/src/views/projects/translations/KeyEdit/KeyEditModal.tsx
dkrizan
marked this pull request as draft
June 17, 2026 10:12
Instead of a save-time confirmation dialog, the key create and edit dialogs now show a non-blocking inline warning under the name field whenever it has leading/trailing spaces, with a Trim action that removes them. Nothing is auto-trimmed without the user's action.
Show the inline warning in the same reserved message slot as the field error (no layout shift when it appears), and use the MUI Link component for the Trim action so it follows the design system.
dkrizan
marked this pull request as ready for review
June 17, 2026 13:39
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@webapp/src/views/projects/translations/KeyCreateForm/FormBody.tsx`:
- Around line 316-318: The button element with type="submit" is triggering form
submission twice: once automatically through the submit button type, and again
through the explicit onClick handler calling form.handleSubmit(). Remove the
onClick handler from the button since the type="submit" attribute on the button
will automatically invoke form submission without needing the explicit
handleSubmit call.
In `@webapp/src/views/projects/translations/KeyEdit/KeyGeneral.tsx`:
- Around line 79-86: The KeyNameWhitespaceWarning component currently displays
whenever errors.name is falsy and values.name contains outer whitespace, but it
doesn't distinguish between pre-existing whitespace and newly introduced
whitespace. To fix this, modify the condition that determines when to show
KeyNameWhitespaceWarning to also compare the current values.name against its
initial or original value. Only display the warning when outer whitespace is
newly added (i.e., when the current values.name has whitespace but the original
value did not), not when the whitespace was already present before editing. This
requires accessing the original name value from the form state and incorporating
that into the conditional logic alongside the existing errors.name check.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 030c5078-3653-4c9e-adf9-5de7b7d0dcd7
📒 Files selected for processing (6)
e2e/cypress/e2e/translations/keyNameWhitespace.cy.tse2e/cypress/support/dataCyType.d.tswebapp/src/fixtures/keyName.tswebapp/src/views/projects/translations/KeyCreateForm/FormBody.tsxwebapp/src/views/projects/translations/KeyEdit/KeyGeneral.tsxwebapp/src/views/projects/translations/KeyNameWhitespaceWarning.tsx
💤 Files with no reviewable changes (1)
- webapp/src/fixtures/keyName.ts
✅ Files skipped from review due to trivial changes (2)
- webapp/src/views/projects/translations/KeyNameWhitespaceWarning.tsx
- e2e/cypress/support/dataCyType.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/cypress/e2e/translations/keyNameWhitespace.cy.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@webapp/src/views/projects/translations/KeyCreateForm/FormBody.tsx`:
- Around line 316-318: The button element with type="submit" is triggering form
submission twice: once automatically through the submit button type, and again
through the explicit onClick handler calling form.handleSubmit(). Remove the
onClick handler from the button since the type="submit" attribute on the button
will automatically invoke form submission without needing the explicit
handleSubmit call.
In `@webapp/src/views/projects/translations/KeyEdit/KeyGeneral.tsx`:
- Around line 79-86: The KeyNameWhitespaceWarning component currently displays
whenever errors.name is falsy and values.name contains outer whitespace, but it
doesn't distinguish between pre-existing whitespace and newly introduced
whitespace. To fix this, modify the condition that determines when to show
KeyNameWhitespaceWarning to also compare the current values.name against its
initial or original value. Only display the warning when outer whitespace is
newly added (i.e., when the current values.name has whitespace but the original
value did not), not when the whitespace was already present before editing. This
requires accessing the original name value from the form state and incorporating
that into the conditional logic alongside the existing errors.name check.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 030c5078-3653-4c9e-adf9-5de7b7d0dcd7
📒 Files selected for processing (6)
e2e/cypress/e2e/translations/keyNameWhitespace.cy.tse2e/cypress/support/dataCyType.d.tswebapp/src/fixtures/keyName.tswebapp/src/views/projects/translations/KeyCreateForm/FormBody.tsxwebapp/src/views/projects/translations/KeyEdit/KeyGeneral.tsxwebapp/src/views/projects/translations/KeyNameWhitespaceWarning.tsx
💤 Files with no reviewable changes (1)
- webapp/src/fixtures/keyName.ts
✅ Files skipped from review due to trivial changes (2)
- webapp/src/views/projects/translations/KeyNameWhitespaceWarning.tsx
- e2e/cypress/support/dataCyType.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/cypress/e2e/translations/keyNameWhitespace.cy.ts
🛑 Comments failed to post (2)
webapp/src/views/projects/translations/KeyCreateForm/FormBody.tsx (1)
316-318:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid double submit invocation on save action.
Line 316 already submits via
type="submit", and Line 317 callsform.handleSubmit()again. In a form context this can fire submit twice and duplicate create requests.Suggested fix
<LoadingButton data-cy="global-form-save-button" loading={form.isSubmitting} color="primary" variant="contained" disabled={!form.isValid || isBaseOverCharLimit} type="submit" - onClick={() => form.handleSubmit()} >📝 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.type="submit" >🤖 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 `@webapp/src/views/projects/translations/KeyCreateForm/FormBody.tsx` around lines 316 - 318, The button element with type="submit" is triggering form submission twice: once automatically through the submit button type, and again through the explicit onClick handler calling form.handleSubmit(). Remove the onClick handler from the button since the type="submit" attribute on the button will automatically invoke form submission without needing the explicit handleSubmit call.webapp/src/views/projects/translations/KeyEdit/KeyGeneral.tsx (1)
79-86:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winEdit warning currently triggers for pre-existing whitespace keys.
This branch shows
KeyNameWhitespaceWarningwhenevererrors.nameis empty andvalues.namehas outer whitespace. Because it doesn’t compare against the original name, existing keys that already contained outer whitespace will be warned immediately, which conflicts with the “warn only when newly introduced” edit-flow behavior in the PR objective.Suggested fix
- const { values, setFieldValue, submitForm, errors } = + const { values, initialValues, setFieldValue, submitForm, errors } = useFormikContext<KeyFormType>(); + const hadOuterWhitespaceInitially = /^\s|\s$/.test(initialValues.name || ''); + const hasOuterWhitespaceNow = /^\s|\s$/.test(values.name || ''); + const showWhitespaceWarning = + !errors.name && hasOuterWhitespaceNow && !hadOuterWhitespaceInitially; ... - {errors.name ? ( + {errors.name ? ( <FieldError error={errors.name} /> - ) : ( + ) : showWhitespaceWarning ? ( <KeyNameWhitespaceWarning value={values.name} onTrim={() => setFieldValue('name', values.name.trim())} /> - )} + ) : null}🤖 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 `@webapp/src/views/projects/translations/KeyEdit/KeyGeneral.tsx` around lines 79 - 86, The KeyNameWhitespaceWarning component currently displays whenever errors.name is falsy and values.name contains outer whitespace, but it doesn't distinguish between pre-existing whitespace and newly introduced whitespace. To fix this, modify the condition that determines when to show KeyNameWhitespaceWarning to also compare the current values.name against its initial or original value. Only display the warning when outer whitespace is newly added (i.e., when the current values.name has whitespace but the original value did not), not when the whitespace was already present before editing. This requires accessing the original name value from the form state and incorporating that into the conditional logic alongside the existing errors.name check.
bdshadow
requested changes
Jun 24, 2026
bdshadow
left a comment
bdshadow
left a comment
Member
There was a problem hiding this comment.
in the pr description, there is:
- mention of a dialogue, which is currently not in the implementation - is it an intentional change?
- "Edit: warns only when such whitespace is newly introduced, so keys that already had it are not flagged" - but i see the warning even when i open an existing key with a whitespace
The warning used trim() while the editor highlight matched spaces only, so tab-prefixed/suffixed key names were detected by one and ignored by the other. Share a single leading/trailing whitespace definition between hasOuterWhitespace and the codemirror decorator so both cover spaces and tabs.
dkrizan
enabled auto-merge (squash)
June 30, 2026 10:29
bdshadow
previously approved these changes
Jun 30, 2026
The new edit test looked up the key by exact text, which never matched because the persisted name carries a trailing space. Find the cell via a whitespace-normalized substring match instead.
dkrizan
disabled auto-merge
June 30, 2026 13:52
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
3 participants
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.
Builds on and supersedes #3750, completing the UI safeguards from #3359.
What this adds
Key names — visualize and warn (the missing half of #3359)
label.lightBluetoken so the highlight is clearly visible in both light and dark mode (the original hardcoded colour washed out in light mode).Project name
Tests
webapp/src/fixtures/__tests__/keyName.test.ts— unit tests forhasOuterWhitespacecovering clean names plus leading/trailing spaces and tabs.e2e/cypress/e2e/translations/keyNameWhitespace.cy.ts— covers: create warns + Trim, edit warns when whitespace is newly added + Trim, clean rename does not warn, and editing a key whose whitespace was already present does warn.Translations
key_whitespace_warning_message/key_whitespace_warning_trimcreated in Tolgee (taggeddraft: pr-3750-whitespace).Resolves #3359. Original key-name visualization work by @uvais724 in #3750.
Co-authored-by: Uvais uvais724@gmail.com.
Summary by CodeRabbit
New Features
Bug Fixes
Tests