Skip to content

refactor(desktop): extract Task Entry feature slice - #3723

Open
chihumyum wants to merge 2 commits into
apache:mainfrom
chihumyum:refactor/task-entry-workspace-feature-slice
Open

refactor(desktop): extract Task Entry feature slice#3723
chihumyum wants to merge 2 commits into
apache:mainfrom
chihumyum:refactor/task-entry-workspace-feature-slice

Conversation

@chihumyum

Copy link
Copy Markdown
Contributor

Summary

  • Extract new-task Runtime Host/Project catalog, selection, target/draft identity, and Workspace Picker projection into features/task-entry.
  • Move add/relink and remote-directory handoff lifecycle behind a narrow Desktop service adapter, with generation-fenced catalog commits and single-flight mutations.
  • Keep Session creation/first send, Composer/readiness, Project Settings, and shared remote-directory browser internals outside this slice while preserving existing UI, IPC, storage, copy, routing, and draft continuity.

Refs #3439

Verification

  • npm run check:asf-headers — passed (2836 covered, 138 reviewed exclusions)
  • npm run lint — passed (2701 files)
  • npm run format:check — passed (1627 files)
  • npm run build — passed
  • npm run typecheck — passed
  • npm run astryx:theme -- --check — passed
  • npm run astryx:surface-inventory — passed (209 files, 1 exclusion)
  • npm --workspace @maka/desktop test — 1407 passed, 0 failed/cancelled
  • Desktop and UI Knip checks — passed
  • Desktop Storybook build and render smoke — 189 stories passed
  • Electron E2E — 62 passed, 1 conditionally skipped
  • Browser WebContentsView semantic smoke — 17/17 passed
  • Alignment audit — all fixtures clean
  • git diff --check upstream/main...HEAD — passed
  • git merge-tree --write-tree upstream/main HEAD — clean against 1e4f38385

No screenshot is attached because this is an intentionally visual-neutral ownership extraction; the full Storybook render smoke and Electron E2E cover the rendered surfaces.

Review focus

The public boundary owns only Task Entry catalog/selection state and its picker/handoff lifecycle. AppShell consumes selectors and explicit commands; only the Desktop adapter touches the newTasks catalog bridge. Session creation, task submission, Composer, and shared directory-browser internals remain separate.

The adversarial AI red/blue pass reproduced and fixed a stale catalog response reopening a newly added remote Host with an obsolete hostId, then narrowed the feature port/public refresh API and added independent add/relink lifecycle coverage. A fresh exact-head review found no remaining actionable P0-P3 issue. This supporting review does not count as independent human approval.

AI use

Select exactly one:

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

Tool(s) and scope: OpenAI Codex implemented the feature extraction, tests, documentation, local verification, and disclosed red/blue review. The affected commit retains a Generated-by: Codex trailer.

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

@chihumyum
chihumyum marked this pull request as ready for review August 24, 2026 14:36

@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 e1de62f8a910806f1484dd8e9b6ed278527d10fc, with test terminal green on that exact head.

Unlike a pure file move, this one rewrites use-new-task-target.ts into a controller plus a model, so similarity detection gives no help and equivalence has to be established by comparison. The two functions that decide what the user ends up creating a task against — selectAvailableProfile and resolveProjectSelection — are line-for-line identical to the originals, including the fallback order: requested, then default project, then the host's current selection, then null only when selectNoProject is available.

The draft key is also unchanged: JSON.stringify(['new-task', profileId, hostId, projectId]) with the same UNRESOLVED_NEW_TASK_DRAFT_KEY fallback. That one matters more than its size — a changed key would silently orphan every saved draft on upgrade, with no error to notice.

The hardening around it is real rather than cosmetic. projectMutationPendingRef gives add and relink single-flight semantics through a ref, so a double click cannot slip through a stale state closure, and the refresh sequence now guards commits so a superseded catalog response cannot overwrite a newer one.

