Conversation
Generated-by: OpenAI Codex
|
Visual reproduction and fix verification for #5138. Before is on the left; after is on the right. Each pair uses the same diagnostic records, localized copy, viewport and light theme. Before the fix, long notes remain on one centered line: their opening and closing text can fall outside the visible transcript. After the fix, they wrap within the reading measure, so the complete message can be read. At the standard 800px reading measure, the first Chinese diagnostic overflowed each side by about 157px; the English diagnostic overflowed each side by about 356px. All four fixed scenarios have zero measured left/right text overflow. Chinese — standard window, 1280×900 Chinese — narrow window, 720×900 English — narrow window, 720×900 English standard-width comparison These captures use Chromium 151 on macOS with the existing Desktop Storybook shell, production ChatView/ChatSurfaceLayout and deterministic stored system_note records. They are not native Electron captures or a live-provider run. Current main has updated the provider-dropping wording since the original issue; both sides use the same current wording. The focused smoke passes the actual story play assertions and accessibility checks in zh-CN and English at both widths. Reinstating the upstream no-wrap styles makes the same assertions fail in all four cases. Short default notes retain their standard-width height; the compaction divider retains its height at both widths. Only the CSS fix, one production-shell regression scenario and its locale/viewport smoke configuration are in the code diff. Screenshot binaries and local capture/verification helpers are kept outside it. |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 457409f0b30e74f599cca7b780b909db0526ce13. I found no P0–P3 issues.
The change is narrowly scoped: packages/ui/src/styles.css:257-263 lets only default transcript system-note content shrink and wrap, while the divider variant remains unchanged. apps/desktop/stories/app-shell.stories.tsx:1499-1534 exercises the production stored-message → materialization → ChatSystemMessage path, and scripts/storybook-visual-smoke.mjs:157-214 runs it in zh-CN and English at 1280 px and 720 px.
I independently ran all four built-Storybook scenarios. The rendered default notes resolved to white-space: normal, flex-shrink: 1, and max-width: 100%; every text fragment stayed inside its 800 px or 366 px note bounds with no scroll overflow. Reinstating the old non-shrinking nowrap behavior in the browser reproduced bilateral clipping in every matrix entry (34–565 px, depending on copy and viewport), while the divider retained its existing 20 px height.
Validation passed: clean install, build:test, full workspace typecheck, UI 498/498, Storybook runner 10/10, Desktop story typecheck, production Storybook build, the four focused smoke/play/AX runs, renderer architecture 112/112 plus the base-relative policy check, Biome, ASF headers, and git diff --check. Hosted test and label are green. The merge tree with current main 9982e86b1da3e25f714417279ac9cdf86a41481a is clean (4a29e09d585a2b4deab9d1554bac326897af46c6).
I did not independently run a packaged native macOS/Windows Electron build; the layout evidence is from Chromium using the production Desktop Storybook shell.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
Current head 457409f is mergeable and CI is green. No code blocker surfaced in this pass; this PR still needs an independent maintainer review and merge decision. Requesting review when available. Automated update notice: This comment was posted by OpenAI Codex on behalf of the PR author; it is not an independent human review or approval. |
Address P3 review coverage with the stored-message AppShell story and locale/viewport smoke matrix adapted from testikun's apache#5390, commit 457409f. Wait for virtualized notes to mount and verify the runner preserves the four locale/viewport jobs. Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Review — comment only
Thanks, this is a clean, well-scoped fix. I went through the diff and the surrounding code independently; no blockers found. Notes below, plus the requested overlap assessment against #5438.
What I verified
- The selector actually matches the rendered DOM.
packages/ui/src/chat-turn.tsx:630-641passes barenote.textfor non-compaction notes, so the only direct child of the root is Astryx's content span (node_modules/@astryxdesign/core/dist/Chat/ChatSystemMessage.js:102, classx3nfvp2 … xuxw1ft), anddata-variantreally is emitted by Astryx'sthemeProps()(node_modules/@astryxdesign/core/src/utils/themeProps.ts:100). Sopackages/ui/src/styles.css:257-263is not dead CSS. - It beats the upstream
white-space: nowrap. StyleX atomic classes are specificity(0,1,0);.maka-chat-system-message[data-variant='default'] > spanis(0,2,1), so the override wins regardless of StyleX's stylesheet injection order — no!importantneeded. overflow-wrap: anywhereis the right property here. It shrinks the min-content contribution, which is what makes a shrinkable flex item actually fit;word-break: break-word(the precedent atstyles.css:307) does not. It also matches how the repo handles unbroken copy elsewhere (apps/desktop/src/renderer/styles/settings/*.css). The diagnostic strings themselves (packages/ui/src/conversation-copy.ts:565-590,:907-926) contain no URLs or long tokens, so this is the correct belt-and-braces choice rather than a required one.- Scoping to
defaultis right. The only notes that take the divider path (running/compacted,chat-turn.tsx:631) carry short copy —contextCompacted/contextCompacting— so leaving the divider alone does not leave a long-label overflow behind. - The assertion technique is sound. Per-rect
Range.getClientRects()bounds checks (app-shell.stories.tsx:1517-1528) is the correct way to catch a centered line overflowing on both sides, wherescrollWidthunder-reports; thescrollWidthcheck is a reasonable complement. Awaitingdocument.fonts.readybefore measuring is correct. - The smoke matrix genuinely exercises what it claims.
storyViewport()would return 1280 for this id (nonarrowin it), and the newjob.viewport ?? storyViewport(...)atscripts/storybook-visual-smoke.mjs:240makes the 720 job real. The story id matches the meta title (Product/Shell Official AppShell).smokeStoryrunsauditAxTreeunconditionally (:286), so all four new jobs are AX-audited, and wrapping does not change therole="status"/aria-labelcontract fromchat-turn.tsx:633. - Fixtures are monotonic in
tsand go through the production stored-message →materializeChat→ localized-copy path rather than hand-built note text. Good.
Overlap with #5438
Yes — these are the same fix. #5438's CSS declarations are equivalent to styles.css:257-263 (only the comment differs), its LongSystemNotes story is byte-identical to app-shell.stories.tsx:1499-1534, and its scripts/storybook-visual-smoke.mjs hunks are identical to yours. Merging both would be a conflict on a duplicate export const LongSystemNotes and a redundant CSS rule.
Only one is needed. #5438 is the strictly more defensive of the two (see the first two nits), so if a maintainer has to pick, that's the one that costs less to keep green — but the CSS here is fine as-is and I'd have no objection to this PR going in if the other is closed.
Nits
- No unit test for the new runner branch (
scripts/storybook-visual-smoke.mjs:160-173). The story id is hardcoded inline, andscripts/storybook-visual-smoke.test.mjsalready has a test for every other special case in this module (palette matrix, dark sentinel, forced colors,storyUrlglobals). Renaming the story export would silently drop the locale/viewport matrix back to a single job with nothing failing. #5438 adds exactly this test; worth porting here. - The play function doesn't wait for the transcript to mount.
app-shell.stories.tsx:1514-1516assertsnotes.length === 4right afterdocument.fonts.ready, but the transcript is virtualized (packages/ui/src/chat-view.tsx:806-814, virtua). Every other DOM assertion in this file wraps inwaitFor(e.g.:965). It presumably passes today, but it's one scheduling change away from a flake; #5438 adds thewaitFor(the import already exists at:21). - Blast radius wider than the comment claims.
context_compaction_failed_opensetscompactionState: "failed"(packages/ui/src/materialize.ts:232), which takes the default variant, so> spanalso restyles its.maka-compaction-statuswrapper. Harmless in practice — the declaration sets don't collide with that rule'sdisplay/gap/font, and the copy is short in all three locales — but it's not what "the divider variant keeps its existing styling" implies, and the story doesn't cover that note. - The "short notes stay centered on one line" claim isn't asserted. The PR relies on it in Verification, but the story only checks overflow. A one-line height/rect-count assertion on the
step_limitnote at 1280 would pin it. - Cosmetic: with the root's
text-align: center(Astryxstyles.root) a 3–4 line diagnostic now centers every line. Legible, and I'm not asking you to change it — just flagging in case a maintainer preferstext-align: starton the wrapped span for long diagnostics.
Wait for the virtualized transcript and pin the locale/viewport smoke matrix. Generated-by: OpenAI Codex
Generated-by: OpenAI Codex



Summary
Long default system notes were forced onto a centered, non-shrinking line. Once wider than the Desktop reading measure, their prefixes and suffixes overflowed and could be clipped by the transcript. Allow the default content span to shrink and wrap within its available width, including unbroken text. Short notes remain centered and the divider variant keeps its existing styling.
Add one scenario to the existing Desktop shell stories using stored context diagnostics and production localization/materialization. It waits for the virtualized notes to mount; its browser assertions check every text fragment against the note bounds, including negative left overflow. The smoke runner exercises this scenario in zh-CN and English at 1280×900 and 720×900.
Fixes #5138
Verification
00c249831).git diff --checkpassed.f05f5436e: full production build, all-workspace typecheck, lint, format and Desktop/UI Knip passed. All four focused browser smoke jobs passed again. Removing the special matrix makes the new runner regression fail.8f7c194ef: https://github.com/apache/maka/actions/runs/35207986859. This is an empty commit overf05f5436e, requested by the author to trigger a fresh run after the prior run failed the unchanged mixed-transcript cold-scroll geometry check. The same intermittent failure reproduced on the Session-reference branch without this CSS change (two passing trials, then one failure). The fresh hosted run passed the original assertions; no thresholds or retries were changed.Before (left) / after (right), zh-CN at 720×900:
Screenshots are stored separately in the fork's evidence branch; no images or capture scripts are included in this PR's diff.
AI use
Tool(s) and scope: OpenAI Codex assisted with reproduction, CSS implementation, browser regression coverage, screenshots and PR text. The commit includes a Generated-by trailer.
Checklist
Does this PR entail a change in behavior?