Skip to content

fix(ui): wrap long default system notes - #5390

Open
testikun wants to merge 3 commits into
apache:mainfrom
testikun:codex/fix-5138-system-note-wrapping
Open

testikun wants to merge 3 commits into
apache:mainfrom
testikun:codex/fix-5138-system-note-wrapping

Conversation

@testikun

@testikun testikun commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

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

  • Desktop story typecheck and production Storybook build passed after rebasing onto main (00c249831).
  • Focused built-Storybook smoke passed all four locale/viewport combinations, including play assertions and accessibility checks.
  • Restoring the upstream no-wrap styles caused the same story assertions to fail in all four combinations.
  • Fixed notes have zero measured left/right overflow. The short default note retains its height at the standard width; compaction dividers retain their height at both widths.
  • Smoke-runner unit tests passed (11 tests), including the locale/viewport matrix and locale URLs; changed-file lint, formatting/pre-commit checks and git diff --check passed.
  • Follow-up head 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.
  • Hosted CI passed at 8f7c194ef: https://github.com/apache/maka/actions/runs/35207986859. This is an empty commit over f05f5436e, 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.
  • Native Electron and the full repository test suite were not run. Screenshots use Chromium and the existing Desktop Storybook shell, real transcript components and deterministic stored diagnostic records.

Before (left) / after (right), zh-CN at 720×900:

Long system notes before and after at narrow width

Screenshots are stored separately in the fork's evidence branch; no images or capture scripts are included in this PR's diff.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Generated-by: OpenAI Codex
@testikun

Copy link
Copy Markdown
Contributor Author

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 standard-width comparison

Chinese — narrow window, 720×900

Chinese narrow-width comparison

English — narrow window, 720×900

English narrow-width comparison

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.

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 16, 2026

@hqhq1025 hqhq1025 left a comment

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.

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.

@testikun

testikun commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

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.

HuYellow added a commit to HuYellow/maka that referenced this pull request Sep 17, 2026
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 Astro-Han left a comment

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.

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-641 passes bare note.text for 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, class x3nfvp2 … xuxw1ft), and data-variant really is emitted by Astryx's themeProps() (node_modules/@astryxdesign/core/src/utils/themeProps.ts:100). So packages/ui/src/styles.css:257-263 is 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'] > span is (0,2,1), so the override wins regardless of StyleX's stylesheet injection order — no !important needed.
  • overflow-wrap: anywhere is 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 at styles.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 default is 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, where scrollWidth under-reports; the scrollWidth check is a reasonable complement. Awaiting document.fonts.ready before measuring is correct.
  • The smoke matrix genuinely exercises what it claims. storyViewport() would return 1280 for this id (no narrow in it), and the new job.viewport ?? storyViewport(...) at scripts/storybook-visual-smoke.mjs:240 makes the 720 job real. The story id matches the meta title (Product/Shell Official AppShell). smokeStory runs auditAxTree unconditionally (:286), so all four new jobs are AX-audited, and wrapping does not change the role="status"/aria-label contract from chat-turn.tsx:633.
  • Fixtures are monotonic in ts and 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

  1. No unit test for the new runner branch (scripts/storybook-visual-smoke.mjs:160-173). The story id is hardcoded inline, and scripts/storybook-visual-smoke.test.mjs already has a test for every other special case in this module (palette matrix, dark sentinel, forced colors, storyUrl globals). 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.
  2. The play function doesn't wait for the transcript to mount. app-shell.stories.tsx:1514-1516 asserts notes.length === 4 right after document.fonts.ready, but the transcript is virtualized (packages/ui/src/chat-view.tsx:806-814, virtua). Every other DOM assertion in this file wraps in waitFor (e.g. :965). It presumably passes today, but it's one scheduling change away from a flake; #5438 adds the waitFor (the import already exists at :21).
  3. Blast radius wider than the comment claims. context_compaction_failed_open sets compactionState: "failed" (packages/ui/src/materialize.ts:232), which takes the default variant, so > span also restyles its .maka-compaction-status wrapper. Harmless in practice — the declaration sets don't collide with that rule's display/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.
  4. 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_limit note at 1280 would pin it.
  5. Cosmetic: with the root's text-align: center (Astryx styles.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 prefers text-align: start on the wrapped span for long diagnostics.

Wait for the virtualized transcript and pin the locale/viewport smoke matrix.

Generated-by: OpenAI Codex
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(ui): long default system notes are clipped at both edges

3 participants