Fix/patched terminal recovery#3343
Conversation
📝 WalkthroughWalkthroughThis PR introduces a "Superset Patched" desktop application variant with app home directory isolation, browser state reset on first launch, terminal refocus recovery burst improvements, PostHog feature-flag bridging to desktop-specific mechanism, workspace client reference counting, and enhanced git status tracking with workspace awareness. Documentation files outline a macOS terminal rendering lifecycle issue and its multi-burst recovery solution. Changes
Sequence Diagram(s)sequenceDiagram
participant Terminal as Terminal Component
participant Lifecycle as useTerminalLifecycle
participant Emulator as Terminal Emulator
participant Recovery as Recovery Task
Terminal->>Lifecycle: Visibility Change / Focus Restore
Lifecycle->>Lifecycle: scheduleRecoveryBurst()
Lifecycle->>Recovery: Immediate reattachRecovery(forceResize)
activate Recovery
Recovery->>Emulator: clearTextureAtlas(), fit(), refresh()
Emulator-->>Recovery: Done
deactivate Recovery
Lifecycle->>Recovery: Schedule 120ms delay
Recovery->>Emulator: clearTextureAtlas(), fit(), refresh()
Emulator-->>Recovery: Done
Lifecycle->>Recovery: Schedule 260ms delay
Recovery->>Emulator: clearTextureAtlas(), fit(), refresh()
Emulator-->>Recovery: Done
sequenceDiagram
participant Provider as WorkspaceClientProvider
participant Cache as Client Cache
participant QueryClient as React Query Client
participant Component as Consuming Component
Component->>Provider: Mount (getWorkspaceClients)
Provider->>Cache: Fetch / Create with refCount=0
Provider->>Cache: Increment refCount
Cache-->>Provider: Return clients
Provider-->>Component: Provide to tree
Component->>Component: Use workspace clients
Component->>Provider: Unmount (releaseWorkspaceClients)
Provider->>Cache: Decrement refCount
alt refCount reaches 0
Provider->>QueryClient: Clear cache
Provider->>Cache: Remove entry
Cache-->>Provider: Done
end
Provider-->>Component: Cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The diff spans heterogeneous changes across terminal lifecycle, feature-flag bridging, git status tracking, workspace client reference counting, and UI component updates. Multiple areas contain logic-dense implementations (Session.attach reordering, recovery burst timing, workspace refcounting) requiring separate reasoning. While the feature-flag replacements follow a repetitive pattern, the variety of modifications in terminal handling, git status sequencing, and workspace visibility logic demands careful attention to control-flow correctness and state management consistency across many files. Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
Greptile SummaryThis PR addresses a terminal repaint/recovery issue (#1873, #3321) where switching between windows or workspaces could leave the terminal in a visually stale state. The core fix — adding a short "recovery burst" (immediate + 120ms + 260ms repaint passes) in However, the PR also includes a set of local-only investigation patches that are explicitly called out in
These local patches need to be stripped before the legitimate terminal recovery fix can be safely merged. Confidence Score: 1/5Not safe to merge — local-only investigation patches (forced DOM renderer, rebranded app identity, patched-build isolation code) are mixed in with the legitimate terminal recovery fix. The terminal recovery burst logic and its tests are correct and desirable. However, three P0 issues block merge: (1) the hard-coded
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["window.focus / visibilitychange"] --> B["scheduleRecoveryBurst(forceResize)"]
B --> C["scheduleReattachRecovery — immediate rAF"]
B --> D["setTimeout 120ms → scheduleReattachRecovery"]
B --> E["setTimeout 260ms → scheduleReattachRecovery"]
C --> F{"within 120ms throttle?"}
F -- No --> G["runReattachRecovery()\nclearTextureAtlas()\nfitAddon.fit()\nxterm.refresh()\nresize if changed\nxterm.focus if focused\nscroll to bottom"]
F -- Yes --> H["NEW: setTimeout remaining+1ms\n→ scheduleReattachRecovery (retry)"]
H --> F
D --> F
E --> F
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const productName = "Superset Patched"; | ||
| const macIconPath = join(pkg.resources, "build/icons/icon.icns"); |
There was a problem hiding this comment.
productName and appId have been changed to "Superset Patched" / "com.superset.desktop.patched". These are the local packaging identity overrides listed in LOCAL_PATCHES.md as investigation-only. Shipping them would rebrand the production desktop app and break auto-update signing, deep-link routing, and OS-level app identity.
| const productName = "Superset Patched"; | |
| const macIconPath = join(pkg.resources, "build/icons/icon.icns"); | |
| const productName = "Superset"; | |
| const macIconPath = join(pkg.resources, "build/icons/icon.icns"); | |
| const linuxIconPath = join(pkg.resources, "build/icons"); | |
| const winIconPath = join(pkg.resources, "build/icons/icon.ico"); | |
| const config: Configuration = { | |
| appId: "com.superset.desktop", | |
| productName, |
| @@ -32,6 +60,52 @@ export function ensureSupersetHomeDirExists(): void { | |||
| } | |||
| } | |||
|
|
|||
| export function isPatchedDesktopBuild( | |||
| execPath: string = process.execPath, | |||
| ): boolean { | |||
| return execPath.includes("Superset Patched.app"); | |||
| } | |||
|
|
|||
| export function resetPatchedBrowserStateIfNeeded( | |||
| homeDir: string = SUPERSET_HOME_DIR, | |||
| execPath: string = process.execPath, | |||
| ): void { | |||
| if (!isPatchedDesktopBuild(execPath)) return; | |||
|
|
|||
| const markerPath = join(homeDir, PATCHED_BROWSER_STATE_MARKER); | |||
| if (existsSync(markerPath)) return; | |||
|
|
|||
| ensureSupersetHomeDirExists(); | |||
|
|
|||
| for (const relativePath of PATCHED_BROWSER_STATE_PATHS) { | |||
| const targetPath = join(homeDir, relativePath); | |||
| if (!existsSync(targetPath)) continue; | |||
| try { | |||
| rmSync(targetPath, { recursive: true, force: true }); | |||
| } catch { | |||
| try { | |||
| unlinkSync(targetPath); | |||
| } catch (error) { | |||
| console.warn( | |||
| "[app-environment] Failed to reset patched browser state:", | |||
| targetPath, | |||
| error, | |||
| ); | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| try { | |||
| writeFileSync(markerPath, `${new Date().toISOString()}\n`, "utf8"); | |||
| } catch (error) { | |||
| console.warn( | |||
| "[app-environment] Failed to write patched browser state marker:", | |||
| markerPath, | |||
| error, | |||
| ); | |||
| } | |||
| } | |||
There was a problem hiding this comment.
isPatchedDesktopBuild, resetPatchedBrowserStateIfNeeded, and PATCHED_BROWSER_STATE_MARKER all exist solely to support the local investigation build (Superset Patched.app). They are guarded by a path-string check, so they would be no-ops on standard builds — but the functions and constants are still being added to the production module.
LOCAL_PATCHES.md explicitly marks the "local app-home / userData isolation experiments" as not intended for upstream. This entire block (lines 29–107) should be removed from the PR.
| # Superset Local Investigation Notes | ||
|
|
||
| This directory contains a workspace-local copy of the Superset Desktop source used to investigate terminal/TUI corruption and repaint issues. | ||
|
|
||
| ## Local-only patches kept here | ||
|
|
||
| These changes were useful for local validation but are not intended to be sent upstream as-is: | ||
|
|
||
| - local packaging identity changes for `Superset Patched.app` | ||
| - local app-home / userData isolation experiments | ||
| - forced DOM renderer in terminal helpers | ||
| - local sidebar sync fix after `Open project` | ||
|
|
||
| ## Upstream candidate patches | ||
|
|
||
| The current upstream candidate focuses on the terminal visibility/focus restore path: | ||
|
|
||
| - `apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts` | ||
| - `apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.test.ts` | ||
|
|
||
| ## Existing local terminal-host patch context | ||
|
|
||
| This copy also includes the `Session.attach()` snapshot/socket ordering fix and tests: | ||
|
|
||
| - `apps/desktop/src/main/terminal-host/session.ts` | ||
| - `apps/desktop/src/main/terminal-host/session.test.ts` | ||
| - `apps/desktop/src/main/terminal-host/session-attach-overlap.test.ts` | ||
|
|
||
| That area appears to overlap with the already-open upstream PRs: | ||
|
|
There was a problem hiding this comment.
LOCAL_PATCHES.md, UPSTREAM_ISSUE_terminal_refocus_repaint.md, and UPSTREAM_PR_terminal_refocus_repaint.md are workspace-local investigation notes. The first file even states: "These changes were useful for local validation but are not intended to be sent upstream as-is."
These three markdown files at the repo root should be removed from the PR (or moved to a private/scratch location outside the repository).
| const reattachRecovery = { | ||
| throttleMs: 120, | ||
| pendingFrame: null as number | null, | ||
| lastRunAt: 0, | ||
| pendingForceResize: false, | ||
| burstTimeouts: [] as number[], | ||
| }; |
There was a problem hiding this comment.
The reattachRecovery object and the cancelReattachRecovery / scheduleRecoveryBurst functions (lines 758–852) use a different indentation level from the surrounding code — they appear to have been pasted in with an extra leading tab/spaces. Same issue in the cleanup section around lines 885–891.
These should be reformatted to match the rest of the useEffect body before merging.
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!
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by cubic
Improves terminal stability when refocusing and isolates the patched desktop build to its own app data, reducing repaint glitches and avoiding conflicts with the stock app. Also tightens analytics/feature-flag behavior and smooths workspace and Git changes UX.
Bug Fixes
Refactors
~/.superset-patched, alignuserData, and clear browser-only state once via marker; terminal host readsSUPERSET_HOME_DIR.isPostHogEnabled, replaceposthog-js/reactwith a lightweightuseDesktopFeatureFlagEnabled, and guard$pageview/identify calls.useGitChangesStatuslistens togit:changedand invalidates precisely.@superset/workspace-clientproviders with cleanup on unmount; add tests.better-authclient chunk, adjust theme background, improve React error logging, and disable unused.codexMCP servers by default.Written for commit 70b15dc. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Tests