One behaviour difference worth knowing about, not blocking. When a refresh is superseded, the old code returned the fresh-but-uncommitted catalog to its caller; the new code returns committedCatalogRef.current. The only caller that uses the return value is chooseProjectForProfile, which looks up the clicked profile and reports catalogUnavailable when it is missing. The new behaviour is the more consistent one — it matches what is actually rendered — but it does mean a catalog change landing at the same moment as the click can now surface an error for a profile the user can see. In practice the previously committed catalog will still contain it, so this is a narrow window; I mention it so the change is a decision rather than an accident.

On merge order, not a blocker: two sibling slice extractions are open against the same app-shell.tsx. Whichever lands first forces the others to redo path-level work, so they should go in one at a time with a rebase between.

简体中文

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

与纯文件搬运不同,这个 PR 把 use-new-task-target.ts 重写成了 controller 加 model,因此相似度检测帮不上忙,等价性必须靠比对来建立。决定"用户最终在哪里建任务"的两个函数——selectAvailableProfileresolveProjectSelection——与原文逐行相同,包括回退顺序:请求值、默认项目、宿主当前选择,最后只有在 selectNoProject 可用时才回到 null

草稿键同样未变:JSON.stringify(['new-task', profileId, hostId, projectId]),以及同一个 UNRESOLVED_NEW_TASK_DRAFT_KEY 兜底。这一点的分量比它的体积大得多——键一旦变了,升级后所有已保存的草稿都会被静默孤立,而且不会有任何错误提示。

围绕它的加固是实的,不是装饰。projectMutationPendingRef 用 ref 给 add 与 relink 提供了 single-flight 语义,双击无法再从过期的 state 闭包里溜过去;refresh 序号现在也守住了提交,被取代的 catalog 响应不会覆盖更新的那份。

有一处行为差异值得知道,不阻塞。 当一次 refresh 被取代时,旧代码把"新取到但未提交"的 catalog 返回给调用方,新代码返回的是 committedCatalogRef.current。唯一使用返回值的调用方是 chooseProjectForProfile,它会去找用户点击的 profile,找不到就报 catalogUnavailable。新行为是更自洽的那个——它与实际渲染出来的内容一致——但这也意味着:如果一次 catalog 变更恰好与点击同时落地,用户明明看得见的 profile 也可能弹出错误。实际上此前提交的那份 catalog 通常仍包含它,所以窗口很窄;我写出来是为了让这处变化是个决定而不是个意外

关于合并顺序,不构成阻塞:另有两个同类切片 PR 正对着同一个 app-shell.tsx 开着。谁先合都会迫使其余重做路径级调整,因此应当串行合并、中间让作者 rebase。

Move new-task Host and Project selection, catalog lifecycle, workspace picker projection, and remote-directory handoff behind a narrow renderer feature boundary.

Generation-fence imperative catalog consumers and cover add and relink mutation lifecycles.

Generated-by: Codex
@chihumyum
chihumyum force-pushed the refactor/task-entry-workspace-feature-slice branch from e1de62f to 8c36d24 Compare August 24, 2026 15:51
@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased onto the latest main and resolved the conflicts.

@Astro-Han

Copy link
Copy Markdown
Contributor

I found one reproducible P2 issue on exact head 8c36d24f8c8df74eda829a5754fd3badf9c8719e.

[P2] Do not let a stale catalog refresh fail the post-onboarding directory handoff

In apps/desktop/src/renderer/features/task-entry/controller/use-task-entry-controller.ts:121-126, when a refresh becomes stale, refresh() returns committedCatalogRef.current, which is the previously committed catalog rather than the result of the current winning refresh. chooseProjectForProfile() immediately treats that return value as authoritative at :235-249.

This is reachable during normal remote-host onboarding:

  1. The committed catalog does not yet contain the newly added remote profile.
  2. AppShell.onRemoteHostAdded calls chooseProjectForProfile(profileId) (app-shell.tsx:3134-3137), starting refresh A.
  3. A subsequent catalog event starts refresh B through the controller subscription (use-task-entry-controller.ts:151-155). The preload profile-change handler emits the task-change notification (preload/preload.ts:250-277), and the new-tasks bridge subscribes to further project/connection/settings changes (:1263-1269).
  4. If A settles after B has started but before B commits, A is considered stale and returns the old catalog. The requested Host is missing, so chooseProjectForProfile() reports “Runtime Hosts unavailable” and returns without opening the directory picker.
  5. B later commits the new Host, but the one-shot handoff is not retried.

