revert(web-shell): restore pre-#8098 composer animations at 50% opacity - #9349
Conversation
This reverts commit d213c85. # Conflicts: # packages/web-shell/client/e2e/web-shell.composer.mobile.spec.ts
…e animation revert The #8098 animation revert removes the position: relative / z-index: 1 on .appChatEmpty .chatViewWrap, so #8601's position: static override (a fix for the containing-block shift that #8098 introduced) is a no-op and its comment describes a dot field that no longer exists. Sync the e2e layout assertions to the restored pre-#8098 values (z-index auto, static wrap at 761px) while keeping the footer-anchoring contract they verify.
Design feedback: the restored composer glow reads too bright. Halve the opacity ceiling for the aura grid, the halo and the pulse glow while keeping the dac-aura-cols dance animation and the focus/pulse dynamics intact.
|
|
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Thanks for the PR! Template looks good ✓ Problem: this is a design-direction revert, not a bug claim — designer feedback that the specular-highlight / dot-field / typewriter set added in #8098 is not the desired visual direction. A design decision like that isn't verifiable from a diff, but the author is a write-access collaborator and the original author of both reverted PRs (#8098, #8263), and the revert restores previously-shipped behavior. This is not theoretical hardening, and a revert carries no reproduction burden. Direction: aligned — restoring a prior visual per design request is normal product iteration, and the 50% opacity ceiling is an explicit design ask on top. Nothing here touches auth, sandbox, model selection, telemetry, release, or public contract, so no direction escalation. Size: not applicable — no core paths are touched; everything sits in Approach: the scope is right. Reverting #8098 together with its companion stability fix #8263 is the correct unit. Dropping #8601's mobile-composer Risk: no elevated risk signals (none of the changed files match the revert-prone paths). Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:这是设计方向类的回退,不是 bug 声明——设计师反馈 #8098 引入的 specular 高光/点阵背景/打字机动画不是期望的视觉方向。这类设计决定无法从 diff 验证,但作者是 write-access collaborator、被回退的两个 PR(#8098、#8263)的原作者,且回退恢复的是此前已发布的行为。不属于理论性加固,回退类 PR 也无复现义务。 方向:对齐——按设计要求恢复原有视觉属于正常产品迭代,50% 透明度上限是明确的附加设计要求。不涉及 auth、sandbox、模型选择、telemetry、release 或公共契约,无需升级方向评审。 规模:不适用——未触及核心路径;全部改动位于 方案:范围合理。#8098 与其配套稳定性修复 #8263 作为一个单元整体回退是正确的;移除 #8601 的移动端 风险:无升级风险信号(改动文件均未命中易回退路径)。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent baseline (written before reading the diff): for "revert #8098 + #8263 and restore the glow at 50% opacity", the minimal path is — revert both commits as one unit, scale the glow's opacity expressions by 0.5, drop the #8601 override that only existed to patch around #8098's containing-block shift, and sync the e2e layout assertions to the restored geometry without weakening the footer-anchoring contract they verify. The PR does exactly this. Fidelity check — I pulled the tree at
One non-blocking nit: No critical issues found. Test evidence (unattended CI run — no local execution)This run does not build or execute PR code. The evidence below is the PR's own CI on the reviewed commit, fetched via the API; nothing here is the author's self-report. At review time the unit suite and the web-shell visuals capture were still in flight — the table below is updated in place once CI settles, and no failures were recorded so far. Note for coverage: the Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Sandboxed verification would settle the remaining runtime claim: Real-scenario testing: N/A for this run — unattended CI never executes PR code; live-behavior signal comes from the lane named above and the PR's own CI. 中文说明代码审查独立基线(读 diff 之前写下):对"回退 #8098 + #8263、以 50% 透明度恢复 glow",最小路径是——把两个 commit 作为一个单元回退、将 glow 的 opacity 表达式缩放 0.5、移除仅为修补 #8098 containing-block 偏移而存在的 #8601 覆盖、并在不削弱 footer 锚定契约的前提下把 e2e 布局断言同步为还原后的几何。本 PR 正是这么做的。 保真度核对——我拉取了 #8098 合入前的紧邻 commit(
一个非阻塞小问题: 未发现阻塞问题。 测试证据(无人值守 CI——不在本地执行)本次运行不构建或执行 PR 代码。以下证据为通过 API 获取的该 commit 自身 CI 信号,不含作者自述。审查时单元测试与 web-shell 视觉采集仍在运行——下表会在 CI 结束后原地更新,目前无失败记录。覆盖说明: 沙箱验证可以补齐剩余的运行时声明: 真实场景测试:本次不适用——无人值守 CI 不执行 PR 代码;运行时信号来自上面指名的验证通道与 PR 自身 CI。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — exemplary revert fidelity, one non-blocking stale-marker nit; the visual/runtime claim awaits the in-flight CI (unit suite + web-shell visuals composite). Stepping back: this is what a careful revert should look like. I sketched the minimal approach before reading the diff — revert the feature and its companion fix as one unit, scale the three glow opacity expressions by 0.5, drop the #8601 override that only patched around #8098's containing-block shift, and sync the e2e geometry assertions without weakening the anchoring contract a different author's fix introduced — and the PR matches it point for point. The byte-level comparison against the pre-#8098 tree shows the restoration is exact rather than a reimplementation, and there is no scope creep in either direction. My one reservation is inherent to the PR type: "the designer prefers the old direction" is a product judgment a gate cannot verify from code — but the author is a write-access collaborator and the original author of what is being reverted, restoring previously-shipped behavior, which is about as low-risk as reverts get. The stale Verdict: approve — but 中文说明置信度:4/5 —— 回退保真度堪称范例,仅有一处非阻塞的过时标记小问题;视觉/运行时声明等待进行中的 CI(单元测试 + web-shell 视觉对比)。 总体看:这是一次谨慎的回退应有的样子。我在读 diff 之前勾勒了最小方案——把特性与其配套修复作为一个单元回退、将三处 glow opacity 表达式缩放 0.5、移除仅为修补 #8098 containing-block 偏移而存在的 #8601 覆盖、并在不削弱另一位作者的修复所引入的锚定契约的前提下同步 e2e 几何断言——本 PR 与之逐点一致。与 #8098 之前代码树的逐字节对比表明这是精确还原而非重新实现,两个方向都没有范围蔓延。唯一的保留意见来自 PR 类型本身:"设计师更倾向旧方向"是门禁无法从代码验证的产品判断——但作者是 write-access collaborator、被回退内容的原作者,恢复的是此前已发布的行为,这几乎是回退类 PR 中风险最低的形态。App.tsx 中过时的 结论:批准——但 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
chiga0
left a comment
There was a problem hiding this comment.
Review Summary
Type: Revert (#8098 + #8263) + intentional 50% opacity tweak
Change size: +222/-1950 (net simplification)
Clean, well-scoped revert. The scope matches the description exactly: removes the WebGL specular effect and dot-field canvas (4 files deleted), restores the CSS-based DAC glow, removes the #8601 mobile containing-block workaround that became a no-op after the revert, and halves the opacity ceiling.
Revert Completeness ✅
SpecularComposerEffect.tsx/.test.tsxdeleted ✅NewSessionDotField.tsx/.module.cssdeleted ✅- Design doc
web-shell-specular-composer-animation.mddeleted ✅ animatePlaceholderprop +TypewriterPlaceholdercomponent removed fromChatEditor✅NewSessionDotFieldmount removed fromApp✅position: relative/z-index: 1on.appChatEmpty .chatViewWrapremoved (was from #8098, dot-field z-index stacking) ✅- Mobile
position: staticoverride from #8601 removed (no-op after revert) ✅ - All related mocks and test cases cleaned from
App.test.tsx,ChatEditor.test.tsx,ChatPane.test.tsx✅ - E2e assertions updated to pre-#8098 layout values ✅
Restored attachComposerGlow — Implementation Verified
The returned imperative function is correctly structured:
- Focus/blur wiring —
if (document.activeElement === inputEl) animateGlow(1)correctly handles already-focused state at mount time ✅ - RAF-based animation — easing formula
start + diff * (1 - (1-t)²)(ease-out quadratic) correct;glowRaf !== undefinedcheck prevents orphaned frames ✅ - Pulse decay — each keydown resets
pulseCurrent = 1synchronously (no race), schedules 100ms decay, separate typing timer debouncesdata-dac-typingattribute ✅ - Cleanup — cancels all pending RAFs (
glowRaf,pulseRaf), clears all timers (pulseDecayTimer,typingTimer), removes all event listeners, resets CSS custom properties to 0 ✅ - No React state — drives animation via
glowRootEl.style.setProperty('--dac-glow-on', …)rather thansetState, correctly avoiding re-renders on every frame ✅ - Blur + in-flight pulse — blur does not cancel
pulseDecayTimer; bothanimateGlow(0)and the remaining pulse decay run concurrently, both converging to 0. No visual corruption ✅
CSS Opacity at 50% — Verified
.dacAuramax opacity:1 * (0.33 + 1 * 0.17)= 0.50 ✅.dacHalomax opacity:1 * 0.5= 0.50 ✅.dacHalo::aftermax opacity:1 * 1 * 0.5= 0.50 ✅prefers-reduced-motion:dac-aura-colsanimation disabled,::afteropacity forced to 0,::before(static grid) stays at 1 — provides a static but visible glow ✅
Two Nits
Nit 1 — data-dac-glow on the container div is not referenced in any CSS selector, test assertion, or e2e selector. It appears to be a debugging/semantic marker left over. Harmless, but adds noise.
Nit 2 — useEffect([editorViewRef]) captures editorViewRef.current?.contentDOM at mount time and closes over it. Since editorViewRef (the ref object) is stable for the component's lifetime, this effect runs once. If CodeMirror internally replaces its view object (uncommon, but triggered by certain immutable prop changes), the listeners would remain on the old contentDOM and the glow would silently stop responding. Low-risk in practice, and a pre-existing pattern in the codebase.
Additional Audit Coverage
- Mobile layout regression: the
position: staticoverride removal only applied when.chatViewWithMobileComposerBottomwas present without thewelcomeMiddle + customFootercarve-out. Withoutposition: relative/z-index: 1on.chatViewWrap, the static override is correctly a no-op — the absolute-positioned footer still anchors to.chatPanewhich retainsposition: relativevia.appChatEmpty .chatPane(also removed from the base rule but kept in the mobile rule). Wait —.appChatEmpty .chatPane { position: relative }was also removed from the base non-media rule. The mobile absolute-footer path still works because.footerinside.chatViewWithMobileComposerBottomisposition: absoluteand its containing block is.chatPanewhich getsposition: relativevia.chatPanebase styles (notappChatEmpty-specific). This needs CI e2e to confirm, which the PR flags. ChatPane.tsxanimatePlaceholder={false}removal: prop no longer exists, correctly cleaned up ✅ChatEditor.tsxeditorAreaposition: relativeremoval: was used to positionTypewriterPlaceholderabsolutely inside it; TypewriterPlaceholder is gone, so the removal is correct ✅
… panels Dropping the #8601 override removed the positioned containing block for the bottom panels (status panel) in the custom-footer mobile welcome variant: that variant renders the composer footer display: contents, so its wrap is the only positioned ancestor the absolutely-positioned bottom panels can anchor to. Restore position: relative on just that variant instead of the full-positioned wrap the deleted rule had excluded, keeping the plain variants static so the footer keeps anchoring to the chat pane.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.13)
chiga0
left a comment
There was a problem hiding this comment.
Re-Review at HEAD 5225f12
Previous review was at base state (4 commits); one new commit (5225f12) pushed since.
Commit Verification
| Previous Finding | Status | Evidence |
|---|---|---|
Additional Audit Coverage — position: relative lost on chatViewWrap for mobile welcome-middle + custom-footer variant |
✅ Fixed | Commit 5225f12: .appChatEmpty .chatViewWithMobileComposerBottom.chatViewWithWelcomeMiddle.chatViewWithCustomFooter { position: relative } correctly restores the containing block for .bottomPanels |
Nit: data-dac-glow unreferenced |
⚪ Author choice — harmless | |
Nit: editorViewRef once-only comment |
⚪ Author choice — harmless | |
R1-1 (qwen-code-ci-bot) — disabled?: boolean; over-deleted from ChatEditorRenderProps |
✗ Still present | ChatEditorRenderProps at HEAD has no disabled field; call sites at lines 599 and 1554 still pass it |
R1-2 (qwen-code-ci-bot) — stale comment at App.tsx:12067 |
✗ Still present — now more urgent: the class is load-bearing after 5225f12 |
New Commit Assessment — 5225f12
The fix is logically correct and narrowly scoped:
- Why the regression happened: The
#8601rule used:not(.chatViewWithWelcomeMiddle.chatViewWithCustomFooter)to exclude the custom-footer variant fromposition: static, effectively keeping the#8098position: relativefor it. After reverting both#8098(which added the rule) and#8601(which overrode it), thechatViewWrapbecameposition: static(browser default) for all variants — including the custom-footer one that must stay as the containing block for the absolutely-positioned.bottomPanels. - What
5225f12does: Re-appliesposition: relativeonly to the triple-class compound selectorchatViewWithMobileComposerBottom + chatViewWithWelcomeMiddle + chatViewWithCustomFooter. Avoids touching plain variants. CSS comment in the rule is accurate. ✅ - Residual gap:
emptyMobileComposerLayout()throws for the custom-footer variant (footer hasdisplay: contents→ no box), so no e2e test walksgotoEmptyMobileWelcomeHarness(page, { customFooter: true })to verify.bottomPanelsanchoring. A future deletion of this rule would be invisible to CI.
Open Items
- Minor —
ChatEditor.test.tsx:342:disabled?: boolean;missing fromChatEditorRenderProps(see inline comment). One-line fix, already suggested byqwen-code-ci-botat R1-1. - Minor —
App.tsx:12067(not in diff — filing here): the commentis now doubly false — the// Marker class (no declarations): keeps the ':not(...)' exclusion in App.module.css matching.:not(...)exclusion was deleted inb55a7ad2, and the class now has a real CSS declaration added by5225f12. A maintainer reading the stale comment would conclude the class is dead and remove it, breaking.bottomPanelslayout. Suggested replacement:// Positioning hook for the mobile custom-footer variant: the compound selector // in App.module.css uses this class to restore position: relative on the wrap, // keeping it the containing block for the absolutely-positioned bottom panels // when the custom footer renders display: contents.
This review was generated by QoderWork AI
- Restore disabled?: boolean on the ChatEditor test render props: two surviving call sites still pass disabled: true, and dropping the member leaves object-literal excess-property violations (latent only while test files stay out of tsc). - Update the stale chatViewWithCustomFooter marker comment in App.tsx: the :not(...) exclusion it described was removed, and the class now completes the compound selector that keeps the custom-footer wrap positioned for the bottom panels. - Document the once-only glow effect invariant and the legacy data-dac-glow marker, both restored verbatim from the pre-#8098 implementation.
chiga0
left a comment
There was a problem hiding this comment.
All findings resolved in the latest commit:
disabled?: booleanrestored toChatEditorRenderProps✅chatViewWithCustomFootercomment updated to reflect its new load-bearing role ✅data-dac-glowmarker andeditorViewRefonce-only invariant both documented ✅
LGTM.
This review was generated by QoderWork AI
|
@qwen-code /verify |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 7561 passed · 0 failed · 7561 total 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:7561 通过 · 0 失败 · 7561 总计 Verification reportPR #9349 Deep Verification — revert(web-shell): restore pre-#8098 composer animations at 50% opacityVerdict: 中文摘要结论: A/B 结论:
发现:仅 1 条建议级——50% 上限目前没有任何仓库内测试钉住(变异 0.9 在全套件中无人捕获,只有本验证的 witness fixture 能捕获),建议把该 computed-style 断言沉淀进 e2e。 未覆盖:还原代码与 pre-#8098 原始实现的逐字节忠实性(浅克隆无法取得 pre-#8098 blob,只能相对 base 证明行为增删);完整 Playwright 套件与 visuals 截图套件;其他 workspace 的测试;Windows/macOS 目视验证。 Central claim and A/B proofCentral claim: the PR cleanly removes the #8098/#8263 animation stack (WebGL specular composer layer, new-session dot field, typewriter placeholder) and restores the pre-#8098 DAC glow with a 50% opacity ceiling, leaving no dangling references and preserving the mobile footer-anchoring contract. Secondary claims: (1) the updated e2e layout assertions are load-bearing for the restored layout; (2) the Unit A/B (identical probe, both arms; vitest/jsdom)The probe is one block appended verbatim to a scratch copy of each tree's own
All four base-arm failures are behavioral assertion mismatches ( Full suites: head 187 files / 3703 tests, all pass (exit 0); base: 189 files collected (its native 188 — one more than head, E2e A/B (real chromium, vite dev server, mock daemon via route interception)
The E4 pass-then-fail pattern is the semantics-equivalence proof for secondary claim 2: the bottom-panel invariant held on base (via the The 50% opacity cap, measured in a real browser
9/9 scripted cap assertions held: halo ceiling exactly Mutation checks (teeth of the new assertions)
Mutation B's survivor-then-kill is the substance of Finding 1 below. Static completeness checks (scripted greps)
CorrectionsNone — first verification round; no prior bot findings to correct. Findings1. (Suggestion) The 50% opacity ceiling is pinned by no test in the repoThe cap is the PR's one deliberate visual delta, but nothing in the unit suite (CSS modules are not evaluated under vitest), the e2e specs, or the visuals suite asserts any opacity value — grep for Not covered
MethodologyEnvironment: CI verify container ( Evidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/ChatEditor.tsx:1918 — [review] restored DAC glow lifecycle has zero test coverage after the revertpackages/web-shell/client/components/ChatEditor.tsx:1918 — [review] restored composer glow drops the prefers-reduced-motion gate both deleted implementations honored
中文说明
无阻断问题。LGTM!✅
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
Released in v0.21.14. |



























What this PR does
Reverts the composer animation enhancement (#8098) and its companion stability fix (#8263), restoring the pre-#8098 DAC glow effect per design request. The restored glow's opacity ceiling is adjusted to 50% as part of the same request. Also removes the mobile-composer CSS override from #8601 that only existed to work around a containing-block shift introduced by #8098 — it is a no-op after the revert — and syncs the e2e layout assertions to the restored pre-#8098 values while keeping the footer-anchoring contract they verify.
Why it's needed
Designer feedback: the specular-highlight / dot-field / typewriter animation set added in #8098 is not the desired visual direction, and the original effect should be restored. The restored glow should also render at 50% opacity instead of full strength.
Reviewer Test Plan
How to verify
npm run dev:daemonin a checkout of this branch, open the web shell, and focus the composer in an empty chat — the composer should show the restored grid-aura glow with a dance animation at 50% opacity (no specular highlight, no dot field, no typewriter placeholder).cd packages/web-shell && npx vitest run— all tests pass.Evidence (Before & After)
N/A — non-UI verification is covered by the unit suite; visual before/after can be captured locally via dev:daemon per step 1.
Tested on
Environment (optional)
Local
npm run dev(vite) against the daemon dev server. Unit tests:npx vitest run(604 relevant tests in App/ChatEditor/ChatPane passed). Full e2e (Playwright) not run locally.Risk & Scope
Linked Issues
References #8098 (reverted feature) and #8263 (reverted companion). No open issues closed.
中文说明
本 PR 做了什么
按设计师要求回退 composer 动画增强(#8098)及其配套稳定性修复(#8263),恢复 #8098 之前的 DAC 光效。同时按要求将还原后的光效透明度上限调整为 50%。一并移除 #8601 中仅为规避 #8098 引入的 containing-block 偏移而存在的移动端 CSS 覆盖规则(回退后已是 no-op),并将 e2e 布局断言同步为还原后的 pre-#8098 数值,保留其验证的 footer 锚定契约。
为什么需要
设计师反馈:#8098 引入的 specular 高光 / 点阵背景 / 打字机动画不符合期望的视觉方向,应还原原有动画效果;且还原后的光效需以 50% 透明度呈现而非全强度。
审查者测试计划
如何验证
npm run dev:daemon,打开 web shell,在空会话中聚焦 composer —— 应显示还原后的网格光晕 + 舞动动画,透明度 50%(无 specular 高光、无点阵背景、无打字机占位符)。cd packages/web-shell && npx vitest run—— 全部通过。前后对比(Evidence)
N/A —— 非 UI 改动由单元测试覆盖;视觉效果可按第 1 步在本地通过 dev:daemon 查看。
测试环境
环境(可选)
本地
npm run dev(vite)+ daemon dev server。单元测试:npx vitest run(App/ChatEditor/ChatPane 相关 604 个测试通过)。完整 e2e(Playwright)未在本地运行。风险与范围
关联 Issues
引用 #8098(被回退的特性)与 #8263(被回退的配套修复)。无待关闭的 issue。