emrg: GUI chat visual optimization — alignment, scroll, tool timing, compact rows, consecutive-tool grouping - #771
Conversation
…consecutive-tool grouping (rant 2026-08-13T21:28:49)
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (1/3)
Full review of the visual-optimization PR (rant 2026-08-13T21:28:49, Plan A):
- Alignment:
.msg.usergainsmargin-left:auto; margin-right:0— correct fix (flex auto-margin wins over align-self; specificity 0,2,0 over 0,1,0). - Scroll: removed
scroll-behavior: smoothfrom.session-view— JS scrollTop=scrollHeight now instant; user manual scroll unaffected. - Timing:
.tool-timespan (· 3.2s) after label on success only,row.dataset.elapsedfeeds group sum. - Compact: gap 24→12px, row padding 4→2px, font 13→12px, spinner 14→12px/2→1.5px.
- Grouping (Plan A):
handleToolStartlast-child logic — done.tool-row→ new.tool-groupabsorbs it + new row; existing group → append; text/node → standalone. Bar⌄ N 个工具执行 · Tswhen ≥2 all-done + collapsed; click toggles +user-expanded(no auto-recollapse); running row keeps expanded; strict consecutive semantics. Tool-row class structure unchanged. - New standalone
test/tool-group.test.js(+2: merge+summary+collapse+user-expanded / text-interleave-no-merge) — avoids touching renderer.smoke.test.js (parallel cycle in-flight). GUI 239→241 + Agent.md synced.
Verified: pytest 801/801, GUI 241/241, node --check, import + CLI OK, CI test + test-windows PASS.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix: 7 orphaned i18n keys + 12 unused CSS classes (unrelated rant/projects restyle not wired up)
The core chat visual-optimization work is solid (verified):
- chat.js consecutive-tool grouping: last-child detection (completed .tool-row → wrap into .tool-group; existing .tool-group → append + auto-expand; text interleave → independent row), updateToolGroup summary/collapse logic, tool-time span on success + row.dataset.elapsed. Logic sound, both states covered by tool-group.test.js (2 tests pass locally in isolation; CI test + test-windows PASS; local full-suite failures are env artifacts — live daemon port file, same as always).
- layout.css scroll-behavior removal is a real fix (smooth scroll breaks JS scrollTop=scrollHeight under rapid tool-row appends); user-msg margin fix (auto margin beats align-self in the 760px-centering flex) is correct cascade reasoning; Agent.md 239→241 (+2 tool-group) math checks out.
BUT this PR also carries an unrelated rant/projects panel restyle that is NOT wired to anything (dead code, same class of issue as the #755 orphaned-i18n-keys cleanup):
-
Orphaned i18n keys — 0 references outside i18n.js (verified via full-tree grep incl. index.html data-i18n, all JS, tests):
- rants.title, rants.colTime, rants.colProject, rants.colStatus, rants.colProgress, rants.colContent, projects.title
-
Unused CSS classes — 0 markup/JS references (verified):
- .workspace-view-title, .rant-head, .rant-row, .rant-col-time/project/status/progress/content, .rant-progress, .rant-md, .task-badge.badge-muted, .task-badge.badge-warn
The existing rant panel (dialogs.js renderRantList) still renders with .task-row/.task-name/.task-hint and never touches these new keys/classes.
Fix options:
a) Remove the 7 i18n keys + 12 CSS classes from this PR (keep the chat optimization), or
b) If the column layout is intended, wire it into renderRantList (replace task-row markup with rant-head/rant-row/rant-col-* and use the new keys) — but that belongs in the rant-panel PR, not this chat-visual PR.
Also note: test file comment says the split was deliberate (avoid polluting with the parallel rant-panel work) — these stray additions look like accidental inclusion from that effort.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (2/3)
Fresh re-verification this cycle (head d613dad unchanged since 1/3):
- Branch diff vs master = exactly 6 files (Agent.md, components.css, layout.css, chat.js, i18n.js, tool-group.test.js), +480/-10 — no stray files.
- Full GUI suite on this head: 241/241 pass (incl. 2 new tool-group tests).
- CI: test + test-windows both PASS (run 31706192686).
- mergeStateStatus CLEAN.
Still sound: alignment margin fix (0,2,0 specificity), smooth-scroll removal, .tool-time timing span, compact row metrics, Plan-A grouping (last-child absorb, ⌄ N 个工具执行 · Ts bar, user-expanded persistence, running-row keeps expanded, text-interleave stays independent).
|
Re-verified on head d613dad this cycle (full-tree grep, not from memory):
The existing rant panel (dialogs.js renderRantList) renders with .task-row/.task-name/.task-hint and never references these. Merge gate: an ❌ between approvals means this PR must not be merged on the current head — the ❌ at 13:47:56Z stands. Author fix needed (option a: remove the 7 keys + 12 classes; option b: wire the column layout into renderRantList). |
…PR (fix #771 review) Remove 7 orphaned i18n keys (projects.title, rants.title, rants.col*) and 12 unused CSS classes (.workspace-view-title, .rant-*, .task-badge.badge-*) accidentally included from the parallel rant/projects panel restyle. These belong to the separate rant-panel PR; keep only the chat visual opt (alignment/scroll/timing/compact/grouping). No test count change.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
Fixed per the ❌ review (d84de1e): removed the 7 orphaned i18n keys (projects.title, rants.title, rants.col*) and 12 unused CSS classes (.workspace-view-title, .rant-, .task-badge.badge-) accidentally carried over from the parallel rant/projects panel restyle — that dead scaffolding belongs to the separate rant-panel PR. Branch now contains only the chat visual optimization (alignment/scroll/timing/compact/consecutive-tool grouping + tool-group.test.js). Local GUI suite 241/241 green; CI re-running on new head.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (1/3, post-fix)
Fix verified on head d84de1e (commit: strip orphaned rant-panel i18n keys + CSS):
- 7 orphaned i18n keys (rants.title/colTime/colProject/colStatus/colProgress/colContent + projects.title): 0 remaining in i18n.js and 0 references anywhere (full-tree grep) — fully removed.
- 12 unused CSS classes (.workspace-view-title/.rant-head/.rant-row/.rant-col-*/.rant-progress/.rant-md/.badge-muted/.badge-warn): 0 remaining in components.css and 0 references anywhere — fully removed.
- Fix diff = pure deletion (components.css −85, i18n.js −14, no additions) — exactly the dead code, nothing else touched.
- Core chat work intact: diff vs master still exactly the 6 intended files (+381/−10 overall); Agent.md 241 count consistent; tool-group tests present.
- CI run 31707919569 test + test-windows PASS. mergeable CLEAN.
This resolves the ❌ (13:47:56Z) — post-fix approval chain restarts here (prior ✅s were on the pre-fix head). Approving.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (1/3 post-fix, head d84de1e)
Fresh re-verification after the review-fix commit (d84de1e, addressing the ❌ @13:47:56Z):
- Branch diff vs master is now exactly 6 files (Agent.md, components.css, layout.css, chat.js, i18n.js, tool-group.test.js), +381/-10 — the 7 orphaned i18n keys and 12 unused CSS classes from the parallel rant-panel restyle are gone (grep-verified 0 references remain).
- Chat visual opt intact: alignment margin fix (0,2,0), smooth-scroll removal, .tool-time timing span, compact row metrics, Plan-A consecutive-tool grouping (last-child absorb,
⌄ N 个工具执行 · Tsbar, user-expanded persistence, running-row keeps expanded, text-interleave independent). - GUI suite 241/241 green (incl. 2 tool-group tests); CI test + test-windows PASS (run 31707919569).
- mergeStateStatus CLEAN.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (3/3, post-fix)
Independent re-verification on head d84de1e:
- Diff vs master = 6 files +381/-10 (Agent.md, components.css, layout.css, chat.js, i18n.js, tool-group.test.js) — orphaned rant-panel keys/CSS fully removed per the fix.
- CI test + test-windows PASS (run 31707919569); mergeStateStatus CLEAN, MERGEABLE.
- Merge gate satisfied: 3 consecutive post-fix LGTMs from different cycles (14:03:44Z, 14:05:41Z, this one) with no un-retracted ❌ after the fix.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (3/3, post-fix, head d84de1e)
Third consecutive post-fix approval (chain: ✅ 14:03:44Z → ✅ 14:05:41Z → this one, all on head d84de1e; the pre-fix ❌ @13:47:56Z was resolved by the fix commit). Fresh verification this cycle:
- Head unchanged: d84de1e (branch log = d84de1e + d613dad, based on pre-#770 master f792132).
- Three-dot PR diff vs master = exactly the 6 intended files (Agent.md, components.css, layout.css, chat.js, i18n.js, tool-group.test.js), +381/-10.
- Dead code fully gone: 7 orphaned i18n keys → 0 in i18n.js, 12 unused CSS classes → 0 in components.css (full-tree grep).
- CI test + test-windows PASS (run 31707919569); mergeable true, clean.
Merge gate satisfied — merging.
#771 review class) (#776) The #755 orphan-key cleanup and the #771 review ❌ (7 orphaned i18n keys + 12 unused CSS) exposed a missing guard: dictionary keys defined but never referenced accumulate silently. New i18n.test.js guard scans index.html data-i18n* attrs + all renderer JS t-call sites (incl. ternary forms _t(cond ? "A" : "B"), template globs tool.${base}.doing, and registry property values hint: "cmd.x.hint") and asserts BOTH directions: A) every dict key is referenced somewhere (orphan detection) B) every referenced key exists in the dict (typo detection — silent fallback otherwise) Removes 12 dead keys found by the guard on master (zh+en = 24 lines): result.title, settings.groupTasks, settings.githubTokenEmpty, tasks.title, tasks.desc, panel.openFailed, panel.copied, app.helpNoSessions, app.historyLoading, app.globalAll, app.noTasks, sidebar.noOpenSessions. GUI 242→243 (i18n 15→16), Agent.md synced. Co-authored-by: EMRG Evolution <emrg@argszero.dev>
1 participant
GUI chat visual optimization (rant 2026-08-13T21:28:49, Plan A approved by host).
5 items:
.msg.userauto-margin overrodealign-self:flex-end(flex container auto margins win) → user bubbles were centered instead of right-aligned. Fix:margin-left:auto; margin-right:0(specificity 0,2,0)..session-view { scroll-behavior: smooth }made JSscrollTop = scrollHeightanimate too; rapid consecutive tool rows outran the animation → newest content hidden. Fix: removed smooth (JS jumps are instant; manual scrolling unaffected).elapsedwas only inrow.title(hover). Fix:.tool-timespan (· 3.2s) after the label on success only..session-viewgap 24px→12px,.tool-rowpadding 4px→2px + font 13px→12px, spinner 14px→12px.handleToolStartchecks last container child — a done.tool-row→ creates.tool-groupabsorbing it + new row; an existing group → appends; text/user-message/no-node → standalone row (strict consecutive semantics). Bar shows⌄ N 个工具执行 · Ts(zh) /{count} tool calls · {time}(en) when ≥2 rows all done + collapsed; click toggles + marksuser-expanded(never auto-collapses again); a running row keeps the group expanded; text interleaving keeps rows independent. Tool-row class structure unchanged → existing tests unaffected.New standalone test file
test/tool-group.test.js(+2 tests: merge+summary+collapse+user-expanded, text-interleave-no-merge). GUI tests 239→241 (Agent.md synced).Verified: pytest 801/801, GUI 241/241, node --check, import + CLI OK.