feat(desktop): replace setup toast with persistent sidebar card#1270
Merged
Conversation
Replace the transient toast notification for missing setup scripts with a persistent card in the workspace sidebar. The card shows for the active project when no setup/teardown scripts are configured, and can be dismissed or navigated to project settings to configure scripts. - Add SetupScriptCard component with animated mount/dismiss - Pass active project props from DashboardLayout through WorkspaceSidebar - Remove setup toast from WorkspaceInitEffects - Remove toast.promise from WorkspaceSidebarFooter open project flow - Move scripts section above branch prefix in project settings - Clean up ScriptsEditor UI (compact textareas, shorter labels) - Check for actual configured scripts, not just config file existence - Invalidate shouldShowConfigToast on script save
Contributor
📝 WalkthroughWalkthroughThis PR refactors the setup script configuration user experience by introducing a new SetupScriptCard component integrated into the workspace sidebar, removing the previous SetupConfigModal dialog, renaming TRPC config endpoints from toast semantics to setup-card semantics, adding async config detection via hasConfiguredScripts, and eliminating config prompts from worktree-opening flows. Changes
Sequence DiagramsequenceDiagram
actor User
participant Sidebar as Workspace Sidebar
participant Card as SetupScriptCard
participant Query as shouldShowSetupCard<br/>Query
participant Router as TRPC Config Router
participant Mutation as dismissSetupCard<br/>Mutation
participant Settings as Project Settings
User->>Sidebar: View workspace sidebar
Sidebar->>Card: Render (activeProjectId, activeProjectName)
Card->>Query: Check if should show setup card
Query->>Router: hasConfiguredScripts(projectPath)
Router->>Router: Read & parse .superset/config.json
Router-->>Query: Return true/false
Query-->>Card: Setup card visibility
alt Card visible & not collapsed
Card->>User: Display "Setup scripts" card with Configure button
User->>Card: Click Configure button
Card->>Settings: Navigate to /settings/project/{projectId}
User->>Settings: Configure scripts
Settings->>Router: Update config
end
alt User dismisses card
User->>Card: Click dismiss (X button)
Card->>Mutation: dismissSetupCard()
Mutation->>Router: Mark as dismissed
Mutation-->>Card: Success - invalidate cache
Card->>Card: Hide card
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
- Remove unused projectName param from ScriptsEditor - Replace non-null assertion with guard in SetupScriptCard
- Add `box` variant to Badge component (sharp corners, accent bg/border) - Use Badge box variant and Button outline variant in SetupScriptCard
![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: 2
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/SetupScriptCard/SetupScriptCard.tsx`:
- Around line 58-64: The dismiss button inside SetupScriptCard is rendered with
only the LuX icon and lacks an accessible label; update the <button> in the
SetupScriptCard component to include an aria-label (e.g., aria-label="Dismiss
setup" or similar) so screen readers announce its purpose, and ensure the
onClick handler handleDismiss remains unchanged; optionally also add a title
attribute to the same <button> for tooltip support.
- Around line 33-41: The dismissMutation.mutate call inside handleDismiss
currently has only onSuccess and silently swallows failures; add an onError
handler to dismissMutation.mutate (inside handleDismiss) that logs the error
with context (include projectId and action name) and surfaces user feedback
(e.g., call your toast/error UI or set an error state) so failures are visible;
ensure you still call any necessary cleanup/invalidation only on success
(utils.config.shouldShowConfigToast.invalidate remains in onSuccess) and include
the unique symbols handleDismiss and dismissMutation.mutate in your change.
Comment on lines
+33
to
+41
| const handleDismiss = () => { | ||
| if (!projectId) return; | ||
| dismissMutation.mutate( | ||
| { projectId }, | ||
| { | ||
| onSuccess: () => | ||
| utils.config.shouldShowConfigToast.invalidate({ projectId }), | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
Mutation errors are silently swallowed.
dismissMutation.mutate has an onSuccess callback but no onError handler. If the dismiss call fails, the user gets no feedback and the error is lost. As per coding guidelines: "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly."
Proposed fix
dismissMutation.mutate(
{ projectId },
{
onSuccess: () =>
utils.config.shouldShowConfigToast.invalidate({ projectId }),
+ onError: (error) =>
+ console.error("[SetupScriptCard/dismiss] failed to dismiss config toast", error),
},
);📝 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
| const handleDismiss = () => { | |
| if (!projectId) return; | |
| dismissMutation.mutate( | |
| { projectId }, | |
| { | |
| onSuccess: () => | |
| utils.config.shouldShowConfigToast.invalidate({ projectId }), | |
| }, | |
| ); | |
| const handleDismiss = () => { | |
| if (!projectId) return; | |
| dismissMutation.mutate( | |
| { projectId }, | |
| { | |
| onSuccess: () => | |
| utils.config.shouldShowConfigToast.invalidate({ projectId }), | |
| onError: (error) => | |
| console.error("[SetupScriptCard/dismiss] failed to dismiss config toast", error), | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/SetupScriptCard/SetupScriptCard.tsx`
around lines 33 - 41, The dismissMutation.mutate call inside handleDismiss
currently has only onSuccess and silently swallows failures; add an onError
handler to dismissMutation.mutate (inside handleDismiss) that logs the error
with context (include projectId and action name) and surfaces user feedback
(e.g., call your toast/error UI or set an error state) so failures are visible;
ensure you still call any necessary cleanup/invalidation only on success
(utils.config.shouldShowConfigToast.invalidate remains in onSuccess) and include
the unique symbols handleDismiss and dismissMutation.mutate in your change.
Comment on lines
+58
to
+64
| <button | ||
| type="button" | ||
| onClick={handleDismiss} | ||
| className="absolute top-2.5 right-2.5 text-muted-foreground hover:text-foreground transition-colors" | ||
| > | ||
| <LuX className="size-3.5" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Dismiss button has no accessible label.
The <button> only contains an icon (<LuX />), so screen readers will announce it as an unlabelled button. Add an aria-label for accessibility.
Proposed fix
<button
type="button"
onClick={handleDismiss}
+ aria-label="Dismiss setup script card"
className="absolute top-2.5 right-2.5 text-muted-foreground hover:text-foreground transition-colors"
>🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/SetupScriptCard/SetupScriptCard.tsx`
around lines 58 - 64, The dismiss button inside SetupScriptCard is rendered with
only the LuX icon and lacks an accessible label; update the <button> in the
SetupScriptCard component to include an aria-label (e.g., aria-label="Dismiss
setup" or similar) so screen readers announce its purpose, and ensure the
onClick handler handleDismiss remains unchanged; optionally also add a title
attribute to the same <button> for tooltip support.
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Extract reusable SidebarCard into @superset/ui with props for badge, title, description, action, and dismiss. Simplify SetupScriptCard to compose SidebarCard instead of duplicating card markup.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/ui/src/components/ui/sidebar-card.tsx`:
- Around line 39-47: The dismiss button contains only the LuX icon and lacks an
accessible name; update the <button> rendered when onDismiss is present to
include an aria-label (e.g., aria-label="Close" or aria-label="Dismiss") so
screen readers announce its purpose—add the aria-label attribute on the button
element that uses onDismiss and LuX (or wire it to an incoming prop like
dismissLabel if you prefer internationalization).
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/SetupScriptCard/SetupScriptCard.tsx (1)
58-65: RedundantprojectIdguard insideonAction.The
onActioncallback can only fire when the card is rendered, which requiresvisible === true, which already asserts!!projectId. The innerif (projectId)check is unnecessary.Proposed fix
onAction={() => { - if (projectId) { - navigate({ - to: "/settings/project/$projectId", - params: { projectId }, - }); - } + navigate({ + to: "/settings/project/$projectId", + params: { projectId: projectId! }, + }); }}
- Rename tRPC routes from *ConfigToast to *SetupCard - Make hasConfiguredScripts async (readFile instead of readFileSync) - Add aria-label to SidebarCard dismiss button - Remove duplicate toast.info from useOpenWorktree/useOpenExternalWorktree - Remove dead SetupConfigModal component and config-modal store - Replace as string casts with early return for proper type narrowing
# Conflicts: # apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarFooter.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/SetupScriptCard/SetupScriptCard.tsx`:
- Around line 26-28: The component currently returns null when isCollapsed,
!projectId, !projectName, or !shouldShow, which unmounts AnimatePresence and
prevents exit animations; instead keep AnimatePresence mounted and move the
conditional render inside it: in SetupScriptCard render AnimatePresence
unconditionally and only conditionally render the animated child (the element
currently gated by the early return) based on isCollapsed, shouldShow, projectId
and projectName; also update handleDismiss in SetupScriptCard to guard against
null/undefined projectId (or narrow the early return to only disable the query
but not unmount AnimatePresence) so dismiss actions don’t assume a present
projectId when the animated child is absent.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx (2)
292-333: Consider extracting a shared import-file handler to reduce duplication.
handleImportSetupFileandhandleImportTeardownFileare nearly identical — they only differ in which state setter is called. The same applies to the other handler pairs (change, add, remove, fileDrop). You could extract a generic helper parameterized by the setter:Example: shared import handler
+const handleImportFile = useCallback( + async (setter: React.Dispatch<React.SetStateAction<ScriptEntry[]>>) => { + try { + const result = await window.ipcRenderer.invoke("open-file-dialog", { + filters: [{ name: "Scripts", extensions: ["sh", "bash", "zsh"] }], + }); + if (result && typeof result === "string") { + const content = await window.ipcRenderer.invoke("read-script-file", result); + if (content && typeof content === "string") { + setter((prev) => [...prev, { id: generateId(), content }]); + setHasChanges(true); + } + } + } catch (error) { + console.error("[scripts-editor/import-file] Failed to import file:", error); + } + }, + [], +);Then call as
handleImportFile(setSetupScripts)/handleImportFile(setTeardownScripts).This would cut ~6 duplicated handler pairs down to shared utilities. Not urgent but worth considering if this file grows further.
101-102: Use prefixed console logging per coding guidelines.The
console.errorcalls on Lines 102, 308, and 331 lack the[domain/operation]prefix required by the project's conventions.-console.error("Failed to read script file:", error); +console.error("[scripts-editor/file-drop] Failed to read script file:", error);-console.error("Failed to import file:", error); +console.error("[scripts-editor/import-file] Failed to import file:", error);As per coding guidelines: "Use prefixed console logging with consistent context pattern: [domain/operation] message".
Also applies to: 307-308, 330-331
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/SetupScriptCard/SetupScriptCard.tsx (1)
17-20: Query passes empty string when disabled — works but is a subtle contract.
projectId ?? ""is passed as the query input even though the query is disabled whenprojectIdis null. This is fine since tRPC won't fire the request, but if theenabledguard is ever accidentally removed, an empty-string project ID would hit the backend. A comment or a skip-token pattern (if supported) would make the intent clearer.
Comment on lines
+26
to
+28
| if (isCollapsed || !projectId || !projectName || !shouldShow) { | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
Exit animation will never fire — AnimatePresence unmounts together with its children.
When shouldShow becomes false (or isCollapsed flips to true), the component returns null at line 26, which unmounts the entire subtree including AnimatePresence itself. For framer-motion exit animations to work, AnimatePresence must stay mounted while its child is conditionally removed.
Move the early-return logic inside AnimatePresence so it remains in the tree:
Proposed fix
- if (isCollapsed || !projectId || !projectName || !shouldShow) {
- return null;
- }
-
const handleDismiss = () => {
dismissMutation.mutate(
{ projectId },
@@
};
+ const isVisible = !isCollapsed && !!projectId && !!projectName && !!shouldShow;
+
return (
<AnimatePresence>
- <motion.div
- key={projectId}
- initial={{ opacity: 0, y: 10 }}
- animate={{ opacity: 1, y: 0 }}
- exit={{ opacity: 0, y: 10 }}
- transition={{ duration: 0.2 }}
- className="px-3 pb-2"
- >
- <SidebarCard
- badge="Setup"
- title="Setup scripts"
- description={`Automate workspace setup for ${projectName}`}
- actionLabel="Configure"
- onAction={() =>
- navigate({
- to: "/settings/project/$projectId",
- params: { projectId },
- })
- }
- onDismiss={handleDismiss}
- />
- </motion.div>
+ {isVisible && projectId && projectName ? (
+ <motion.div
+ key={projectId}
+ initial={{ opacity: 0, y: 10 }}
+ animate={{ opacity: 1, y: 0 }}
+ exit={{ opacity: 0, y: 10 }}
+ transition={{ duration: 0.2 }}
+ className="px-3 pb-2"
+ >
+ <SidebarCard
+ badge="Setup"
+ title="Setup scripts"
+ description={`Automate workspace setup for ${projectName}`}
+ actionLabel="Configure"
+ onAction={() =>
+ navigate({
+ to: "/settings/project/$projectId",
+ params: { projectId },
+ })
+ }
+ onDismiss={handleDismiss}
+ />
+ </motion.div>
+ ) : null}
</AnimatePresence>
);Note: with this approach, handleDismiss also needs to guard against a null projectId (or keep a narrower early return just for the query-disabled case above handleDismiss).
Also applies to: 40-64
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/SetupScriptCard/SetupScriptCard.tsx`
around lines 26 - 28, The component currently returns null when isCollapsed,
!projectId, !projectName, or !shouldShow, which unmounts AnimatePresence and
prevents exit animations; instead keep AnimatePresence mounted and move the
conditional render inside it: in SetupScriptCard render AnimatePresence
unconditionally and only conditionally render the animated child (the element
currently gated by the early return) based on isCollapsed, shouldShow, projectId
and projectName; also update handleDismiss in SetupScriptCard to guard against
null/undefined projectId (or narrow the early return to only disable the query
but not unmount AnimatePresence) so dismiss actions don’t assume a present
projectId when the animated child is absent.
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
Test plan
Summary by CodeRabbit
Release Notes
New Features
Improvements
Refactor