-
Notifications
You must be signed in to change notification settings - Fork 952
Fix/patched terminal recovery #3343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ea5b8c6
3a25f47
8cd5352
70b15dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| # 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: | ||
|
|
||
| - #3081 | ||
| - #3310 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| ## Summary | ||
|
|
||
| Returning to a terminal pane after switching app focus / window / workspace can still leave the terminal in a partially stale visual state even after the underlying text content has recovered. | ||
|
|
||
| In my local repro with Codex CLI running inside Superset Desktop on macOS, the most visible symptom was that the TUI content could repaint, but some terminal chrome remained stale for a beat longer than the text layer. In practice this showed up as blank strips / stale repaint artifacts, and in some runs the Codex input box styling lagged behind the recovered content. | ||
|
|
||
| This looks like a separate renderer lifecycle problem from the attach-time snapshot/socket overlap bug. The pane content itself may no longer be duplicated or shifted, but a single focus-time `fit() + refresh()` pass can still miss the final settled layout after the window becomes visible again. | ||
|
|
||
| ## Environment | ||
|
|
||
| - Superset Desktop: built from current `main` as of 2026-04-10 | ||
| - macOS: Apple Silicon | ||
| - Repro app inside terminal: Codex CLI | ||
|
|
||
| ## Reproduction | ||
|
|
||
| 1. Open a terminal pane in Superset Desktop. | ||
| 2. Start Codex CLI or another full-screen / dense TUI. | ||
| 3. Switch away from the app or switch workspaces/tabs long enough for the pane to fully unmount/occlude. | ||
| 4. Return to the same terminal pane. | ||
| 5. Observe that the text content may recover, but the terminal can still briefly retain stale visual state from before focus returned. | ||
|
|
||
| ## Expected behavior | ||
|
|
||
| When the terminal becomes visible again, the entire pane should repaint into a fully consistent final state in one restore cycle, including terminal background/chrome and TUI styling. | ||
|
|
||
| ## Actual behavior | ||
|
|
||
| - content can recover first | ||
| - stale blank space or stale visual styling can remain briefly afterward | ||
| - a single recovery pass on `window.focus` / `visibilitychange` does not always seem to be enough once layout finishes settling | ||
|
|
||
| ## Suspected area | ||
|
|
||
| `apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts` | ||
|
|
||
| Specifically the focus / visibility recovery path around: | ||
|
|
||
| - `clearTextureAtlas()` | ||
| - `fitAddon.fit()` | ||
| - `xterm.refresh()` | ||
| - resize reporting after refocus | ||
|
|
||
| My local patched build stopped reproducing once I changed the restore path to run a short recovery burst instead of a single pass: | ||
|
|
||
| - immediately on visibility/focus restore | ||
| - again after ~120ms | ||
| - again after ~260ms | ||
|
|
||
| That suggests the terminal container is still settling for a short period after focus is restored, so a single repaint can happen too early. | ||
|
|
||
| ## Related issues | ||
|
|
||
| - #1830 | ||
| - #1873 | ||
| - #2507 | ||
| - #2968 | ||
| - #3080 | ||
| - #3208 | ||
| - #3309 | ||
|
|
||
| I am opening a PR with the small recovery-burst change that fixed the repro locally. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| ## Summary | ||
|
|
||
| - add a short recovery burst when a terminal becomes visible again after focus / visibility changes | ||
| - keep the existing throttle logic, but retry repaint/fit twice after the initial restore attempt | ||
| - add a regression test that models the new burst behavior | ||
|
|
||
| ## Why | ||
|
|
||
| The existing terminal restore path already does the right kinds of work on refocus: | ||
|
|
||
| - clear the WebGL texture atlas | ||
| - re-fit the terminal to its container | ||
| - force an xterm refresh | ||
|
|
||
| The remaining problem is timing. In practice, one refocus recovery can still happen slightly before the terminal container has fully settled after the app/window/workspace becomes visible again. That leaves stale blank space or stale terminal/TUI styling until some later repaint happens. | ||
|
|
||
| Running a short recovery burst fixed the repro locally: | ||
|
|
||
| - immediate restore | ||
| - second restore after ~120ms | ||
| - third restore after ~260ms | ||
|
|
||
| This keeps the change small and local to the focus/visibility recovery path, while giving xterm another chance to repaint once layout has actually stabilized. | ||
|
|
||
| ## Test plan | ||
|
|
||
| - `bun test ./src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.test.ts` | ||
|
|
||
| ## Related | ||
|
|
||
| - Closes #3321 | ||
| - Related: #1830 | ||
| - Related: #1873 | ||
| - Related: #2507 | ||
| - Related: #2968 | ||
| - Related: #3080 | ||
| - Related: #3208 | ||
| - Related: #3309 |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,13 +14,13 @@ import { | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| const currentYear = new Date().getFullYear(); | ||||||||||||||||||||||
| const author = pkg.author?.name ?? pkg.author; | ||||||||||||||||||||||
| const productName = pkg.productName; | ||||||||||||||||||||||
| const productName = "Superset Patched"; | ||||||||||||||||||||||
| const macIconPath = join(pkg.resources, "build/icons/icon.icns"); | ||||||||||||||||||||||
|
Comment on lines
+17
to
18
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
| const linuxIconPath = join(pkg.resources, "build/icons"); | ||||||||||||||||||||||
| const winIconPath = join(pkg.resources, "build/icons/icon.ico"); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const config: Configuration = { | ||||||||||||||||||||||
| appId: "com.superset.desktop", | ||||||||||||||||||||||
| appId: "com.superset.desktop.patched", | ||||||||||||||||||||||
| productName, | ||||||||||||||||||||||
| copyright: `Copyright © ${currentYear} — ${author}`, | ||||||||||||||||||||||
| electronVersion: pkg.devDependencies.electron.replace(/^\^/, ""), | ||||||||||||||||||||||
|
|
@@ -91,9 +91,9 @@ const config: Configuration = { | |||||||||||||||||||||
| ...(existsSync(macIconPath) ? { icon: macIconPath } : {}), | ||||||||||||||||||||||
| category: "public.app-category.utilities", | ||||||||||||||||||||||
| target: "default", | ||||||||||||||||||||||
| hardenedRuntime: true, | ||||||||||||||||||||||
| hardenedRuntime: false, | ||||||||||||||||||||||
| gatekeeperAssess: false, | ||||||||||||||||||||||
| notarize: true, | ||||||||||||||||||||||
| notarize: false, | ||||||||||||||||||||||
| entitlements: join(pkg.resources, "build/entitlements.mac.plist"), | ||||||||||||||||||||||
| entitlementsInherit: join( | ||||||||||||||||||||||
| pkg.resources, | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import { afterEach, describe, expect, it } from "bun:test"; | ||
| import { | ||
| existsSync, | ||
| mkdirSync, | ||
| mkdtempSync, | ||
| readFileSync, | ||
| rmSync, | ||
| writeFileSync, | ||
| } from "node:fs"; | ||
| import { join } from "node:path"; | ||
| import { | ||
| isPatchedDesktopBuild, | ||
| resetPatchedBrowserStateIfNeeded, | ||
| } from "./app-environment"; | ||
|
|
||
| describe("app-environment patched browser state reset", () => { | ||
| const tempDirs: string[] = []; | ||
|
|
||
| afterEach(() => { | ||
| for (const dir of tempDirs) { | ||
| rmSync(dir, { recursive: true, force: true }); | ||
| } | ||
| tempDirs.length = 0; | ||
| }); | ||
|
|
||
| it("detects patched desktop builds from the app bundle path", () => { | ||
| expect( | ||
| isPatchedDesktopBuild( | ||
| "/Applications/Superset Patched.app/Contents/MacOS/Superset Patched", | ||
| ), | ||
| ).toBe(true); | ||
| expect( | ||
| isPatchedDesktopBuild( | ||
| "/Applications/Superset.app/Contents/MacOS/Superset", | ||
| ), | ||
| ).toBe(false); | ||
| }); | ||
|
|
||
| it("clears browser-only persisted state once for patched builds", () => { | ||
| const homeDir = mkdtempSync("/tmp/superset-patched-browser-reset-"); | ||
| tempDirs.push(homeDir); | ||
| mkdirSync(join(homeDir, "Local Storage"), { recursive: true }); | ||
| mkdirSync(join(homeDir, "Partitions"), { recursive: true }); | ||
| writeFileSync(join(homeDir, "Preferences"), '{"foo":true}', "utf8"); | ||
| writeFileSync(join(homeDir, "app-state.json"), '{"tabsState":{}}', "utf8"); | ||
|
|
||
| resetPatchedBrowserStateIfNeeded( | ||
| homeDir, | ||
| "/Applications/Superset Patched.app/Contents/MacOS/Superset Patched", | ||
| ); | ||
|
|
||
| expect(existsSync(join(homeDir, "Local Storage"))).toBe(false); | ||
| expect(existsSync(join(homeDir, "Partitions"))).toBe(false); | ||
| expect(existsSync(join(homeDir, "Preferences"))).toBe(false); | ||
| expect(existsSync(join(homeDir, "app-state.json"))).toBe(true); | ||
| expect( | ||
| readFileSync(join(homeDir, ".patched-browser-state-reset-v1"), "utf8") | ||
| .length, | ||
| ).toBeGreaterThan(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOCAL_PATCHES.md,UPSTREAM_ISSUE_terminal_refocus_repaint.md, andUPSTREAM_PR_terminal_refocus_repaint.mdare 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).