Repository navigation
fix(ui-web): a picture in a delegation card fits the card instead of widening it - #539
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the full target-to-head diff, the generated-shot and delegation/prose callers, selector specificity, relevant stylesheet history, backward compatibility, and the Web UI architecture/CSS constraints. The flex item can now shrink to the card width, the image retains its aspect ratio, and the existing more-specific artifact-icon sizing remains intact. I also checked the repository rules and confirmed that this change does not weaken tests.
Verification:
npm test: 181 files passed, 2518 tests passed (the run emitted the existing happy-dom aborted-fetch noise)npm run type-check: passednpm run lint: passed with 0 errors and 5 warnings in untouched filesnpm run build: passedpython3 build.py: passed; both boot snapshots matchednode scripts/check-page.mjs: passedgit diff --check: passed
The initial npm test attempt could not start because dependencies were absent; after npm ci, the unmodified suite passed as reported above. The environment uses unsupported Node 23, so npm warned that the locked Vitest/ESLint tooling expects Node 20, 22, or 24+, but the checks completed successfully.
|
The frontend rebuild landed on main in #612, as 83 individual commits rather than a squash, so This PR is not lost and was deliberately left open rather than closed -- deleting the branch would have closed it with no way to reopen it. To land the change:
Retargeting before the rebase will show the whole gap between the two branches rather than your change, so do them in that order. Shout if the rebase turns out to be more than it looks and we will sort it out. |
|
Blocking: the branch must be rebased onto main and the PR retargeted to main. Confirmed the PR is still at I did not rewrite or push the author's branch: repository rules require explicit authorization for the history rewrite and push. After the author rebases, force-pushes with lease, and retargets the PR, the new head will need its own review verdict. Verification on the unchanged head: |
…widening it Two ways a picture widened a card past its column, both measured in the served page against the real stylesheet: A generated image (`.gshots`, what an image_generate or ppt_generate_image call opens) was capped by height only: a 1600x400 banner scaled to 260px tall comes out 1040px wide in a 562px column and hands the tool card a horizontal scrollbar. The column now caps the width too, with `min-width: 0` on the flex item so the cap can apply. An image or inline svg inside prose (`.prose img`, `.prose svg`) had no cap at all and was laid out at its natural width; in a spawn card that box is the run's own scroll pane, which then scrolled sideways under the report. Both now fit the column, height following so an attribute-sized image keeps its aspect ratio. The artifact chip's icon keeps its own 15px rule, which is the more specific of the two. Nothing else in prose does this: a table scrolls inside `.tw`, a code block inside `pre`, long words break under `overflow-wrap`. Verified against this head in a real gateway's page, 900px window, on the markup features/transcript/TranscriptPage.tsx builds for a media tool call: the card body went from scrollWidth 1611 in a 562px box (a 1040x260 picture) to 562 in 562 (540x135), and a 1600px-wide prose image from 1600 to 540. No horizontal scrollbar either way. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com> Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
2705a60 to
e022cef
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The rebase and retarget are complete. I reviewed the full github/main...HEAD diff and used git range-diff to confirm that the substantive CSS patch was preserved. I also checked the current generated-media and delegated-report callers, including the newer DAG terminal-output path, selector specificity, stylesheet history, backward compatibility, test integrity, and the applicable AGENTS.md, CONTEXT-MAP.md, Web UI context, and CSS architecture rules. The image wrapper can shrink within its flex row, image aspect ratios are retained, and existing prose SVG consumers keep their intended sizing. No tests were removed or weakened; the revision changes only ui-web/src/styles/page.css.
Verification on the rebased head:
npm ci: passednpm test: 188 files passed, 2464 tests passednpm run type-check: passednpm run lint: passed with 0 errors and 4 warnings in untouched filesnpm run build: passedpython3 build.py: passed; both boot snapshots matchednode scripts/check-page.mjs: passedgit diff --check github/main...HEAD: passed- source-language gate via the Makefile target's exact
uvcommand: passed (makeis unavailable in this environment)
GitHub CI was still pending when this review was submitted.
|
Not a blocker -- the fix works, and I measured it. Two of the three lines do not do what their comments say, which is worth knowing before the next person reads them as precedent. Everything below is It does fix the reported bugA 1600x400 picture in a delegation card, card 520px wide: 1040 is exactly the number your comment predicts (1600x400 capped at 260 tall). And both claims your comments make about what must survive hold: I checked the cascade rather than trusting it: 1.
|
Summary
Two ways a picture widened a delegation or tool card past its column, both measured in the served page against the real stylesheet.
A generated image (
.gshots, what an image_generate or ppt_generate_image call opens) was capped by height only. A 1600x400 banner scaled to 260px tall comes out 1040px wide in a 562px column and hands the tool card a horizontal scrollbar. The column now caps the width too, withmin-width: 0on the flex item so the cap can apply.An image or inline svg inside prose (
.prose img,.prose svg) had no cap at all and was laid out at its natural width. In a spawn card that box is the run's own scroll pane, which then scrolled sideways under the report. Both now fit the column, height following so an attribute-sized image keeps its aspect ratio. The artifact chip's icon keeps its own 15px rule, which is the more specific of the two.Nothing else in prose does this: a table scrolls inside
.tw, a code block insidepre, long words break underoverflow-wrap. One file, two rules, no markup change.Rebased onto main and retargeted there. This was opened against
refactor/ui_web_architecture, which is retired and read-only now that the frontend rebuild landed on main; the single commit was replayed onto main, where it applied cleanly. Both problems were re-confirmed on main before the move: the rules this adds are absent from main's stylesheet, andfeatures/transcript/TranscriptPage.tsxstill builds.gshotsfor the two media tools whilelib/prose.tsstill renders.prose.Type
Verification
npm run lint --prefix ui-web: 0 errors (the 4 warnings are present on main unchanged)npm run type-check --prefix ui-web: passesnpx vitest runin ui-web: 188 files, 2464 tests passed, including the CSS gates (css-one-owner, css-balance, check-css) and check-class-namespacenpm run build --prefix ui-web,ui-web/build.pyandscripts/check-page.mjs: build passes and both boot snapshots match their goldens unchanged, since no markup movesServed this head's build from a real gateway in a 900px window and measured the markup
features/transcript/TranscriptPage.tsxbuilds for a media tool call. Before: the card body had scrollWidth 1611 in a 562px box, holding a 1040x260 picture, and a 1600px-wide prose image in a 540px column. After: 562 in 562, the picture at 540x135 and the prose image at 540. No horizontal scrollbar either way, and toggling the two rules off in the live stylesheet brings the overflow back.Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
CSS only. A picture that already fit its column is unchanged; only one wider than the column is scaled down. Reverting the commit restores the previous rules.
Related Issues
N/A