Skip to content

refactor(desktop): extract Session Navigation feature slice - #3719

Merged
Astro-Han merged 1 commit into
apache:mainfrom
chihumyum:refactor/session-navigation-feature-slice
Aug 25, 2026
Merged

refactor(desktop): extract Session Navigation feature slice#3719
Astro-Han merged 1 commit into
apache:mainfrom
chihumyum:refactor/session-navigation-feature-slice

Conversation

@chihumyum

Copy link
Copy Markdown
Contributor

Summary

  • Extract Session rail projection, layout persistence, navigation intents, and row-mutation lifecycle into features/session-navigation.
  • Add a narrow Desktop services adapter, feature host/controller, and architecture tests while preserving existing routes, storage keys, revision-family semantics, and UI behavior.

Refs #3439

Verification

  • npm ci
  • npm run check:asf-headers
  • npm run lint
  • npm run format:check
  • npm run typecheck
  • npm run build
  • npm run astryx:theme -- --check
  • npm run astryx:surface-inventory
  • Desktop tests: 1419 passed
  • Desktop and UI Knip checks passed
  • Storybook build and render smoke: 189 stories passed
  • Electron E2E: 62 passed, 1 conditionally skipped
  • Browser observe/act smoke: 17/17 passed
  • Astryx alignment audit: all fixtures clean
  • git merge-tree --write-tree upstream/main HEAD

Review focus

Session catalog authority remains in the existing AppShell session-list boundary. The feature owns rail/view/mutation behavior; AppShell keeps explicit cross-feature intents.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex implemented the renderer feature extraction, tests, and red/blue review under human direction.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving on eff7627bdcb336d973125cf8ed1417d908c6fe32, with test terminal green on that exact head.

For a move of this size the only question worth answering is whether anything changed semantics under cover of changing location, so I checked it mechanically rather than by reading. Comparing each moved file against its original with the licence header stripped:

  • session-rail.ts, session-revisions.ts, session-nav-filter.ts, branch-banner.ts — byte-identical.
  • session-list-layout.ts and session-project-grouping.ts — import paths only.
  • session-row-actions.ts — the one file with real edits, and every one is mechanical: window.maka.sessions.* becomes the injected service.*, and setMessages([]) becomes clearActiveMessages(), which app-shell.tsx:1660 defines as useCallback(() => setMessages([]), [setMessages]). The revisionFamily semantics, the requireArchived and restored branches, the pending de-duplication, and the list read-back verification are all unchanged.

The three persisted layout keys and the 200 ms trailing debounce moved without modification, and openSession keeps its original ordering — exit WorkHub, select, activate, then set or clear the scroll target — including the suppression of activeId while WorkHub is active.

One incidental fix worth calling out in the description. deriveBranchBanner's memo dependencies were [activeSession?.parentSessionId, sessions] and are now [activeSession, sessions]. The old list was incomplete: switching between two sessions that share a parentSessionId did not recompute, so the banner could keep showing the previous parent's name after it changed. The new list is correct. That is a behaviour change — a fix, not a regression — and it should be mentioned rather than left inside a refactor.

The boundary is clean: no imports of app-shell, preload or main from inside the feature, the Desktop bridge is touched only in platform/desktop/create-session-navigation-services.ts, and the new boundary tests pin the "external code enters through index/testing only" rule rather than leaving it as a convention.

On merge order, not a blocker: this PR rewrites large parts of app-shell.tsx and deletes six files from the renderer root. Two sibling slice extractions are open against the same code. Whichever lands first will force the others to redo path-level work, so they should go in one at a time with a rebase between, rather than in parallel.

简体中文

已在 eff7627bdcb336d973125cf8ed1417d908c6fe32 上 approve,该 exact head 的 test 为终态绿。

这种规模的搬运,唯一值得回答的问题是「有没有借着换位置改语义」,所以我用机械方式核对,而不是靠通读。把每个被移动的文件与其原文件在剥掉 licence 头后逐一比对:

  • session-rail.tssession-revisions.tssession-nav-filter.tsbranch-banner.ts——逐字节相同。
  • session-list-layout.tssession-project-grouping.ts——只改了 import 路径。
  • session-row-actions.ts——唯一有实际改动的文件,而每一处都是机械的:window.maka.sessions.* 换成注入的 service.*setMessages([]) 换成 clearActiveMessages(),后者在 app-shell.tsx:1660 定义为 useCallback(() => setMessages([]), [setMessages])revisionFamily 语义、requireArchivedrestored 分支、pending 去重、list 回读校验,全部未变。

