Conversation
Allow default diagnostic content to shrink and wrap within the reading measure while preserving divider styles. Fixes apache#5138 Generated-by: OpenAI Codex
hqhq1025
left a comment
There was a problem hiding this comment.
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.ts → chat-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.
|
|
||
| /* 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 { |
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
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→ AstryxChatSystemMessage. In the default variant it emits a rootdiv(display:flex; justify-content:center; text-align:center) plus one direct child<span>carryingflexShrink: 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 })emitsdata-variant, andvariantdefaults to'default', so.maka-chat-system-message[data-variant='default'] > spanmatches 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/**andpackages/ui/src/styles.css— nothing else setswhite-space/flex-shrinkon a system-message span. The divider variant keeps Astryx's separateDividerpath, and theicongrandchild (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) isdisplay:flex; width:100%; contain: layout style paint, somax-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-notesfollowstitle: 'Product/Shell Official AppShell'+ export name;waitForis already imported fromstorybook/test(app-shell.stories.tsx:21). localeis a registered global (apps/desktop/.storybook/preview.tsx:88-123, defaultzh-CN), so the;locale:suffix instoryUrl()genuinely re-localizes instead of being silently dropped.- The four stored records are valid
SystemNoteMessageshapes and all four kinds passisUserVisibleSessionSystemNote(packages/core/src/session.ts:801), so 3 default + 1 divider is what the assertions expect;context_provider_dropping/context_window_overruncopy exists in zh-CN and en (packages/ui/src/conversation-copy.ts:566,571,908,913). RENDER_VIEWPORT/NARROW_RENDER_VIEWPORTalready 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
waitForuses 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 (#1879atstyles.css:255). One token, and it is what makes the rule greppable later. - nit:
jobLabel()derives${job.viewport.width}pxfromjob.localebeing set — two independent fields. Guard onjob.viewportso 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
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):
git diff --checkpassed.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):
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
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
Does this PR entail a change in behavior?