Skip to content

fix: reranker model dropdown labels#3704

Merged
joeyorlando merged 7 commits intomainfrom
codex/reranker-dropdown-full-names
Apr 11, 2026
Merged

fix: reranker model dropdown labels#3704
joeyorlando merged 7 commits intomainfrom
codex/reranker-dropdown-full-names

Conversation

@joeyorlando
Copy link
Copy Markdown
Contributor

@joeyorlando joeyorlando commented Apr 10, 2026

Summary

  • Adds opt-in popover width control to the shared searchable select.
  • Lets the LLM model selector render untruncated, wrapping option labels when requested.
  • Applies the wider, non-truncating behavior to the Settings > Knowledge reranker model dropdown so long model names are visible.

@joeyorlando joeyorlando changed the title [codex] Fix reranker model dropdown labels fix: reranker model dropdown labels Apr 10, 2026
@joeyorlando joeyorlando marked this pull request as ready for review April 10, 2026 18:37
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 10, 2026

Claude finished @joeyorlando's task in 2m 39s —— View job


PR Review: fix: reranker model dropdown labels

  • Read CLAUDE.md for repo-specific guidance
  • Read changed files
  • Analyze the changes
  • Post review feedback

Overall this is a clean, well-scoped fix for a real UX problem (long reranker model names getting truncated). The change is additive, backwards-compatible, and ships with focused tests for the new behavior. Nice work.

Summary of changes

  • ui/searchable-select.tsx — adds opt-in props: contentClassName, contentSide, contentAlign, contentAvoidCollisions, listClassName. Also switches the hard-coded max-h-[300px] on the list to a viewport-aware max-h-[min(300px,calc(var(--radix-popover-content-available-height)-3rem))].
  • llm-model-select.tsx — threads those popover props through as popoverContentClassName / popoverListClassName / popoverSide / popoverAlign / popoverAvoidCollisions, and adds truncateOptionLabels to allow wrapping option labels.
  • app/settings/knowledge/page.tsx — applies the new behavior to both the embedding and reranker model selects via two module-level constants.
  • Adds tests for SearchableSelect (side/align/listClassName) and LlmModelSearchableSelect (fit-content popover + wrapping labels).

What I like

  • Opt-in defaults preserve current behavior everywhere else (truncateOptionLabels = true, no contentClassName by default). Good backwards compatibility.
  • Shared KNOWLEDGE_MODEL_POPOVER_CLASS / KNOWLEDGE_MODEL_POPOVER_LIST_CLASS constants avoid duplication between embedding and reranker selects in knowledge/page.tsx:80-83.
  • The new max-h-[min(300px,calc(var(--radix-popover-content-available-height)-3rem))] on the list is a nice generic improvement — it keeps the 300px cap on tall viewports but clamps to the available space on short ones. Relying on --radix-popover-content-available-height is correct here since avoidCollisions defaults to true.
  • Using cn() (tailwind-merge) means a caller-provided max-h-[...] in listClassName cleanly overrides the default, which the knowledge/page.tsx override at KNOWLEDGE_MODEL_POPOVER_LIST_CLASS depends on. I confirmed this works correctly.
  • Tests use real user interactions (userEvent.click) and check observable behavior (classes actually applied, data-side / data-align attributes) rather than implementation details — this matches the "Test What Matters" guideline in platform/CLAUDE.md.

