refactor(desktop): polish NewProjectModal — toast errors, Label, spinner#4037
Merged
Conversation
The renderer applies user-select: none globally on body, which prevented copying error messages out of the New Project modal — exactly the text users want to share when reporting issues.
… toast Errors in the New Project modal now surface via sonner toasts instead of an inline destructive panel. Toasts are selectable and copyable by default (no select-text override needed), match the convention used across the rest of the desktop app, and free up modal real estate. Also: - Use shadcn Label for form labels - Add a spinning loader to the Clone button while cloning
Contributor
Greptile SummaryThis PR polishes Confidence Score: 4/5Safe to merge; only style-level concerns, no runtime defects introduced. All findings are P2. The logic for async error paths is correct; the only concern is UX ergonomics around synchronous validation messages being routed through auto-dismissing toasts. No files require special attention; the single changed file is straightforward.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/components/NewProjectModal/NewProjectModal.tsx | Replaces inline error state+block with sonner toasts, swaps <label> for shadcn <Label>, and adds LuLoaderCircle spinner to the Clone button. Logic is sound; only P2 concerns around using auto-dismissing toasts for synchronous validation errors and potential button layout. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([Clone button clicked]) --> B{activeHostUrl?}
B -- No --> T1[toast.error: Host service not available]
B -- Yes --> C{url trimmed?}
C -- No --> T2[toast.error: Please enter a repository URL]
C -- Yes --> D{parentDir trimmed?}
D -- No --> T3[toast.error: Please select a project location]
D -- Yes --> E{name derived from URL?}
E -- No --> T4[toast.error: Could not derive project name]
E -- Yes --> F[setWorking true]
F --> G[client.project.create.mutate]
G -- success --> H[finalizeSetup → onSuccess → reset → close]
G -- error --> I{isLeakedSql?}
I -- Yes --> J[console.error + generic message]
I -- No --> K[raw message]
J --> L[toast.error 'Could not create project' + description]
K --> L
L --> M[onError callback]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/components/NewProjectModal/NewProjectModal.tsx:130-142
**Transient toasts for synchronous validation errors**
Validation errors like "Please enter a repository URL" and "Please select a project location" are synchronous checks that apply to the current form state. Routing them through `toast.error` (which auto-dismisses after ~4 s by default in Sonner) means the user's attention must jump to the toast corner at the moment of submission, and the message disappears before they may have finished reading it or correcting the input. The old inline error block was persistent and contextually anchored to the form. For async operation failures (`handleBrowse`, the actual `create` call) toasts are the right pattern; for these synchronous field-level validations, consider keeping them inline (or at minimum using `{ duration: Infinity }` so the user can dismiss on their own terms).
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/components/NewProjectModal/NewProjectModal.tsx:288-295
**Spinner button may need `gap` for layout**
The `working` state renders an icon + text fragment inside `<Button>`. If the `Button` component's default styles don't include a flex `gap`, the spinner and "Cloning…" text will be flush against each other. Verify that `@superset/ui/button` applies `gap-2` (or similar) to its children, or add it explicitly. Alternatively wrap both in a `<span className="flex items-center gap-2">` if the Button doesn't handle it.
Reviews (1): Last reviewed commit: "refactor(desktop): replace inline error ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
130
to
142
| if (!trimmedUrl) { | ||
| setError("Please enter a repository URL"); | ||
| toast.error("Please enter a repository URL"); | ||
| return; | ||
| } | ||
| if (!trimmedParent) { | ||
| setError("Please select a project location"); | ||
| toast.error("Please select a project location"); | ||
| return; | ||
| } | ||
| const name = deriveProjectNameFromUrl(trimmedUrl); | ||
| if (!name) { | ||
| setError("Could not derive a project name from the URL or path"); | ||
| toast.error("Could not derive a project name from the URL or path"); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Validation errors like "Please enter a repository URL" and "Please select a project location" are synchronous checks that apply to the current form state. Routing them through toast.error (which auto-dismisses after ~4 s by default in Sonner) means the user's attention must jump to the toast corner at the moment of submission, and the message disappears before they may have finished reading it or correcting the input. The old inline error block was persistent and contextually anchored to the form. For async operation failures (handleBrowse, the actual create call) toasts are the right pattern; for these synchronous field-level validations, consider keeping them inline (or at minimum using { duration: Infinity } so the user can dismiss on their own terms).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/components/NewProjectModal/NewProjectModal.tsx
Line: 130-142
Comment:
**Transient toasts for synchronous validation errors**
Validation errors like "Please enter a repository URL" and "Please select a project location" are synchronous checks that apply to the current form state. Routing them through `toast.error` (which auto-dismisses after ~4 s by default in Sonner) means the user's attention must jump to the toast corner at the moment of submission, and the message disappears before they may have finished reading it or correcting the input. The old inline error block was persistent and contextually anchored to the form. For async operation failures (`handleBrowse`, the actual `create` call) toasts are the right pattern; for these synchronous field-level validations, consider keeping them inline (or at minimum using `{ duration: Infinity }` so the user can dismiss on their own terms).
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+288
to
+295
| {working ? ( | ||
| <> | ||
| <LuLoaderCircle className="size-4 animate-spin" /> | ||
| Cloning… | ||
| </> | ||
| ) : ( | ||
| "Clone" | ||
| )} |
Contributor
There was a problem hiding this comment.
gap for layout
The working state renders an icon + text fragment inside <Button>. If the Button component's default styles don't include a flex gap, the spinner and "Cloning…" text will be flush against each other. Verify that @superset/ui/button applies gap-2 (or similar) to its children, or add it explicitly. Alternatively wrap both in a <span className="flex items-center gap-2"> if the Button doesn't handle it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/components/NewProjectModal/NewProjectModal.tsx
Line: 288-295
Comment:
**Spinner button may need `gap` for layout**
The `working` state renders an icon + text fragment inside `<Button>`. If the `Button` component's default styles don't include a flex `gap`, the spinner and "Cloning…" text will be flush against each other. Verify that `@superset/ui/button` applies `gap-2` (or similar) to its children, or add it explicitly. Alternatively wrap both in a `<span className="flex items-center gap-2">` if the Button doesn't handle it.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNewProjectModal now surfaces all validation and operation failures via toast.error instead of local component state, removes the inline destructive error banner and mode picker, uses Label components for headings, and shows a spinner with “Cloning…” on the primary action while cloning. ChangesClone modal — error-reporting and UI simplification
Sequence Diagram(s)(Skipped — changes are UI-focused and do not introduce a multi-component sequential flow requiring diagramming.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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)
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. Review rate limit: 0/8 reviews remaining, refill in 54 minutes and 7 seconds.Comment |
- Tighter modal: rounded-xl, shadow-2xl, sm:max-w-md, edge-to-edge layout (gap-0 p-0) so the footer can render as a distinct band - Footer band: muted background with top border, rounded-b corners inherited via overflow-hidden — matches 1Code's CanvasDialogFooter - Mode option cards: rounded-xl, visible selected state (border-primary/40 + bg-primary/5 + shadow-sm), hover lift - Buttons: subtle press feedback (active:scale-[0.97]) - Reordered: mode picker first, then location/url (matches the user's decision flow — pick mode, then configure)
Reverts the 1Code-imported edge-to-edge layout (gap-0/p-0 with muted footer band, font-semibold title, active:scale buttons) since it conflicts with the rest of our dialogs. Aligns with RenameBranchDialog: default DialogContent at max-w-[420px], default footer, ghost Cancel. Keeps the genuinely useful changes: narrower modal width, polished mode cards with primary-tinted selected state, and the clone spinner.
Empty and Template were placeholder cards labelled "(coming soon)" — they took up modal real estate without doing anything. With only clone shipping, drop the picker entirely and present the clone form directly. The dialog title now says "Clone a repository" so the action is obvious.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
…ner (superset-sh#4037) * fix(desktop): make NewProjectModal error text selectable The renderer applies user-select: none globally on body, which prevented copying error messages out of the New Project modal — exactly the text users want to share when reporting issues. * refactor(desktop): replace inline error block in NewProjectModal with toast Errors in the New Project modal now surface via sonner toasts instead of an inline destructive panel. Toasts are selectable and copyable by default (no select-text override needed), match the convention used across the rest of the desktop app, and free up modal real estate. Also: - Use shadcn Label for form labels - Add a spinning loader to the Clone button while cloning * style(desktop): polish NewProjectModal with 1Code visual DNA - Tighter modal: rounded-xl, shadow-2xl, sm:max-w-md, edge-to-edge layout (gap-0 p-0) so the footer can render as a distinct band - Footer band: muted background with top border, rounded-b corners inherited via overflow-hidden — matches 1Code's CanvasDialogFooter - Mode option cards: rounded-xl, visible selected state (border-primary/40 + bg-primary/5 + shadow-sm), hover lift - Buttons: subtle press feedback (active:scale-[0.97]) - Reordered: mode picker first, then location/url (matches the user's decision flow — pick mode, then configure) * style(desktop): match our dialog DNA in NewProjectModal Reverts the 1Code-imported edge-to-edge layout (gap-0/p-0 with muted footer band, font-semibold title, active:scale buttons) since it conflicts with the rest of our dialogs. Aligns with RenameBranchDialog: default DialogContent at max-w-[420px], default footer, ghost Cancel. Keeps the genuinely useful changes: narrower modal width, polished mode cards with primary-tinted selected state, and the clone spinner. * refactor(desktop): drop disabled mode selector from NewProjectModal Empty and Template were placeholder cards labelled "(coming soon)" — they took up modal real estate without doing anything. With only clone shipping, drop the picker entirely and present the clone form directly. The dialog title now says "Clone a repository" so the action is obvious.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
superset-sh#4037 NewProjectModal: drop fork-only OPTIONS multi-mode selector and adopt upstream v2-only simplification (toast errors + Label + spinner). The 'empty'/'template' modes were 'coming soon' disabled entries with no implementation behind them; no user-facing fork feature lost. superset-sh#4191 DashboardSidebarDeleteDialog: keep fork's 'Checking…' confirm-button label UX while incorporating upstream's fixed-height warning banner slot. Restore isCheckingStatus destructure + prop pass; add to DestroyConfirmPane interface so the deleting-button disabled state still gates on it. superset-sh#4045 schema.ts: backfill sidebarFileLinks field into v2UserPreferencesSchema and DEFAULT_V2_USER_PREFERENCES (upstream introduced these in superset-sh#3988 which fork will adopt in Phase 5). Also add 'shift' tier to linkTierMapSchema (matches superset-sh#3988 LinkTier expansion) so superset-sh#4045's added schema tests pass. superset-sh#4212 tray Quit Completely: replace upstream quitAppCompletely() with fork's existing requestQuit('stop') which already performs full host- service + pty-daemon teardown via prepareQuit('stop') + getTodoScheduler stop + cleanupMainWindowResources. Tray menu entry retained. superset-sh#4225 automations/page.tsx: drop iconUrl prop from ProjectThumbnail call (fork ProjectThumbnail uses githubOwner, iconUrl arrives with superset-sh#3823 v2 project icon settings in cycle 41). Pass githubOwner={null} as transient fallback. superset-sh#4226 useBranchPickerController: fork's useWorkspaceCreates.submit returns Promise<void> (in-flight tracker carries error state); upstream changed it to Promise<SubmitResult>. Wrap submit in try/catch and navigate using snapshot.id so the picker compiles. Note: foreign-worktree adopt path with canonical workspaceId waiting is deferred to cycle 44b when WorkspaceCreatesManager / submit API are upgraded.
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
Following up on #4036, align the New Project modal with the rest of the desktop app (and 1Code-style conventions):
toast.errorfrom sonner. Toasts are selectable/copyable by default, match every other error surface in the app, and free up modal real estate.Labelcomponent for the form labels.LuLoaderCircleto the Clone button while a clone is in flight.Test plan
Summary by cubic
Polished the New Project modal for a simpler, consistent UX. Errors now use selectable toasts, the dialog matches desktop styles, and the clone form is shown by default.
toast.errorfrom@superset/ui/sonner(selectable/copyable).Labelfrom@superset/ui/label.LuLoaderCircleto the Clone button while cloning.max-w-[420px]), default footer, ghost Cancel.Written for commit c3a1e29. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes