fix: align side menu drag handle with table top (#2604)#2679
fix: align side menu drag handle with table top (#2604)#2679betsyschultz wants to merge 2 commits intoTypeCellOS:mainfrom
Conversation
The side menu's drag handle floated ~8px above the visible top of table blocks because Floating UI anchored to the block's outer `.bn-block` element, ignoring the `.tableWrapper` child's intentional top padding (~9px) — that padding reserves space for in-block row/column handles. Add a `tableWrapperOffset` Floating UI offset middleware that locates the wrapper at its known DOM path (`.bn-block-content > .tableWrapper`) and shifts the popover down by the wrapper's computed `padding-top`. The middleware is a no-op when no wrapper is present, so paragraph, heading, list, image, code alignment is unchanged. The helper is exported from `@blocknote/react` so consumers who supply their own `middleware` array via `floatingUIOptions.useFloatingOptions` can re-compose it: `middleware: [offset(tableWrapperOffset), ...other]`. Includes a regression test asserting the handle's y-coord stays within 5px of the table element's y-coord.
|
Someone is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds a Floating UI middleware Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/src/end-to-end/draghandle/draghandle.test.ts (2)
60-60: 5px tolerance is reasonable but loose — consider tightening if it proves stable.The pre-fix delta was ~8px, so
<= 5correctly distinguishes fixed vs. broken. Post-fix the actual delta should be sub-pixel. If the test proves stable across browsers in CI, tightening to e.g.<= 2would catch future drift earlier; otherwise leave as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/end-to-end/draghandle/draghandle.test.ts` at line 60, The assertion tolerance in the test uses a 5px threshold which is looser than needed; update the expectation expression that compares handleY and tableY (the expect(Math.abs(handleY - tableY)).toBeLessThanOrEqual(...) call) to use a tighter threshold (e.g., 2) by replacing 5 with 2 so the line becomes toBeLessThanOrEqual(2); if this fails intermittently in CI after the change, revert to the original 5 until stability is achieved.
49-61: Minor: clarify the boundingBox null-handling with an explicit check.Line 54 uses
(...)!.y— a non-null assertion. IfboundingBox()ever returnsnull(element detached/invisible), the failure surface is a crypticTypeErrorinstead of a clear assertion. An explicitexpect(box).not.toBeNull()reads better in CI logs.Clearer null check
- const tableY = (await page.locator(`${TABLE_SELECTOR} table`).boundingBox())!.y; + const tableBox = await page.locator(`${TABLE_SELECTOR} table`).boundingBox(); + expect(tableBox).not.toBeNull(); + const tableY = tableBox!.y;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/end-to-end/draghandle/draghandle.test.ts` around lines 49 - 61, The test "Draghandle should align vertically with the top of a table block" uses a non-null assertion on the result of page.locator(`${TABLE_SELECTOR} table`).boundingBox() — replace that with an explicit null check: capture the boundingBox into a variable (e.g., box), assert expect(box).not.toBeNull() and then use box!.y (or destructure after the assertion) when computing tableY; this change should be applied around the code that calls boundingBox() in the test and references TABLE_SELECTOR so failures show a clear assertion rather than a TypeError.packages/react/src/components/SideMenu/SideMenuController.tsx (1)
104-104: Confirm consumers passing custommiddlewarestill gettableWrapperOffsetapplied.The spread on Line 105 (
...props.floatingUIOptions?.useFloatingOptions) means a consumer passing their ownmiddlewarearray will fully replace the[offset(tableWrapperOffset)]default and lose the table-alignment fix. The exportedtableWrapperOffsetlets them re-compose it, but this is an easy footgun. Consider either:
- documenting the requirement to re-include
offset(tableWrapperOffset)near the export, or- merging arrays so the default is preserved.
Optional preserve-default approach
- useFloatingOptions: { - open: show, - placement: "left-start", - whileElementsMounted, - middleware: [offset(tableWrapperOffset)], - ...props.floatingUIOptions?.useFloatingOptions, - }, + useFloatingOptions: { + open: show, + placement: "left-start", + whileElementsMounted, + ...props.floatingUIOptions?.useFloatingOptions, + middleware: [ + offset(tableWrapperOffset), + ...(props.floatingUIOptions?.useFloatingOptions?.middleware ?? []), + ], + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/SideMenu/SideMenuController.tsx` at line 104, The current middleware assignment in SideMenuController overwrites the default offset(tableWrapperOffset) when a consumer provides floatingUIOptions.useFloatingOptions.middleware; update the middleware composition so the default offset(tableWrapperOffset) is merged with any consumer-provided array (e.g., combine props.floatingUIOptions?.useFloatingOptions?.middleware with offset(tableWrapperOffset) instead of replacing it) ensuring tableWrapperOffset remains applied; touch SideMenuController where middleware is set and use tableWrapperOffset symbol to compose the final middleware array (or alternatively add a clear comment near the exported tableWrapperOffset documenting that consumers must include it).
🤖 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/react/src/components/SideMenu/SideMenuController.tsx`:
- Line 104: The current middleware assignment in SideMenuController overwrites
the default offset(tableWrapperOffset) when a consumer provides
floatingUIOptions.useFloatingOptions.middleware; update the middleware
composition so the default offset(tableWrapperOffset) is merged with any
consumer-provided array (e.g., combine
props.floatingUIOptions?.useFloatingOptions?.middleware with
offset(tableWrapperOffset) instead of replacing it) ensuring tableWrapperOffset
remains applied; touch SideMenuController where middleware is set and use
tableWrapperOffset symbol to compose the final middleware array (or
alternatively add a clear comment near the exported tableWrapperOffset
documenting that consumers must include it).
In `@tests/src/end-to-end/draghandle/draghandle.test.ts`:
- Line 60: The assertion tolerance in the test uses a 5px threshold which is
looser than needed; update the expectation expression that compares handleY and
tableY (the expect(Math.abs(handleY - tableY)).toBeLessThanOrEqual(...) call) to
use a tighter threshold (e.g., 2) by replacing 5 with 2 so the line becomes
toBeLessThanOrEqual(2); if this fails intermittently in CI after the change,
revert to the original 5 until stability is achieved.
- Around line 49-61: The test "Draghandle should align vertically with the top
of a table block" uses a non-null assertion on the result of
page.locator(`${TABLE_SELECTOR} table`).boundingBox() — replace that with an
explicit null check: capture the boundingBox into a variable (e.g., box), assert
expect(box).not.toBeNull() and then use box!.y (or destructure after the
assertion) when computing tableY; this change should be applied around the code
that calls boundingBox() in the test and references TABLE_SELECTOR so failures
show a clear assertion rather than a TypeError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb7c2902-6054-4c8e-bb42-3d1010b1536d
📒 Files selected for processing (2)
packages/react/src/components/SideMenu/SideMenuController.tsxtests/src/end-to-end/draghandle/draghandle.test.ts
Compose default offset middleware with consumer-provided middleware in SideMenuController so the tableWrapperOffset isn't silently overwritten when consumers pass their own floatingUIOptions.middleware. Tighten regression test tolerance from 5px to 2px — post-fix delta is sub-pixel by construction (offset compensates the exact padding value). Skipped CodeRabbit's suggestion to replace the boundingBox non-null assertion with an explicit expect().not.toBeNull() — the inline `!` pattern is consistent across the test suite (colors, images, copypaste, indentation, ai-selection, keyboardhandlers all use it).
|
Thanks @coderabbitai — addressed two of the three nitpicks in 84b7cc9: 1. Middleware override (SideMenuController.tsx:104) — fixed. The default useFloatingOptions: {
...
...props.floatingUIOptions?.useFloatingOptions,
middleware: [
offset(tableWrapperOffset),
...(props.floatingUIOptions?.useFloatingOptions?.middleware ?? []),
],
}Worth noting the same latent bug exists in 2. Test tolerance (draghandle.test.ts:60) — tightened from 5px → 2px. The fix subtracts the exact 3. Non-null assertion on |
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react/src/components/SideMenu/SideMenuController.tsx (2)
105-108: Consumer-providedoffsetmiddleware will stack, not override.Because
tableWrapperOffsetis hardcoded as the first entry, a consumer who passes their ownoffset(...)viaprops.floatingUIOptions.useFloatingOptions.middlewarewill end up with both running (Floating UI stacks multipleoffsetmiddlewares additively). Per the PR description, the intent is that consumers re-compose using the exportedtableWrapperOffset, but there's no way to opt out of the prepended default if they're unaware. Consider documenting onSideMenuController/floatingUIOptionsthat consumers overridingmiddlewareshould explicitly includeoffset(tableWrapperOffset)and that a top-leveloffsetthey pass will compound.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/SideMenu/SideMenuController.tsx` around lines 105 - 108, The current middleware array always prepends offset(tableWrapperOffset), causing consumer-provided offset(...) to stack; update SideMenuController so you first pull consumer middleware from props.floatingUIOptions.useFloatingOptions.middleware, check whether that array already contains an offset middleware (e.g., detect by middleware.name === 'offset' or equivalent), and only prepend offset(tableWrapperOffset) when no offset is present; reference the symbols tableWrapperOffset and props.floatingUIOptions.useFloatingOptions.middleware when making this change and add a short comment or docs note on SideMenuController that consumers who want the default behavior should include offset(tableWrapperOffset) themselves if they override middleware.
29-45: Consider adding a placement-aware guard for defensive extensibility.The
:scope > .bn-block-content > .tableWrapperselector andVirtualElement.contextElementfallback are correct.parseFloatonpaddingTopis safe, and thepadding > 0guard handles theNaNedge case implicitly.Minor consideration: The function assumes
placement: "left-start"(documented in lines 26–27), and while the placement is hardcoded in the default config, consumers who export and reusetableWrapperOffsetwith different placements would encounter misaligned offsets. SincecrossAxismaps to the vertical axis only for left/right placements, adding a placement check would make this middleware more robust:- return padding > 0 ? { mainAxis: 0, crossAxis: padding } : 0; + if (!(padding > 0)) { + return 0; + } + // crossAxis is vertical only for left/right placements + const side = state.placement.split("-")[0]; + return (side === "left" || side === "right") ? { mainAxis: 0, crossAxis: padding } : 0;This prevents silent misalignment if the middleware is reused outside its intended placement context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/SideMenu/SideMenuController.tsx` around lines 29 - 45, tableWrapperOffset currently assumes a left/right placement mapping (it returns crossAxis as vertical padding) which can misalign if reused for other placements; update the tableWrapperOffset(state) function to read state.placement (or accept placement) and short-circuit/return 0 for placements where crossAxis is not the vertical axis (e.g., "top", "bottom" or any placement that doesn't map crossAxis to vertical), otherwise compute and return the existing { mainAxis: 0, crossAxis: padding } result; reference the tableWrapperOffset function and the state.placement property when making this defensive placement-aware guard.
🤖 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/react/src/components/SideMenu/SideMenuController.tsx`:
- Around line 105-108: The current middleware array always prepends
offset(tableWrapperOffset), causing consumer-provided offset(...) to stack;
update SideMenuController so you first pull consumer middleware from
props.floatingUIOptions.useFloatingOptions.middleware, check whether that array
already contains an offset middleware (e.g., detect by middleware.name ===
'offset' or equivalent), and only prepend offset(tableWrapperOffset) when no
offset is present; reference the symbols tableWrapperOffset and
props.floatingUIOptions.useFloatingOptions.middleware when making this change
and add a short comment or docs note on SideMenuController that consumers who
want the default behavior should include offset(tableWrapperOffset) themselves
if they override middleware.
- Around line 29-45: tableWrapperOffset currently assumes a left/right placement
mapping (it returns crossAxis as vertical padding) which can misalign if reused
for other placements; update the tableWrapperOffset(state) function to read
state.placement (or accept placement) and short-circuit/return 0 for placements
where crossAxis is not the vertical axis (e.g., "top", "bottom" or any placement
that doesn't map crossAxis to vertical), otherwise compute and return the
existing { mainAxis: 0, crossAxis: padding } result; reference the
tableWrapperOffset function and the state.placement property when making this
defensive placement-aware guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 793abc7e-5dc9-4f60-9386-b8ccd90848d4
📒 Files selected for processing (2)
packages/react/src/components/SideMenu/SideMenuController.tsxtests/src/end-to-end/draghandle/draghandle.test.ts
1 participant
PR Draft — fix: align side menu drag handle with table top (#2604)
What
Closes #2604. The side menu drag handle (and the
+add button next to it) now align with the visible top of a table block instead of floating ~8px above it.Why it was broken
SideMenuControlleruses Floating UI withplacement: "left-start", which anchors the popover's top edge to the reference element's top edge. The reference is the block's outer wrapper (bn-block-content).For text blocks (paragraph, heading, list, code) this looks correct because the inline content sits flush at the top of the wrapper.
For table blocks the DOM is:
.tableWrapper's top padding is intentional — it reserves vertical space for the in-block row handles (the small drag dots that appear above each row on hover). That padding pushes the visible<table>element 9px below the wrapper top, but the side menu had no awareness of it.Measured before fix (initial empty editor, fresh table): drag handle top =
12px, table element top =20px→ 8px gap.The fix
Add a Floating UI
offsetmiddleware that, when the reference element contains a direct.tableWrapperchild, shifts the popover down by that wrapper's computedpadding-top. The middleware is a no-op for any reference without a.tableWrapperchild, so non-table blocks are unaffected.The padding value is read from
getComputedStylerather than hardcoded so that the fix tracks the--bn-table-handle-sizeCSS variable automatically.After fix: drag handle top =
21px, table element top =20px→ 1px gap (matches paragraph alignment, where the handle SVG is intentionally 1px below the inline content top).Files changed
packages/react/src/components/SideMenu/SideMenuController.tsx(+22 / −0)tests/src/end-to-end/draghandle/draghandle.test.ts(+24 / −1)Test plan
draghandle.test.ts— assertsMath.abs(handleY - tableY) <= 5. Fails onmain(8px gap), passes with the fix (1px gap).pnpm exec tsc --noEmitclean for@blocknote/reactnx lint @blocknote/reactclean (--max-warnings 0)nx build @blocknote/reactsucceedsWhat was NOT run locally
The full E2E Playwright suite (
tests/) was not run locally — relies on CI for the cross-browser snapshot suite. The added regression test follows the same conventions as the other tests indraghandle.test.ts(usesexecuteSlashCommand,moveMouseOverElement,boundingBox()).Edge cases considered
.tableWrapper:scope > .tableWrapper(direct child only) prevents an outer block's reference from picking up a nested table's wrapper.tableWrapper(or by extension a similarly named wrapper) gets correct alignment automaticallyScreenshots
After fix (drag handle aligned with top of table):
(Run the playground at
pnpm dev→/basic/minimal→/tableEnter, then hover the table to reproduce.)Notes for reviewer
--bn-table-handle-sizeis themed.tableWrapperOffsetis exported from@blocknote/reactso consumers who pass their ownmiddleware: [...]viafloatingUIOptions.useFloatingOptionscan re-compose it:middleware: [offset(tableWrapperOffset), ...other]. Without this, customisingmiddlewarewould silently overwrite the fix (this matches the existing spread pattern inFloatingComposerControlleretc., so the footgun isn't new — but the helper makes it recoverable).placement: "left-start"(the default for the side menu). Consumers who change placement keep the offset, butcrossAxismay map to a different visual axis depending on placement; documented in the helper's JSDoc.tableWrapper's widget-spacing from padding to absolute-positioned widgets), that's a larger architectural change — let me know and I can spin a separate PR.Summary by CodeRabbit