Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Battleplus wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f59cdd
test(desktop): fence staged steering before side chat
Battleplus Aug 13, 2026
efe541b
test(desktop): wait for steering submit completion
Battleplus Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions apps/desktop/e2e/slash-command-menu.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,11 @@ test('dispatches a staged slash command instead of steering it into a running tu
await expect(page.getByRole('button', { name: '停止' })).toBeVisible();

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

@M4n5ter M4n5ter Aug 13, 2026

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 驱动，因此它被移除才是该测试真正需要的完成边界。

@Battleplus Battleplus Aug 14, 2026

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.

await expect(steerSubmit).not.toHaveAttribute('aria-busy', 'true');

await composer.fill('/');

@Astro-Han Astro-Han Aug 18, 2026

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.

const menu = page.getByRole('listbox', { name: '命令和技能' });
Expand All @@ -133,6 +136,5 @@ test('dispatches a staged slash command instead of steering it into a running tu
await composer.press('Enter');

await expect(page.locator('.maka-quote-workbar-panel')).toHaveCount(1);
await expect(page.getByText(/Acknowledged steering: \/compact explain/)).toBeVisible();
await page.getByRole('button', { name: '停止' }).click();
});
Loading