Skip to content

test(desktop): fence staged steering before side chat - #2936

Closed
Battleplus wants to merge 2 commits into
apache:mainfrom
Battleplus:test/fence-staged-steering
Closed

test(desktop): fence staged steering before side chat#2936
Battleplus wants to merge 2 commits into
apache:mainfrom
Battleplus:test/fence-staged-steering

Conversation

@Battleplus

@Battleplus Battleplus commented Aug 13, 2026

Copy link
Copy Markdown

Summary

Move the existing steering acknowledgement assertion before the /side flow so it acts as an observable completion fence for sendCurrent(). This prevents the test from submitting an overlapping command under CI load without increasing timeouts.

Fixes #2916

Verification

  • npm --workspace @maka/desktop run e2e -- e2e/slash-command-menu.spec.ts --grep "dispatches a staged slash command" --repeat-each=5 (5 passed)
  • npm run build
  • npm run lint
  • npm run format:check
  • npm run typecheck

OpenAI Codex assisted with implementation and verification; I reviewed the change and am the human contributor of record.

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

Wait for the observable steering acknowledgement before submitting the next slash command, so the E2E cannot overlap sendCurrent calls under CI load.

Generated-by: OpenAI Codex

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One ordering issue should be addressed before merging; see the inline comment.

await composer.fill('/compact explain');
await expect(page.getByRole('button', { name: '插入消息' })).toBeVisible();
await composer.press('Enter');
await expect(page.getByText(/Acknowledged steering: \/compact explain/)).toBeVisible();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

English

Thank you for your first contribution to Maka, and for taking on this timing-sensitive CI flake. The change is focused and preserves the existing behavioral assertion. I found one ordering detail that should be tightened before merging:

Important: wait on the composer's pending state directly.

The steering acknowledgement does not strictly prove that sendCurrent() has finished. The acknowledgement arrives through the session-event stream, while sendPendingRef is released only after ipcRenderer.invoke('sessions:steer') resolves. Those are independent asynchronous delivery paths, so the acknowledgement may become visible while sendPendingRef.current is still true.

In that ordering, the test can still continue into /side, and the final Enter can still be ignored by the overlapping-submit guard—the same failure this PR intends to eliminate.

Please keep this assertion to verify that the message was routed as steering, then wait for the composer-owned pending signal before starting the /side flow. For example:

const steerSubmit = page.getByRole('button', { name: '插入消息' });
await expect(steerSubmit).toBeVisible();
await composer.press('Enter');

await expect(page.getByText(/Acknowledged steering: \/compact explain/)).toBeVisible();
await expect(steerSubmit).not.toHaveAttribute('aria-busy', 'true');

aria-busy is driven directly by sendPending, so its removal establishes the completion boundary the test actually needs.

简体中文

感谢你第一次为 Maka 贡献代码,也感谢你主动处理这个对时序敏感的 CI flaky test。这个改动很聚焦,并且保留了原有的行为断言。不过在合并前,还有一个异步顺序问题需要收紧:

重要:请直接等待 Composer 的 pending 状态结束。

Steering acknowledgement 并不能严格证明 sendCurrent() 已经完成。Acknowledgement 通过 session event 流到达,而 sendPendingRef 只有在 ipcRenderer.invoke('sessions:steer') 返回后才会释放。这是两条独立的异步传递路径,因此 acknowledgement 可能已经渲染,但 sendPendingRef.current 仍然是 true

在这种时序下,测试仍可能继续进入 /side 流程,最终的 Enter 依然会被重叠提交保护逻辑忽略,也就是这个 PR 想解决的同一种失败。

建议保留当前断言,用于验证消息确实被路由为 steering;然后在开始 /side 流程前,等待 Composer 自己的 pending 信号结束。例如:

const steerSubmit = page.getByRole('button', { name: '插入消息' });
await expect(steerSubmit).toBeVisible();
await composer.press('Enter');

await expect(page.getByText(/Acknowledged steering: \/compact explain/)).toBeVisible();
await expect(steerSubmit).not.toHaveAttribute('aria-busy', 'true');

aria-busy 直接由 sendPending 驱动,因此它被移除才是该测试真正需要的完成边界。

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in efe541b. The test now keeps the steering acknowledgement assertion for routing, then waits for the same Insert message button to clear aria-busy before starting the /side flow. This makes the completion fence follow the composer-owned sendPending state, as requested. Verified with the focused Electron E2E repeated 5 times (5/5 passed), the complete slash-command-menu spec (4/4 passed), the Desktop typecheck, Biome, and git diff --check.

Wait for the composer-owned aria-busy signal to clear before starting the side chat flow, while retaining the steering acknowledgement assertion.

Generated-by: OpenAI Codex
@M4n5ter