三个持久化布局键与 200 ms trailing debounce 原样搬入;openSession 保持原有顺序——退出 WorkHub、选中、激活、再设置或清除 scroll target——包括 WorkHub 激活时对 activeId 的抑制。

有一处顺带的修复,建议写进正文。 deriveBranchBanner 的 memo 依赖由 [activeSession?.parentSessionId, sessions] 改为 [activeSession, sessions]。旧的依赖列表是不完整的:在两个共享同一 parentSessionId 的 session 之间切换时不会重算,因此父级名称变化后横幅可能仍显示旧名字。新的依赖是对的。这是一处行为变化——是修复而非回归——应当明说,而不是留在重构里。

边界是干净的:feature 内部没有对 app-shellpreloadmain 的导入;Desktop bridge 只在 platform/desktop/create-session-navigation-services.ts 触碰;新增的边界测试把「外部只能经 index/testing 进入」这条规则用机器锁住,而不是留作约定。

关于合并顺序,不构成阻塞:本 PR 重写了 app-shell.tsx 的大段内容,并删除了 renderer 根目录下的六个文件。另有两个同类切片 PR 正对着同一片代码开着。谁先合,都会迫使其余两个重做路径级的调整,因此应当串行合并、中间让作者 rebase,而不是并行推进。

@chihumyum
chihumyum force-pushed the refactor/session-navigation-feature-slice branch from eff7627 to 8ee8545 Compare August 24, 2026 16:03
@chihumyum
chihumyum force-pushed the refactor/session-navigation-feature-slice branch from 8ee8545 to 6375a3d Compare August 25, 2026 15:47
@chihumyum
chihumyum requested a review from Astro-Han August 25, 2026 18:10
@Astro-Han

Copy link
Copy Markdown
Contributor

Not a change request — this PR follows the established slice convention correctly and the extraction itself is clean. This is an observation about the convention's cost, aimed at #3439 rather than at this PR.

The numbers here: app-shell.tsx gives up 135 lines net, and four files move into model/ for free (pure renames, 0/0 in numstat). Against that, 12 new files add 1124 lines — roughly 610 of production scaffolding (controller 287, host 122, ports 59, services-context 43, adapter 44, groups 57) plus 384 of tests and a 67-line README.

That ratio isn't this slice's fault; it's the per-slice fixed cost, and it doesn't amortize. goals, module-hub and workbar already carry the same seven-file shape, and #3723 makes five. With identifiers normalized, the four services-context.tsx files differ by 3–7 lines, and ports.ts + services-context.tsx + testing.ts + README.md comes to 225 lines in goals against 228 here.

The adapter is the clearest case. create-session-navigation-services.ts declares DesktopSessionNavigationBridge = Pick<MakaBridge, 'sessions'> and then hand-writes six forwarders whose signatures are identical to the ones they forward to — the function is equivalent to { sessions: bridge.sessions }. Across the four landed slices that's 58 identity forwarders (5 + 14 + 33 + 6), each one a place where a signature can drift from the bridge it mirrors without anything noticing.

Worth adding to #3439 as a consolidation item before more slices land: the ports/context/adapter triple looks mechanically derivable from Pick<MakaBridge, K>, and a shared createFeatureServices + createServicesContext pair would leave each slice owning only its controller, host, and model. Nothing here blocks this PR.

简体中文

不是修改要求——这个 PR 按既有约定办事,抽取本身很干净。这条是关于约定成本的观察,针对的是 #3439 而不是这个 PR。

这次的账:app-shell.tsx 净减 135 行,另有四个文件纯改名挪进 model/,不花钱;对面是 12 个新文件 +1124 行,其中生产侧脚手架约 610 行。

这个比例不是这个 slice 的问题,是每个 slice 的固定开销,而且不摊薄。goalsmodule-hubworkbar 已经是同样的七件套,加上 #3723 就是五个。把标识符归一化后,四个 services-context.tsx 只差 3–7 行。

adapter 最能说明问题:它先声明自己是 Pick<MakaBridge, 'sessions'>,再手写六个签名逐字相同的转发,整个函数等价于 { sessions: bridge.sessions }。四个 slice 加起来 58 个恒等转发,每一个都是签名可能悄悄漂移的地方。

建议在更多 slice 落地前把这条记进 #3439:这个三件套看起来能由 Pick<MakaBridge, K> 机械推导,收成一对共享的 createFeatureServices + createServicesContext 之后,每个 slice 只需要自己的 controller、host 和 model。

Reviewed with help from Claude.

@Astro-Han
Astro-Han merged commit ee7da14 into apache:main Aug 25, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants