Skip to content

fix(web-shell): stabilize mobile composer after resume #8263

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

Merged
wenshao merged 3 commits into from
Aug 1, 2026
Merged

fix(web-shell): stabilize mobile composer after resume #8263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7106b84
fix(web-shell): stabilize mobile composer effects
Jul 31, 2026
9d6a339
Merge branch 'main' into codex/fix-mobile-resume-layout
ytahdn Aug 1, 2026
eca1d7a
fix(web-shell): tear down all specular listeners on WebGL context los…
qwen-code-dev-bot Aug 1, 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
1 change: 1 addition & 0 deletions packages/web-shell/client/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10149,6 +10149,7 @@ export function App({
composerInput={composerInput}
composerInputVersion={composerInputVersion}
placeholderText={composerPlaceholderText}
animatePlaceholder={isChatEmptyState}
Comment on lines 10150 to +10152

@qwen-code-ci-bot qwen-code-ci-bot Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The positive branch of this wiring — animated placeholder ON in the welcome/empty state — has no test at the App integration level; only the negative branch (off in-session) is pinned, by the new mobile e2e assertion. — Failure scenario: the mutation animatePlaceholder={isChatEmptyState} → animatePlaceholder={false} survives every test in this diff: the ChatEditor unit tests render the component directly and never exercise App's prop wiring, and the mobile e2e visits /session/<id> where isChatEmptyState is already false, so toHaveCount(0) passes either way. The welcome page could silently lose its animated placeholder and CI would ship green. Suggested fix: add an e2e (or App-level render test) that loads the pre-session welcome state and asserts the typewriter is present, pairing the mobile spec's in-session count-0 assertion so both sides of the wiring are pinned.

// e.g. in a welcome-state spec
await page.goto('/');
await expect(
  page.locator('[data-web-shell-composer-typewriter]'),
).toHaveCount(1);
中文说明

这段接线的正向分支——在欢迎/空状态下开启动画占位文本——在 App 集成层面没有测试；只有负向分支（会话内关闭）被新的移动 e2e 断言固定。— 失败场景：将 animatePlaceholder={isChatEmptyState} 突变为 animatePlaceholder={false} 后，本 diff 中的所有测试仍能通过：ChatEditor 单元测试直接渲染组件，从不验证 App 的 prop 接线；而移动 e2e 访问的是 /session/<id>，此时 isChatEmptyState 已为 false，因此 toHaveCount(0) 无论如何都会通过。欢迎页可能会悄无声息地失去动画占位文本，而 CI 仍会绿灯通过。建议修复：增加一个 e2e（或 App 层渲染测试），加载会话前的欢迎状态并断言打字机存在，与移动 spec 中会话内 count-0 的断言配对，从而固定接线的两侧。

— qwen3.8-max-preview via Qwen Code /review

@qwen-code-dev-bot qwen-code-dev-bot Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Declined — not worth the diff-growth/flakiness tradeoff. The component-level default (animatePlaceholder=true) is already pinned: the pre-existing ChatEditor typewriter test renders the component with the prop omitted and asserts the typewriter mounts and plays. The remaining gap you correctly identify is the App-level wiring's positive branch (welcome state → typewriter present). Pinning that needs a welcome-state (/) e2e or an App-render test; every existing web-shell e2e visits /session/<id> against the mock daemon, so a welcome-state spec would add new scaffolding and flakiness risk disproportionate to the risk of a single-line, review-visible prop pass-through mutating to false. Happy to add a welcome-state e2e if a maintainer wants both sides of the wiring pinned.

中文说明

已拒绝——不值得以 diff 膨胀/不稳定性为代价。组件层默认值（animatePlaceholder=true）已被固定：已有的 ChatEditor 打字机测试在不传该 prop 的情况下渲染组件，并断言打字机挂载并播放。你正确指出的剩余缺口是 App 层接线的正向分支（欢迎状态 → 打字机存在）。要固定它需要一个欢迎状态（/）e2e 或 App 渲染测试；而现有所有 web-shell e2e 都在 mock daemon 下访问 /session/<id>，因此一个欢迎状态的用例会引入新的脚手架，且其不稳定性风险与“单行、审查可见的 prop 透传被改为 false”这一低风险不相称。如果维护者希望固定接线的两侧，我很乐意补充一个欢迎状态的 e2e。

/>
{CustomComposerFooter && (
<CustomComposerFooter
Expand Down
13 changes: 13 additions & 0 deletions packages/web-shell/client/components/ChatEditor.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ function renderChatEditor(props: {
onSelectModel?: (model: string) => void;
onAttachmentsChange?: (hasAttachments: boolean) => void;
placeholderText?: string;
animatePlaceholder?: boolean;
disabled?: boolean;
followupState?: UseDaemonFollowupSuggestionReturn['followupState'];
customization?: WebShellCustomization;
Expand Down Expand Up @@ -400,6 +401,18 @@ describe('ChatEditor animation layers', () => {
expect(container.querySelector('[data-typewriter-visible]')).toBeNull();
});

it('does not mount the typewriter when placeholder animation is disabled', () => {
const container = renderChatEditor({
placeholderText: 'abc',
animatePlaceholder: false,
});

expect(
container.querySelector('[data-web-shell-composer-typewriter]'),
).toBeNull();
expect(container.querySelector('[data-typewriter-visible]')).toBeNull();
});

it('shows the full placeholder without a caret under prefers-reduced-motion', () => {
const originalMatchMedia = window.matchMedia;
window.matchMedia = vi.fn(
Expand Down
3 changes: 3 additions & 0 deletions packages/web-shell/client/components/ChatEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ interface ChatEditorProps {
cancelArmed?: boolean;
disabled?: boolean;
placeholderText?: string;
animatePlaceholder?: boolean;
commands: CommandInfo[];
skills?: SkillInfo[];
slashCommandCategoryOrder?: CommandDisplayCategoryOrder;
Expand Down Expand Up @@ -1148,6 +1149,7 @@ export const ChatEditor = memo(
cancelArmed = false,
disabled = false,
placeholderText = 'Type a message...',
animatePlaceholder = true,
commands,
skills = [],
slashCommandCategoryOrder,
Expand Down Expand Up @@ -1285,6 +1287,7 @@ export const ChatEditor = memo(
const hasSlashMenu = Boolean(slashMenu);
const hasAtMenu = Boolean(atMenu);
const showTypewriterPlaceholder =
animatePlaceholder &&
!disabled &&
Boolean(placeholderText) &&
!core.hasInput() &&
Expand Down
1 change: 1 addition & 0 deletions packages/web-shell/client/components/ChatPane.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,7 @@ export function ChatPane({
sessionId={connection.sessionId}
atWorkspaceCwd={paneWorkspaceCwd}
placeholderText={t('splitView.composerPlaceholder')}
animatePlaceholder={false}
/>
{CustomComposerFooter && (
<CustomComposerFooter
Expand Down
42 changes: 39 additions & 3 deletions packages/web-shell/client/components/SpecularComposerEffect.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ describe('specular effect pointer leave', () => {
});

describe('specular effect WebGL cleanup', () => {
it('releases WebGL resources on unmount', () => {
it('removes a lost canvas and releases WebGL resources on unmount', () => {
const loseContext = vi.fn();
const glStub = {
ARRAY_BUFFER: 0x8892,
Expand Down Expand Up @@ -521,8 +521,12 @@ describe('specular effect WebGL cleanup', () => {
vi.spyOn(HTMLCanvasElement.prototype, 'getContext').mockReturnValue(
glStub as unknown as WebGL2RenderingContext,
);
vi.spyOn(window, 'requestAnimationFrame').mockImplementation(() => 1);
vi.spyOn(window, 'cancelAnimationFrame').mockImplementation(() => {});
const requestAnimationFrameSpy = vi
.spyOn(window, 'requestAnimationFrame')
.mockImplementation(() => 1);
const cancelAnimationFrameSpy = vi
.spyOn(window, 'cancelAnimationFrame')
.mockImplementation(() => {});

function Harness() {
const composerRef = useRef<HTMLDivElement>(null);
Expand All @@ -545,6 +549,38 @@ describe('specular effect WebGL cleanup', () => {
container.querySelector('[data-web-shell-composer-specular] canvas'),
).not.toBeNull();

const removeWindowListenerSpy = vi.spyOn(window, 'removeEventListener');
const removeDocumentListenerSpy = vi.spyOn(
document.documentElement,
'removeEventListener',
);

const canvas = container.querySelector(
'[data-web-shell-composer-specular] canvas',
)!;
act(() => canvas.dispatchEvent(new Event('webglcontextlost')));

expect(cancelAnimationFrameSpy).toHaveBeenCalled();
expect(removeWindowListenerSpy).toHaveBeenCalledWith(
'pointermove',
expect.any(Function),
);
expect(removeDocumentListenerSpy).toHaveBeenCalledWith(
'pointerleave',
expect.any(Function),
);
expect(
container.querySelector('[data-web-shell-composer-specular] canvas'),
).toBeNull();

requestAnimationFrameSpy.mockClear();
act(() => {
window.dispatchEvent(
new MouseEvent('pointermove', { clientX: 100, clientY: 100 }),
);
});
expect(requestAnimationFrameSpy).not.toHaveBeenCalled();

act(() => root.unmount());
container.remove();

Expand Down
36 changes: 24 additions & 12 deletions packages/web-shell/client/components/SpecularComposerEffect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ export function SpecularComposerEffect({
};

const startLoop = () => {
if (!running) {
if (!running && !disposed) {
running = true;
lastFrame = performance.now();
frameId = window.requestAnimationFrame(draw);
Expand Down Expand Up @@ -303,6 +303,28 @@ export function SpecularComposerEffect({
};

const resizeObserver = new ResizeObserver(resize);
let disposed = false;
const teardown = () => {
if (disposed) return;
disposed = true;
running = false;
window.cancelAnimationFrame(frameId);
frameId = 0;
resizeObserver.disconnect();
window.removeEventListener('pointermove', onPointerMove);
document.documentElement.removeEventListener(
'pointerleave',
onPointerLeave,
);
target.removeEventListener('focusin', onFocusIn);
target.removeEventListener('focusout', onFocusOut);
canvas.removeEventListener('webglcontextlost', onContextLost);
canvas.remove();
};
const onContextLost = () => {
teardown();
};
canvas.addEventListener('webglcontextlost', onContextLost);
resizeObserver.observe(target);
resize();
window.addEventListener('pointermove', onPointerMove, { passive: true });
Expand All @@ -318,17 +340,7 @@ export function SpecularComposerEffect({
startLoop();

return () => {
running = false;
window.cancelAnimationFrame(frameId);
resizeObserver.disconnect();
window.removeEventListener('pointermove', onPointerMove);
document.documentElement.removeEventListener(
'pointerleave',
onPointerLeave,
);
target.removeEventListener('focusin', onFocusIn);
target.removeEventListener('focusout', onFocusOut);
canvas.remove();
teardown();
gl.deleteBuffer(buffer);
gl.deleteProgram(program);
gl.deleteShader(vertexShader);
Expand Down
32 changes: 32 additions & 0 deletions packages/web-shell/client/e2e/web-shell.composer.mobile.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,38 @@ test('renders the textarea backend instead of CodeMirror on touch devices', asyn

await expect(page.locator(COMPOSER_TEXTAREA)).toBeVisible();
await expect(page.locator('.cm-editor')).toHaveCount(0);
await expect(
page.locator('[data-web-shell-composer-typewriter]'),
).toHaveCount(0);
});

test('keeps the composer usable after WebGL context loss', async ({
page,
}, testInfo) => {
const scenario = createWebShellDaemonScenario();
const daemon = await installScenario(page, scenario, testInfo);

await gotoSession(page, scenario, daemon);
const hasWebgl2 = await page.evaluate(
() => !!document.createElement('canvas').getContext('webgl2'),
);
test.skip(!hasWebgl2, 'WebGL2 is unavailable on this runner');
const canvas = page.locator('[data-web-shell-composer-specular] canvas');
await expect(canvas).toBeVisible();

await canvas.evaluate((element) => {
const gl = element.getContext('webgl2');
const extension = gl?.getExtension('WEBGL_lose_context');
if (!extension) throw new Error('WEBGL_lose_context unavailable');
extension.loseContext();
});

await expect(canvas).toHaveCount(0);
const textarea = page.locator(COMPOSER_TEXTAREA);
await expect(textarea).toBeVisible();
await textarea.fill('still usable');
await expect(textarea).toHaveValue('still usable');
await expect(page.locator('[data-web-shell-composer-submit]')).toBeEnabled();
});

test('keeps voice controls reachable on an extra-narrow touch viewport', async ({
Expand Down
Loading