M4n5ter commented Aug 14, 2026

Copy link
Copy Markdown
Member
English

Thank you for the update. I verified that the new aria-busy fence directly follows the Composer’s pending state, closes the original overlapping-submit race, and preserves the steering behavior assertion.

This is a focused, low-risk test reliability fix. No further changes are requested; we will merge it once the required checks pass.

中文

感谢更新。我已确认新增的 aria-busy fence 直接跟随 Composer 的 pending 状态,能够消除原来的重叠提交竞态,同时保留 steering 行为断言。

这是一项范围明确、风险很低的测试稳定性修复。目前不再要求修改代码,等待 required checks 通过后即可合并。

@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.

Thanks for targeting the real steering/composer race. I reviewed this one-file PR with an independent @reviewer pass plus a targeted read-only ollama-cloud/deepseek-v4-flash:high pass.

The acknowledgement is a useful assertion that steering was consumed, but it is not the missing synchronization boundary. The actual invariant is that the remounted contentEditable is focused and the slash menu has positively populated before selecting /side. This head still calls fill('/') immediately after a submit-button condition, and the current e2e check is failing.

Current main already contains the smaller, stronger fix at the same test: click the composer, type / sequentially, and wait for the /side option to be visible. That positive UI condition directly owns the race; the negated aria-busy assertion is tied to a button that can remount/disappear and does not prove composer focus. The best Occam outcome is to close/supersede this PR or rebase and keep only any acknowledgement assertion that adds value without replacing main's focus/menu readiness sequence.

No local test suite was run during this review; conclusions are based on source, current-main, and current CI inspection. Codex performed the final adjudication; external-model output was treated as unverified until checked against the code.

中文摘要

感谢定位 steering/composer 竞态。acknowledgement 能证明 steering 已被消费,但不是缺失的同步边界。真正不变量是:remount 后的 contentEditable 已获得焦点,并且 slash menu 已正向填充,再选择 /side。当前 head 仍在 submit-button 条件之后直接 fill('/'),当前 e2e 也在失败。

Current main 已在同一测试采用更小且更强的修复:click composer、顺序输入 /、等待 /side 可见。这个正向 UI 条件直接覆盖竞态;aria-busy 的否定断言绑定在可能 remount/disappear 的按钮上,也不能证明 composer focus。最符合奥卡姆剃刀的结果是关闭/标记 superseded,或 rebase 后保留 main 的 focus/menu readiness,仅保留确有价值的 acknowledgement。

本次未运行本地测试套件;结论来自源码、current-main 与 CI 检查。外部模型输出在核对代码前均视为未验证输入。

await expect(page.getByText(/Acknowledged steering: \/compact explain/)).toBeVisible();
await expect(steerSubmit).not.toHaveAttribute('aria-busy', 'true');

await composer.fill('/');

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.

P1 — Wait for composer focus and a populated menu, not the old submit button. Steering clears/remounts the composer, so fill('/') can land before the contentEditable is focused and leave the command group empty; this head's e2e is failing on that path. The preceding negated aria-busy check neither proves the new composer is attached/focused nor gives a positive menu-ready condition. Please adopt current main's composer.click()pressSequentially('/')expect(side).toBeVisible() sequence (or close this PR as superseded), then rerun only this spec repeatedly.

@Astro-Han

Copy link
Copy Markdown
Contributor

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@jackwener

jackwener commented Aug 20, 2026

Copy link
Copy Markdown
Member

Thank you for the focused work on this E2E race. Current main now contains the narrower synchronization boundary in commit 5ef39c3: it refocuses the remounted composer, types the slash sequentially, and positively waits for the /side option to become visible. That directly proves the UI is ready and supersedes the aria-busy fence proposed here.

This PR is now superseded and can be closed to keep the backlog clear. If the steering acknowledgement assertion still provides unique value, it can be proposed separately as a small change on current main. Thanks again for tracing the original race.

中文

感谢你针对这个 E2E race 做出的聚焦修复。当前 main 已在 commit 5ef39c3 中采用了更直接的同步边界:重新聚焦 remount 后的 composer、顺序输入 slash,并正向等待 /side option 可见。这能直接证明 UI 已准备好,因此已经取代这里的 aria-busy fence。

此 PR 现在已被 supersede,可以关闭以保持 backlog 清晰。如果 steering acknowledgement 断言仍有独立价值,欢迎基于最新 main 另提一个更小的改动。再次感谢你定位原始竞态。

@M4n5ter M4n5ter closed this Aug 20, 2026
@M4n5ter

M4n5ter commented Aug 20, 2026

Copy link
Copy Markdown
Member

ref #3291

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.

test(desktop): fence staged steering in slash-command E2E

4 participants