Suggestions / nits

  1. RerankerModelSelector empty state loses stylingplatform/frontend/src/app/settings/knowledge/page.tsx:337-348 renders a disabled LlmModelSearchableSelect as a placeholder when no API key is selected, but it doesn't pass the new popoverContentClassName / popoverListClassName / popoverSide / popoverAlign / truncateOptionLabels props. Since the dropdown is disabled the popover never opens, so this is purely cosmetic/consistency. Still, if someone toggles between states, having consistent props reduces cognitive load. Optional cleanup.

    Fix this →

  2. Prop surface is growingLlmModelSearchableSelect now takes 6 popover-related props (popoverContentClassName, popoverListClassName, truncateOptionLabels, popoverSide, popoverAlign, popoverAvoidCollisions). If you anticipate more call sites needing custom popover behavior, consider collapsing these into a single popoverProps?: { contentClassName?, listClassName?, side?, align?, avoidCollisions? } object. Not necessary for this PR, but worth keeping in mind if this grows again.

  3. LlmModelOptionLabel truncation only applies to the model lineplatform/frontend/src/components/llm-model-select.tsx:65-72: when truncateOptionLabels={false} but showPricing or description is set, the secondary lines still use truncate. Probably intentional for the knowledge use case (no pricing/description there), but if this prop gets reused for a pricing-heavy context later, the description line will still get clipped. Consider either documenting that limitation or propagating the wrap behavior.

  4. Test brittleness — data-slot selector — in searchable-select.test.tsx:38-40 and llm-model-select.test.tsx:38-40, the tests use .closest("[data-slot='popover-content']"). This couples to the shadcn popover internal structure. It works today, but if the popover implementation changes its data-slot value the tests break silently with confusing errors. A getByRole("dialog") or explicit data-testid on the PopoverContent would be more resilient. Minor.

  5. llm-model-select.test.tsx — exact class assertion — line 40-44 asserts that the popover content has all three classes w-max, min-w-[var(--radix-popover-trigger-width)], and max-w-[min(32rem,calc(100vw-2rem))]. Since SearchableSelect itself applies w-[var(--radix-popover-trigger-width)] first and then merges contentClassName, this depends on tailwind-merge behavior. It's currently correct (w-max wins), but asserting on the merged class string is fragile to Tailwind/twMerge version bumps. Consider asserting on computed width via getBoundingClientRect, or accept this as a tradeoff. Minor.

  6. Naming consistency — the underlying SearchableSelect uses contentSide / contentAlign but the wrapper LlmModelSearchableSelect renames them to popoverSide / popoverAlign. Both naming schemes are valid but the asymmetry is a small papercut when tracing props through. Not worth changing here, just flagging.

Correctness / behavior checks I performed

  • cn() / tailwind-merge correctly resolves w-[var(...)] vs w-max (caller override wins).
  • max-h-[min(300px,calc(var(--radix-popover-content-available-height)-3rem))] gracefully falls back since avoidCollisions defaults to true on Radix popovers.
  • --radix-popover-content-available-height only exists when collision detection is enabled — default in this codebase, so safe.
  • ✅ The listClassName test asserts parentElement has the class — verified that in searchable-select.tsx:144-150 the scroll container is indeed the direct parent of option buttons.
  • ✅ No try/catch added in components (matches the "Never throw on HTTP errors" guideline, though not applicable here).
  • ✅ Additive prop changes — no breaking changes to existing call sites.
  • LlmModelOptionLabel signature change is additive (new optional prop with default).

Questions

  • Did you confirm the fix visually on both the embedding and reranker dropdowns on a short-viewport (e.g. 720px) display? The max-h-[min(220px,...)] cap in KNOWLEDGE_MODEL_POPOVER_LIST_CLASS is tighter than the default 300px — just want to make sure long model lists still scroll well there.
  • Any reason for the asymmetric defaults (popoverAlign="end" / popoverSide="bottom" at the call sites in knowledge/page.tsx) rather than the general defaults? The recent commit history (Pin knowledge model popovers to bottom, Align knowledge model popovers to trigger end) suggests these are intentional pinning fixes — a short comment near KNOWLEDGE_MODEL_POPOVER_CLASS explaining why they're pinned would help future readers.

Verdict

LGTM with the minor suggestions above. The tests cover the new behavior well, the change is surgical, and the implementation follows the existing component patterns. None of the suggestions are blockers.
Branch

github-merge-queue bot pushed a commit that referenced this pull request Apr 10, 2026
## Summary
- keep normal first-class PR checks together in `On Pull Requests`, and
run that workflow on merge queue so required checks are produced for
merge-group SHAs
- add `Docker Image Builds` to PR pushes and merge queue so both
platform and MCP server base images must build successfully
- preserve release-please auto-commit behavior for generated `docs/` and
`platform/` changes such as OpenAPI/codegen updates
- move PR title linting into its own workflow so title edits do not
create skipped main CI checks, while posting a no-op merge-queue status
so it can remain required
- inline full E2E into `Platform E2E Tests`, triggered by the `run-e2e`
label, manual dispatch, and automatically on merge queue
- split Docker Scout scanning into `Docker Image Scanning`, triggered
automatically on merge queue or on demand with the `run-docker-scan`
label
- delete the old `Platform Linting and Tests (Required)` reusable
workflow

