Skip to content

fix(app): make model picker height adaptive to model count - #718

Merged
Astro-Han merged 5 commits into
devfrom
claude/fix-model-picker-height
May 18, 2026
Merged

fix(app): make model picker height adaptive to model count#718
Astro-Han merged 5 commits into
devfrom
claude/fix-model-picker-height

Conversation

@Astro-Han

@Astro-Han Astro-Han commented May 18, 2026

Copy link
Copy Markdown
Owner

Summary

Switch the model selector popover from a fixed h-[400px] to max-h-[400px] so it hugs its natural content height and only scrolls when models exceed the 400px cap. Add an E2E spec that locks the geometry invariant.

Why

When a user only has a handful of visible models (e.g. the OpenCode Zen free tier with four entries), the popover was still forced to 400px tall, leaving a ~190px empty band between the last model row and the Thinking row at the bottom. The popover should adapt to model count.

Root cause: packages/app/src/components/prompt-input/model-picker.tsx:218 set a fixed height on the Kobalte.Content. The inner ModelList uses flex-1 min-h-0, so it expanded to fill the fixed 400px and produced a visible empty band above the Thinking row.

Related Issue

Closes #719.

Human Review Status

Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.

Review Focus

  • The single CSS class change on Kobalte.Content. The flex-1 min-h-0 chain on ModelList -> list-scroll still works under max-h because the chain stays unbroken when the cap kicks in; when content fits, the column shrinks to content height instead.
  • The new E2E spec: it asserts the fixture renders a short list that fits without scrolling (precondition), then asserts there is no empty band between the last [data-slot="list-item"] and [data-action="prompt-model-thinking-trigger"], and finally that popover.height <= 400. If a future fixture grows large enough to overflow the cap, the precondition fails loudly rather than silently skipping the regression check.
  • The spec is tagged @smoke so the regression is enforced by the PR e2e-artifacts gate. The smoke inventory in packages/opencode/test/config/e2e-smoke-tagging.test.ts is updated in lockstep (alphabetical order).

Risk Notes

None. The fix is a single CSS class swap with no behavioral side effects. The new spec runs in the existing Playwright project, no infra changes.

How To Verify

Negative check (revert fix, run spec): 1 failed — gap below last model = 193.25px
Positive check (apply fix, run spec):  1 passed — popover hugs content
bun --cwd packages/app run test:e2e:local e2e/model-picker-height.spec.ts

Smoke inventory contract: bun --cwd packages/opencode test test/config/e2e-smoke-tagging.test.ts -> 2 pass.

Screenshots or Recordings

E2E captures a screenshot artifact at e2e/test-results/.../model-picker.png on every run. With the fix applied the popover ends right under the four visible models and the Thinking row, with no empty band. Local screenshot reviewed and confirmed visually before opening this PR.

Checklist

  • Human review status is stated above as pending, approved, or not required
  • I linked the related issue, or stated why there is no issue
  • This PR has exactly one type label (bug, enhancement, task, or documentation), at least one primary routing label (app, ui, platform, harness, or ci), and exactly one priority label (P0 to P3), or I requested maintainer labeling
  • I described the review focus and any meaningful risks
  • I listed the relevant verification steps and the key result for each
  • I did not introduce unrelated refactors, dependencies, generated files, or file changes beyond the stated scope
  • I manually checked visible UI or copy changes when needed, with screenshots or recordings
  • I considered macOS and Windows impact for platform, packaging, updater, signing, paths, shell, or permissions changes
  • I called out docs, release notes, dependencies, permissions, credentials, deletion behavior, generated content, or local file changes when relevant
  • I reviewed the final diff for unrelated changes and suspicious dependency changes
  • I am targeting dev, and my PR title and commit messages use Conventional Commits in English

Pin h-[400px] -> max-h-[400px] so the popover hugs the natural content
height and only scrolls when models exceed the 400px cap. Add an E2E
spec that locks the geometry invariant (no empty band between the last
visible model and the Thinking row) and captures a screenshot artifact
for visual review.
@coderabbitai

coderabbitai Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@Astro-Han has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 54 minutes and 16 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e45b38c-571c-4c69-bfa0-5cc6a79576f5

📥 Commits

Reviewing files that changed from the base of the PR and between b57d673 and 2c1b53a.

📒 Files selected for processing (3)
  • packages/app/e2e/model-picker-height.spec.ts
  • packages/app/src/components/prompt-input/model-picker.tsx
  • packages/opencode/test/config/e2e-smoke-tagging.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/fix-model-picker-height

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Astro-Han Astro-Han added the bug Something isn't working label May 18, 2026
@github-actions github-actions Bot added app Application behavior and product flows ui Design system and user interface labels May 18, 2026
@Astro-Han Astro-Han added the P2 Medium priority label May 18, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested priority: P2 (includes user-path files (packages/app/src/components/prompt-input/model-picker.tsx)).

P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the model picker's styling to use a maximum height instead of a fixed height, ensuring the container fits its content without leaving empty space. A new E2E test was added to validate this behavior. The review feedback suggests improving the robustness of the E2E test by using a more specific selector to avoid ambiguity and potential fragility.

Comment thread packages/app/e2e/model-picker-height.spec.ts Outdated
@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown

Perf delta summary

Comparator: pass

Profile / Scenario interaction median interaction worst long task max tbt frame gap p95 frame gap max jank count cls status
default / homepage-cold 24 -> 24 (0) 56 -> 48 (-8) 75 -> 66 (-9) 25 -> 16 (-9) 16.8 -> 16.8 (0) 166.6 -> 116.7 (-49.9) 4 -> 3 (-1) 0 -> 0 (0) pass
default / long-session-input-lag 48 -> 48 (0) 48 -> 64 (+16) 0 -> 0 (0) 0 -> 0 (0) 16.7 -> 16.7 (0) 16.8 -> 16.8 (0) 0 -> 0 (0) 0 -> 0 (0) pass
default / session-streaming-long 48 -> 40 (-8) 64 -> 64 (0) 0 -> 0 (0) 0 -> 0 (0) 16.8 -> 16.8 (0) 33.4 -> 16.8 (-16.6) 0 -> 0 (0) 0 -> 0 (0) pass
default / tool-call-expand 16 -> 24 (+8) 40 -> 40 (0) 0 -> 0 (0) 0 -> 0 (0) 16.7 -> 16.8 (+0.1) 16.7 -> 16.8 (+0.1) 0 -> 0 (0) 0 -> 0 (0) pass
default / tool-default-open-heavy-bash 24 -> 32 (+8) 24 -> 40 (+16) 64 -> 61 (-3) 14 -> 11 (-3) 50 -> 50 (0) 116.7 -> 100 (-16.7) 3 -> 2 (-1) 0 -> 0 (0) pass
default / terminal-side-panel-open 48 -> 48 (0) 64 -> 56 (-8) 0 -> 0 (0) 0 -> 0 (0) 33.3 -> 33.4 (+0.1) 33.3 -> 33.4 (+0.1) 0 -> 0 (0) 0 -> 0 (0) pass
default / session-scroll-reading 32 -> 24 (-8) 32 -> 32 (0) 0 -> 0 (0) 0 -> 0 (0) 16.7 -> 16.7 (0) 16.7 -> 16.7 (0) 0 -> 0 (0) 0.505 -> 0.505 (0) warn: cls

Astro-Han added 2 commits May 18, 2026 11:40
The variant (Thinking) sub-popover also renders [data-picker-content].
.first() works today because the variant content only mounts when its
trigger is expanded, but scoping by [data-slot="list-scroll"] keeps
the locator unambiguous if future test changes interact with the
variant popover.
The previous spec wrapped the no-empty-band assertion in
'if (!listOverflowing)', which silently skipped the regression check
if a future fixture rendered enough models to overflow the 400px cap.
Promote the short-list expectation to a precondition assertion so the
spec fails loudly when the fixture grows, prompting maintainers to
restore a short-list scenario or add a separate overflow spec.
@Astro-Han

Copy link
Copy Markdown
Owner Author

Thanks for the audit. Addressed the two P3 items:

  • P3-1 (silent skip of gap assertion): Fixed in cc10500. The if (!listOverflowing) guard is gone — the spec now asserts scrollHeight <= clientHeight + 1 as a precondition, so if a future fixture grows large enough to overflow the 400px cap the spec fails loudly instead of skipping the regression check.
  • P3-2 (not in @smoke): Decided against adding @smoke. This is a low-frequency visual regression guard, not a critical-path smoke check, and widening the PR smoke surface costs CI time on every PR. Full E2E sweeps will catch regressions. PR body now states this explicitly.

unit-opencode and all other CI jobs are green at the latest commit.

Promote the regression to the PR smoke gate. The bug is real (user-
reported visible UI break) and the cost is minimal: ~4s per PR plus a
one-time alphabetical-order entry in the opencode smoke inventory.
Catching a regression at PR time keeps the bug from sneaking through
to a release cut before nightly E2E runs.
@Astro-Han

Copy link
Copy Markdown
Owner Author

Reversing my earlier call on P3-2. After a second pass through the three-question check:

  • "Could this be even less?" — I was wrong to treat "skip the smoke gate" as the cheaper path. The actual cost is ~4 seconds per PR plus a one-time inventory entry; the cost of not gating is that a regression of this real, user-reported visible bug can slip past PR check and reach a release cut before nightly E2E catches it.
  • "Is what remains good enough?" — No. The good-enough bar for a regression of a real user-reported visible bug is "don't let it reach users again", not "nightly will eventually find it".
  • "Is it reassuring?" — Future maintainers touching model-picker.tsx benefit from PR-time feedback, not next-day feedback.

Tagged @smoke and synced the inventory in alphabetical order in ae1007f5. PR body updated.

@github-actions github-actions Bot added the harness Model harness, prompts, tool descriptions, and session mechanics label May 18, 2026
…r-height

# Conflicts:
#	packages/opencode/test/config/e2e-smoke-tagging.test.ts
@Astro-Han
Astro-Han merged commit c46b602 into dev May 18, 2026
28 checks passed
@Astro-Han
Astro-Han deleted the claude/fix-model-picker-height branch May 18, 2026 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app Application behavior and product flows bug Something isn't working harness Model harness, prompts, tool descriptions, and session mechanics P2 Medium priority ui Design system and user interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Model picker popover keeps fixed 400px height when only a few models are visible

1 participant