[qwist1233-cpu]

Summary

Opening a provider connection in Settings → 模型 renders every chat-capable model as a page row, so a fetched catalog of a few hundred models made the settings page itself hundreds of rows tall: the model filter scrolled out of view while browsing it, and the advanced request settings below sat under the whole list.

The model rows now get their own scroll viewport. The filter, the model count and the section's buttons stay outside it, so they hold still while the rows scroll, and the sections below the list are a short scroll away instead of 400 rows away.

Two deliberate choices:

• max-height: min(32rem, 60vh), not a fixed height. A small catalog, a filtered-down list and the empty-result row stand at their own height, with no blank box reserved under them, and in a short window the 60vh term keeps the surrounding section on screen.
• The wrapper is a second .settingsRows rather than a plain div: the row styling in rows.css is written as .settingsRows > …, so the rows keep their edge-to-edge track, their hover wash and the 460px container query inside the viewport too.
• overscroll-behavior is left at auto on purpose, so a wheel that reaches the end of the list carries on down the settings page instead of stopping dead.

Fixes #6164

Verification

apps/desktop on Windows 11, Node 22.

• Storybook render smoke — npm run build-storybook then the render smoke over the three provider stories below. The play functions are the test: they assert geometry in a real Chromium layout, and scripts/storybook-visual-smoke.mjs fails a render when a play function throws.
  • Product/Settings/Providers/LargeConnectionDetail (444-model fixture, 1280×900): the rows' viewport scrolls (scrollHeight > clientHeight), its height stays within min(32rem, 60vh) read from the live window, the settings pane's scrollHeight is now smaller than the list's — i.e. the page no longer carries the catalog — nothing overflows sideways, the filter and the 添加模型 button are not inside the viewport, and after scrolling to the end the last row's bottom edge lies inside the viewport rect. Filtering to model-04 leaves 10 rows with scrollHeight === clientHeight, so a short list reserves no empty box.
  • LargeConnectionDetailShortWindow (new story, 480×800): the same assertions where 60vh (480px) is the binding term rather than the 32rem cap — the case the viewport-relative half of the bound exists for. storyViewport() in the smoke harness gives this story that size, with a unit test in scripts/storybook-visual-smoke.test.mjs covering both the size and why 900px tall would not exercise it.
  • ConnectionDetailPage (2-model catalog): the viewport is present but unscrolled and under the bound, and the filter stays absent below its 8-model threshold.
• Fails without the change — reverting the two source files and rebuilding Storybook fails all three stories (.providerModelRowsViewport is absent, and the page-height assertion is the inverse of the old behaviour).
• node --test scripts/storybook-visual-smoke.test.mjs — 17/17.
• npx biome check on the changed files — clean.
• npm run typecheck:stories — 33 pre-existing errors on main in unrelated files, 33 after this change, none in the touched files. Not fixed here.
• Not run: the full test suite and packaging checks; nothing outside the renderer settings surface is touched.

AI use

Select exactly one:

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

Tool(s) and scope: Claude Code (Claude Opus 5) wrote the CSS rule, the JSX wrapper, the Storybook play-function assertions and the harness viewport case, under human direction and review; the commit carries a Generated-by: Claude Code 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 with Claude Code

[me2seeks]

Screenshots before and after

[Astro-Han]

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Reviewed range: first review, 6772a345..42c08684 (single commit 42c0868, merge-base 6772a34 with main). No prior findings.

Is the problem real on main? Yes. On main, Settings -> 模型 -> connection detail renders every chat-capable model as a direct row of the section, with no bounded viewport. #6164's measurements (444-model fixture, settings pane about 20.6k px tall) match the source. The approach fits #6164's proposed scope: native CSS max-height + overflow-y: auto, the filter and actions kept outside, and no virtualization.

Verification (local, Linux, Chromium via the repo's smoke harness helpers):

• node --test scripts/storybook-visual-smoke.test.mjs: 17/17 pass. biome check on the five touched files: clean.
• Built Storybook (after build:workspace-deps) and ran ConnectionDetailPage, LargeConnectionDetail and the new LargeConnectionDetailShortWindow through smokeStory: all three play functions pass on the head.
• Merged with current main (6c09ba7), resolved the conflict by keeping both sides' assertions, rebuilt and reran: all three still pass. In the 480x800 story, the #6162 sticky save bar stays inside the pane at the top, middle and end of the pane scroll.
• The fixture/filter arithmetic checks out: model-04 matches 040-049, so 10 rows.

Findings

1. P2: the new scroll container clips the keyboard focus ring of every model switch on its inline-end edge. overflow-y: auto also makes overflow-x non-visible, so the viewport clips to its padding box. The rows are edge-to-edge (padding-inline: 0), and each row's last control (the enable Switch) sits flush against the viewport's right edge. Chromium paints the Switch's :focus-visible ring (outline: auto) about 3px outside the element. Measured on a Tab-focused switch: ring right edge 1036px vs. clip edge 1033px at 1280 wide, and 445 vs. 442 in the 480-wide story. A zoomed screenshot shows the ring and the track end cut off flat. With overflow: visible forced on the same element, the ring renders whole. rows.css ("The group's own corners") calls out this exact case: it avoids overflow: hidden on the row group because it "would cut that ring off ... a keyboard-visibility regression". The same happens at the top and bottom edges for a row that is only partly scrolled into view. One fix: give the viewport a small inset and take it back, e.g. padding: var(--space-1); margin-inline: calc(-1 * var(--space-1));. That leaves room for the ring while the row track stays aligned with the heading. A play-function assertion that a focused switch's rect plus about 4px fits inside the viewport's client rect would lock this in.

2. P3: merge conflict with main (#6162). GitHub reports CONFLICTING/DIRTY. The only textual conflict is apps/desktop/stories/settings/provider-settings.stories.tsx, in LargeConnectionDetail.play: #6162 (merged as 6c09ba7) added the save-bar visibility assertions to the same play function. provider-connection-detail.tsx and provider-editor.css auto-merge cleanly. A rebase should keep both sets of assertions (I checked that the combination passes). #6164's acceptance criteria also ask that the #6162 save controls stay reachable and that the large-catalog story check this. After the rebase that coverage comes from #6162's existing loop, which is fine, but it is worth saying so in the PR description.

3. P3: comments made stale by the rebase. #6162's CSS comment on .providerModelsSaveBar ("The settings pane remains the only scroll owner") and its story comment ("The production pane is the sole scroll owner") stop being true once this PR adds a nested scroller. Please update them in the rebase so the next reader isn't misled.

Minor, no action required: the inner viewport is the outer group's :last-child, so it inherits the bottom --radius-surface corners, and with overflow: auto those corners clip the bottom-end of the last visible row slightly. Not visible in practice at a 10px radius against a transparent background.

Overlap with other PRs: #6158 (Astryx Selector, merged) touches only composer/model-picker files, so no overlap. Open PRs touching the same files: #5803 (provider-connection-detail.tsx), #6012 (provider stories), and #5557/#5438 (storybook-visual-smoke*). These are different hunks, but whichever lands later will need a trivial rebase.

CI: the CI workflow run on this head is action_required (waiting for maintainer approval for a first-time contributor). Only the PR effort label job has run, and it passed. There is no CI signal yet beyond my local runs.

Protocol/epoch impact: none. Renderer CSS/TSX, stories and smoke harness only.

Verdict: the change is well scoped, solves a real problem, and its tests are meaningful. Fix the focus-ring clipping (P2), then rebase onto main keeping #6162's assertions.

[Astro-Han]

P2: overflow-y: auto also makes overflow-x clip, and the rows are edge-to-edge, so the trailing Switch's :focus-visible ring (about 3px outside the element) gets cut off on the right edge. I measured and screenshotted this in both the 1280 and the 480 story. rows.css documents exactly this as a keyboard-visibility regression to avoid. A small inset that you take back keeps the ring visible and the track aligned, e.g. padding: var(--space-1); margin-inline: calc(-1 * var(--space-1));.

[qwist1233-cpu]

Here you go — both shots are the provider settings pane with a 444-model connection, dark scheme, 1280x900, captured headlessly with the repo's own tooling (scripts/storybook-visual-diff.mjs capture) from a temporary story that scrolls the pane to the same offset on both branches, so the two frames are comparable.

Before (main) — the model list runs the full height of the pane; everything below it (Advanced request settings, headers, extra body) is pushed off and only reachable after scrolling through all 444 rows:

After (this PR) — the rows live in their own bounded viewport, so the rest of the connection settings stay on the same screen and the save bar is no longer pushed away by the list:

Also rebased on current main; the conflict was in provider-settings.stories.tsx, where your side had added the model-row assertions to LargeConnectionDetail.play — I kept both, so that play now checks the bounded viewport, the save-bar visibility at the top/middle/end of the pane, and the filtered-down case with no empty box under the rows.

[me2seeks]

Automated Codex review under user direction; not an independent human review.

Reviewed head 63767a3. The existing P2 focus-ring finding remains unresolved: .providerModelRowsViewport adds overflow-y: auto without an inset, while rows.css keeps model rows flush with the inline edges (padding-inline: 0) and the Switch is the final control. The new overflow boundary clips the trailing control's outward keyboard focus indicator. Please leave room for the indicator inside the clipping boundary and add keyboard-focus coverage; the current scroll-width/row-bottom assertions do not cover focus paint. I confirmed the source path underlying the existing thread, but did not rerun its browser screenshots.

Validation: smoke harness unit tests pass 17/17. Storybook browser plays were not rerun. CI run 39391636880 is action_required and there are no check runs on this head, so hosted CI remains unvalidated. The previous merge conflict is resolved (currently mergeable).

[me2seeks]

Follow-up automated Codex review: exercised the live PR Storybook at 600x320, 600x480, 600x600, 800x320, 480x320, 480x800 and 1280x900. Found another P2 below; existing request changes remains. No horizontal overflow with the 444-model fixture at these sizes.

[me2seeks]

[P2] Keep keyboard-focused model controls above the sticky Save bar in short windows

At the supported 320px minimum window height, this nested scroll container can bring keyboard focus completely underneath .providerModelsSaveBar. Reproduced on the live LargeConnectionDetailShortWindow story: resize to 600x320, toggle a model to show the Save bar, focus model 443's Switch as setup, then press Tab to model 444's parameter button. The focused button is at y=288..316 while Save occupies y=251..316; elementFromPoint at the button center hits the Save bar. The list's y=138..330 is only partly visible, and inner scrolling does not compensate for the sticky overlay. Also reproduced at 600x480 and 800x320. Switching off this rule and the max-height (restoring the old page scroll behavior) makes the same Tab target visible at y=190..218, above Save.

Please coordinate focus scrolling with the sticky bar/visible portion of the inner viewport, or ensure the entire list viewport fits in the unobscured pane. Add a short-height keyboard-focus assertion: current tests only check that Save is visible and that the last row lies inside the list's own rect, which both pass while the focused control is completely obscured. Screenshot retained locally at /tmp/6165-600x320-focus.png. Existing 480x800 case does not expose this.

[Astro-Han]

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review at 63767a3a9784ba9bbda355acd753e60037c156cf, compared with the previous reviewed head 42c0868422addba79a5fe01be4f3c004b7557298.

The production scroll-container change is equivalent to the previous version. The rebase preserves main's sticky Save-bar story assertions and updates the story comment. The previous merge-conflict concern is resolved: a trial merge with main e66a655e4f262c68205bbda6e76613311db2b3c2 is clean, although I did not test the merged tree.

Two already reported P2 issues remain; I independently reproduced both and am linking their existing threads rather than adding duplicate inline comments:

• Clipped keyboard-focus outline: in actual Chromium at 1280×900 and 480×800, the rightmost model switch ends exactly at the new viewport's clipping edge (x=1023 and x=432 respectively). Its 1px outline plus 2px outline offset extends beyond that edge, and the viewport has no padding. The outward right-hand focus ring is clipped in both captures.
• Sticky Save bar covers the focused final model control: after changing a model and tabbing from model 443's switch to model 444's parameter button, the button is entirely covered at both 600×320 and 600×480. At 600×320 the button spans y=288–316 while the Save bar spans y=251–316; hit-testing the button's center returns the Save bar. Disabling only the new viewport's max-height/overflow rules restores the same focused button above the bar at both heights. The 320px height is supported by the application's window minimum.

Validation: all three affected Storybook play scenarios passed in real Chromium; the focus-outline checks and both short-window head/control pairs completed successfully. The 17 smoke-harness tests, fresh dependency installation/patch, normal clean root test build, Storybook build, workspace type checks, renderer build, lint, format, ASF checks, and protocol-epoch guard passed. No protocol change is introduced. The CSS comment that calls the settings pane the only scroll owner remains a minor documentation inconsistency.

CI run 39391636880 for this exact head has been approved and is currently in progress; I am not treating it as green. This review does not cover the full repository test suite, native Electron/Windows behavior, or installers.

[qwist1233-cpu]

Both points addressed in edc83c7.

Focus ring. The viewport now steps out by --space-2 and takes the same amount straight back as padding, so the ring has room inside the clip without moving the rows. Measured in headless Chromium on large-connection-detail-short-window: 18px of space to the right of the trailing Switch, where before the fix it was 0.0px — the ring needs 4px (--focus-ring-width 2 + --focus-ring-offset 2).

Save bar. You were right that the row-bottom assertion says nothing about focus paint. The bar is sticky over the pane, so scrolling inside the viewport can never move a row out from under it; the pane now reserves the bar's measured height as scroll-padding-block-end and scrolls itself instead. The height is published by a ResizeObserver, the same way use-focused-preview.ts publishes --maka-focused-composer-space; it resolves to 65px here, and the property defaults to 0px so the other settings pages are untouched.

To be sure this was real and not just a plausible story, I walked 11 pane scroll positions at 600x320, 480x800 and 1280x900 and focused the last Switch at each one. Without the fix there is exactly one position per size where the focused control is covered and document.elementFromPoint on its centre returns the Save bar; with the fix there is none.

Coverage. The play function now does that same walk and asserts the focused control is clear of the bar by geometry and by hit test. It fails without the fix at 480x800, 600x480, 600x320 and 1280x900 ("expected false to be true") and passes with it. large-connection-detail, large-connection-detail-short-window and connection-detail-page still pass at their own viewports; typecheck and biome lint are clean on the three files.

[Astro-Han]

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review at edc83c70051f53bdfcef2099e4a5855d189eab83, following 63767a3a9784ba9bbda355acd753e60037c156cf. The preceding commit is unchanged; the additional commit adds viewport padding and measures the sticky Save bar to provide scroll clearance. I reviewed the complete five-file change and the new commit separately.

Both previous focus P2 issues are fixed in the tested browser cases. In actual Chromium at 1280×900 and 480×800, the keyboard-focus ring now has 8px of room at the viewport edge for its 3px outward extent. Removing only the new viewport padding reproduces clipping at both sizes. At 600×320, 600×480 and 800×320, tabbing from model 443's switch to model 444's parameter button leaves that button above the sticky Save bar. Removing only the new pane scroll padding reproduces the covered target at all three sizes. At 600×320, the fixed button ends at y=251, exactly where the bar starts; without clearance it occupies y=278–306 beneath the bar. I also inspected the paired captures.

One new P2 remains: the new resize/lifecycle work fails the renderer architecture gate. provider-connection-detail.tsx:169–177 adds useLayoutEffect, useRef and ResizeObserver to a component whose recorded architecture permits none of those additional calls. The exact-head CI run fails for these additions. I reproduced the same failure locally with npm --workspace @maka/desktop run check:architecture -- --base origin/main --strict-base: the checker reports environmentCapabilities changing from {} to {"ResizeObserver":1}, plus the two additional hooks. An analysis of current main's component with the same analyzer confirms that it has none of those added calls. This needs an allowed ownership arrangement before CI can pass; a clean browser result does not resolve the gate failure.

Validation otherwise passed: the three affected Storybook play scenarios, all ten additional browser head/control checks described above, 17 smoke-harness tests, the normal clean root test build, all five Desktop TypeScript configurations, Storybook and production renderer builds, Biome on the five changed files, and git diff --check. All 168 architecture checker tests pass; the subsequent check of this PR's application code fails as described above.

No protocol change is introduced. A static merge with main 9826db1b9dad32897be105b2c1c621becedb27ce is clean and passes the protocol guard at epoch 226; I did not build that merged tree. Native Electron, Windows/macOS and the complete repository suite were not exercised. The CI failure above remains unresolved.

[Astro-Han]

P2: this new lifecycle/resize hook makes the required renderer architecture check fail. CI run 39569088879 and the same strict check locally both report the newly added ResizeObserver capability and useLayoutEffect/useRef calls against this component's existing architecture record. Current main has none of these calls. Please place this work under an allowed owner and satisfy the architecture gate before merging; the browser focus fixes themselves pass the head/control checks.

[qwist1233-cpu]

Architecture gate fixed in f593196, and it was the right call to flag it rather than let me widen the ledger.

The clearance no longer lives in the detail view. attachModelsSaveBarClearance is a ref callback in features/connection-settings, re-exported from the feature's public entry, and the view just hands it to the bar: ref={attachModelsSaveBarClearance}. So the view's hookCalls and environmentCapabilities go back to exactly what main records — no useLayoutEffect, no useRef, no ResizeObserver — and the feature's public entry was already a sanctioned dependency for it, so renderer-architecture.json needed no edit at all.

Verified at this head:

• npm --workspace @maka/desktop run check:architecture passes, and so does node scripts/check-renderer-architecture.mjs --base origin/main --strict-base — the ratchet run that failed before. Its only output is the expected notice that the AppShell closure newly uses attachModelsSaveBarClearance from the feature entry (reported, not ratcheted).
• Both fixes still hold in headless Chromium on large-connection-detail-short-window, 444-model fixture, dark scheme. At 600x320: focused parameter button bottom 137px against a bar top of 251px, elementFromPoint on its centre returns the button; published clearance 65px; 18px of room to the right of the trailing Switch. At 1280x900: 413px against 831px, same hit result. Removing only the new CSS reproduces both faults in the same run — covered button (hit-test returns the bar) and 10px of room where the ring needs 3px plus offset.
• tsc -p tsconfig.renderer.json and tsc -p tsconfig.storybook.json clean, Biome clean on the four changed files, and the six provider-settings play scenarios that touch the detail view pass (large-connection-detail, large-connection-detail-short-window, connection-detail-page, model-parameter-save, retired-free-connection, model-capabilities).

One behavioural note: the bar only renders while the connection is live, so a retired connection publishes no custom property and the pane's scroll-padding-block-end falls back to 0px. Other settings pages are untouched for the same reason.

[Astro-Han]

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review at f593196304259241e46bcc872ce13861eb35fd80, following edc83c70. The renderer architecture P2 is fixed; I found no new P0–P2 issue. I read the complete seven-file PR diff and the four-file increment. The measurement now belongs to the existing connection-settings feature and is attached through a stable React ref callback. Its cleanup disconnects the observer and removes the pane's custom property. The architecture ledger is unchanged. All 168 checker tests and the subsequent strict architecture check against current main passed; the same analyzer reports the detail view's hooks and environment capabilities matching main, with the previous useLayoutEffect, useRef and ResizeObserver additions removed.

The earlier focus fixes remain effective in real Chromium. All three original Storybook play scenarios passed. At 1280×900 and 480×800, the focused switch has 8px clearance for its 3px outward ring; removing only the viewport padding recreates clipping. At 600×320, 600×480 and 800×320, tabbing from model 443's switch to model 444's parameter button leaves the button clear of the sticky Save bar. Removing only the pane's scroll padding covers the target in all three cases. I inspected the paired 600×320 captures. These are production components and styles in the existing Storybook frame, not native Electron checks.

A separate Chromium check mounted the actual new helper as a React callback ref under StrictMode. It retained one active observer, updated the rounded clearance after resizing, cleared the observer/property on removal, registered correctly on remount and cleaned up all four created observers on final unmount. This specifically verifies the lifecycle change beyond the unchanged layout checks.

Fresh dependency installation/patching, the normal clean root build:test, all five Desktop TypeScript configurations, Storybook and production renderer builds, 17 smoke-runner tests, Biome on the four changed files, ASF headers and whitespace checks passed. A static merge with main ca4ac75c9d157bb49bd7d0feae975f2574a7ca55 is clean and passes the epoch guard at 227; the merged tree was not separately built. The PR introduces no protocol change.

P3 — generation trailers: the substantive follow-up commits edc83c70 and f5931963 have Co-Authored-By but still lack the required Generated-by: <tool> trailer. Add it to affected commits and preserve it when squashing. The existing CSS comment describing the pane as the only scroll owner also remains inaccurate with the nested model viewport, without affecting runtime behavior.

Exact-head CI was awaiting workflow approval; I approved the reviewed run and it is now in progress. I did not run the complete repository suite, native Windows/macOS/Electron interaction or installer checks. Final CI still needs to complete.