refactor(desktop): extract Task Entry feature slice - #3723
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
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,因此相似度检测帮不上忙,等价性必须靠比对来建立。决定"用户最终在哪里建任务"的两个函数——selectAvailableProfile 与 resolveProjectSelection——与原文逐行相同,包括回退顺序:请求值、默认项目、宿主当前选择,最后只有在 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
e1de62f to
8c36d24
Compare
|
Rebased onto the latest main and resolved the conflicts. |
|
I found one reproducible P2 issue on exact head [P2] Do not let a stale catalog refresh fail the post-onboarding directory handoff In This is reachable during normal remote-host onboarding:
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 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 简体中文在 exact head [P2] 不要让过期的 catalog 刷新失败 post-onboarding 的目录交接 在 这条路径可在正常的远程 Host onboarding 中发生:
如果过期的 A reject、当前的 B success,也会丢失这次交接:命令式调用 reject,而 AppShell 调用方会按设计吞掉该 reject。现有 请让命令式 refresh 等待并返回当前获胜的 generation,而不是此前已提交的 snapshot。需要补充 stale-first/current-success 与 stale-reject/current-success 两种覆盖;直接返回过期请求自己的结果仍会让过期数据驱动这次交接。 本 exact head 的验证:desktop build 通过,typecheck 通过,Task Entry 定向测试 15/15 通过, |
|
Clarification on the CI note above: the |
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
Summary
features/task-entry.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— passednpm run typecheck— passednpm run astryx:theme -- --check— passednpm run astryx:surface-inventory— passed (209 files, 1 exclusion)npm --workspace @maka/desktop test— 1407 passed, 0 failed/cancelledgit diff --check upstream/main...HEAD— passedgit merge-tree --write-tree upstream/main HEAD— clean against1e4f38385No 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
newTaskscatalog 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:
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: Codextrailer.Checklist
Does this PR entail a change in behavior?