The same loss occurs when stale A rejects and current B succeeds: the imperative call rejects while the AppShell caller intentionally swallows that rejection. The existing test at task-entry-controller.test.ts:338-384 covers only the opposite completion order, where B commits first and therefore does not exercise this case.

Please make an imperative refresh await/return the current winning generation rather than the last previously committed snapshot. Add coverage for both stale-first/current-success and stale-reject/current-success; returning the stale read’s own result would still allow stale data to drive the handoff.

Verification on this exact head: desktop build passed, typecheck passed, focused Task Entry tests passed (15/15), and git diff --check passed. The required exact-head CI run 32747441192 is not green: 62 tests passed, 1 was skipped, and apps/desktop/e2e/slash-command-menu.spec.ts:166 failed by timeout.

简体中文

在 exact head 8c36d24f8c8df74eda829a5754fd3badf9c8719e 上发现一个可复现的 P2 问题。

[P2] 不要让过期的 catalog 刷新失败 post-onboarding 的目录交接

apps/desktop/src/renderer/features/task-entry/controller/use-task-entry-controller.ts:121-126,当一次 refresh 变成过期请求时,refresh() 返回的是 committedCatalogRef.current,也就是此前已提交的 catalog,而不是当前获胜的 refresh 结果。chooseProjectForProfile():235-249 立即把这个返回值当作权威数据使用。

这条路径可在正常的远程 Host onboarding 中发生:

  1. 已提交的 catalog 还没有新加入的远程 profile。
  2. AppShell.onRemoteHostAdded 调用 chooseProjectForProfile(profileId)app-shell.tsx:3134-3137),启动 refresh A。
  3. 随后的 catalog 事件通过 controller subscription 启动 refresh B(use-task-entry-controller.ts:151-155)。preload 的 profile-change handler 会发出 task-change 通知(preload/preload.ts:250-277),new-tasks bridge 还会订阅后续的 project/connection/settings 变化(:1263-1269)。
  4. 如果 A 在 B 已启动但尚未提交时完成,A 会被判定为过期并返回旧 catalog。请求的 Host 不在其中,于是 chooseProjectForProfile() 报告 “Runtime Hosts unavailable” 并返回,目录选择器不会打开。
  5. B 随后提交了新 Host,但这个一次性交接不会重试。

如果过期的 A reject、当前的 B success,也会丢失这次交接:命令式调用 reject,而 AppShell 调用方会按设计吞掉该 reject。现有 task-entry-controller.test.ts:338-384 只覆盖了相反的完成顺序,即 B 先提交、所以没有覆盖这个问题。

请让命令式 refresh 等待并返回当前获胜的 generation,而不是此前已提交的 snapshot。需要补充 stale-first/current-success 与 stale-reject/current-success 两种覆盖;直接返回过期请求自己的结果仍会让过期数据驱动这次交接。

本 exact head 的验证:desktop build 通过,typecheck 通过,Task Entry 定向测试 15/15 通过,git diff --check 通过。required exact-head CI run 32747441192 尚未通过:62 个测试通过、1 个跳过,apps/desktop/e2e/slash-command-menu.spec.ts:166 超时失败。

@Astro-Han

Copy link
Copy Markdown
Contributor

Clarification on the CI note above: the slash-command-menu.spec.ts:166 failure is a known repository-level flaky test tracked in issue #3727, and is unrelated to this PR. The reason this PR is not approved is the P2 race described above, not that unrelated CI failure.

Keep imperative catalog reads attached to the latest refresh generation so remote Host onboarding cannot lose its directory handoff. Cover stale success and stale failure ordering against the winning refresh.

Generated-by: Codex
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