fix(dropdown): align overflow and shortcut behavior with the public contract#313
Merged
RtlZeroMemory merged 1 commit intomainfrom Mar 24, 2026
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR implements per-dropdown window state tracking for long menus. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Interaction
participant Keyboard as Keyboard Routing<br/>(routeDropdownKey)
participant WindowCalc as Window Calculation<br/>(computeDropdownWindow)
participant State as Widget State<br/>(dropdownWindowStartById)
participant Render as Rendering<br/>(renderOverlayWidget)
participant Screen as Screen
User->>Keyboard: Press arrow key in dropdown
Keyboard->>Keyboard: Compute nextSelectedIndex
Keyboard->>WindowCalc: computeDropdownWindow(itemCount,<br/>nextSelectedIndex, visibleRows)
WindowCalc->>State: Update dropdownWindowStartById[id]<br/>with new startIndex
State->>Render: Pass window state to renderer
Render->>Render: Iterate items from<br/>startIndex to endIndex only
Render->>Render: Render scrollbar<br/>if overflow
Render->>Screen: Draw visible window + scrollbar
sequenceDiagram
participant User as User Click
participant Mouse as Mouse Routing<br/>(routeDropdownMouse)
participant WindowCalc as Window Calculation<br/>(computeDropdownWindow)
participant State as Widget State<br/>(dropdownWindowStartById<br/>dropdownSelectedIndexById)
participant Render as Rendering<br/>(renderOverlayWidget)
participant Screen as Screen
User->>Mouse: Click on dropdown item<br/>(in visible window)
Mouse->>WindowCalc: computeDropdownWindow<br/>for current state
Mouse->>Mouse: Translate click Y to item<br/>index using window.startIndex
Mouse->>State: Update dropdownSelectedIndexById[id]<br/>and dropdownWindowStartById[id]
State->>Render: Pass updated window<br/>and selection state
Render->>Render: Render item at new<br/>window position highlighted
Render->>Screen: Draw updated dropdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/app/widgetRenderer/overlayShortcuts.ts (1)
82-89: Use the shareddescribeThrownhelper instead of redefining it locally.This duplicates existing logic and creates drift risk for future changes.
♻️ Proposed refactor
+import { describeThrown } from "../../debug/describeThrown.js"; @@ -function describeThrown(value: unknown): string { - if (value instanceof Error) return `${value.name}: ${value.message}`; - try { - return String(value); - } catch { - return "[unstringifiable thrown value]"; - } -} - export function selectDropdownShortcutItem(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/widgetRenderer/overlayShortcuts.ts` around lines 82 - 89, Remove the local describeThrown function in overlayShortcuts.ts and replace its usages with the shared helper: import the shared describeThrown (the existing canonical implementation exported from the shared utilities module) at the top of the file, delete the local function declaration (function describeThrown(value: unknown): string { ... }), and update any calls to confirm they reference the imported describeThrown symbol so only the single shared implementation is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/app/widgetRenderer/overlayShortcuts.ts`:
- Around line 82-89: Remove the local describeThrown function in
overlayShortcuts.ts and replace its usages with the shared helper: import the
shared describeThrown (the existing canonical implementation exported from the
shared utilities module) at the top of the file, delete the local function
declaration (function describeThrown(value: unknown): string { ... }), and
update any calls to confirm they reference the imported describeThrown symbol so
only the single shared implementation is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12bb2f8d-c73b-46d0-96fe-a698d23413de
📒 Files selected for processing (16)
docs/widgets/dropdown.mdpackages/core/src/app/widgetRenderer.tspackages/core/src/app/widgetRenderer/mouseRouting.tspackages/core/src/app/widgetRenderer/overlayShortcuts.tspackages/core/src/app/widgetRenderer/overlayState.tspackages/core/src/app/widgetRenderer/routeEngineEvent.tspackages/core/src/layout/__tests__/dropdownGeometry.test.tspackages/core/src/layout/dropdownGeometry.tspackages/core/src/renderer/__tests__/overlay.edge.test.tspackages/core/src/renderer/__tests__/renderPackets.test.tspackages/core/src/renderer/__tests__/renderer.damage.test.tspackages/core/src/renderer/renderToDrawlist.tspackages/core/src/renderer/renderToDrawlist/renderTree.tspackages/core/src/renderer/renderToDrawlist/types.tspackages/core/src/renderer/renderToDrawlist/widgets/overlays.tspackages/core/src/widgets/__tests__/dropdown.position.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Dropdowns now use a deterministic overflow window with matching renderer and mouse hit-testing behavior, and topmost dropdown shortcuts are documented and isolated safely from user callback failures.
Problem
Overflowed dropdowns only clipped drawing geometrically, so long menus had no real visible-window contract and mouse hit-testing could target the wrong item. The docs also said item shortcuts were display-only even though runtime activation executed them, and shortcut callback warning paths were not consistently hardened.
Root cause
Geometry budgeting existed without persistent windowing state across renderer and routing paths, so rendering and hit-testing disagreed once menus exceeded the viewport. Shortcut handling was implemented in a separate overlay path with weaker thrown-value handling than the runtime routing helpers.
Fix
Added a real dropdown window helper with preserved scroll-start state, used it in keyboard routing, mouse routing, and overlay rendering, and rendered a scrollbar gutter when menus overflow. Added focused tests for overflow windowing, scrollbar hit-testing, renderer visibility, and shortcut callback isolation, and updated the dropdown docs to match the shipped shortcut contract.
Tests
npm run lint
npm run typecheck
npm run build
node scripts/run-tests.mjs --filter dropdown
node scripts/run-tests.mjs --filter overlay.edge
node scripts/run-tests.mjs
Notes
This change is scoped to dropdown behavior and the shared overlay plumbing needed to keep dropdown rendering and routing internally consistent.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation