fix: show whitespace-only error on Create workspace and collection modal#7883
Open
prateek-bruno wants to merge 8 commits intousebruno:mainfrom
Open
fix: show whitespace-only error on Create workspace and collection modal#7883prateek-bruno wants to merge 8 commits intousebruno:mainfrom
prateek-bruno wants to merge 8 commits intousebruno:mainfrom
Conversation
prateek-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
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:
WalkthroughAdds local submit wrappers that validate and mark touched fields before delegating to Formik, normalizes validation messages to "can't be empty", auto-synchronizes sanitized folder names from workspace/collection names when folder-name editing is inactive, and updates Playwright tests to assert whitespace-only name validation. ChangesCreateWorkspace component
CreateCollection component
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/components/WorkspaceSidebar/CreateWorkspace/index.js`:
- Around line 122-126: The current snapshot-based call to formik.setValues({
...formik.values, workspaceName, workspaceFolderName:
sanitizeName(workspaceName) }) can overwrite async updates (e.g.,
browseDirectory setting workspaceLocation); change it to use a functional
updater so you merge into the latest state: call formik.setValues(prev => ({
...prev, workspaceName, workspaceFolderName: sanitizeName(workspaceName) })) to
avoid losing workspaceLocation or other concurrent changes from async operations
like browseDirectory.
🪄 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: c8c7e82b-19b5-4fdc-94a8-8d082df2ead2
📒 Files selected for processing (1)
packages/bruno-app/src/components/WorkspaceSidebar/CreateWorkspace/index.js
prateek-bruno
force-pushed
the
bugfix/validate-create-workspace-btn
branch
from
cb81f05 to
40261e9
Compare
prateek-bruno
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/workspace/create-workspace/create-workspace.spec.ts (1)
391-391: 💤 Low value
submitButtonlocator is duplicated across twotest.stepblocks.Lines 382 and 391 each create the same locator. Since Playwright locators are lazy, you can hoist it above both steps.
♻️ Suggested refactor
+ const modal = page.locator('.bruno-modal-card').filter({ hasText: 'Create Workspace' }); + const submitButton = modal.getByRole('button', { name: 'Create Workspace' }); + await test.step('Enter whitespace-only name and submit', async () => { - const modal = page.locator('.bruno-modal-card').filter({ hasText: 'Create Workspace' }); await modal.waitFor({ state: 'visible', timeout: 5000 }); - const submitButton = modal.getByRole('button', { name: 'Create Workspace' }); await modal.locator('#workspace-name').fill(' '); await expect(submitButton).toBeEnabled(); await submitButton.click(); }); await test.step('Verify validation error appears and modal stays open', async () => { - const modal = page.locator('.bruno-modal-card').filter({ hasText: 'Create Workspace' }); - const submitButton = modal.getByRole('button', { name: 'Create Workspace' }); await expect(modal).toBeVisible(); await expect(submitButton).toBeEnabled(); await expect(modal.getByText("Workspace name can't be empty")).toBeVisible({ timeout: 2000 }); });🤖 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 `@tests/workspace/create-workspace/create-workspace.spec.ts` at line 391, The locator submitButton is created twice inside separate test.step blocks; hoist its creation to a shared scope above both steps by moving const submitButton = modal.getByRole('button', { name: 'Create Workspace' }); out of the individual test.step blocks so both steps reuse the same Playwright locator (referencing modal and submitButton) to avoid duplication and keep the lazy locator semantics.
🤖 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 `@tests/workspace/create-workspace/create-workspace.spec.ts`:
- Around line 385-386: The test's assertions on submitButton contradict the PR
intent: with a whitespace-only name the form is invalid yet the test uses
submitButton.toBeEnabled() (and again at the later check), so either change both
assertions that reference submitButton (the ones around the initial click and
the post-submit check) to toBeDisabled() to reflect a live-disabled button on
validation failure, or if the intended behavior is validate-on-submit (button
remains enabled), remove the redundant post-submit toBeEnabled() check and
update the PR description to state validate-on-submit; locate the submitButton
assertions in this spec (the expectations currently calling toBeEnabled() at the
two places noted) and apply the corresponding assertion/removal based on the
chosen design.
---
Nitpick comments:
In `@tests/workspace/create-workspace/create-workspace.spec.ts`:
- Line 391: The locator submitButton is created twice inside separate test.step
blocks; hoist its creation to a shared scope above both steps by moving const
submitButton = modal.getByRole('button', { name: 'Create Workspace' }); out of
the individual test.step blocks so both steps reuse the same Playwright locator
(referencing modal and submitButton) to avoid duplication and keep the lazy
locator semantics.
🪄 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: 21cbd270-c502-4ce9-a392-5db217f45919
📒 Files selected for processing (2)
packages/bruno-app/src/components/WorkspaceSidebar/CreateWorkspace/index.jstests/workspace/create-workspace/create-workspace.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-app/src/components/WorkspaceSidebar/CreateWorkspace/index.js
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
prateek-bruno
changed the title
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/bruno-app/src/components/Sidebar/CreateCollection/index.js (1)
78-93: 💤 Low value
handleSubmitwrapper is redundant — consider simplifying.Formik v2's
SUBMIT_ATTEMPTreducer already setstouched: setNestedObjectValues(state.values, true)across all fields, and Formik skips callingonSubmitwhenvalidateFormreturns errors. The custom wrapper reproduces this behavior and also triggers a doublevalidateFormcall (line 81 + Formik's internal run insidehandleSubmiton line 92). With.trim()in the Yup schema as the actual fix, the wrapper is purely additive.♻️ Simplified approach (drops the wrapper entirely)
- const handleSubmit = async (e) => { - e?.preventDefault?.(); - - const errors = await formik.validateForm(); - if (Object.keys(errors).length > 0) { - formik.setTouched({ - collectionName: true, - collectionFolderName: true, - collectionLocation: true, - format: true - }); - return; - } - - formik.handleSubmit(); - }; // ... - <form className="bruno-form" onSubmit={handleSubmit}> + <form className="bruno-form" onSubmit={formik.handleSubmit}>🤖 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 `@packages/bruno-app/src/components/Sidebar/CreateCollection/index.js` around lines 78 - 93, The custom handleSubmit wrapper is redundant and causes duplicate validation; remove the wrapper and wire the form submit directly to formik.handleSubmit (drop the async handleSubmit function that calls formik.validateForm and formik.setTouched), since Formik's SUBMIT_ATTEMPT already sets touched across values and Formik itself will skip onSubmit if validateForm returns errors; ensure any submit handlers or JSX that referenced handleSubmit now call formik.handleSubmit directly.tests/collection/create/create-collection.spec.ts (2)
10-31: ⚡ Quick winAdd
test.stepblocks — required per coding guidelines.The test has three distinct phases (open modal → submit invalid input → close), each of which maps cleanly to a named step. Without them, failed runs in Playwright reports produce a flat stack with no phase context.
♻️ Proposed refactor with `test.step`
test('should show validation error for whitespace-only name in modal and keep modal open', async ({ page }) => { + const createCollectionModal = page.locator('.bruno-modal-card').filter({ hasText: 'Create Collection' }); + + await test.step('Open Create Collection modal', async () => { await page.getByTestId('collections-header-add-menu').click(); await page.locator('.tippy-box .dropdown-item').filter({ hasText: 'Create collection' }).click(); const inlineCreator = page.locator('.inline-collection-creator'); await inlineCreator.waitFor({ state: 'visible', timeout: 5000 }); await inlineCreator.locator('.cog-btn').click(); - const createCollectionModal = page.locator('.bruno-modal-card').filter({ hasText: 'Create Collection' }); await createCollectionModal.waitFor({ state: 'visible', timeout: 5000 }); + }); + await test.step('Submit whitespace-only name and verify validation error', async () => { const submitButton = createCollectionModal.getByRole('button', { name: 'Create', exact: true }); await createCollectionModal.getByLabel('Name').fill(' '); await expect(submitButton).toBeEnabled(); await submitButton.click(); await expect(createCollectionModal).toBeVisible(); await expect(submitButton).toBeEnabled(); await expect(createCollectionModal.getByText('Collection name can\'t be empty')).toBeVisible({ timeout: 2000 }); + }); + await test.step('Close the modal', async () => { await createCollectionModal.getByRole('button', { name: 'Cancel' }).click(); + }); });As per coding guidelines: "Promote the use of
test.stepas much as possible so the generated reports are easier to read."🤖 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 `@tests/collection/create/create-collection.spec.ts` around lines 10 - 31, Wrap the three logical phases of the test ("open modal", "submit invalid input", "close modal") in Playwright test.step calls with descriptive names to improve report readability: surround the initial flow that clicks the add menu, selects "Create collection", waits for inlineCreator and clicks the cog (referencing inlineCreator), then a step for filling the Name with whitespace, asserting submitButton state and clicking it (referencing submitButton and createCollectionModal), and finally a step for asserting the validation message and clicking the Cancel button (referencing createCollectionModal); keep existing awaits and assertions but move them into these test.step blocks so each phase appears as a named step in test reports.
12-19: ⚖️ Poor tradeoffCSS class selectors are fragile — prefer
data-testidfor Playwright locators.Lines 12–19 use
.tippy-box .dropdown-item,.inline-collection-creator,.cog-btn, and.bruno-modal-card— all of which break silently on CSS renames. The guideline prescribesdata-testidfor testable elements.collections-header-add-menuon line 11 already follows this pattern correctly.Adding
data-testidattributes to the corresponding component files (inline creator, cog button, modal card) would allow these locators to be migrated togetByTestId(...)calls, making the test resilient to style changes.As per coding guidelines: "Add
data-testidto testable elements for Playwright."🤖 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 `@tests/collection/create/create-collection.spec.ts` around lines 12 - 19, Replace fragile CSS class locators with stable data-testid locators: add data-testid attributes to the inline collection creator component, its cog button, and the modal card (the elements currently targeted by the selectors '.inline-collection-creator', '.cog-btn', and '.bruno-modal-card') and update the test to use Playwright's getByTestId (or page.getByTestId) calls instead of '.tippy-box .dropdown-item' / '.inline-collection-creator' / '.cog-btn' / '.bruno-modal-card'; keep the existing 'collections-header-add-menu' testid pattern as a reference when naming the new testids so the test becomes resilient to CSS renames.
🤖 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 `@packages/bruno-app/src/components/Sidebar/CreateCollection/index.js`:
- Around line 78-93: The custom handleSubmit wrapper is redundant and causes
duplicate validation; remove the wrapper and wire the form submit directly to
formik.handleSubmit (drop the async handleSubmit function that calls
formik.validateForm and formik.setTouched), since Formik's SUBMIT_ATTEMPT
already sets touched across values and Formik itself will skip onSubmit if
validateForm returns errors; ensure any submit handlers or JSX that referenced
handleSubmit now call formik.handleSubmit directly.
In `@tests/collection/create/create-collection.spec.ts`:
- Around line 10-31: Wrap the three logical phases of the test ("open modal",
"submit invalid input", "close modal") in Playwright test.step calls with
descriptive names to improve report readability: surround the initial flow that
clicks the add menu, selects "Create collection", waits for inlineCreator and
clicks the cog (referencing inlineCreator), then a step for filling the Name
with whitespace, asserting submitButton state and clicking it (referencing
submitButton and createCollectionModal), and finally a step for asserting the
validation message and clicking the Cancel button (referencing
createCollectionModal); keep existing awaits and assertions but move them into
these test.step blocks so each phase appears as a named step in test reports.
- Around line 12-19: Replace fragile CSS class locators with stable data-testid
locators: add data-testid attributes to the inline collection creator component,
its cog button, and the modal card (the elements currently targeted by the
selectors '.inline-collection-creator', '.cog-btn', and '.bruno-modal-card') and
update the test to use Playwright's getByTestId (or page.getByTestId) calls
instead of '.tippy-box .dropdown-item' / '.inline-collection-creator' /
'.cog-btn' / '.bruno-modal-card'; keep the existing
'collections-header-add-menu' testid pattern as a reference when naming the new
testids so the test becomes resilient to CSS renames.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 70eca5eb-8ddf-4790-a0c2-d000601c57ab
📒 Files selected for processing (2)
packages/bruno-app/src/components/Sidebar/CreateCollection/index.jstests/collection/create/create-collection.spec.ts
gopu-bruno
reviewed
May 7, 2026
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
gopu-bruno
reviewed
May 7, 2026
Collaborator
gopu-bruno
left a comment
gopu-bruno
left a comment
There was a problem hiding this comment.
Would like to have the original empty name test alongside the new whitespace one. Rest LGTM
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
gopu-bruno
approved these changes
May 8, 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
2 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.
Description
Jira
What this does:
After:
Create collection modal:
Screen.Recording.2026-05-07.at.3.53.07.PM.mov
Create workspace modal:
Screen.Recording.2026-05-07.at.3.52.07.PM.mov
Before:
Create collection modal:
Screen.Recording.2026-05-07.at.3.54.17.PM.mov
Create workspace modal:
Screen.Recording.2026-01-09.at.5.15.58.PM.mov
Contribution Checklist: