Skip to content

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

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

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

Conversation

@HuYellow

@HuYellow HuYellow commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Long default transcript system notes overflow both ends of the reading column because Astryx's content span cannot shrink or wrap. Allow the span to shrink and wrap, including unbroken text, while preserving divider styling and centered presentation.

Add a production AppShell Storybook regression built from stored system-note records, exercising localization and materialization in zh-CN and English at 1280×900 and 720×900. Each rendered fragment must remain inside the note bounds. The story waits for virtualized notes to mount; a runner unit test verifies the four locale/viewport jobs and their locale URLs.

Fixes #5138.

Overlapping PR and provenance

This overlaps #5390; its CSS declarations are equivalent. In response to the P3 review, the production story and smoke matrix are adapted from @testikun's #5390 commit 457409f0b30e74f599cca7b780b909db0526ce13, with attribution in the follow-up commit. This branch additionally waits for the virtualized notes to mount and tests the runner matrix. Maintainers should select only one overlapping implementation.

Verification

On the updated branch with Node 24.19.0 and a clean dependency install (install scripts skipped, repository dependency patches and model metadata preparation explicitly applied):

  • Workspace dependency build, Desktop story typecheck, UI typecheck and production Storybook build passed.
  • UI suite: 491/491; smoke-runner suite: 12/12 passed.
  • All four focused production Storybook jobs passed, including play assertions and AX checks.
  • Removing only this selector's rule from the served production CSS makes all four jobs fail their geometry assertions. The source and built artifact are not modified by this negative control.
  • Full repository lint and format checks, ASF header audit, changed-file checks and git diff --check passed.
  • The latest completed hosted run (35194935373) passed typecheck and the workspace tests, then failed the unchanged Electron WorkHub test at workhub-layout.spec.ts:99: the native menu close did not produce aria-expanded="false" (35 E2E cases passed, 1 failed). Storybook build/smoke were skipped after that failure. This differs from the earlier Storybook failures; a green hosted run is still required.
  • The earlier component harness also passed 21 layout cases, reproduced 12 baseline failures and retained 9 short/divider/icon layouts.
  • Packaged native Electron, the complete repository test suite, all-workspace typecheck and full-catalog Storybook smoke were not run locally.

Production AppShell before/after at the same zh-CN 720×900 viewport (screenshots and measurements live on a separate fork evidence branch, not in this PR diff):

Without the fix With the fix
Before After

Four-case positive/negative results

Review follow-up

Moved the CSS rule alongside the transcript styles and added the #5138 reference. jobLabel() now guards locale and viewport independently, with regression tests for either field alone and both together.

Kept the current condition-based virtualizer mount wait rather than increasing its timeout without a reproduced mount-timeout failure. The focused four-job production run passes, and the Storybook AGENTS.md advises against compensating for failures with longer timeouts. Optional additional note-kind coverage remains out of scope; all note kinds share the scoped default-content rule.

AI use

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

Tool(s) and scope: OpenAI Codex authored the CSS fix, adapted the attributed regression coverage, ran validation and prepared the PR. Commits include Generated-by: OpenAI Codex.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally — targeted scope and unrun checks are detailed above.

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Allow default diagnostic content to shrink and wrap within the reading measure while preserving divider styles.

Fixes apache#5138

Generated-by: OpenAI Codex
@github-actions github-actions Bot added the effort/XS Under 10 readable lines label Sep 17, 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 bfb932a8247b7767d130eb1e997e1e50e998c4b5. The CSS fix is correctly scoped to the direct content span of default transcript system notes; divider notes retain Astryx’s separate DOM path. The production render chain is materialize.tschat-turn.tsx:627-642 → Astryx ChatSystemMessage, whose default span is centered, non-shrinking, and nowrap; these five declarations allow that span to shrink and break long CJK, English, and unbroken text inside the reading measure.

I found one P3 test-coverage issue inline. The equivalent production Storybook regression from the overlapping PR passes for zh-CN and English at 1280 px and 720 px with these declarations, and all four cases fail when the old non-shrinking nowrap behavior is restored. Whichever implementation is retained should carry that regression.

Validation passed on this head: clean install, build:test, full workspace typecheck, UI 491/491, Storybook production build, lint, format, ASF headers, changed-file Biome, and git diff --check. The merge tree with current main f12d30535c44d67f93459cd954627567dfdb07f6 is clean (c91edc59d6020ec870af3cbfada505a34768353d). Hosted test is currently red in two unchanged Storybook cases; focused reruns passed the WorkHub case 10/10, while the history case reproduced intermittently (8/10), and neither scene exercises this system-note selector. The gate still needs a green rerun before merge.

I did not run a packaged native macOS/Windows Electron build. Maintainers also need to select only one of the two overlapping PRs.

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.

Comment thread packages/ui/src/styles.css Outdated

/* Default transcript notes can contain full diagnostics. Let Astryx's content
span shrink and wrap inside the reading measure; divider labels stay compact. */
.maka-chat-system-message[data-variant='default'] > span {

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.

[P3] Please keep a production-path regression with whichever implementation is merged. This branch has no committed assertion for the behavior, so a later Astryx or cascade change could restore the centered flex-shrink: 0 / nowrap clipping without a targeted failure here. The overlapping PR already provides a reusable AppShell Storybook case built from stored system_note records and runs it for zh-CN and English at 1280 px and 720 px; I verified those four cases pass with this rule and all four fail when the old span behavior is restored.

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: fix(ui) — wrap long default transcript system notes

Comment only. I re-derived the fix from the DOM path rather than trusting the screenshots.

What I verified

The rule hits exactly the offending node, and nothing else.

  • Render chain: packages/ui/src/chat-turn.tsx:627-642 → Astryx ChatSystemMessage. In the default variant it emits a root div (display:flex; justify-content:center; text-align:center) plus one direct child <span> carrying flexShrink: 0; white-space: nowrap — that span is the root cause (node_modules/@astryxdesign/core/src/Chat/ChatSystemMessage.tsx, styles.content).
  • themeProps('chat-system-message', { variant }) emits data-variant, and variant defaults to 'default', so .maka-chat-system-message[data-variant='default'] > span matches that span and only that span. Specificity (0,2,1) beats the StyleX class (0,1,0) regardless of where the rule sits in the file.
  • No competing rule: I grepped apps/desktop/src/renderer/styles/** and packages/ui/src/styles.css — nothing else sets white-space/flex-shrink on a system-message span. The divider variant keeps Astryx's separate Divider path, and the icon grandchild (chat-turn.tsx:827) is untouched — that notice is default-variant, so it is fixed too, incidentally.
  • Containing block: .maka-transcript-turn (apps/desktop/src/renderer/styles/chat-message.css:61-67) is display:flex; width:100%; contain: layout style paint, so max-width: 100% resolves to the reading measure and the old overflow was paint-clipped by that row — consistent with #5138's "clipped at both edges".

Wrapping, not truncating, and it survives unbroken tokens. min-width: 0 + flex-shrink: 1 + max-width: 100% + white-space: normal is the right shape: no text is dropped. overflow-wrap: anywhere (rather than break-word) is also the right knob here, because it lets a single unbroken token/URL reduce the min-content contribution, which break-word alone does not. Long numeric token strings like 129127 tokens are covered by the same mechanism.

Kind-agnostic. All runtime note kinds funnel through systemNoteLabel() (packages/ui/src/materialize.ts:147-189), so context_window_suggestion, context_reported_window_exceeded and context_overflow_after_compaction are covered without further rules.

Accessibility. Wrapping is presentational only: the note stays one text node, and the root's role="status" + aria-label (chat-turn.tsx:633) are unchanged. No AX-tree or copy/paste change.

Conventions. Product composition CSS in packages/ui/src/styles.css matches docs/frontend-css-governance.md; no !important. The astryx:surface-inventory gate is unaffected — its CSS analyzer only flags off-rhythm heights (scripts/generate-astryx-surface-inventory.mjs:575-610).

Tests are wired to real machinery.

  • Story id product-shell-official-appshell--long-system-notes follows title: 'Product/Shell Official AppShell' + export name; waitFor is already imported from storybook/test (app-shell.stories.tsx:21).
  • locale is a registered global (apps/desktop/.storybook/preview.tsx:88-123, default zh-CN), so the ;locale: suffix in storyUrl() genuinely re-localizes instead of being silently dropped.
  • The four stored records are valid SystemNoteMessage shapes and all four kinds pass isUserVisibleSessionSystemNote (packages/core/src/session.ts:801), so 3 default + 1 divider is what the assertions expect; context_provider_dropping/context_window_overrun copy exists in zh-CN and en (packages/ui/src/conversation-copy.ts:566,571,908,913).
  • RENDER_VIEWPORT/NARROW_RENDER_VIEWPORT already exist (scripts/storybook-visual-smoke.mjs:27-28); the special case is keyed on a string literal, but the new unit test asserts exactly 4 jobs and their locale URLs, so a rename fails loudly.

Overlap with #5390

My own read: these are the same change, and only one is needed. The five CSS declarations are identical; the story is the same four stored records with the same assertions; the catalogJobs special case, ;locale: globals, job.viewport and jobLabel changes are equivalent line for line.

The one substantive difference is this branch's waitFor on .maka-chat-system-message before measuring geometry. That is not cosmetic: #5366 virtualized the transcript, and rows mount only after the virtualizer measures its scroller — the repo says so at app-shell.stories.tsx:2394 and uses an explicit 10s wait for it in tailSettled. #5390's play asserts notes.length === 4 immediately after document.fonts.ready, which is a cold-mount race on the very path #5366 introduced.

So: keep either, but if #5390 is the one merged, port the waitFor; if this branch is merged, keep it. Whichever loses should be closed. Attribution for the derived story is recorded in commit 6ccec74cc, which covers the provenance concern.

Nits

  • nit: the new waitFor uses the testing-library default timeout (~1s), while the repo's virtualizer waits set an explicit { timeout: 10_000 } (app-shell.stories.tsx:2403). Under a production Storybook build on CI that's the remaining flake surface this wait exists to close — worth passing the timeout explicitly.
  • nit: the rule lands at packages/ui/src/styles.css:43, in the section-header area, while the transcript rules it belongs with sit at 208-254 (.maka-turn-elapsed, .maka-compaction-status) — which is exactly where #5390 put the same rule. Purely cosmetic (specificity decides, order doesn't), but the neighbouring placement reads better.
  • nit: the CSS comment doesn't cite #5138, though neighbouring rules do (#1879 at styles.css:255). One token, and it is what makes the rule greppable later.
  • nit: jobLabel() derives ${job.viewport.width}px from job.locale being set — two independent fields. Guard on job.viewport so a future locale-only job doesn't throw inside label construction.
  • nit (optional coverage): the story covers the two reported kinds plus one short default and one divider note. The longest kinds (context_overflow_after_compaction, context_window_suggestion) and the icon-bearing abort notice (chat-turn.tsx:827) share the same CSS path but aren't asserted; long divider notes are deliberately out of scope since that path is unchanged.

One merge-readiness note (author-disclosed, not a code issue): hosted test is currently red on two unchanged Storybook cases, so the gate needs a green rerun before this lands.

Guard locale and viewport separately in job labels, cover their independent combinations, and group the issue-tagged system note rule with transcript styles.

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

Labels

effort/XS Under 10 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