Uh oh!
There was an error while loading. Please reload this page.
test(desktop): reduce P1 Electron coverage at owning boundaries - #4803
test(desktop): reduce P1 Electron coverage at owning boundaries#4803Astro-Han wants to merge 2 commits into
Conversation
e1ed74b to
906c124CompareGenerated-by: Codex
514d6d3 to
6279195Compare
jackwener
left a comment
There was a problem hiding this comment.
Reviewed at exact head 62791959 under an adversarial pass — three independent seats, each sealing before reading the others. [P1] — not approving. Four P2s alongside it.
First, a fact about this head: relative to 906c1241 it is a rebase plus the draft flag being cleared. The same 20 files, the Goal / revision-draft / IME / rail / About replacement blobs byte-identical, and the summary's destination table unchanged. Nothing raised on the previous head was addressed.
[P1] The deletion-safety contract loses its only owner
parent-session-deletion.spec.ts does not merely check that a linked child ends up archived. It exclusively owns a destructive-entry contract: the Session rail may only archive, and permanent deletion must happen on the Settings archived-tasks surface.
The named replacements verify deleteSession, preview, requireArchived, Host counts and storage outcomes. None of them renders the rail, and none inspects which commands it exposes.
Demonstrated rather than argued. On this exact head, SessionRowActions was given an optional onDelete and a Delete menu item, wired to the existing deleteSession from SessionNavigationProvider — putting irreversible deletion back into the dense rail:
| Result | |
|---|---|
| Named destinations + existing UI row-action / WorkHub suites, with that mutation live | 62/62 green |
| The deleted E2E restored verbatim, real Electron | fails exactly at the original guard — getByRole('menuitem', {name:'删除'}) expected 0, actual 1 |
| Clean head, same suites | 51/51 green |
So a future refactor can return irreversible deletion to the rail and every surviving owner stays green. The contract was not relocated; it was removed.
The reason this is easy to miss is worth naming: the surviving tests cover the outcome of deletion — that the right thing was deleted, that the child was archived. The deleted spec covered the route — that deletion can only be initiated from one place. Outcome coverage passing says nothing about route coverage existing.
Fix: keep a lower-tier rendered rail contract that actually opens the row menu and asserts Delete is absent, and let the Settings archived-task surface continue to own the permanent-delete affordance and confirmation. The Host/storage linked-child tests are complementary, not a substitute.
The AX journeys: a live spec is deleted, and the named destination disclaims the job
Counting across all twenty deleted journeys: 9 have a complete, defensible lower-tier owner; 11 lose at least one shell-composition assertion. The eleven are WorkHub reopen-after-leaving, the two new-task Project switches, Module Hub shell routing, and seven AX journeys.
Two things compound on the AX row.
The file being deleted is green and current. The merge base carries accessibility-coverage.spec.ts as updated by #4789 — open-workbar-face rather than the retired 任务待办, the 打开或关闭工作栏的面 control, a registry without Task. That journey was brought back in line with this shell and verified 7/7. Deleting a red spec and deleting a freshly repaired one are different acts, and only the first is argued for anywhere.
The named destination disclaims the job in the same tree.auditAxTree (storybook-visual-smoke.mjs:260-268) checks generic shapes — unnamed actionables, state, landmarks. It does not check that a particular route, registry member, shortcut, or focus transition exists. app-shell.stories.tsx:2312-2321 says so directly: it is not a substitute for exposing a given string.
Measured: removing browser from the production WORKBAR_TOOL_DEFINITIONS, so the launcher and the [+] menu both lose Browser, leaves the full smoke at 292 stories / 318 renders green — while the deleted Workbar AX journey would fail when the registry loop looks for Browser.
[P2] Restore a per-journey owner for what those seven covered, or state plainly that this coverage is being dropped. The generic audit cannot be cited as the destination.
[P2] Two new-task Project journeys have no owner for the picker → draft/staging wiring
chat-composer-region-draft-handoff.test.ts and new-task-staged-content.test.ts swap keys by hand; task-entry-controller.test.ts inspects the initial draftKey and picker model but never calls onSelectProject / onSelectNoProject. Making both handlers no-ops leaves the claimed controller/draft/staging suites at 29/29 green, while the deleted text and attachment journeys would fail because the picker label never changes.
[P2] WorkHub reopen-after-leaving has no owner for the AppShell seam
The controller/session-port/surface suites cover durable projection, active-link reconstruction and replace, but never drive openWorkHub at app-shell.tsx:1142-1145. Making it a no-op leaves those suites at 106/106 green; the deleted journey fails on the second WorkHub click. The lower tests prove that if it reopens the state rebuilds — not that the shell can still reopen it.
[P2] Two destinations from the previous head, still unaddressed
WorkHub "rebuild after navigating away and back" and "replace the exact linked delegation across Sessions" have no matching destination — the announce-copy static markup test is not those journeys. Parent + linked-subagent deletion: the storage archive-on-parent-remove test is not the Settings UI confirm plus archived list.
What is genuinely covered
Nine journeys have complete lower-tier owners, and they are worth keeping in view: WorkHub create/replace, revision-draft clear/restore, the Goal budget unit test, ImeCommitDoesNotSend, the rail render budget, the About story play, and linked-subagent retirement. Module Hub leaf resolution and rendering are covered; only its shell navigation and the Command Palette → dialog wiring fall into the AX gap above.
Clean-head focused owners run 179/179, and the full smoke rebuilds green at 292/318 — which is the point: every mutation above passes through a green suite.
On grading
One seat graded the live-spec deletion P1 and another graded the entry-contract loss P1, while a third graded all of it P2. The facts agreed; only severity differed. I have kept exactly one P1: the deletion-safety contract, because losing it means an irreversible action can be re-exposed with nothing objecting. The AX and shell-composition losses are serious coverage regressions and are recoverable by adding owners — that is a P2, however unwelcome. Deleting a currently-green spec is a process problem that needs a stated reason, but its consequence is still coverage.
简体中文
在 62791959 上以对抗性方式评审——三个独立席位,各自封存后才互看。[P1]——不予批准。 另有四条 P2。
先说这个 head 的一个事实:相对 906c1241,它是一次 rebase 加上清除 draft 标记。同样的 20 个文件,Goal / revision-draft / IME / rail / About 那几块替代 blob 逐字节相同,摘要里那张去处表也没有变。上一轮在旧 head 上提出的问题,没有一处被处理。
[P1] 删除安全契约失去了它唯一的 owner
parent-session-deletion.spec.ts 并不只是检查一个 linked child 最终被归档。它独占一条破坏性入口契约:Session rail 只能归档,而永久删除必须发生在 Settings 的已归档任务界面上。
被点名的替代品验证的是 deleteSession、preview、requireArchived、Host 计数与存储结果。它们没有一个渲染 rail,也没有一个检查 rail 暴露了哪些命令。
这是被演示出来的,不是论证出来的。在这个 exact head 上,给 SessionRowActions 加一个可选 onDelete 与一个 Delete 菜单项,接到 SessionNavigationProvider 中现成的 deleteSession——也就是把不可逆删除放回那条密集的 rail:
| 结果 | |
|---|---|
| 带着该变异,跑被点名的去处 + 现有 UI row-action / WorkHub 套件 | 62/62 绿 |
| 把被删的 E2E 原样恢复,跑真实 Electron | 精确失败在原来的 guard——getByRole('menuitem', {name:'删除'}) 期望 0、实际 1 |
| 干净 head,同样的套件 | 51/51 绿 |
所以未来的一次重构可以把不可逆删除放回 rail,而每一个幸存的 owner 都仍然是绿的。这条契约不是被转移了,是被移除了。
它之所以容易被漏掉,值得点名:幸存的测试覆盖的是删除的结果——删对了东西、child 被归档了;而被删的那条覆盖的是删除的路径——它只能从一个地方发起。结果覆盖通过,对「路径覆盖是否存在」什么也没说。
修法: 保留一条 lower-tier 的已渲染 rail 契约,真正打开 row menu 并断言 Delete 缺席;让 Settings 的已归档任务界面继续拥有永久删除的入口与确认。Host/storage 的 linked-child 测试是互补,不是替代。
AX journeys:被删的是一条活的 spec,而被点名的去处在同一棵树里否认自己能做这份工作
在全部二十条被删 journey 上计数:9 条有完整、可辩护的 lower-tier owner;11 条至少丢失一段 shell-composition 断言。 这十一条是 WorkHub 离开后重开、两条新任务 Project 切换、Module Hub shell routing,以及七条 AX journey。
AX 这一行上有两件事叠加。
被删的这个文件是绿的,而且是最新的。 merge base 上的 accessibility-coverage.spec.ts 正是 #4789 更新后的版本——open-workbar-face 而非已退役的 任务待办,打开或关闭工作栏的面 控件,registry 中没有 Task。那条 journey 已经与这个 shell 对齐,并被验证 7/7 通过。删除一条红的 spec 与删除一条刚被修好的 spec 是两种行为,而只有前者在任何地方被论证过。
被点名的去处在同一棵树里否认了这份工作。auditAxTree(storybook-visual-smoke.mjs:260-268)检查的是通用形状——未命名的可操作元素、state、landmark。它不检查某个特定 route、registry 成员、快捷键或焦点转换是否存在。app-shell.stories.tsx:2312-2321 直说了这一点:它不是「某个给定字符串确实被暴露」的替代品。
实测:从生产的 WORKBAR_TOOL_DEFINITIONS 中移除 browser,使 launcher 与 [+] 菜单同时失去 Browser,完整 smoke 仍然是 292 stories / 318 renders 全绿——而被删的那条 Workbar AX journey 会在 registry 循环寻找 Browser 时失败。
[P2] 为那七条所覆盖的内容恢复 per-journey owner,或者直白地说明这部分覆盖被放弃了。那个通用审计不能被当作去处引用。
[P2] 两条新任务 Project journey,picker → draft/staging 的接线没有 owner
chat-composer-region-draft-handoff.test.ts 与 new-task-staged-content.test.ts 都是手工换 key;task-entry-controller.test.ts 检查初始 draftKey 与 picker model,但从不调用 onSelectProject / onSelectNoProject。把这两个 handler 都改成 no-op,被声称覆盖的 controller/draft/staging 套件仍然 29/29 绿,而被删的文字与附件 journey 会因为 picker 标签根本不变而失败。
[P2] WorkHub「离开后重开」的 AppShell 接缝没有 owner
controller/session-port/surface 套件覆盖持久投影、active-link 重建与替换,但从不驱动 app-shell.tsx:1142-1145 的 openWorkHub。把它改成 no-op,那些套件仍然 106/106 绿;被删的 journey 会在第二次点击 WorkHub 时失败。这些下层测试证明的是「如果能重开,状态会重建」——而不是「shell 仍然能重开它」。
[P2] 上一个 head 就有的两处去处,仍未处理
WorkHub 的「导航离开再回来重建」与「跨 Session 替换那条 exact linked delegation」没有对应去处——announce-copy 的静态标记测试不是这两条 journey。父会话 + linked-subagent 删除:storage 的 archive-on-parent-remove 测试不是 Settings UI 的确认流加归档列表。
确实被覆盖的部分
九条 journey 有完整的 lower-tier owner,值得同时看见:WorkHub create/replace、revision-draft clear/restore、Goal 预算单测、ImeCommitDoesNotSend、rail 渲染预算、About story play,以及 linked-subagent 退役。Module Hub 的 leaf 解析与渲染有覆盖;只有它的 shell 导航与 Command Palette → dialog 接线落进了上面那个 AX 缺口。
干净 head 上的 focused owner 跑 179/179,完整 smoke 重建后也是 292/318 绿——而这正是要点:上面每一处变异,都是穿过一套绿色套件的。
关于定级
一个席位把「删除活 spec」定为 P1,另一个把「入口契约丢失」定为 P1,第三个把全部定为 P2。事实是一致的,分歧只在严重性。我只保留了一条 P1:删除安全契约——因为失去它意味着一个不可逆的动作可以被重新暴露,而没有任何东西会反对。 AX 与 shell-composition 的丢失是严重的覆盖回退,可以通过补 owner 恢复——那是 P2,尽管并不好听。删除一条当前绿着的 spec 是一个需要给出理由的过程问题,但它的后果仍然是覆盖。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 62791959dcfac2b5381892be5ccab82ab466d624. I independently confirmed the already-reported current-head coverage gaps around the destructive rail route, Project-picker wiring, WorkHub reopening, and route-specific accessibility contracts, so I am not duplicating those comments. I found one additional P2 in the successful edit-and-resend replacement owner, attached inline.
The diff removes 20 Electron journeys in ten files, adds focused Goal/revision tests and Storybook interactions, and extracts revision-draft retirement from AppShell. I inspected the full diff, each deleted journey against its named destination, the relevant AppShell/controller/UI call paths, current reviews, exact-head CI, and the merge against current main (5846521372d2dd0d3d2d33dc7784dd046dc3f7c8). The merge tree is clean.
Validation on the exact head: Node 24/npm 11 dependency patches and package/Desktop test builds passed; 163 focused controller, draft, deletion, and WorkHub tests passed; format, lint, ASF headers, renderer architecture (101/101), git diff --check, and Playwright discovery (58 tests in 25 files) passed. GitHub's exact-head test job is green. I did not rerun the full Electron suite or native macOS/Windows behavior.
No additional P0/P1 findings beyond the existing current-head review. This head is not ready while those blockers and the inline P2 remain.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Uh oh!
There was an error while loading. Please reload this page.
jackwener
left a comment
There was a problem hiding this comment.
Correction to the grading in my previous comment: the AX-journey loss should be [P1], not [P2]. The findings and evidence are unchanged; only the severity was wrong, and it was wrong for a reason worth stating.
I separated the two on "safety boundary versus coverage regression" and stopped there. That ignores how reachable each consequence is, which is the dimension that actually orders them.
The deletion-entry loss requires someone to deliberately re-expose deletion in the rail. It is a real gap — nothing would object — but it needs a specific future change to become harmful, and that change would itself be reviewed.
The AX loss is reached by ordinary work. The workbar registry and its surrounding shell are edited routinely; #4789 was one such change this week. With the seven journeys gone, the next edit that drops a registry member, a shortcut, or a focus transition produces a green suite — as measured, removing browser from WORKBAR_TOOL_DEFINITIONS leaves the smoke at 292/318 green. No deliberate act is required for the regression to land; the ordinary case is enough.
Two further properties push it above, not below, the entry-contract finding:
- The failure is silent to the people who are not affected and invisible to the people who are. A sighted developer sees nothing wrong. A keyboard or screen-reader user encounters a surface that no longer announces or reaches what it used to, with no error anywhere to trace it to.
- The deleted file is the one that existed to prevent exactly this, and it is currently green and current — repaired in #4789 to match this shell and verified 7/7. Removing the guard that covers a high-frequency edit path is different from removing one that covers a rare deliberate act.
So the corrected grading is:
- [P1] — seven AX journeys lose their owner while the named destination (
auditAxTree) disclaims the job in the same tree, and the deleted spec is live. - [P1] — the deletion-safety contract loses its only owner.
- [P2] — the two new-task Project journeys, WorkHub reopen-after-leaving, and the two destinations carried over from the previous head.
Nothing about the required fixes changes. What changes is which one should be done first: the AX owners, because that is the gap ordinary work will fall into.
Recording the reasoning error as well as the correction: I graded by the kind of thing lost — a safety property reads as more serious than a test — instead of by how easily each loss is reached. Two of the three seats had graded the AX row P1 already; I overrode that on a distinction that does not survive the reachability question.
简体中文
更正上一条评论中的定级:AX journey 的丢失应为 [P1],而非 [P2]。 发现与证据不变;只是严重性判错了,而错的理由值得写出来。
我用「安全边界 vs 覆盖回退」把两者分开,然后就停在那里了。那忽略了每种后果有多容易被触及——而那恰恰是真正给它们排序的那个维度。
删除入口的丢失,需要有人刻意把删除重新暴露在 rail 上。 那是一个真实的缺口——不会有任何东西反对——但它需要一次特定的未来改动才会造成伤害,而那次改动本身还会被评审。
AX 的丢失,则由日常工作就能到达。 workbar registry 及其周边 shell 是被例行编辑的;#4789 本周就是这样一次改动。在那七条 journey 消失之后,下一次删掉某个 registry 成员、某个快捷键或某个焦点转换的编辑,产出的会是一套绿色套件——如实测所示,从 WORKBAR_TOOL_DEFINITIONS 移除 browser,完整 smoke 仍是 292/318 全绿。回归的落地不需要任何刻意行为;普通情形就足够了。
另有两项性质把它推到入口契约那条之上,而不是之下:
- 这个失败对没受影响的人是静默的,对受影响的人是不可见的。 一个视力正常的开发者什么异常也看不到。而一个键盘或读屏用户遇到的,是一个不再播报、不再可达它原本可达之物的界面,而且任何地方都没有错误可供追溯。
- 被删的这个文件,正是为防止此事而存在的,而且它当前是绿的、是最新的——在 #4789 中被修好以匹配这个 shell,并被验证 7/7。移除一道覆盖高频编辑路径的守卫,与移除一道覆盖罕见刻意行为的守卫,是不同的。
所以更正后的定级是:
- [P1] —— 七条 AX journey 失去 owner,而被点名的去处(
auditAxTree)在同一棵树里否认这份工作,且被删的 spec 是活的。 - [P1] —— 删除安全契约失去它唯一的 owner。
- [P2] —— 两条新任务 Project journey、WorkHub 离开后重开,以及从上一个 head 带过来的那两处去处。
所需的修复没有任何变化。变的是哪一个该先做:AX 的 owner——因为那是日常工作会掉进去的那个缺口。
把推理错误连同更正一并记录:我是按丢失之物的种类来定级的——一个安全属性读起来比一条测试更严重——而不是按每种丢失有多容易被触及。三个席位中已有两个把 AX 这一行定为 P1;我用一个经不起「可及性」追问的区分推翻了它们。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Restore revision and WorkHub integration coverage plus the focused draft-focus journey. Extend existing picker and row-menu contracts, remove the revision helper extraction, and retire broad route inventories explicitly. Generated-by: Codex
Astro-Han
commented
Sep 5, 2026
I addressed the coverage gaps in
I accept that the original destination table overstated equivalence. I do not agree with grading these coverage changes as current P1 product defects solely from hypothetical future mutations: no current destructive action or broken accessibility route was demonstrated on the clean head. That grading disagreement does not excuse the missing draft and entry protections, which are addressed above. Validation: both targeted mutations fail, the clean Storybook smoke passes 292 stories / 318 renders, and build, renderer/Storybook typechecks, lint, format, ASF headers and architecture checks pass. Playwright discovers 63 tests in 28 files. The retained native journeys were attempted locally, but the shared window fixture timed out before the first test body; the other four did not run after the first-failure stop. I am not claiming a passing local native run. The updated PR description records these limits and the deliberate coverage reductions. |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 72287bbf00fbc98ccff36c0a35f4d7e7f1e0eeee. The restored revision, WorkHub, and draft-focus Electron journeys, together with the project-picker callback test, address the prior coverage gaps. Two blocking coverage gaps remain inline.
Validation on this exact head included a clean install, test build, Desktop Storybook typecheck, renderer architecture tests (101/101), focused unit/integration tests (157/157), changed-file Biome, full formatting and lint checks, ASF header audit, renderer build, and the restored Electron journeys under Linux/Xvfb (5/5). Hosted checks are green. The patch also merges cleanly onto current main (dd7d1d595b7f9284e01fe76cf547c979a6d84a0a) without changing the patch-id. I did not independently run native macOS or Windows Electron coverage.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| await waitFor(() => expect(page.getByRole('menu')).toBeVisible()); | ||
| expect(page.getByRole('menuitem', { name: '重命名' })).toBeVisible(); | ||
| expect(page.getByRole('menuitem', { name: /^归档$/ })).toBeVisible(); | ||
| expect(page.queryByRole('menuitem', { name: /删除/ })).toBeNull(); |
There was a problem hiding this comment.
[P1] Exercise the production rail boundary, not a fixture without delete
This assertion only checks the story's fixed rowActions fixture, which omits any delete action. Production constructs these actions in SessionNavigationProvider, where deleteSession is available. On this exact head, I added an optional Delete action to the UI component and wired it through that provider; the build and the full Storybook smoke suite still passed all 292 stories / 318 renders, including this story, while irreversible deletion was exposed in the rail again. Please mount the production provider/composition here or retain a focused Electron route test so the guard actually covers the safety boundary.
| // after it is opened from the [+] menu, which is the only entry point once a | ||
| // face is on screen. The faces stay open behind one another — the strip is a | ||
| // `tablist` now, so each one has to keep reporting its own tab. | ||
| const workbarPanels = ['侧边对话', '变更', '终端', '浏览器', '生成文件', '追踪'] as const; |
There was a problem hiding this comment.
[P1] Keep an owner for route-specific accessible shell inventory
The generic Storybook accessibility sweep does not own the production workbar registry or its keyboard entry routes. On this exact head, I removed Browser from WORKBAR_TOOL_DEFINITIONS, rebuilt Storybook, and the full smoke suite still passed all 292 stories / 318 renders even though Browser disappeared from both the launcher and [+] menu; the direct Browser stories continued to pass. Please retain a focused per-route inventory/keyboard journey for critical shell entries, or add a lower-tier test that composes the production registry and entry points. Declaring this broad journey retired does not replace that ownership.
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head 72287bbf. Both P1s from the previous round are downgraded to [P2], and your grading push back is accepted. Four repairs verified as discriminating. One correction to a claim in the PR description is below — it does not change the verdict, but it changes which mechanism is actually holding the line.
The grading argument, accepted
Your position was that coverage changes should not be graded as current P1 product defects from hypothetical future mutations, absent a demonstrated destructive action or broken route on the clean head. That is right, and applying it consistently is what moves both findings.
A mutation that reddens nothing establishes that a guard is missing. It does not establish that the clean head misbehaves. The separation that matters is between a consequence reproduced as it already occurs and a consequence reachable only through a future, deliberate, reviewable change. The rail and AX findings are the second kind. I have applied the same principle to the other PR in this series and withdrawn a P1 there as well.
Verified: four repairs, each with a discriminating test
Each was checked by breaking the production path and requiring the new test to fail:
- Project picker — both
onSelectNoProjectandonSelectProjectstubbed to no-op; the target test fails at the first selection,project-awherenullwas expected. - WorkHub reopen —
openWorkHubin AppShell stubbed; the navigate-away/reopen journey fails at the secondwaitForWorkHubReady, while the cross-Session exact-link replacement stays green. The attribution is clean: the new journey pins the shell reopen seam by itself. - Revision draft — the helper and direct-call substitutes are gone (
revision-draft-lifecycle.tsandapp-shell-revision-actions.test.tsdeleted), and clearing child/source drafts, completing the copy, and clearing revision state run through the AppShell production call site again. Two real journeys prove a successful retry empties both composers, and that cancelling after a failure restores the previous text and the Project Skill chip. - Relabelled scope — the description now states removal of 15 Electron tests instead of implying equivalence. That was the right correction.
Electron 5/5 on the exact head, Desktop and Storybook builds pass, hosted required test green, no drift on refetch.
[P2] The new row-menu assertion does not hold the contract you attribute to it
ActiveTaskActionsOpen asserts Rename and Archive are present and that no /删除/ menu item exists. That assertion cannot fail for the mutation it is meant to catch.
The story renders SessionRail with panelProps(...), which spreads the module-level rowActions const declared at line 77 of session-list-panel.stories.tsx — onToggleFlag, onArchive, onUnarchive, onRename. It never composes the production provider. So re-exposing deletion in production leaves the story green: the rail would render a Delete item only when handed a delete callback, and this fixture never hands it one. The assertion goes red only if the fixture is edited in the same change.
It therefore kills "the component hard-codes a Delete item", not "the provider exposes an optional Delete". The second is the mutation the deleted journey used to catch.
What is actually holding the line today is the type.SessionRowActions in session-history-list.tsx declares exactly four members — flag, archive, unarchive, rename — and the file contains no delete-shaped identifier at all. Re-exposing rail deletion requires adding a member to that interface: a typed edit that shows up in review. That is a real barrier, and it is the concrete reason this is P2 rather than P1 — not merely the absence of a reproduced defect.
The gap worth closing is small and does not need the parent-deletion journey back. Either would do:
- a static assertion that
SessionRowActionsexposes no destructive member, which pins the invariant at the place that actually enforces it; or - a render contract that composes the production provider rather than a fixture, so the menu under test is the menu users get.
Both are cheap. Restoring the full Electron journey to recover this one bit is not the proportionate answer, and I am not asking for it.
[P2] The AX and Module Hub coverage subtraction
Same principle, same grade. The six broad journeys and the route sweep are retired deliberately, and no currently broken keyboard or screen-reader route was reproduced on this head. That makes it a disclosed coverage trade, recorded here so it is a decision rather than an omission.
The one thing the earlier browser mutant did establish is narrow: a generic accessibility smoke does not own the production surface inventory. That supports adding a small registry or keyboard-entry contract that does own it — it does not justify restoring six broad journeys. If a genuinely broken route is ever reproduced on a clean head, that becomes a P1 on its own merits, independent of this PR.
Neither P2 blocks. The merge decision remains a human's.
简体中文
在 exact head 72287bbf 上批准。上一轮的两条 P1 全部降级为 [P2],你对定级的 push back 我接受。 四处修复均已验证具备区分力。下面有一处对 PR 描述中某项声称的更正——它不改变结论,但它改变了「究竟是什么在守住这条线」。
接受你的定级论证
你的立场是:在干净 head 上没有演示出破坏性动作或坏掉的路由时,不应仅凭假设性的未来变异,就把覆盖变更定为当前的 P1 产品缺陷。这是对的,而把它一致地适用下去,正是两条 finding 移动的原因。
一次不让任何东西变红的变异,确立的是「守卫缺失」,而不是「干净 head 行为错误」。 要紧的区分在于:一个后果是作为「已经在发生的事」被复现出来的,还是只能经由一次未来的、刻意的、可被 review 的改动才可达。rail 与 AX 两条属于后者。同一原则我已适用于本系列的另一个 PR,并同样撤回了那里的一条 P1。
已验证:四处修复,各自都有区分力测试
每一处都通过「打断生产路径、要求新测试变红」来检验:
- Project picker——
onSelectNoProject与onSelectProject双双改为 no-op;目标测试在第一次选择即失败,期望null实得project-a。 - WorkHub reopen——AppShell 的
openWorkHub改为 no-op;navigate-away/reopen journey 在第二次waitForWorkHubReady失败,而跨 Session exact-link replacement 仍绿。归因干净:新 journey 独自钉住 shell reopen 接缝。 - revision draft——helper 与 direct-call 替代已撤除(
revision-draft-lifecycle.ts与app-shell-revision-actions.test.ts已删),清 child/source draft、完成 copy、清 revision state 重新走 AppShell 生产 call site。两条真实 journey 分别证明成功 retry 后两个 composer 都空,以及失败后取消能恢复先前文本与 Project Skill chip。 - 重新标注范围——描述现在明说移除 15 个 Electron 测试,而不再暗示等价。这处更正是对的。
exact head 上 Electron 5/5,Desktop 与 Storybook build 通过,hosted required test 绿,重新 fetch 未漂。
[P2] 新增的行菜单断言,并不守住你所赋予它的那个合同
ActiveTaskActionsOpen 断言 Rename 与 Archive 存在、且不存在 /删除/ 菜单项。对于它意图捕捉的那个变异,这条断言不可能失败。
该 story 以 panelProps(...) 渲染 SessionRail,而 panelProps 展开的是 session-list-panel.stories.tsx第 77 行的模块级 rowActions 常量——onToggleFlag、onArchive、onUnarchive、onRename。它从不组合生产 provider。 所以在生产侧重新暴露删除,这条 story 仍然是绿的:rail 只有在被传入删除 callback 时才会渲染 Delete 项,而这个 fixture 永远不会传。 只有在同一次改动里连 fixture 一起改,断言才会变红。
因此它杀掉的是「组件里无条件写死了 Delete 项」,而不是「provider 暴露了一个可选的 Delete」。 后者才是被删掉的那条 journey 原本能接住的变异。
今天真正守住这条线的,是类型。session-history-list.tsx 中的 SessionRowActions 恰好声明四个成员——flag、archive、unarchive、rename——而整个文件里不存在任何 delete 形状的标识符。 重新暴露 rail 删除,必须往这个 interface 上加成员:那是一次会出现在 review 里的、带类型的改动。这才是它是 P2 而非 P1 的具体理由——不仅仅因为「没有复现出缺陷」。
值得补的缺口很小,而且不需要把 parent-deletion journey 搬回来。二者取一即可:
- 一条静态断言:
SessionRowActions不暴露任何破坏性成员——把不变量钉在真正强制它的地方;或 - 一条 render contract:组合生产 provider 而非 fixture,让被测的菜单就是用户拿到的菜单。
两者都很便宜。为了找回这一个 bit 而恢复整条 Electron journey,不是相称的答案,我也没有要求。
[P2] AX 与 Module Hub 的覆盖减法
同一原则,同一等级。六条宽 journey 与 route sweep 是被刻意退役的,而在此 head 上没有复现出任何当前坏掉的键盘或屏幕阅读器路由。因此这是一笔已披露的覆盖取舍,记录于此,使它成为一个决定而不是一处遗漏。
先前那个浏览器变异确实确立的东西很窄:通用无障碍 smoke 并不拥有生产表面清单。这支持补一条小的 registry 或键盘入口 contract 去拥有它——它并不 justify 恢复六条宽 journey。 若将来能在干净 head 上复现一条真正坏掉的路由,那将凭其自身成为 P1,与本 PR 无关。
两条 P2 都不阻塞。合并与否仍由人决定。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Summary
P1 Electron tests mixed reusable component contracts with shell journeys. This change removes 15 Electron tests while retaining five journeys whose integration coverage is still useful. Playwright lists 63 tests in 28 files, versus 78 tests in 35 files on the PR base.
The revision-draft helper extraction has been removed: production AppShell and its conversation exports are unchanged from the base. Successful edit-and-resend and cancellation remain real Electron journeys. WorkHub reopening and cross-Session delegation replacement also remain, along with the focused session-row draft/focus journey extracted from the old AX sweep.
Refs #4761
Verification
Coverage decisions
The review correctly identified missing integration coverage, but a removed assertion is not automatically a requirement to recreate the full journey. This revision retains the destructive-entry and draft/focus contracts at the smallest existing boundaries, preserves WorkHub/revision integration where equivalence is not established, and explicitly retires broad route inventories. Generic AX auditing is no longer described as equivalent to all seven deleted AX journeys.
AI use
Select exactly one:
Tool(s) and scope: Codex — test migration, review adjudication, focused mutation checks and this description. AI-authored commits carry Generated-by: Codex.
Checklist
Does this PR entail a change in behavior?