fix(widgets): handle dropdown viewport overflow and warn on missing anchor#174
Merged
RtlZeroMemory merged 3 commits intomainfrom Feb 23, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughImplements scroll-aware dropdown sizing that clamps height to the viewport and budgets width for a scrollbar when needed, and adds a development-only warning for missing dropdown anchors; includes tests for geometry behavior and the anchor-missing warning. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33663aefda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/layout/__tests__/dropdownGeometry.test.ts`:
- Around line 1-4: Imports in dropdownGeometry.test.ts are out of order per
Biome organizeImports; open the file and reorder the import statements (the ones
referencing computeDropdownGeometry, Rect, DropdownProps and the test utilities
import from "@rezi-ui/testkit") to match your project's Biome import ordering
rules, or simply run the Biome organizer/formatter (e.g., biome format or biome
organizeImports) to automatically fix the ordering and save the file.
In `@packages/core/src/layout/dropdownGeometry.ts`:
- Around line 31-37: When computing finalW for the overlay, guard against the
scrollbar-expanded width exceeding the viewport: if needsScroll is true, compute
the expanded width (totalW + 1) but clamp it to viewport.cols (e.g.,
Math.min(viewport.cols, totalW + 1)) so finalW never surpasses the visible
columns; update the calculation that sets finalW (referencing needsScroll,
totalW, viewport.cols, and finalW used in calculateAnchorPosition) to apply this
clamp.
In `@packages/core/src/renderer/renderToDrawlist/widgets/overlays.ts`:
- Around line 199-204: The warning block using DEV_MODE, anchor, props.anchorId
and warnDev needs Biome formatting; run the repo formatter (e.g., `biome check
--fix --unsafe` or the project's format command) to reformat the block around
the conditional that computes dropdownId and calls warnDev so the CI formatting
checks pass.
In `@packages/core/src/runtime/__tests__/overlayUnification.test.ts`:
- Around line 192-218: The test "warns when dropdown anchor id is missing"
currently instantiates WidgetRenderer directly; replace that with
createTestRenderer() from the testing module so the test uses the standard test
harness. Specifically, in this test swap new WidgetRenderer<void>({...}) for a
call to createTestRenderer() (ensuring it provides the same noop
backend/requestRender semantics the original used), keep the rest of the test
flow (submit, ui.dropdown with id "menu" and anchorId "missing-anchor",
captureConsoleWarn) unchanged, and run the same assertion to verify the warning
message.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/layout/__tests__/dropdownGeometry.test.ts (2)
71-82: Good minimum bounds test.Validates the
Math.max(2, ...)guards in the implementation produce correct minimum geometry (3×2) for empty item lists.Consider adding edge case tests for null anchor and zero-dimension viewport to complete coverage of the early-return paths in
computeDropdownGeometry.📝 Additional edge case tests
test("returns null when anchor is null", () => { const props = dropdownProps([{ id: "a", label: "item" }]); const viewport = { cols: 40, rows: 20 }; const rect = computeDropdownGeometry(props, null, viewport); assert.equal(rect, null); }); test("returns null when viewport has zero dimensions", () => { const props = dropdownProps([{ id: "a", label: "item" }]); const anchor: Rect = { x: 0, y: 0, w: 1, h: 1 }; assert.equal(computeDropdownGeometry(props, anchor, { cols: 0, rows: 20 }), null); assert.equal(computeDropdownGeometry(props, anchor, { cols: 40, rows: 0 }), null); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/__tests__/dropdownGeometry.test.ts` around lines 71 - 82, Add unit tests covering the early-return branches of computeDropdownGeometry: one test that passes a null anchor and asserts the function returns null (use dropdownProps with at least one item and a normal viewport), and one test that calls computeDropdownGeometry with an anchor and viewports having cols === 0 and rows === 0 respectively and asserts null is returned; place these tests alongside the existing cases in dropdownGeometry.test.ts so computeDropdownGeometry, dropdownProps, and the Rect/viewport shapes are imported/used consistently.
16-32: Tests correctly verify scroll-aware geometry calculations.The test validates that:
- Height is clamped to viewport rows (20) when items exceed viewport
- Width includes +1 for scrollbar (6 → 7) when scrolling is needed
Minor style suggestion:
assert.equal(rect.h <= viewport.rows, true)could be simplified.💅 Prefer assert.ok() for boolean conditions
- assert.equal(rect.h <= viewport.rows, true); + assert.ok(rect.h <= viewport.rows, "height should be clamped to viewport");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/__tests__/dropdownGeometry.test.ts` around lines 16 - 32, The boolean assertion in the test "clamps dropdown height to viewport and budgets width for scrollbar" is verbose; replace the line that currently uses assert.equal(rect.h <= viewport.rows, true) with a direct boolean assertion using assert.ok(rect.h <= viewport.rows) to simplify and follow style conventions—you can find this in the test that constructs items, calls computeDropdownGeometry(props, anchor, viewport) and inspects rect.h and rect.w.
🤖 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/layout/__tests__/dropdownGeometry.test.ts`:
- Around line 71-82: Add unit tests covering the early-return branches of
computeDropdownGeometry: one test that passes a null anchor and asserts the
function returns null (use dropdownProps with at least one item and a normal
viewport), and one test that calls computeDropdownGeometry with an anchor and
viewports having cols === 0 and rows === 0 respectively and asserts null is
returned; place these tests alongside the existing cases in
dropdownGeometry.test.ts so computeDropdownGeometry, dropdownProps, and the
Rect/viewport shapes are imported/used consistently.
- Around line 16-32: The boolean assertion in the test "clamps dropdown height
to viewport and budgets width for scrollbar" is verbose; replace the line that
currently uses assert.equal(rect.h <= viewport.rows, true) with a direct boolean
assertion using assert.ok(rect.h <= viewport.rows) to simplify and follow style
conventions—you can find this in the test that constructs items, calls
computeDropdownGeometry(props, anchor, viewport) and inspects rect.h and rect.w.
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
Testing
Summary by CodeRabbit