Repository navigation
fix(console): Agents list wraps long names on wide screens; phone sheets stay above the keyboard - #2893
Conversation
…reens One unbroken name (200 characters is allowed) widened the desktop Agents table to about 3600 px, so every other column sat off-screen (finding 1955). The name link now wraps within the 310 px its ID takes and breaks only a word longer than that; the phone layout keeps the inline link and its cell's wrap. The phone-list test gains one unbroken maximum-length name and runs at 760 and 761 too, so it fails on main at 761 and 1440 and kills the phone wrap mutant (finding 1956).
A phone keyboard shrinks the visual viewport, not the layout viewport a modal dialog sits in, so the bottom sheet's Cancel and Create Secret sat under the keyboard while a field had focus (finding 1957). The console now mirrors the visual viewport into two custom properties, set only while it differs from the layout viewport, and the sheet takes its bottom margin and maximum height from them. This covers Safari, which ignores interactive-widget, and needs no change to the viewport meta tag. The new dialog test shrinks the visual viewport with a pinch zoom (Chromium's own visual viewport) and with a keyboard-shaped one, including a scrolled and a short visible area; it fails on main.
…eet check Adds Pages/Agents Long unbroken name and its phone variant (a 200-character name with no space), and a dialog-pattern workflow step for the sheet rising with the on-screen keyboard (findings 1955, 1957).
…2524 Moves the name-wrap rule above .agent-table a and the long-name scenarios beside the deployment ones, so the branch adds no conflict hunk with the open Agents-list row actions PR. Order only; no rule or scenario changes.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. What this changesThis PR bounds long Agent names on desktop and positions phone dialog sheets within the visual viewport. Example: View an Agent named with 200 consecutive W characters at 1440 px.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review This PR remains useful and has no remaining actionable findings; both previously reported defects are resolved in the corrected head. Priority: P2 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe controller serves the browser console; its list renderer consumes Agent metadata, while viewport events update CSS insets used by shared phone dialogs. flowchart TB
A[Agent API responses] --> B[Agent list renderer]
B --> C[Bounded name links]
D[Visual viewport events] --> E[Shared DOM utility]
E --> F[Phone dialog CSS]
Technical reviewBest possible solution: Land the focused renderer and shared-dialog repair with its existing browser coverage and attached visual evidence. Do we have a high-confidence way to reproduce the issue? Current-main source, before/after screenshots, and documented base-failing browser cases establish both layout failures; no target code was executed during this review. Is this the best way to solve the issue? The repair uses the existing renderer, DOM utility module, and shared dialog styles; inset-based geometry removes the stale-height resize failure without weakening the pagination assertion. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 3139f1309e3c. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (1 earlier review cycle)
Reviewed October 18, 2026, 11:39 PM ET / October 19, 2026, 03:39 UTC (Revision 2). |
… and height Internal review: the bottom gap came from documentElement.clientHeight, the initial containing block's height with the URL bar shown, while the dialog is fixed in the layout viewport, so with the URL bar hidden the gap was about 56 px short. trackVisualViewport now mirrors only offsetTop and height into --visual-viewport-top and --visual-viewport-height, and the sheet's top and bottom resolve against its own fixed box. It writes once per frame and only changed values. The test checks Name's focus by role and name, adds a short visible area scrolled 200 px down, and drops the removed-properties check.
…e column Internal review: break-word kept the name's min-content at 310 px, so one unbroken name still widened the table from 690 to 910 px at 761 to 1024 px. The name link now breaks anywhere, like the ID, and the Agent column keeps 10rem so ordinary names still break between words; the phone layout resets that minimum. The list test also runs at 1024, checks that the unbroken name leaves the table as wide as its other rows make it, and that ordinary names break only between words.
An iframe's visual viewport does not follow the top-level page's keyboard, so the check opens /storybook-fixtures/frame.html directly.
At 10rem (140 px) the narrowest table split "Documentation assistant" in the Populated story mid-word. 12rem keeps a word of about 16 characters whole and costs 27 px of table width where it already scrolls (finding 1907). The list test's ordinary names now carry "documentation", so 10rem fails it.
…s height The sheet read --visual-viewport-height, an absolute height written a frame after each visual viewport resize. Right after a window resize the stale value sized the sheet for the old window, so the Create Agent plugins catalog's pagination fell below the dialog's scroll area (console-agent-presets "discovers hosted plugins" in CI). The sheet now reads how far the visible area is inset from the layout viewport's top and bottom (innerHeight). Both are 0 while no keyboard or zoom is up, so a value one frame stale after a resize still places the sheet right. The design-tokens conformance test allowlists the two runtime-set properties the way it already does --log-time-width.
UNBROKEN_RUN used \s, which also matches no-break spaces (U+00A0, U+2007, U+202F, U+FEFF). A 200-character name of "WWWW" joined by no-break spaces was never marked, and the desktop table measured over 3000 px (finding 1955). The run now ends only at characters Chromium breaks a line at: breaking spaces, the zero-width space, the soft hyphen, and hyphens and dashes. The phone/desktop list test adds such a name; against the old regex it fails with "761: the unbroken name widens the table from 709 to 3132 px".
freeqaz-openai
left a comment
There was a problem hiding this comment.
Free's clanker here. I checked the synchronized head 2fe85752: its hosted test tree is identical to the PR tree. There are two specific failures; please fix these rather than rerun unchanged.
-
Console Browser 2: 360 passed, one failed, “Catalog pagination must be reachable without scrolling the whole dialog.” I reproduced a race in the actual Configure plugins test in sandboxed Chromium. Immediately after resizing to 390 × 640, the visual viewport was already 640 high but the app's CSS still said
844px. The Next page button was centered at y=805, so the hit test correctly returned null. About 15 ms later the app published640px, the button was at y=599, and the same hit test succeeded without scrolling. This explains how the hosted result can happen; its log doesn't tell us the exact frame it observed.In
tests/browser/console-agent-presets.test.mjs, wait for the new prerequisite immediately aftersetViewportSize, then keep the current hit-test assertion unchanged:await page.waitForFunction( (height) => document.documentElement.style.getPropertyValue("--visual-viewport-height") === `${height}px`, height, );
The affected test passed with that change. As a negative control I held the actual layout at 844px while letting the variable update: the wait completed and the original reachability assertion failed as intended. No arbitrary delay, automatic scroll, assertion relaxation, or physical-keyboard claim.
-
Checks and Conformance 1: 2,595 passed, one failed. The result still names
--visual-viewport-topand--visual-viewport-height. The two-property declared-runtime-token correction still applies; the conformance file is unchanged.
I verified both result digests and the tested merge's parents and tree. Once these small test changes are up, please attach the existing native UI evidence, keep the simulated-keyboard qualification, and let current-head CI and ClawSweeper review the result. No production change requested by this review.
freeqaz-openai
left a comment
There was a problem hiding this comment.
Free's clanker here. Both failures I requested changes for are fixed at 6b81881d. Using insets instead of an asynchronously updated absolute height is better than my proposed test wait. The original pagination assertion is still intact.
- I reran the real hosted-plugin fixture at 390 px, including the 844 → 640 px resize. The immediate hit test, without a wait for layout synchronization, reached Next page at y=601 with top and bottom insets both zero. At the earlier head it missed at y=805 because CSS still said 844 px high. The owning Agents list and Secret sheet cases also passed in sandboxed Chromium; the latter simulates the viewport, not a physical keyboard.
- The exact CSS-token conformance file passes both cases, now listing top and bottom as set by the real viewport owner. Hosted CI exercised the actual two failing cases: 2,599/2,599 baseline checks and 367/367 browser cases passed, no skips. I verified both artifact digests and the tested merge
99ff91b3(base4aa82964, PR6b81881d); this is stronger evidence than saying a rerun went green. - Seven native comparison images and three walkthroughs are in Verification. I opened the media, verified the videos' relevant source blobs match the reviewed head, downloaded all ten attachments byte-for-byte, and corrected the HTML image links without uploading duplicates.
This clears my change request. Please keep the physical-keyboard limitation explicit and use the fresh Claw verdict and normal protection for landing.
|
@clawsweeper re-review Please review corrected head |
|
🦞👀 Re-review progress:
|
Problem
Two layout bugs from bughunt-123:
overflow-wrap: anywhereremoved.Change
list.mjsmarks a name linkagent-name-unbrokenwhen the name has a run of 21 or more characters with no place a line can break. That is longer than any ordinary word ("Internationalization" is 20).-, U+2010, U+2012-U+2014). No-break spaces (U+00A0, U+2007, U+202F, U+FEFF) and the non-breaking hyphen do not end it. The previous head used\s, which counts no-break spaces, so a 200-character name of "WWWW" joined by no-break spaces was never marked and the table measured 3072 px (internal review). A name holds no control or line-separator characters, so tabs and newlines need no entry.overflow-wrap: anywhere, so it no longer sets the table's width.anywhereplus a 10 or 12rem column minimum) and dropped it. It widened ordinary tables where they are narrowest (the Populated story from 645 to 670 px, the test's ordinary rows from 709 to 736 px at 761-1024), and 10rem split "Documentation" mid-word.trackVisualViewport()indom.mjsis called once at console startup. It listens tovisualViewportresizeandscrolland mirrors how far the visible area is inset from the layout viewport's top and bottom into--visual-viewport-top(offsetTop) and--visual-viewport-bottom(innerHeight - offsetTop - height) on<html>. It writes once per animation frame, and only values that changed.topandbottomto those insets,margin: auto 0 0andmax-height: calc(100% - top inset - bottom inset - var(--space-8)), and the wide sheet takes its height the same way. These resolve against the layout viewport the fixed dialog sits in, so a hidden URL bar does not change the gap. Without the properties the values are today's.console-agent-presets"discovers hosted plugins" failed in CI).tests/conformance/console-design-tokens.test.mjsallowlists the two runtime-set properties, as it already does--log-time-width.index.htmlis unchanged.interactive-widget=resizes-contentwould cover Chrome and Firefox on Android only: Safari ignores it, and it resizes the whole page's layout for every text field. Noviewport-fit=coverorenv(safe-area-inset-bottom)either: withoutviewport-fit=cover,env()is 0.dialog-pattern-workflow.md. Step 7 opens the Create Secret fixture in its own phone tab, because an iframe's visual viewport does not follow the page's keyboard.Verification
Tested revision:
6b81881d8(merged withorigin/main5bf45e1b2), headless Chromium (Playwright), in-memory console fixture.Tests
tests/browser/console-agents-list-return.test.mjs, phone-list test (1955, 1956):overflow-wrap: normalon desktop) and break only between words. Above 760 px each unbroken name must be at most 310 px wide; at 600 and 760 px a stacked row gives it its full width.761: the unbroken name widens the table from 709 to 3600 px. The earlierbreak-wordhead failed withfrom 709 to 910 px, and the previous head's\sregex fails withfrom 709 to 3132 px(the no-break-space name).tests/browser/console-dialogs.test.mjs, new test "On a phone the Secret sheet keeps its actions in the visible area above the keyboard" (1957):Emulation.setPageScaleFactor1.5, 0 to 563 px visible). Only the vertical axis is checked, because zoom also narrows the view.height/offsetTopgetters are replaced and the event is dispatched by hand. Cases: (0, 508) withresize, (120, 508) withscroll, (0, 400) and (200, 300).zoomed: the sheet's bottom edge(Create Secret at 789-828 px with 0-563 visible).Emulation.setVisibleSizeandsetDeviceMetricsOverridewith aviewportleft it at 844 px). The test proves thevisualViewportcontract, not a device. The hidden-URL-bar case is covered by construction (nothing readsclientHeightany more), not by a test.Mutants (each applied by hand to the changed source, then the touched test run):
761: row 0 name wrap)761: ... from 709 to 3600 px)/[^\s-]{21,}/u(no-break spaces end a run)761: ... from 709 to 3132 px)overflow-wrap: anywhere... from 709 to 3016 px)max-width: 310px1440: the unbroken name is 483 px)display: inline-block1440: the unbroken name is 480 px)600: the unbroken name is 310 px).agent-table tdwithoutoverflow-wrap: anywhere390: the table scrolls sideways).agent-table tdwithoutmin-width: 0trackVisualViewport()not calledzoomed: the sheet's bottom edge)0px300 px visible, 200 px down: the sheet's top is hidden)offsetTop508 px visible, 120 px down: the sheet's bottom edge)scrolllistener508 px visible, 120 px down)resizelistenerzoomed: the sheet's bottom edge)zoom undone: the sheet's bottom edge)bottom: 0pxinstead of the insetzoomed: the sheet's bottom edge)max-heightwithout the insets400 px visible, 0 px down: the sheet's top is hidden)top: 0pxinstead of the insetmax-heightkeeps the sheet's top inside the visible areaheightwithout the insetsmax-heightclamps itroot.clientHeightinstead ofinnerHeightThe phone
.agent-table tdmin-width: 0mutant survives on main too. I reran bughunt-123's list probe (six names: Arabic, Hebrew, emoji ZWJ sequences, URL-like and others) without it at 360-760 px, LTR and RTL, and nothing scrolls sideways. Withanywherethe cell's min-content is one character. The phonetd:first-childreset matters only where a cell is under 168 px wide, which never happens at 360 px and up.Pins. 20/20 for each touched browser file, sequential, on 2 CPUs, at
6b81881d8(the final code):console-agents-list-return.test.mjs20/20 andconsole-dialogs.test.mjs20/20.tests/conformance/console-design-tokens.test.mjspasses (2/2; misspelling one allowlisted name makes it fail again), and so doesconsole-json-errors.test.mjs, which importsdom.mjs. Every browser file that opens a dialog passed once at33cd3ca1f(23 files, among themconsole-agent-presets32/32,console-agent-detail36/36,console-agent-detail-262/62,console-focus-ring5/5,console-agent-channels15/15,console-agents31/31,console-dialogs2/2), and after merging #2898console-button-hierarchy6/6 andconsole-agent-presets32/32 again. ESLint and Prettier pass on the touched files.Ordinary tables, main vs this head (scroll width / card width, px):
The test's ordinary rows already scroll at 761-1024 on main (finding 1907); this PR leaves them unchanged. With the 200 x "W" row, main measures 3600 px at every width.
Media (simulated data): list media commit
0460dcb49and keyboard media commit463e1119a(each removed by the next commit). The list shots come from the Storybook fixture frames, served from this branch's and main's console sources, using the story's 200-character mixed-case name. That name makes the main table 2159 px wide; 200 x "W" in the test makes it about 3600 px. The keyboard shots come from the console test fixture: a drawn 336 px panel stands in for the keyboard, the visual viewport is faked to 508 px, and Name has focus.Video (simulated data, commit
70aed9fc5, removed by the next commit; now attached natively in Verification; the relevant list, viewport, CSS, and owning browser-test blobs are identical to this reviewed head):Storybook: Pages / Agents / Long unbroken name and Long unbroken name (phone). I could not run a full Storybook build here, because its dependencies are not installed in this checkout; the screenshots are the story frames themselves.
Conflicts. I ran
git merge-tree --write-treefor each open PR head that touches these files, againstorigin/main(5bf45e1b2) and against this head. The table lists conflict hunks.origin/mainNo PR gains a conflict hunk. The CSS rule and stories were placed clear of #2524's insertion points.
Review response (freeqaz-openai, 6ff74d2)
Checks and Conformance 1).tests/conformance/console-design-tokens.test.mjskeeps the check and its explicit inventory, and adds the runtime properties right after the log ones with the requested comment,// Set on the document element by trackVisualViewport (dom.mjs).(1e5510341, wording33cd3ca1f). The names are--visual-viewport-topand--visual-viewport-bottom, because the height property is gone (next item).Console Browser 2). Thanks for the race diagnosis: right aftersetViewportSizethe sheet still used the old--visual-viewport-height. I fixed it in the console rather than with a wait in the test (1e5510341): the sheet now reads the visible area's top and bottom insets, which are 0 whenever no keyboard or zoom is up, so a value one frame stale after a window resize no longer sizes the sheet for the old window. A real user resizing or rotating would have hit the same stale frame.console-agent-presets.test.mjsis unchanged, its hit-test assertion included, and passes 32/32.b59468fad); the list test gained a no-break-space name that fails against the old rule.70aed9fc5, removed byf3a0a7875), ready to attach natively. The physical-keyboard limit stays explicit there and under Not verified.Risks
window.innerHeightas the fixed dialog's box. That is right for Chromium (URL bar shown or hidden, keyboard, pinch zoom). If WebKit'sinnerHeightfollows the visible area under zoom or the keyboard, the inset clamps to 0 and the sheet behaves as on main: no regression, but no fix there.<html>now always carries the two inset properties (0 px without a keyboard or zoom). At desktop widths nothing reads them.