Stop Settings navigation from retaining closed views (#4228)

* fix(desktop): stop retaining closed Settings views

Settings rotations left detached detail panes reachable from the web style registry's strong element sets. Backport the upstream weak cleanup design and cover the real Electron bundle with a forced-GC regression.\n\nKeep the established sibling-route topology; compact Back relies on route-name identity.

* test(app): synchronize Mermaid completion layout assertion

The streaming acceptance test can observe the intentional completion remount between visibility and layout sampling. Consult the existing completion promise around that measurement so the transition is not reported as diagram loss.

* test(app): isolate worktree restart from provider startup
This commit is contained in:
Mohamed Boudra
2026-09-02 21:27:30 +02:00
committed by GitHub
parent f75dff2322
commit 7178583612
7 changed files with 520 additions and 3 deletions
+10
View File
@@ -77,6 +77,16 @@ only use local param fallback during cold mount (`/` or empty pathname), or a
hidden workspace can overwrite the remembered workspace before Settings or
History returns.
Settings detail routes are separate siblings on purpose. Keep
`settings/[section]`, the host routes, the projects index, and project detail as
distinct route names. `router.dismissTo()` ultimately matches stack entries by
route name. A single catch-all Settings route would make project detail and the
projects index the same route; Back would update params in place and leave a
phantom detail entry underneath. The host routes also stay outside the
store-ready protected group so a cold host deep link can survive daemon startup.
Do not collapse this topology with a catch-all, `getId`, or
`dangerouslySingular` workaround.
## Agent Targets
Notifications and agent URLs enter the router with different authoritative
+12
View File
@@ -349,6 +349,18 @@ Gotchas:
- **The app shell uses one `AppearanceStyleBoundary`.** Runtime-patched numeric theme values are baked into Unistyles web classes rather than CSS variables, while parsed/memoized content also does not naturally re-run when appearance tokens change. The boundary sits below stable runtime providers in `app/_layout.tsx` and remounts the visual shell once. `applyAppearance` patches the active theme before inactive registry entries so its subscribers receive the committed values in the same update. Do not add local appearance keys or nested boundaries.
- **Dynamic font tokens stay widened.** `fontFamily`, `fontSize`, and `lineHeight` on `commonTheme` are annotated `string`/`number` (not narrowed by `as const`) so the updater's return assigns; the platform default stacks live in `DEFAULT_UI_FONT_STACK` / `DEFAULT_MONO_FONT_STACK`.
## Patching The Web Runtime
When backporting a Unistyles web fix, patch the TypeScript source and both
shipped JavaScript builds. Native Metro resolves the package's `react-native`
export to `src`, but browser and Electron Metro resolve its `browser` export to
`lib/module`; CommonJS consumers use `lib/commonjs`. A source-only patch leaves
Electron running the old code.
Register every dependency patch in `scripts/postinstall-patches.mjs`. A file in
`patches/` is inert unless that script knows which installed package activates
it.
## Debugging
To inspect what the Babel plugin sees, temporarily enable [`debug: true`](https://www.unistyl.es/v3/other/babel-plugin#debug) in `packages/app/babel.config.js`:
@@ -4,7 +4,7 @@ import { expect, type Page } from "@playwright/test";
import { metroTest as test } from "../support/fixtures";
import { gotoAppShell } from "../support/helpers/app";
import {
createIdleAgent,
createMockIdleAgent,
expectSessionRowArchived,
openSessions,
} from "../support/helpers/archive-tab";
@@ -97,7 +97,7 @@ test.describe("Worktree restore after daemon restart", () => {
createdProjectIds.add(worktree.projectKey);
createdWorktreeDirectories.add(worktree.workspaceDirectory);
const agent = await createIdleAgent(client, {
const agent = await createMockIdleAgent(client, {
cwd: worktree.workspaceDirectory,
workspaceId: worktree.workspaceId,
title: `restart-restore-${randomUUID().slice(0, 8)}`,
+11 -1
View File
@@ -13,6 +13,7 @@ import { experimental_createMCPClient } from "ai";
import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js";
import { chromium } from "playwright";
import { runAppearanceFontSizeRegression } from "./appearance-font-size.electron.mjs";
import { runSettingsMemoryRegression } from "./settings-memory.electron.mjs";
const scriptDir = path.dirname(fileURLToPath(import.meta.url));
const desktopDir = path.resolve(scriptDir, "..");
@@ -961,6 +962,15 @@ async function main() {
const page = await waitForAppPage(browser, expoPort);
const status = await waitForDesktopStatus(page);
const settingsMemory = await runSettingsMemoryRegression(page);
if (process.env.PASEO_DESKTOP_SETTINGS_MEMORY_ONLY === "1") {
writeJson(path.join(artifactDir, "result.json"), { settingsMemory });
console.log(
`Desktop Settings memory regression passed: ${settingsMemory.detachedAfterWarmRound} detached panes after warm and stress rotations.`,
);
return;
}
await runAppearanceFontSizeRegression(page);
const callerAgentId = await createCallerAgent(daemonPort);
@@ -978,7 +988,7 @@ async function main() {
callerAgentId,
artifactDir,
});
writeJson(path.join(artifactDir, "result.json"), report);
writeJson(path.join(artifactDir, "result.json"), { ...report, settingsMemory });
console.log(
`Browser desktop browser E2E passed: WebContents ${report.originalWebContentsId} remained ${report.finalWebContentsId}; viewport, inactive capture, focus continuity, list, snapshot, click, local-page selectors passed.`,
);
@@ -0,0 +1,160 @@
import { createWriteStream } from "node:fs";
const GC_ATTEMPTS = 6;
const SETTINGS_PANE_TEST_ID = "settings-detail-pane";
const STRESS_ROUNDS = 4;
// React Native Web's document-level pointer listener can retain its last target.
const MAX_TRANSIENT_DETACHED_PANES = 2;
const SETTINGS_DESTINATIONS = [
"General",
"Appearance",
"Layout",
"Editor",
"Shortcuts",
"Integrations",
"Notifications",
"Permissions",
"Diagnostics",
"About",
"Overview",
"Projects",
"Connections",
"Pair device",
"Agents",
"Metadata",
"Workspaces",
"Providers",
"Usage",
"Terminals",
"Plugins",
];
function assert(condition, message) {
if (!condition) throw new Error(message);
}
async function openSettingsDestination(page, title) {
const sidebar = page.locator('[data-testid="settings-sidebar"]:visible');
const button = sidebar.getByRole("button", { name: title, exact: true });
await button.waitFor({ state: "visible" });
await button.click();
await page.waitForFunction(
(expectedTitle) =>
[...document.querySelectorAll('[data-testid="settings-detail-header-title"]')].some(
(element) =>
element instanceof HTMLElement &&
element.getBoundingClientRect().width > 0 &&
element.textContent === expectedTitle,
),
title,
);
}
async function rotateSettings(page) {
await page.getByRole("button", { name: "Settings", exact: true }).click();
await page.locator('[data-testid="settings-sidebar"]:visible').waitFor({ state: "visible" });
for (const title of SETTINGS_DESTINATIONS) {
await openSettingsDestination(page, title);
}
await page.locator('[data-testid="settings-back-to-workspace"]:visible').click();
await page.waitForFunction(() => !location.pathname.startsWith("/settings"));
}
async function countDetachedSettingsPanes(session) {
const objectGroup = "settings-retention-check";
try {
const prototype = await session.send("Runtime.evaluate", {
expression: "HTMLDivElement.prototype",
objectGroup,
});
const prototypeObjectId = prototype.result.objectId;
assert(prototypeObjectId, "CDP did not return HTMLDivElement.prototype");
const objects = await session.send("Runtime.queryObjects", {
prototypeObjectId,
objectGroup,
});
const objectsId = objects.objects.objectId;
assert(objectsId, "CDP did not return HTMLDivElement instances");
const count = await session.send("Runtime.callFunctionOn", {
objectId: objectsId,
functionDeclaration: `function () {
return Array.prototype.reduce.call(this, (total, node) => {
const isDetachedSettingsPane =
node instanceof HTMLDivElement &&
!node.isConnected &&
node.getAttribute("data-testid") === ${JSON.stringify(SETTINGS_PANE_TEST_ID)};
return total + (isDetachedSettingsPane ? 1 : 0);
}, 0);
}`,
returnByValue: true,
});
assert(
typeof count.result.value === "number",
"CDP did not return a detached Settings pane count",
);
return count.result.value;
} finally {
await session.send("Runtime.releaseObjectGroup", { objectGroup }).catch(() => undefined);
}
}
async function collectDetachedSettingsPanes(session) {
let detachedPanes = Number.POSITIVE_INFINITY;
for (let attempt = 0; attempt < GC_ATTEMPTS; attempt += 1) {
await session.send("HeapProfiler.collectGarbage");
detachedPanes = await countDetachedSettingsPanes(session);
if (detachedPanes === 0) return 0;
}
return detachedPanes;
}
async function captureHeapSnapshot(session, outputPath) {
const output = createWriteStream(outputPath);
session.on("HeapProfiler.addHeapSnapshotChunk", ({ chunk }) => output.write(chunk));
await session.send("HeapProfiler.takeHeapSnapshot", {
reportProgress: false,
captureNumericValue: true,
exposeInternals: false,
});
await new Promise((resolve, reject) => {
output.end(resolve);
output.on("error", reject);
});
}
export async function runSettingsMemoryRegression(page) {
const session = await page.context().newCDPSession(page);
await session.send("HeapProfiler.enable");
await rotateSettings(page);
const detachedAfterWarmRound = await collectDetachedSettingsPanes(session);
for (let round = 0; round < STRESS_ROUNDS; round += 1) {
await rotateSettings(page);
}
const detachedAfterStress = await collectDetachedSettingsPanes(session);
if (process.env.PASEO_DESKTOP_SETTINGS_HEAP_SNAPSHOT) {
await captureHeapSnapshot(session, process.env.PASEO_DESKTOP_SETTINGS_HEAP_SNAPSHOT);
}
assert(
detachedAfterWarmRound <= MAX_TRANSIENT_DETACHED_PANES,
`A warm Settings rotation retained ${detachedAfterWarmRound} detached detail panes`,
);
assert(
detachedAfterStress <= detachedAfterWarmRound,
`Settings rotations grew detached detail panes from ${detachedAfterWarmRound} to ${detachedAfterStress}`,
);
return {
warmRounds: 1,
stressRounds: STRESS_ROUNDS,
detachedAfterWarmRound,
detachedAfterStress,
};
}
+319
View File
@@ -0,0 +1,319 @@
diff --git a/node_modules/react-native-unistyles/lib/commonjs/web/registry.js b/node_modules/react-native-unistyles/lib/commonjs/web/registry.js
index 120ff60..d1ff102 100644
--- a/node_modules/react-native-unistyles/lib/commonjs/web/registry.js
+++ b/node_modules/react-native-unistyles/lib/commonjs/web/registry.js
@@ -10,6 +10,17 @@ class UnistylesRegistry {
stylesheets = new Map();
stylesCache = new Set();
stylesCounter = new Map();
+ cleanupGeneration = 0;
+ finalizerTokens = new WeakMap();
+ finalizationRegistry = typeof FinalizationRegistry === 'undefined' ? undefined : new FinalizationRegistry(({
+ hash,
+ generation
+ }) => {
+ if (generation !== this.cleanupGeneration) {
+ return;
+ }
+ this.decrementAndMaybeCleanup(hash);
+ });
disposeListenersMap = new Map();
dependenciesMap = new Map();
constructor(services) {
@@ -50,26 +61,48 @@ class UnistylesRegistry {
this.disposeListenersMap.set(stylesheet, dispose);
};
connect = (ref, hash) => {
- const stylesCounter = this.stylesCounter.get(hash) ?? new Set();
- stylesCounter.add(ref);
- this.stylesCounter.set(hash, stylesCounter);
+ this.stylesCounter.set(hash, (this.stylesCounter.get(hash) ?? 0) + 1);
+ if (!this.finalizationRegistry) {
+ return;
+ }
+ const token = {};
+ const tokensByHash = this.finalizerTokens.get(ref) ?? new Map();
+ const tokens = tokensByHash.get(hash) ?? [];
+ tokens.push(token);
+ tokensByHash.set(hash, tokens);
+ this.finalizerTokens.set(ref, tokensByHash);
+ this.finalizationRegistry.register(ref, {
+ hash,
+ generation: this.cleanupGeneration
+ }, token);
};
remove = (ref, hash) => {
- const stylesCounter = this.stylesCounter.get(hash) ?? new Set();
- stylesCounter.delete(ref);
- if (stylesCounter.size === 0) {
- // Move this to the end of the event loop so the element is removed from the DOM
- return Promise.resolve().then(() => {
- // Check if element is still in the DOM
- if (document.querySelector(`.${hash}`)) {
- return false;
- }
- this.css.remove(hash);
- this.stylesCache.delete(hash);
- return true;
- });
+ const token = this.finalizerTokens.get(ref)?.get(hash)?.pop();
+ if (token) {
+ this.finalizationRegistry?.unregister(token);
+ }
+ return this.decrementAndMaybeCleanup(hash);
+ };
+ decrementAndMaybeCleanup = hash => {
+ const next = (this.stylesCounter.get(hash) ?? 0) - 1;
+ if (next > 0) {
+ this.stylesCounter.set(hash, next);
+ return Promise.resolve(false);
}
- return Promise.resolve(false);
+ this.stylesCounter.delete(hash);
+ // Move this to the end of the event loop so the element is removed from the DOM
+ return Promise.resolve().then(() => {
+ // Check if element is still in the DOM
+ if ((this.stylesCounter.get(hash) ?? 0) > 0) {
+ return false;
+ }
+ if (document.querySelector(`.${hash}`)) {
+ return false;
+ }
+ this.css.remove(hash);
+ this.stylesCache.delete(hash);
+ return true;
+ });
};
add = (value, forChild) => {
const generatedHash = (0, _utils.generateHash)(value);
@@ -91,7 +124,10 @@ class UnistylesRegistry {
this.css.add(hash, value);
};
reset = () => {
+ this.cleanupGeneration += 1;
+ this.disposeListenersMap.forEach(dispose => dispose());
this.css.reset();
+ this.stylesheets.clear();
this.stylesCache.clear();
this.dependenciesMap.clear();
this.disposeListenersMap.clear();
diff --git a/node_modules/react-native-unistyles/lib/module/web/registry.js b/node_modules/react-native-unistyles/lib/module/web/registry.js
index 6362f43..8e769f8 100644
--- a/node_modules/react-native-unistyles/lib/module/web/registry.js
+++ b/node_modules/react-native-unistyles/lib/module/web/registry.js
@@ -6,6 +6,17 @@ export class UnistylesRegistry {
stylesheets = new Map();
stylesCache = new Set();
stylesCounter = new Map();
+ cleanupGeneration = 0;
+ finalizerTokens = new WeakMap();
+ finalizationRegistry = typeof FinalizationRegistry === 'undefined' ? undefined : new FinalizationRegistry(({
+ hash,
+ generation
+ }) => {
+ if (generation !== this.cleanupGeneration) {
+ return;
+ }
+ this.decrementAndMaybeCleanup(hash);
+ });
disposeListenersMap = new Map();
dependenciesMap = new Map();
constructor(services) {
@@ -46,26 +57,48 @@ export class UnistylesRegistry {
this.disposeListenersMap.set(stylesheet, dispose);
};
connect = (ref, hash) => {
- const stylesCounter = this.stylesCounter.get(hash) ?? new Set();
- stylesCounter.add(ref);
- this.stylesCounter.set(hash, stylesCounter);
+ this.stylesCounter.set(hash, (this.stylesCounter.get(hash) ?? 0) + 1);
+ if (!this.finalizationRegistry) {
+ return;
+ }
+ const token = {};
+ const tokensByHash = this.finalizerTokens.get(ref) ?? new Map();
+ const tokens = tokensByHash.get(hash) ?? [];
+ tokens.push(token);
+ tokensByHash.set(hash, tokens);
+ this.finalizerTokens.set(ref, tokensByHash);
+ this.finalizationRegistry.register(ref, {
+ hash,
+ generation: this.cleanupGeneration
+ }, token);
};
remove = (ref, hash) => {
- const stylesCounter = this.stylesCounter.get(hash) ?? new Set();
- stylesCounter.delete(ref);
- if (stylesCounter.size === 0) {
- // Move this to the end of the event loop so the element is removed from the DOM
- return Promise.resolve().then(() => {
- // Check if element is still in the DOM
- if (document.querySelector(`.${hash}`)) {
- return false;
- }
- this.css.remove(hash);
- this.stylesCache.delete(hash);
- return true;
- });
+ const token = this.finalizerTokens.get(ref)?.get(hash)?.pop();
+ if (token) {
+ this.finalizationRegistry?.unregister(token);
+ }
+ return this.decrementAndMaybeCleanup(hash);
+ };
+ decrementAndMaybeCleanup = hash => {
+ const next = (this.stylesCounter.get(hash) ?? 0) - 1;
+ if (next > 0) {
+ this.stylesCounter.set(hash, next);
+ return Promise.resolve(false);
}
- return Promise.resolve(false);
+ this.stylesCounter.delete(hash);
+ // Move this to the end of the event loop so the element is removed from the DOM
+ return Promise.resolve().then(() => {
+ // Check if element is still in the DOM
+ if ((this.stylesCounter.get(hash) ?? 0) > 0) {
+ return false;
+ }
+ if (document.querySelector(`.${hash}`)) {
+ return false;
+ }
+ this.css.remove(hash);
+ this.stylesCache.delete(hash);
+ return true;
+ });
};
add = (value, forChild) => {
const generatedHash = generateHash(value);
@@ -87,7 +120,10 @@ export class UnistylesRegistry {
this.css.add(hash, value);
};
reset = () => {
+ this.cleanupGeneration += 1;
+ this.disposeListenersMap.forEach(dispose => dispose());
this.css.reset();
+ this.stylesheets.clear();
this.stylesCache.clear();
this.dependenciesMap.clear();
this.disposeListenersMap.clear();
diff --git a/node_modules/react-native-unistyles/src/web/registry.ts b/node_modules/react-native-unistyles/src/web/registry.ts
index 39e0d9d..a21ba95 100644
--- a/node_modules/react-native-unistyles/src/web/registry.ts
+++ b/node_modules/react-native-unistyles/src/web/registry.ts
@@ -9,7 +9,24 @@ import { error, extractUnistyleDependencies, generateHash } from './utils'
export class UnistylesRegistry {
private readonly stylesheets = new Map<StyleSheetWithSuperPowers<StyleSheet>, StyleSheet>()
private readonly stylesCache = new Set<string>()
- private readonly stylesCounter = new Map<string, Set<HTMLElement>>()
+ // Use counter instead of Set<HTMLElement> to avoid retaining detached DOM nodes
+ // FR is a fallback when remove() is missed
+ private readonly stylesCounter = new Map<string, number>()
+ // Bumped on reset() so stale FR callbacks are ignored
+ private cleanupGeneration = 0
+ // Per-registration unregister token, so remove() cancels only its own registration
+ private readonly finalizerTokens = new WeakMap<HTMLElement, Map<string, Array<object>>>()
+ // FR isn't available on every runtime - fall back to counter-only cleanup
+ private readonly finalizationRegistry =
+ typeof FinalizationRegistry === 'undefined'
+ ? undefined
+ : new FinalizationRegistry<{ hash: string; generation: number }>(({ hash, generation }) => {
+ if (generation !== this.cleanupGeneration) {
+ return
+ }
+
+ this.decrementAndMaybeCleanup(hash)
+ })
private readonly disposeListenersMap = new Map<object, VoidFunction>()
private readonly dependenciesMap = new Map<StyleSheetWithSuperPowers<StyleSheet>, Set<UnistyleDependency>>()
readonly css: CSSState
@@ -78,33 +95,61 @@ export class UnistylesRegistry {
}
connect = (ref: HTMLElement, hash: string) => {
- const stylesCounter = this.stylesCounter.get(hash) ?? new Set()
+ this.stylesCounter.set(hash, (this.stylesCounter.get(hash) ?? 0) + 1)
- stylesCounter.add(ref)
- this.stylesCounter.set(hash, stylesCounter)
+ if (!this.finalizationRegistry) {
+ return
+ }
+
+ const token = {}
+ const tokensByHash = this.finalizerTokens.get(ref) ?? new Map<string, Array<object>>()
+ const tokens = tokensByHash.get(hash) ?? []
+
+ tokens.push(token)
+ tokensByHash.set(hash, tokens)
+ this.finalizerTokens.set(ref, tokensByHash)
+ this.finalizationRegistry.register(ref, { hash, generation: this.cleanupGeneration }, token)
}
remove = (ref: HTMLElement, hash: string) => {
- const stylesCounter = this.stylesCounter.get(hash) ?? new Set()
+ const token = this.finalizerTokens.get(ref)?.get(hash)?.pop()
- stylesCounter.delete(ref)
+ if (token) {
+ this.finalizationRegistry?.unregister(token)
+ }
- if (stylesCounter.size === 0) {
- // Move this to the end of the event loop so the element is removed from the DOM
- return Promise.resolve().then(() => {
- // Check if element is still in the DOM
- if (document.querySelector(`.${hash}`)) {
- return false
- }
+ return this.decrementAndMaybeCleanup(hash)
+ }
+
+ private decrementAndMaybeCleanup = (hash: string): Promise<boolean> => {
+ const next = (this.stylesCounter.get(hash) ?? 0) - 1
- this.css.remove(hash)
- this.stylesCache.delete(hash)
+ if (next > 0) {
+ this.stylesCounter.set(hash, next)
- return true
- })
+ return Promise.resolve(false)
}
- return Promise.resolve(false)
+ // Drop the empty bucket from the Map
+ this.stylesCounter.delete(hash)
+
+ // Move this to the end of the event loop so the element is removed from the DOM
+ return Promise.resolve().then(() => {
+ // New connect raced in during the microtask
+ if ((this.stylesCounter.get(hash) ?? 0) > 0) {
+ return false
+ }
+
+ // Check if element is still in the DOM
+ if (document.querySelector(`.${hash}`)) {
+ return false
+ }
+
+ this.css.remove(hash)
+ this.stylesCache.delete(hash)
+
+ return true
+ })
}
add = (value: UnistylesValues, forChild?: boolean) => {
@@ -126,7 +171,11 @@ export class UnistylesRegistry {
}
reset = () => {
+ // Invalidate pending FR callbacks from this generation
+ this.cleanupGeneration += 1
+ this.disposeListenersMap.forEach((dispose) => dispose())
this.css.reset()
+ this.stylesheets.clear()
this.stylesCache.clear()
this.dependenciesMap.clear()
this.disposeListenersMap.clear()
+6
View File
@@ -16,6 +16,12 @@ const patchedPackages = [
nodeModulesPath: "node_modules/react-native",
patchPrefix: "react-native+",
},
// Remove after react-native-unistyles ships
// https://github.com/jpudysz/react-native-unistyles/pull/1203.
{
nodeModulesPath: "node_modules/react-native-unistyles",
patchPrefix: "react-native-unistyles+",
},
{
nodeModulesPath: "node_modules/react-native-draggable-flatlist",
patchPrefix: "react-native-draggable-flatlist+",