You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "Astro-Han (via GitHub)" <gi...@apache.org> on 2026/08/29 03:06:11 UTC
[PR] refactor(desktop): gate the shell's hook call sites and move the first one [maka]
Astro-Han opened a new pull request, #4125:
URL: https://github.com/apache/maka/pull/4125
## Summary
`AppShellContent` calls 40 hooks across 70 call sites, and every one of them scopes its state to the entire tree. That is why a session switch produces about 19 React commits, 14 of them full-tree renders, of which 96–99% is recoverable work. The fix is to move call sites into feature providers, one feature at a time; this PR adds the gate that makes that progress visible, and moves the first one.
**Why a gate rather than a checklist.** #3439 tracks this as a list of extractions, but extracting a feature into its own slice does not change the scope of any state. Session Navigation proves it: a complete `model / controller / ui / ports` slice whose controller is still invoked from AppShell's render body, so the rail is still re-rendered by every shell commit. A call site, unlike an extraction, has a definition of done — the hook is either called there or it is not.
So `scripts/check-app-shell-hooks.mjs` counts them against a hand-edited inventory that may only shrink. A migration deletes its entry and the same diff shows the gate converging; a hook that appears without one fails, which is not a prohibition but a refusal to let it happen silently. There is deliberately no `--write` mode — regenerating the inventory on demand would let a new hook be accepted by rerunning a command, and the friction is the point. `useMemo` / `useCallback` / `useRef` / `useId` are ignored: they hold no state and subscribe to nothing, so they cannot widen what a render reaches.
**The first migration.** `useComposerMentions` reloads the Skill catalog on every session switch and on every MCP or session change event, several times per switch. Held in the shell, each reload set state above the whole tree to repaint two popups. It now lives in a `ComposerMentionsProvider`, whose `children` is the element AppShell already built, so React bails out of the subtree. That also removed the reason to thread `mentionSkills`, `mentionSkillsUnavailable`, `mentionSkillsLoading` and `onSearchMentionFiles` through the workbar controller, host and surface to reach two consumers; `QuoteCompanionPanel` reads the same context directly.
The gate goes from 40 hooks / 70 call sites to **39 / 69**.
The gate and the migration ship together on purpose. A gate with nothing behind it verifies nothing, and a migration with no gate does not hold — the same reason the follow-up will land the catalog authority and the rail's subscription as one change rather than two.
Refs #4109
Refs #3439
## Verification
- `npm run check:app-shell-hooks` → `ok (39 hooks, 69 call sites in AppShellContent)`
- `node --test --test-concurrency=1 scripts/check-app-shell-hooks.test.mjs` → 10 passed
- `npx playwright test composer-skill-invocation composer-plus-menu-stability` → 8 passed, covering the catalog-refresh, context-switch and failed-refresh paths the projection has to keep
- `npx tsc -p apps/desktop/tsconfig.renderer.json --noEmit`, `npm run format:check`, `npm run lint`, `node scripts/asf-license-headers.mjs check` → clean
- Not run: the repository-wide suites, and any performance measurement of this PR on its own. The mention projection is one of ~19 commits per switch; a single-slice timing claim would not survive the run-to-run variance, so the gate's count is the honest measure here. The timed acceptance belongs to the rail change in #4109.
## Review focus
`ComposerMentionsProvider` wraps AppShell's existing return value, so the `return (` block is intentionally not re-indented — re-indenting ~500 lines would bury the change. Rendered output is unchanged.
The projection itself was moved, not rewritten: the fail-closed `contextKey` derivation and the same-context refresh that keeps its settled verdict (#2667) are byte-for-byte the same, which is what the two e2e specs check.
## AI use
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Claude Code. Diagnosis (CDP profiling, `Function.prototype.bind` interception of React's `dispatchSetState`, fiber-tree walks on a running dev build), the gate script and its tests, and the mention migration. Affected commits carry `Generated-by` trailers. Reviewed by the human contributor of record before submission.
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [ ] Yes — described under Summary above
- [x] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] perf(desktop): give the shell's state the scope of its readers [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #4125:
URL: https://github.com/apache/maka/pull/4125
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] perf(desktop): give the shell's state the scope of its readers [maka]
Posted by "M4n5ter (via GitHub)" <gi...@apache.org>.
M4n5ter commented on code in PR #4125:
URL: https://github.com/apache/maka/pull/4125#discussion_r3885886782
##########
apps/desktop/src/renderer/features/session-navigation/ui/session-navigation-provider.tsx:
##########
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { useLayoutEffect, useMemo, useRef, type ReactNode } from 'react';
+import type { ProjectRecord } from '@maka/core/project';
+import type { ScheduledTask } from '@maka/core/scheduled-task';
+import {
+ SessionRailProvider,
+ type NavModuleMemory,
+ type NavSelection,
+ type ProjectRowActions,
+ type SessionRailChrome,
+ type SessionRailData,
+ type SessionRowActions,
+ type SidebarUpdateReminder,
+} from '@maka/ui';
+import {
+ useSessionNavigationController,
+ type SessionNavigationPorts,
+} from '../controller/use-session-navigation-controller.js';
+import type { SessionNavigationRowActions } from '../controller/session-row-actions.js';
+import {
+ SESSION_LIST_EXPANDED_MAX_WIDTH,
+ SESSION_LIST_EXPANDED_MIN_WIDTH,
+} from '../model/session-list-layout.js';
+import type { SessionRailProjection } from '../model/session-rail.js';
+import { sessionRailLayoutStore } from '../model/session-rail-layout-store.js';
+import type { SessionNavigationSession } from '../ports.js';
+
+/** The chrome the shell owns and the rail only displays. */
+export interface SessionNavigationChromeInput {
+ selection: NavSelection;
+ scheduledTasks?: readonly ScheduledTask[];
+ moduleMemory?: NavModuleMemory;
+ updateReminder?: SidebarUpdateReminder;
+ workHubActive: boolean;
+ workHubEntry?: { active: boolean; label: string; onSelect(): void };
+ projectActions?: ProjectRowActions;
+ onSelect(selection: NavSelection): void;
+ onOpenSettings(): void;
+ onOpenUpdate?(): void;
+ onNew(): void;
+ onExitWorkHub(): void;
+ onSelectSession(sessionId: string): void;
+}
+
+export interface SessionNavigationProviderProps extends SessionNavigationChromeInput {
+ rail: SessionRailProjection<SessionNavigationSession>;
+ projects: readonly ProjectRecord[];
+ streamingSessionIds: ReadonlySet<string>;
+ staleSessionIds: ReadonlySet<string>;
+ ports: SessionNavigationPorts;
+ /**
+ * Where the shell reads the row mutations it issues from elsewhere — the
+ * archived-tasks sweep, the chat header's rename. They are calls made from
+ * event handlers, never values read during a render, so a ref is the whole
+ * carrier they need and the shell does not re-render to receive them.
+ */
+ commandsRef: { current: SessionNavigationRowActions | null };
+ children?: ReactNode;
+}
+
+/**
+ * The Session rail's own scope (#4109).
+ *
+ * The shell renders this on every one of its ~14 commits per session switch,
+ * and that is fine: this component is one fiber, its `children` element is
+ * built once by the shell, and React skips an unchanged child. What reaches the
+ * rail below is the two context values — and those change when the rail's data
+ * changes, not when the shell renders. The rail's ~1,000 fibers are on the
+ * first, the few dozen fibers of permanent chrome on the second.
+ */
+export function SessionNavigationProvider(props: SessionNavigationProviderProps) {
+ const controller = useSessionNavigationController({
+ rail: props.rail,
+ projects: props.projects,
+ ports: props.ports,
+ });
+
+ useLayoutEffect(() => {
+ props.commandsRef.current = controller.commands;
+ }, [controller.commands, props.commandsRef]);
+
+ const rowActions = useMemo<SessionRowActions>(
+ () => ({
+ onToggleFlag: (sessionId, next) => {
+ void controller.commands.flagSession(sessionId, next);
+ },
+ onArchive: (sessionId) => {
+ void controller.commands.archiveSession(sessionId);
+ },
+ onUnarchive: (sessionId) => {
+ void controller.commands.unarchiveSession(sessionId);
+ },
+ onRename: (sessionId, name) => {
+ void controller.commands.renameSession(sessionId, name);
+ },
+ onDelete: (sessionId) => {
+ void controller.commands.deleteSession(sessionId);
+ },
+ }),
+ [controller.commands],
+ );
+
+ // Project row mutations are commands too, and they arrive from a different
+ // feature entirely. Read through a ref for the same reason as the ports: what
+ // the rail needs is that they can be CALLED, and their identity says nothing
+ // about whether a row should be redrawn. Only their presence does, so that is
+ // what this depends on.
+ const chromeRef = useRef(props);
+ useLayoutEffect(() => {
+ chromeRef.current = props;
+ });
+ const hasProjectActions = props.projectActions !== undefined;
+ const hasRelink = props.projectActions?.onRelink !== undefined;
+ const projectActions = useMemo<ProjectRowActions | undefined>(
+ () =>
+ hasProjectActions
+ ? {
+ onNew: (projectId) => chromeRef.current.projectActions?.onNew(projectId),
+ onRename: (projectId, name) =>
+ chromeRef.current.projectActions?.onRename(projectId, name),
+ onArchive: (projectId) => chromeRef.current.projectActions?.onArchive(projectId),
+ onRestore: (projectId) => chromeRef.current.projectActions?.onRestore(projectId),
+ ...(hasRelink
+ ? {
+ onRelink: (projectId: string) =>
+ chromeRef.current.projectActions?.onRelink?.(projectId),
+ }
+ : {}),
+ }
+ : undefined,
+ [hasProjectActions, hasRelink],
+ );
+
+ const data = useMemo<SessionRailData>(
+ () => ({
+ sessions: props.rail.sessions,
+ activeId: props.workHubActive ? undefined : props.rail.activeRowId,
+ streamingSessionIds: props.streamingSessionIds,
+ staleSessionIds: props.staleSessionIds,
+ worktreeSessionIds: controller.selectors.worktreeSessionIds,
+ groups: controller.layout.viewMode === 'project' ? controller.selectors.groups : undefined,
+ groupVariant: controller.layout.viewMode,
+ sessionMeta: controller.selectors.sessionMeta,
+ onSelectSession: props.onSelectSession,
+ rowActions,
+ projectActions,
+ }),
+ [
+ controller.layout.viewMode,
+ controller.selectors.groups,
+ controller.selectors.sessionMeta,
+ controller.selectors.worktreeSessionIds,
+ props.onSelectSession,
+ projectActions,
+ props.rail,
+ props.staleSessionIds,
+ props.streamingSessionIds,
+ props.workHubActive,
+ rowActions,
+ ],
+ );
+
+ // Deliberately NOT memoized. Its readers are the nav rows and the footer —
+ // a few dozen fibers — and every field on it follows the shell, so a
+ // comparator here would run more often than it would save.
+ const chrome: SessionRailChrome = {
+ collapsed: controller.layout.collapsed,
+ onCollapsedChange: sessionRailLayoutStore.setCollapsed,
+ collapseHandleRef: sessionRailLayoutStore.collapseHandleRef,
+ width: controller.layout.width,
+ onWidthChange: sessionRailLayoutStore.setWidth,
Review Comment:
[P2] Do not persist Astryx's collapsed-width sentinel as the user's expanded width.
Astryx calls `onSizeChange(0)` when the rail collapses, including drag-collapse. This direct forwarding sends that zero into `sessionRailLayoutStore.setWidth`, which clamps it to 180 and writes `maka-chat-list-width-v1` after 200 ms. A reachable sequence is: resize the expanded rail to 400 px, collapse it, wait for persistence, reload or restart while it is collapsed, then expand it. The saved preference is now 180 px instead of 400 px. The old host avoided this by forwarding only values at or above `SESSION_LIST_EXPANDED_MIN_WIDTH`. Please keep the collapsed geometry separate from the remembered expanded width (or ignore the collapse sentinel here) and cover collapse + reload + expand.
---
_Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong._
<details><summary>简体中文</summary>
_本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。_
</details>
##########
scripts/check-app-shell-hooks.mjs:
##########
@@ -0,0 +1,411 @@
+#!/usr/bin/env node
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Convergence gate for #4109: which hooks are still called in the render bodies
+ * of the shell components that sit above the whole tree.
+ *
+ * In React the position in the tree IS the scope of the state. A hook called in
+ * the shell's render body has the whole tree as its scope; the same hook called
+ * in a provider has its readers as its scope. `AppShellContent` today carries
+ * 536 hooks above the entire tree, so a session switch produces ~19 commits,
+ * ~14 of them full-tree renders, of which 96-99% is recoverable work.
+ *
+ * The fix is to move call sites, one feature at a time. That is slow, and a
+ * checklist cannot tell whether it is progressing — extracting a feature into
+ * its own slice does not change the scope of any state, and Session Navigation
+ * proves it: a complete `model / controller / ui / ports` slice whose
+ * controller is still invoked from this render body.
+ *
+ * A call site does have a definition of done, so this gate counts them. The
+ * inventory below is EXACT, not a ceiling: growing past an entry fails, and so
+ * does falling below one. Only failing on growth would let the counts drift
+ * quietly downward and record no progress, which is the one thing this gate
+ * exists to make visible. The cost is real — a change that removes an effect
+ * has to edit this file — and that edit IS the record.
+ *
+ * Deliberately NOT a `--write` mode. Regenerating the inventory on demand would
+ * let a new hook be accepted by rerunning a command, and the friction of
+ * editing it by hand is the point. This is the opposite shape from
+ * `check-astryx-surface-inventory.mjs`, whose invariant is "regenerate and
+ * compare bytes, never a hand-written list". That is right for an inventory
+ * that only has to describe the tree; this one has to RESIST it.
+ *
+ * Two shapes of migration, because the failure message cannot tell them apart:
+ * when nothing in the shell body reads the feature, the call site moves into a
+ * provider and React bails out of `children` (the mention catalog, #4109) — the
+ * entry disappears. When the shell body DOES read it, moving the call site
+ * still leaves a selector read behind, and the honest outcome is a smaller
+ * entry rather than none. "Call it from the provider" is not advice to follow
+ * blindly: cross-feature intent the shell issues must stay an explicit command,
+ * never an implicit subscription bought to lower a number.
+ *
+ * The count is unweighted, so it measures migration progress and not render
+ * cost: a subscription that fires five times per session switch and a hook that
+ * runs once at mount both count as one. Falling counts do not by themselves
+ * demonstrate a faster switch.
+ *
+ * Both `AppShell` and `AppShellContent` are counted. `AppShell` wraps the
+ * content in the root providers, so a hook hoisted one level up has if anything
+ * a WIDER scope — an inventory that watched only the inner component would
+ * accept that move as progress.
+ *
+ * The counting is textual, because TypeScript 7 exposes no stable parser to
+ * JavaScript (only `typescript/unstable/*`), and a gate that goes red whenever
+ * a dependency moves is its own kind of noise. Textual counting is therefore
+ * written to fail closed: `stripNonCode` blanks comments, strings and regex
+ * literals in place, the body is delimited by brace balancing over that blanked
+ * text rather than by a `\n}\n` guess, and a hook name is only counted where a
+ * call actually follows it — including through an explicit type argument, which
+ * an earlier version of this gate silently skipped over.
+ *
+ * Purely derived hooks (`useMemo`, `useCallback`, `useRef`, `useId`) are
+ * ignored: they hold no state and subscribe to nothing, so they cannot widen
+ * the scope of a render. Everything else counts, including React's own
+ * `useState` / `useEffect`, because a custom hook is only a name for them.
+ *
+ * Run: npm run check:app-shell-hooks
+ * Fix: move the call site into the feature's provider, then delete its entry.
+ */
+import { readFileSync } from 'node:fs';
+import { join } from 'node:path';
+import { fileURLToPath, pathToFileURL } from 'node:url';
+
+const root = join(fileURLToPath(new URL('..', import.meta.url)));
+const shellFile = 'apps/desktop/src/renderer/app-shell.tsx';
+
+/** Hooks that hold no state and subscribe to nothing. */
+const DERIVED_HOOKS = new Set(['useMemo', 'useCallback', 'useRef', 'useId']);
+
+/**
+ * Hooks still called in each shell component's render body, with the number of
+ * call sites. This inventory may only shrink. Each entry is one scope that is
+ * still the whole tree rather than its readers.
+ */
+export const ALLOWED = {
+ AppShell: {
+ useState: 2,
+ useSystemUiLocale: 1,
+ },
+ AppShellContent: {
+ useActiveExecutionBoundary: 1,
+ useActiveSessionEvents: 1,
+ useAppShellBootstrapSubscriptions: 1,
+ useAppShellComposerQuotes: 1,
+ useAppShellHostEffects: 1,
+ useAppShellNavRefSync: 1,
+ useAppShellPersistenceEffects: 1,
+ useAppShellProjectContext: 1,
+ useAppShellSessionUiReads: 1,
+ useAppShellSessionWorkspace: 1,
+ useAppShellTurnPresentation: 1,
+ useCommandPalette: 1,
+ useComposerAttachments: 1,
+ useEffect: 14,
+ useGoalController: 1,
+ useKeyboardHelp: 1,
+ useLayoutEffect: 2,
+ useModuleHubController: 1,
+ useNewTaskChoice: 1,
+ useOnboardingSnapshot: 1,
+ usePlanModeState: 1,
+ useSessionEventHealthPolling: 1,
+ // Replaces `useSessionNavigationController`, which is now called inside
+ // `SessionNavigationProvider`. The entry shrinks rather than disappearing,
+ // because the shell body does read the rail: the command palette lists the
+ // same visible sessions, the titlebar shows the linked parent, and the
+ // frame publishes the rail's width as `--maka-sidenav-width`. What is left
+ // holds nothing — three `useMemo`s over the catalog the shell already reads
+ // and one subscription to the rail's geometry — where the controller it
+ // replaces put three `useState`, four effects and a `useStableActions`
+ // facade on this fiber.
+ useSessionNavigationReads: 1,
+ useSessionSettingIntent: 2,
+ useSettingsModal: 1,
+ useShellAppearance: 1,
+ useShellChatModel: 1,
+ useShellConnections: 3,
+ useShellLiveTurn: 1,
+ useShellMemoryPill: 1,
+ useShellResume: 1,
+ useShellRunUpdates: 1,
+ useShellSearch: 1,
+ useStableActions: 7,
+ useState: 16,
+ useTaskEntryController: 1,
+ useTaskSubmissionReadiness: 1,
+ useToast: 1,
+ // The last of the three `useKeyedPendingRegistry` call sites this entry
+ // replaces: #4113 moved the other two onto the session UI store, which is
+ // already an external store, so their scope left this fiber entirely. This
+ // one stays because the shell body reads `keys` to build the turn footer's
+ // disabled mask.
+ useTurnActionRegistry: 1,
+ useWorkbarController: 1,
+ },
+};
+
+/**
+ * After these, a `/` opens a regex literal; anywhere else it is division.
+ *
+ * `<` and `>` are deliberately absent. They would qualify in expression
+ * position, but in TSX they are far more often a tag: treating the `/` of
+ * `</div>` as the start of a regex swallows everything up to the next slash,
+ * which is exactly the silent under-count this gate must not have.
+ */
+const REGEX_MAY_FOLLOW = new Set([
+ '(',
+ ',',
+ '=',
+ ':',
+ '[',
+ '!',
+ '&',
+ '|',
+ '?',
+ '{',
+ '}',
+ ';',
+ '+',
+ '-',
+ '*',
+ '%',
+ '^',
+ '~',
+ '\n',
+]);
+
+/**
+ * Blanks comments, string bodies and regex literals, replacing each with spaces
+ * so that offsets and line numbers still line up with the original.
+ *
+ * Prose naming a hook is not a call site, and the shell's comments name hooks
+ * at length. The two cases worth spelling out are the ones that break a naive
+ * scanner: an apostrophe in JSX text (`<span>don't</span>`) is not a string
+ * delimiter, and a regex literal may contain quotes (`/['"]/`) that would
+ * otherwise swallow the rest of the file. Both would UNDER-count, which is the
+ * dangerous direction for a gate whose only product is a number.
+ */
+export function stripNonCode(source) {
+ const out = [...source];
+ let index = 0;
+ let lastMeaningful = '\n';
+ const blank = (from, to) => {
+ for (let i = from; i < to && i < out.length; i += 1) {
+ if (out[i] !== '\n') out[i] = ' ';
+ }
+ };
+ while (index < source.length) {
+ const char = source[index];
+ const next = source[index + 1];
+ if (char === '/' && next === '/') {
+ const end = source.indexOf('\n', index);
+ const stop = end === -1 ? source.length : end;
+ blank(index, stop);
+ index = stop;
+ continue;
+ }
+ if (char === '/' && next === '*') {
+ const end = source.indexOf('*/', index + 2);
+ const stop = end === -1 ? source.length : end + 2;
+ blank(index, stop);
+ index = stop;
+ continue;
+ }
+ if (char === '/' && REGEX_MAY_FOLLOW.has(lastMeaningful)) {
+ let scan = index + 1;
+ let inClass = false;
+ while (scan < source.length) {
+ const c = source[scan];
+ if (c === '\\') scan += 2;
+ else if (c === '[') (inClass = true), (scan += 1);
+ else if (c === ']') (inClass = false), (scan += 1);
+ else if (c === '\n') break;
+ else if (c === '/' && !inClass) break;
+ else scan += 1;
+ }
+ // An unterminated candidate was division after all; leave it alone.
+ if (scan < source.length && source[scan] === '/') {
+ blank(index, scan + 1);
+ lastMeaningful = '/';
+ index = scan + 1;
+ continue;
+ }
+ }
+ if (char === "'" || char === '"' || char === '`') {
+ // In JSX text an apostrophe follows a word or a closing bracket, and no
+ // JavaScript expression can put a string literal there.
+ if (char === "'" && /[A-Za-z0-9_$>)\]}]/.test(lastMeaningful)) {
+ lastMeaningful = char;
+ index += 1;
+ continue;
+ }
+ let scan = index + 1;
+ while (scan < source.length && source[scan] !== char) {
+ scan += source[scan] === '\\' ? 2 : 1;
+ }
+ blank(index + 1, scan);
+ lastMeaningful = char;
+ index = Math.min(scan + 1, source.length);
+ continue;
+ }
+ if (!/\s/.test(char) || char === '\n') lastMeaningful = char;
+ index += 1;
+ }
+ return out.join('');
+}
+
+/**
+ * The body of a top-level component, delimited by brace balancing over text
+ * whose strings and comments are already blanked. A `\n}\n` guess would stop at
+ * the first brace that formatting happened to put in column zero.
+ */
+export function findComponentBody(blanked, name) {
+ const declaration = new RegExp(
+ String.raw`^(?:export\s+)?(?:default\s+)?(?:async\s+)?function\s+${name}\b|^(?:export\s+)?(?:default\s+)?const\s+${name}\s*(?::[^=]+)?=`,
+ 'm',
+ );
+ const match = declaration.exec(blanked);
+ if (match === null) return null;
+ // Skip the parameter list. `function AppShellContent({ ... })` opens a brace
+ // for its destructured parameter, and balancing from there returns the
+ // parameter object rather than the body.
+ const paren = blanked.indexOf('(', match.index);
+ if (paren === -1) return null;
+ let parens = 0;
+ let afterParams = -1;
+ for (let i = paren; i < blanked.length; i += 1) {
+ if (blanked[i] === '(') parens += 1;
+ else if (blanked[i] === ')') {
+ parens -= 1;
+ if (parens === 0) {
+ afterParams = i + 1;
+ break;
+ }
+ }
+ }
+ if (afterParams === -1) return null;
+ const open = blanked.indexOf('{', afterParams);
+ if (open === -1) return null;
+ let depth = 0;
+ for (let i = open; i < blanked.length; i += 1) {
+ if (blanked[i] === '{') depth += 1;
+ else if (blanked[i] === '}') {
+ depth -= 1;
+ if (depth === 0) return blanked.slice(open, i + 1);
+ }
+ }
+ return null;
+}
+
+/**
+ * `useX(`, `useX<T>(` and `useX\n (` all call a hook; `obj.useX(` and
+ * `ReturnType<typeof useX>` do not.
+ */
+// One level of nesting is enough for `useX<Record<string, number>>(...)`;
+// deeper generics in a hook call do not occur here and would only cost a
+// miscount in the safe direction (a failure, not a silent pass).
+const TYPE_ARGUMENTS = String.raw`<(?:[^<>()]|<[^<>()]*>)*>`;
+const HOOK_CALL = new RegExp(
Review Comment:
[P2] Count every supported React hook call form in this convergence gate.
This expression only recognizes unqualified names matching `use[A-Z]`; the adjacent test even fixes `React.useState(0)` as a non-hook. React 19 also has the supported bare `use(...)` hook, which does not match the capital-letter rule. Both forms are valid in this repository and I found no lint rule that forbids them. Feeding `React.useState(0); useToast()` or `use(Promise.resolve(1)); useToast()` through the exported parser reports only `{ useToast: 1 }`. A future AppShell subscription can therefore widen the shell's render scope while this exact inventory remains green. Please make the scanner import-aware, or at minimum count the React namespace forms and bare `use`, and keep adversarial tests for both.
---
_Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong._
<details><summary>简体中文</summary>
_本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。_
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] perf(desktop): give the shell's state the scope of its readers [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #4125:
URL: https://github.com/apache/maka/pull/4125#discussion_r3885901392
##########
scripts/check-app-shell-hooks.mjs:
##########
@@ -0,0 +1,411 @@
+#!/usr/bin/env node
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Convergence gate for #4109: which hooks are still called in the render bodies
+ * of the shell components that sit above the whole tree.
+ *
+ * In React the position in the tree IS the scope of the state. A hook called in
+ * the shell's render body has the whole tree as its scope; the same hook called
+ * in a provider has its readers as its scope. `AppShellContent` today carries
+ * 536 hooks above the entire tree, so a session switch produces ~19 commits,
+ * ~14 of them full-tree renders, of which 96-99% is recoverable work.
+ *
+ * The fix is to move call sites, one feature at a time. That is slow, and a
+ * checklist cannot tell whether it is progressing — extracting a feature into
+ * its own slice does not change the scope of any state, and Session Navigation
+ * proves it: a complete `model / controller / ui / ports` slice whose
+ * controller is still invoked from this render body.
+ *
+ * A call site does have a definition of done, so this gate counts them. The
+ * inventory below is EXACT, not a ceiling: growing past an entry fails, and so
+ * does falling below one. Only failing on growth would let the counts drift
+ * quietly downward and record no progress, which is the one thing this gate
+ * exists to make visible. The cost is real — a change that removes an effect
+ * has to edit this file — and that edit IS the record.
+ *
+ * Deliberately NOT a `--write` mode. Regenerating the inventory on demand would
+ * let a new hook be accepted by rerunning a command, and the friction of
+ * editing it by hand is the point. This is the opposite shape from
+ * `check-astryx-surface-inventory.mjs`, whose invariant is "regenerate and
+ * compare bytes, never a hand-written list". That is right for an inventory
+ * that only has to describe the tree; this one has to RESIST it.
+ *
+ * Two shapes of migration, because the failure message cannot tell them apart:
+ * when nothing in the shell body reads the feature, the call site moves into a
+ * provider and React bails out of `children` (the mention catalog, #4109) — the
+ * entry disappears. When the shell body DOES read it, moving the call site
+ * still leaves a selector read behind, and the honest outcome is a smaller
+ * entry rather than none. "Call it from the provider" is not advice to follow
+ * blindly: cross-feature intent the shell issues must stay an explicit command,
+ * never an implicit subscription bought to lower a number.
+ *
+ * The count is unweighted, so it measures migration progress and not render
+ * cost: a subscription that fires five times per session switch and a hook that
+ * runs once at mount both count as one. Falling counts do not by themselves
+ * demonstrate a faster switch.
+ *
+ * Both `AppShell` and `AppShellContent` are counted. `AppShell` wraps the
+ * content in the root providers, so a hook hoisted one level up has if anything
+ * a WIDER scope — an inventory that watched only the inner component would
+ * accept that move as progress.
+ *
+ * The counting is textual, because TypeScript 7 exposes no stable parser to
+ * JavaScript (only `typescript/unstable/*`), and a gate that goes red whenever
+ * a dependency moves is its own kind of noise. Textual counting is therefore
+ * written to fail closed: `stripNonCode` blanks comments, strings and regex
+ * literals in place, the body is delimited by brace balancing over that blanked
+ * text rather than by a `\n}\n` guess, and a hook name is only counted where a
+ * call actually follows it — including through an explicit type argument, which
+ * an earlier version of this gate silently skipped over.
+ *
+ * Purely derived hooks (`useMemo`, `useCallback`, `useRef`, `useId`) are
+ * ignored: they hold no state and subscribe to nothing, so they cannot widen
+ * the scope of a render. Everything else counts, including React's own
+ * `useState` / `useEffect`, because a custom hook is only a name for them.
+ *
+ * Run: npm run check:app-shell-hooks
+ * Fix: move the call site into the feature's provider, then delete its entry.
+ */
+import { readFileSync } from 'node:fs';
+import { join } from 'node:path';
+import { fileURLToPath, pathToFileURL } from 'node:url';
+
+const root = join(fileURLToPath(new URL('..', import.meta.url)));
+const shellFile = 'apps/desktop/src/renderer/app-shell.tsx';
+
+/** Hooks that hold no state and subscribe to nothing. */
+const DERIVED_HOOKS = new Set(['useMemo', 'useCallback', 'useRef', 'useId']);
+
+/**
+ * Hooks still called in each shell component's render body, with the number of
+ * call sites. This inventory may only shrink. Each entry is one scope that is
+ * still the whole tree rather than its readers.
+ */
+export const ALLOWED = {
+ AppShell: {
+ useState: 2,
+ useSystemUiLocale: 1,
+ },
+ AppShellContent: {
+ useActiveExecutionBoundary: 1,
+ useActiveSessionEvents: 1,
+ useAppShellBootstrapSubscriptions: 1,
+ useAppShellComposerQuotes: 1,
+ useAppShellHostEffects: 1,
+ useAppShellNavRefSync: 1,
+ useAppShellPersistenceEffects: 1,
+ useAppShellProjectContext: 1,
+ useAppShellSessionUiReads: 1,
+ useAppShellSessionWorkspace: 1,
+ useAppShellTurnPresentation: 1,
+ useCommandPalette: 1,
+ useComposerAttachments: 1,
+ useEffect: 14,
+ useGoalController: 1,
+ useKeyboardHelp: 1,
+ useLayoutEffect: 2,
+ useModuleHubController: 1,
+ useNewTaskChoice: 1,
+ useOnboardingSnapshot: 1,
+ usePlanModeState: 1,
+ useSessionEventHealthPolling: 1,
+ // Replaces `useSessionNavigationController`, which is now called inside
+ // `SessionNavigationProvider`. The entry shrinks rather than disappearing,
+ // because the shell body does read the rail: the command palette lists the
+ // same visible sessions, the titlebar shows the linked parent, and the
+ // frame publishes the rail's width as `--maka-sidenav-width`. What is left
+ // holds nothing — three `useMemo`s over the catalog the shell already reads
+ // and one subscription to the rail's geometry — where the controller it
+ // replaces put three `useState`, four effects and a `useStableActions`
+ // facade on this fiber.
+ useSessionNavigationReads: 1,
+ useSessionSettingIntent: 2,
+ useSettingsModal: 1,
+ useShellAppearance: 1,
+ useShellChatModel: 1,
+ useShellConnections: 3,
+ useShellLiveTurn: 1,
+ useShellMemoryPill: 1,
+ useShellResume: 1,
+ useShellRunUpdates: 1,
+ useShellSearch: 1,
+ useStableActions: 7,
+ useState: 16,
+ useTaskEntryController: 1,
+ useTaskSubmissionReadiness: 1,
+ useToast: 1,
+ // The last of the three `useKeyedPendingRegistry` call sites this entry
+ // replaces: #4113 moved the other two onto the session UI store, which is
+ // already an external store, so their scope left this fiber entirely. This
+ // one stays because the shell body reads `keys` to build the turn footer's
+ // disabled mask.
+ useTurnActionRegistry: 1,
+ useWorkbarController: 1,
+ },
+};
+
+/**
+ * After these, a `/` opens a regex literal; anywhere else it is division.
+ *
+ * `<` and `>` are deliberately absent. They would qualify in expression
+ * position, but in TSX they are far more often a tag: treating the `/` of
+ * `</div>` as the start of a regex swallows everything up to the next slash,
+ * which is exactly the silent under-count this gate must not have.
+ */
+const REGEX_MAY_FOLLOW = new Set([
+ '(',
+ ',',
+ '=',
+ ':',
+ '[',
+ '!',
+ '&',
+ '|',
+ '?',
+ '{',
+ '}',
+ ';',
+ '+',
+ '-',
+ '*',
+ '%',
+ '^',
+ '~',
+ '\n',
+]);
+
+/**
+ * Blanks comments, string bodies and regex literals, replacing each with spaces
+ * so that offsets and line numbers still line up with the original.
+ *
+ * Prose naming a hook is not a call site, and the shell's comments name hooks
+ * at length. The two cases worth spelling out are the ones that break a naive
+ * scanner: an apostrophe in JSX text (`<span>don't</span>`) is not a string
+ * delimiter, and a regex literal may contain quotes (`/['"]/`) that would
+ * otherwise swallow the rest of the file. Both would UNDER-count, which is the
+ * dangerous direction for a gate whose only product is a number.
+ */
+export function stripNonCode(source) {
+ const out = [...source];
+ let index = 0;
+ let lastMeaningful = '\n';
+ const blank = (from, to) => {
+ for (let i = from; i < to && i < out.length; i += 1) {
+ if (out[i] !== '\n') out[i] = ' ';
+ }
+ };
+ while (index < source.length) {
+ const char = source[index];
+ const next = source[index + 1];
+ if (char === '/' && next === '/') {
+ const end = source.indexOf('\n', index);
+ const stop = end === -1 ? source.length : end;
+ blank(index, stop);
+ index = stop;
+ continue;
+ }
+ if (char === '/' && next === '*') {
+ const end = source.indexOf('*/', index + 2);
+ const stop = end === -1 ? source.length : end + 2;
+ blank(index, stop);
+ index = stop;
+ continue;
+ }
+ if (char === '/' && REGEX_MAY_FOLLOW.has(lastMeaningful)) {
+ let scan = index + 1;
+ let inClass = false;
+ while (scan < source.length) {
+ const c = source[scan];
+ if (c === '\\') scan += 2;
+ else if (c === '[') (inClass = true), (scan += 1);
+ else if (c === ']') (inClass = false), (scan += 1);
+ else if (c === '\n') break;
+ else if (c === '/' && !inClass) break;
+ else scan += 1;
+ }
+ // An unterminated candidate was division after all; leave it alone.
+ if (scan < source.length && source[scan] === '/') {
+ blank(index, scan + 1);
+ lastMeaningful = '/';
+ index = scan + 1;
+ continue;
+ }
+ }
+ if (char === "'" || char === '"' || char === '`') {
+ // In JSX text an apostrophe follows a word or a closing bracket, and no
+ // JavaScript expression can put a string literal there.
+ if (char === "'" && /[A-Za-z0-9_$>)\]}]/.test(lastMeaningful)) {
+ lastMeaningful = char;
+ index += 1;
+ continue;
+ }
+ let scan = index + 1;
+ while (scan < source.length && source[scan] !== char) {
+ scan += source[scan] === '\\' ? 2 : 1;
+ }
+ blank(index + 1, scan);
+ lastMeaningful = char;
+ index = Math.min(scan + 1, source.length);
+ continue;
+ }
+ if (!/\s/.test(char) || char === '\n') lastMeaningful = char;
+ index += 1;
+ }
+ return out.join('');
+}
+
+/**
+ * The body of a top-level component, delimited by brace balancing over text
+ * whose strings and comments are already blanked. A `\n}\n` guess would stop at
+ * the first brace that formatting happened to put in column zero.
+ */
+export function findComponentBody(blanked, name) {
+ const declaration = new RegExp(
+ String.raw`^(?:export\s+)?(?:default\s+)?(?:async\s+)?function\s+${name}\b|^(?:export\s+)?(?:default\s+)?const\s+${name}\s*(?::[^=]+)?=`,
+ 'm',
+ );
+ const match = declaration.exec(blanked);
+ if (match === null) return null;
+ // Skip the parameter list. `function AppShellContent({ ... })` opens a brace
+ // for its destructured parameter, and balancing from there returns the
+ // parameter object rather than the body.
+ const paren = blanked.indexOf('(', match.index);
+ if (paren === -1) return null;
+ let parens = 0;
+ let afterParams = -1;
+ for (let i = paren; i < blanked.length; i += 1) {
+ if (blanked[i] === '(') parens += 1;
+ else if (blanked[i] === ')') {
+ parens -= 1;
+ if (parens === 0) {
+ afterParams = i + 1;
+ break;
+ }
+ }
+ }
+ if (afterParams === -1) return null;
+ const open = blanked.indexOf('{', afterParams);
+ if (open === -1) return null;
+ let depth = 0;
+ for (let i = open; i < blanked.length; i += 1) {
+ if (blanked[i] === '{') depth += 1;
+ else if (blanked[i] === '}') {
+ depth -= 1;
+ if (depth === 0) return blanked.slice(open, i + 1);
+ }
+ }
+ return null;
+}
+
+/**
+ * `useX(`, `useX<T>(` and `useX\n (` all call a hook; `obj.useX(` and
+ * `ReturnType<typeof useX>` do not.
+ */
+// One level of nesting is enough for `useX<Record<string, number>>(...)`;
+// deeper generics in a hook call do not occur here and would only cost a
+// miscount in the safe direction (a failure, not a silent pass).
+const TYPE_ARGUMENTS = String.raw`<(?:[^<>()]|<[^<>()]*>)*>`;
+const HOOK_CALL = new RegExp(
Review Comment:
Fixed in `b01cc29d1`. `HOOK_CALL` now matches `React.useX(` (counted under the bare name — the inventory names the hook, not the syntax) and React 19's bare `use(`. The qualifier is the literal `React.` rather than any identifier: the shell really does call `shellCopy.useSkillPrompt(name)` at app-shell.tsx:1142, so a general `<name>.useX(` rule would count a locale copy function as a hook. That leaves `import * as X from 'react'`, which the scanner cannot resolve, so `assertCountableReactImport` fails the gate on it instead of guessing. Three adversarial tests added: namespace form, bare `use` against `user(`/`usedBy(`/`router.use(`, and the refused alias import. Counts unchanged at 42 hooks / 81 call sites. Graded P3 rather than P2 — no runtime consequence, and the repo has zero `React.` hook calls and zero namespace imports today — but the gate documents itself as fail-closed, so the hole is worth closing.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
apps/desktop/src/renderer/features/session-navigation/ui/session-navigation-provider.tsx:
##########
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { useLayoutEffect, useMemo, useRef, type ReactNode } from 'react';
+import type { ProjectRecord } from '@maka/core/project';
+import type { ScheduledTask } from '@maka/core/scheduled-task';
+import {
+ SessionRailProvider,
+ type NavModuleMemory,
+ type NavSelection,
+ type ProjectRowActions,
+ type SessionRailChrome,
+ type SessionRailData,
+ type SessionRowActions,
+ type SidebarUpdateReminder,
+} from '@maka/ui';
+import {
+ useSessionNavigationController,
+ type SessionNavigationPorts,
+} from '../controller/use-session-navigation-controller.js';
+import type { SessionNavigationRowActions } from '../controller/session-row-actions.js';
+import {
+ SESSION_LIST_EXPANDED_MAX_WIDTH,
+ SESSION_LIST_EXPANDED_MIN_WIDTH,
+} from '../model/session-list-layout.js';
+import type { SessionRailProjection } from '../model/session-rail.js';
+import { sessionRailLayoutStore } from '../model/session-rail-layout-store.js';
+import type { SessionNavigationSession } from '../ports.js';
+
+/** The chrome the shell owns and the rail only displays. */
+export interface SessionNavigationChromeInput {
+ selection: NavSelection;
+ scheduledTasks?: readonly ScheduledTask[];
+ moduleMemory?: NavModuleMemory;
+ updateReminder?: SidebarUpdateReminder;
+ workHubActive: boolean;
+ workHubEntry?: { active: boolean; label: string; onSelect(): void };
+ projectActions?: ProjectRowActions;
+ onSelect(selection: NavSelection): void;
+ onOpenSettings(): void;
+ onOpenUpdate?(): void;
+ onNew(): void;
+ onExitWorkHub(): void;
+ onSelectSession(sessionId: string): void;
+}
+
+export interface SessionNavigationProviderProps extends SessionNavigationChromeInput {
+ rail: SessionRailProjection<SessionNavigationSession>;
+ projects: readonly ProjectRecord[];
+ streamingSessionIds: ReadonlySet<string>;
+ staleSessionIds: ReadonlySet<string>;
+ ports: SessionNavigationPorts;
+ /**
+ * Where the shell reads the row mutations it issues from elsewhere — the
+ * archived-tasks sweep, the chat header's rename. They are calls made from
+ * event handlers, never values read during a render, so a ref is the whole
+ * carrier they need and the shell does not re-render to receive them.
+ */
+ commandsRef: { current: SessionNavigationRowActions | null };
+ children?: ReactNode;
+}
+
+/**
+ * The Session rail's own scope (#4109).
+ *
+ * The shell renders this on every one of its ~14 commits per session switch,
+ * and that is fine: this component is one fiber, its `children` element is
+ * built once by the shell, and React skips an unchanged child. What reaches the
+ * rail below is the two context values — and those change when the rail's data
+ * changes, not when the shell renders. The rail's ~1,000 fibers are on the
+ * first, the few dozen fibers of permanent chrome on the second.
+ */
+export function SessionNavigationProvider(props: SessionNavigationProviderProps) {
+ const controller = useSessionNavigationController({
+ rail: props.rail,
+ projects: props.projects,
+ ports: props.ports,
+ });
+
+ useLayoutEffect(() => {
+ props.commandsRef.current = controller.commands;
+ }, [controller.commands, props.commandsRef]);
+
+ const rowActions = useMemo<SessionRowActions>(
+ () => ({
+ onToggleFlag: (sessionId, next) => {
+ void controller.commands.flagSession(sessionId, next);
+ },
+ onArchive: (sessionId) => {
+ void controller.commands.archiveSession(sessionId);
+ },
+ onUnarchive: (sessionId) => {
+ void controller.commands.unarchiveSession(sessionId);
+ },
+ onRename: (sessionId, name) => {
+ void controller.commands.renameSession(sessionId, name);
+ },
+ onDelete: (sessionId) => {
+ void controller.commands.deleteSession(sessionId);
+ },
+ }),
+ [controller.commands],
+ );
+
+ // Project row mutations are commands too, and they arrive from a different
+ // feature entirely. Read through a ref for the same reason as the ports: what
+ // the rail needs is that they can be CALLED, and their identity says nothing
+ // about whether a row should be redrawn. Only their presence does, so that is
+ // what this depends on.
+ const chromeRef = useRef(props);
+ useLayoutEffect(() => {
+ chromeRef.current = props;
+ });
+ const hasProjectActions = props.projectActions !== undefined;
+ const hasRelink = props.projectActions?.onRelink !== undefined;
+ const projectActions = useMemo<ProjectRowActions | undefined>(
+ () =>
+ hasProjectActions
+ ? {
+ onNew: (projectId) => chromeRef.current.projectActions?.onNew(projectId),
+ onRename: (projectId, name) =>
+ chromeRef.current.projectActions?.onRename(projectId, name),
+ onArchive: (projectId) => chromeRef.current.projectActions?.onArchive(projectId),
+ onRestore: (projectId) => chromeRef.current.projectActions?.onRestore(projectId),
+ ...(hasRelink
+ ? {
+ onRelink: (projectId: string) =>
+ chromeRef.current.projectActions?.onRelink?.(projectId),
+ }
+ : {}),
+ }
+ : undefined,
+ [hasProjectActions, hasRelink],
+ );
+
+ const data = useMemo<SessionRailData>(
+ () => ({
+ sessions: props.rail.sessions,
+ activeId: props.workHubActive ? undefined : props.rail.activeRowId,
+ streamingSessionIds: props.streamingSessionIds,
+ staleSessionIds: props.staleSessionIds,
+ worktreeSessionIds: controller.selectors.worktreeSessionIds,
+ groups: controller.layout.viewMode === 'project' ? controller.selectors.groups : undefined,
+ groupVariant: controller.layout.viewMode,
+ sessionMeta: controller.selectors.sessionMeta,
+ onSelectSession: props.onSelectSession,
+ rowActions,
+ projectActions,
+ }),
+ [
+ controller.layout.viewMode,
+ controller.selectors.groups,
+ controller.selectors.sessionMeta,
+ controller.selectors.worktreeSessionIds,
+ props.onSelectSession,
+ projectActions,
+ props.rail,
+ props.staleSessionIds,
+ props.streamingSessionIds,
+ props.workHubActive,
+ rowActions,
+ ],
+ );
+
+ // Deliberately NOT memoized. Its readers are the nav rows and the footer —
+ // a few dozen fibers — and every field on it follows the shell, so a
+ // comparator here would run more often than it would save.
+ const chrome: SessionRailChrome = {
+ collapsed: controller.layout.collapsed,
+ onCollapsedChange: sessionRailLayoutStore.setCollapsed,
+ collapseHandleRef: sessionRailLayoutStore.collapseHandleRef,
+ width: controller.layout.width,
+ onWidthChange: sessionRailLayoutStore.setWidth,
Review Comment:
Confirmed and fixed in `e347eb152`, at the store rather than at this call site. `useResizable.collapse()` and the drag path both call `onSizeChange(0)` (`@astryxdesign/core/dist/Resizable/useResizable.js:134` and `:162`), and every non-sentinel width it reports is already clamped to `minWidth`, so `setWidth` now ignores anything below `SESSION_LIST_EXPANDED_MIN_WIDTH`.
Putting the guard in `sessionRailLayoutStore.setWidth` rather than back in the provider is the point: the store is the only authority for the width and its persistence, and a moving call site is exactly how the old `session-navigation-host.tsx` guard was lost. Covered by a unit regression in `session-list-layout.test.ts` (drag to 400, collapse, tick past the 200 ms debounce, assert both the state and `maka-chat-list-width-v1` still read 400) — verified red without the fix. P2 as graded: a real normal-path preference loss, recoverable by re-dragging.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org