## Context
The example run for PR #3704 was triggered by a PR title edit. Because
title edits and labels shared the main PR workflow, GitHub created
skipped check runs for unrelated jobs. The previous required reusable
workflow also created fast-pass placeholder checks for Helm chart
linting, Docker image scanning, and E2E jobs on normal PRs.

After this change, ordinary PR runs include real checks in `On Pull
Requests`: release freeze, license compliance, supply-chain policy,
dependency review, platform lint/unit plus codegen/drizzle/migration
validation, Docker image builds, and Helm chart linting/tests. The same
workflow also listens to `merge_group` because merge queue requires
required-check statuses on the temporary merge-group SHA. Dependency
Review is PR-context-only, so that check passes explicitly on merge
queue after having validated the PR before queueing.

PR title linting remains separate because it must run on title edits. It
also listens to `merge_group` with a no-op pass step so `PR Title
Linter` can remain a required check when merge queue is enabled. Full
E2E is its own first-class workflow that runs by label, by manual
dispatch, or automatically through merge queue. Docker Scout scanning is
also first-class but not paid on every PR push: it runs on merge queue
or when a maintainer applies `run-docker-scan`; fork PR label events are
filtered at the job level so secrets-backed scan work is not scheduled.

Release-please PRs keep the old generated-file behavior: if `pnpm
codegen && pnpm lint:fix` changes checked-in `docs/` or `platform/`
files, CI generates a GitHub App token, waits for `drizzle-kit check`,
commits the generated updates back to the release-please branch, and
exits successfully so the follow-up push re-runs CI.

## Follow-up hardening
- restored the `pnpm db:generate` timeout/prompt detection so
interactive Drizzle rename prompts fail fast instead of hanging CI
- scoped generated-file diff checks to `docs/` and `platform/`
- made E2E test jobs depend on a successful image build instead of
trying to run after build failure
- retained full Playwright report artifacts for 30 days if any E2E
shard, quickstart, or Vault K8s job fails
- kept the `docs/openapi.json` version bump in this branch intentionally

## Validation
- ruby YAML parse for changed workflow files
- pnpm dlx github-actionlint -ignore custom Blacksmith runner labels for
changed workflow files
- git diff --check

## Docs
- audited docs/pages; no docs changes needed for CI workflow internals
@joeyorlando joeyorlando merged commit ebd1c8a into main Apr 11, 2026
39 checks passed
@joeyorlando joeyorlando deleted the codex/reranker-dropdown-full-names branch April 11, 2026 01:53
joeyorlando added a commit that referenced this pull request Apr 11, 2026
🤖 I have created a release *beep* *boop*
---


##
[1.2.10](platform-v1.2.9...platform-v1.2.10)
(2026-04-11)


### Features

* make posthog analytics configurable
([#3707](#3707))
([7ae9101](7ae9101))


### Bug Fixes

* `/llm/costs` table scrolling
([#3722](#3722))
([6a42ba8](6a42ba8))
* apply MCP OAuth lifetime for gateway slugs
([#3711](#3711))
([362aaec](362aaec))
* Bedrock tool name encoding
([#3706](#3706))
([0e2c2d1](0e2c2d1))
* costs timeframes and surface limit reset settings
([#3709](#3709))
([6e4154b](6e4154b))
* jira oauth discovery overrides
([#3721](#3721))
([2c4cf8f](2c4cf8f))
* OIDC discovery trusted origins for IdP registration
([#3714](#3714))
([adb5f5e](adb5f5e))
* preserve shared chat agents on fork
([#3715](#3715))
([252edfc](252edfc))
* reranker model dropdown labels
([#3704](#3704))
([ebd1c8a](ebd1c8a))
* session logs loading state
([#3712](#3712))
([ffba126](ffba126))


### Miscellaneous Chores

* **ci:** add ID-JAG MCP e2e test
([#3702](#3702))
([1a5078a](1a5078a))
* **deps:** bump next from 16.1.7 to 16.2.3 in /platform/frontend
([#3708](#3708))
([d47967c](d47967c))
* use neutral token prefixes with legacy support
([#3719](#3719))
([db5929c](db5929c))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: archestra-ci[bot] <222894074+archestra-ci[bot]@users.noreply.github.com>
Co-authored-by: Joey Orlando <joey@archestra.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant