Repository navigation
feat(desktop): show teardown logs in modal instead of error toast - #1277
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughBackend teardown result now carries optional diagnostic output; deletion paths propagate that output on failure. Frontend deletion dialogs were updated to surface teardown failures with a "View Logs" action. A new TeardownLogsDialog component provides a modal to view (ANSI-stripped) teardown logs and a global showTeardownLogs trigger. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Renderer (Dialog)
participant TRPC as Backend (delete mutation)
participant Logs as TeardownLogsDialog
User->>Frontend: Click "Delete" (with show logs option)
Frontend->>TRPC: call delete/deleteWorktree mutation
TRPC-->>Frontend: response { success: true } OR { success: false, output?: string, error }
alt success
Frontend->>User: show success toast
else teardown failed with output
Frontend->>User: show error toast with "View Logs" action
User->>Frontend: clicks "View Logs"
Frontend->>Logs: showTeardownLogs(output) (ANSI stripped)
Logs-->>User: modal with logs and copy button
else teardown failed without output
Frontend->>User: show generic error toast
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
saddlepaddle
force-pushed
the
teardown-failing-no-blokc
branch
3 times, most recently
from
February 7, 2026 06:10
26bcb1a to
fd33c3c
Compare
![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
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/components/TeardownLogsDialog/TeardownLogsDialog.tsx`:
- Around line 36-39: The module-level assignment to showLogsFn inside
TeardownLogsDialog is a render-side effect; move that assignment into a
useEffect so it runs after mount and is cleaned up on unmount. Specifically, in
TeardownLogsDialog use useEffect to set showLogsFn = (newLogs) => {
setLogs(newLogs); setIsOpen(true); } when the component mounts and reset
showLogsFn = undefined (or a no-op) in the effect cleanup to avoid stale
closures and state updates after unmount; also add useEffect to the React
import.
Comment on lines
+36
to
+39
| showLogsFn = (newLogs) => { | ||
| setLogs(newLogs); | ||
| setIsOpen(true); | ||
| }; |
Contributor
There was a problem hiding this comment.
Side effect during render — move showLogsFn assignment into useEffect with cleanup.
Assigning a module-level mutable inside the render body runs on every render and is a side effect that violates React's expectation of pure render functions. More importantly, if TeardownLogsDialog ever unmounts, showLogsFn retains a stale reference to the old setLogs/setIsOpen, which will trigger React state-update-on-unmounted-component warnings.
Wrap in useEffect so the callback is registered after mount and cleared on unmount:
Proposed fix
export function TeardownLogsDialog() {
const [logs, setLogs] = useState<string | null>(null);
const [isOpen, setIsOpen] = useState(false);
- showLogsFn = (newLogs) => {
- setLogs(newLogs);
- setIsOpen(true);
- };
+ useEffect(() => {
+ showLogsFn = (newLogs) => {
+ setLogs(newLogs);
+ setIsOpen(true);
+ };
+ return () => {
+ showLogsFn = null;
+ };
+ }, []);You'll need to add useEffect to the React import.
📝 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
| showLogsFn = (newLogs) => { | |
| setLogs(newLogs); | |
| setIsOpen(true); | |
| }; | |
| export function TeardownLogsDialog() { | |
| const [logs, setLogs] = useState<string | null>(null); | |
| const [isOpen, setIsOpen] = useState(false); | |
| useEffect(() => { | |
| showLogsFn = (newLogs) => { | |
| setLogs(newLogs); | |
| setIsOpen(true); | |
| }; | |
| return () => { | |
| showLogsFn = null; | |
| }; | |
| }, []); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/components/TeardownLogsDialog/TeardownLogsDialog.tsx`
around lines 36 - 39, The module-level assignment to showLogsFn inside
TeardownLogsDialog is a render-side effect; move that assignment into a
useEffect so it runs after mount and is cleaned up on unmount. Specifically, in
TeardownLogsDialog use useEffect to set showLogsFn = (newLogs) => {
setLogs(newLogs); setIsOpen(true); } when the component mounts and reset
showLogsFn = undefined (or a no-op) in the effect cleanup to avoid stale
closures and state updates after unmount; also add useEffect to the React
import.
saddlepaddle
force-pushed
the
teardown-failing-no-blokc
branch
from
February 7, 2026 06:15
fd33c3c to
e036118
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/routes/_authenticated/components/TeardownLogsDialog/TeardownLogsDialog.tsx`:
- Around line 13-18: Replace the custom ANSI_REGEX and local stripAnsi function
with the existing strip-ansi library: remove the ANSI_REGEX constant and the
stripAnsi(text: string) implementation, import the library (e.g. import
stripAnsi from 'strip-ansi') at the top of the file, and update any local calls
to use the imported stripAnsi function; also remove the biome-ignore comment
that was tied to the custom regex. This keeps usage consistent with other
modules like commandBuffer.ts and avoids duplicating regex logic.
Comment on lines
+13
to
+18
| // biome-ignore lint/suspicious/noControlCharactersInRegex: matching ANSI escape sequences | ||
| const ANSI_REGEX = /\x1b\[[0-9;]*[a-zA-Z]/g; | ||
|
|
||
| function stripAnsi(text: string): string { | ||
| return text.replace(ANSI_REGEX, ""); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if strip-ansi is available in the project dependencies
find . -name "package.json" -type f -exec grep -l "strip-ansi" {} \;Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check if strip-ansi is a direct dependency in apps/desktop/package.json
grep -A 2 -B 2 "strip-ansi" ./apps/desktop/package.json
# Search for any existing usage of strip-ansi in the codebase
rg "from\s+['\"]strip-ansi" --type ts --type tsx --type js --type jsx
rg "import.*strip-ansi"Repository: superset-sh/superset
Length of output: 393
Use strip-ansi library instead of custom regex for ANSI code removal.
The library is already in the dependency tree (strip-ansi@^7.1.2) and used elsewhere in the codebase (commandBuffer.ts). Replace the custom regex with the established library for consistency and maintainability.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/components/TeardownLogsDialog/TeardownLogsDialog.tsx`
around lines 13 - 18, Replace the custom ANSI_REGEX and local stripAnsi function
with the existing strip-ansi library: remove the ANSI_REGEX constant and the
stripAnsi(text: string) implementation, import the library (e.g. import
stripAnsi from 'strip-ansi') at the top of the file, and update any local calls
to use the imported stripAnsi function; also remove the biome-ignore comment
that was tied to the custom regex. This keeps usage consistent with other
modules like commandBuffer.ts and avoids duplicating regex logic.
When a teardown script fails during workspace/worktree deletion, show a brief "Teardown failed" toast with a "View Logs" button that opens a modal displaying the full terminal output in a CodeBlock, instead of dumping the raw output into the toast message. - Return teardown output from tRPC delete procedures - Add TeardownLogsDialog using imperative Alerter pattern - Use CodeBlock with language="log" for styled log display - Replace native checkbox with Checkbox + Label components - Fix stream flushing with setImmediate in teardown exit handler
saddlepaddle
force-pushed
the
teardown-failing-no-blokc
branch
from
February 7, 2026 07:05
e036118 to
fb16dff
Compare
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
6 tasks
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
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
language="log")teardownResult.outputfrom bothdeleteanddeleteWorktreetRPC proceduressetImmediatein teardown exit handler to let buffered stream data flush before reading combined output<input type="checkbox">withCheckbox+Labelfrom@superset/uiTest plan
exit 1in.superset/config.json)Summary by CodeRabbit
New Features
Bug Fixes