fix: bi-directional links show button overlapping with database view settings#14710
fix: bi-directional links show button overlapping with database view settings#14710ibex088 wants to merge 9 commits intotoeverything:canaryfrom
Conversation
📝 WalkthroughWalkthroughThree files were modified to fix a z-index layering issue affecting UI element visibility and adjust popup menu positioning behavior. Changes include CSS styling updates, popup anchor configuration adjustments, and simplification of language change handling in settings. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #14710 +/- ##
==========================================
- Coverage 57.33% 56.78% -0.55%
==========================================
Files 3092 3092
Lines 167614 167614
Branches 24730 24592 -138
==========================================
- Hits 96095 95184 -911
- Misses 68439 69272 +833
- Partials 3080 3158 +78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ibex088
changed the title
There was a problem hiding this comment.
This fixes the data-view “View settings” popup layering issue where the bi-directional links “Show” button could appear above the menu. The root cause was that this popup was being mounted inside the local editor stacking context, so its z-index could still lose to sibling UI. This change only lifts the view-settings popup flow to document.body, which keeps the fix scoped to this menu family instead of changing popup behavior globally.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts`:
- Around line 77-83: The code is forcing the modal container to document.body in
the popViewOptions call which bypasses the scoped root selection from
getDefaultModalRoot; change the root passed into popViewOptions to be derived
from the computed default modal root instead of hardcoding document.body (e.g.,
compute const root = getDefaultModalRoot(target.root ?? document.body) or call
getDefaultModalRoot() and fall back to document.body only if that returns null),
then pass { ...target, root } and keep this.dataViewLogic; this preserves
scoped/embedded contexts while still allowing a fallback for z-index fixes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2849c8c2-f782-46c1-bf68-45928a700080
📒 Files selected for processing (1)
blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts
| popViewOptions( | ||
| { | ||
| ...target, | ||
| root: document.body, | ||
| }, | ||
| this.dataViewLogic | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect PopupTarget construction and popup target helper implementation.
rg -n -C3 "popupTargetFromElement|type PopupTarget" --type ts --type js
# 2) Confirm popup root is used as the modal container.
rg -n -C3 "createModal\\(target\\.root\\)|target\\.root" --type ts --type js blocksuite/affine/components/src/context-menu
# 3) Find other places overriding root to document.body (to assess consistency/risk).
rg -n -C2 "root:\\s*document\\.body" --type ts --type jsRepository: toeverything/AFFiNE
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find PopupTarget type definition
echo "=== PopupTarget Type Definition ==="
rg -n "type PopupTarget|interface PopupTarget" --type ts --type js -A 5
# Find popupTargetFromElement implementation
echo -e "\n=== popupTargetFromElement Implementation ==="
rg -n "export.*popupTargetFromElement|function popupTargetFromElement" --type ts --type js -A 10 | head -60
# Look for where root: document.body pattern appears
echo -e "\n=== root: document.body Overrides ==="
rg -n "root:\s*document\.body" --type ts --type js
# Check the specific file context around lines 77-83
echo -e "\n=== File Context around lines 70-85 ==="
sed -n '70,85p' blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.tsRepository: toeverything/AFFiNE
Length of output: 14173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the complete popupTargetFromElement implementation
echo "=== Complete popupTargetFromElement Implementation ==="
sed -n '407,430p' blocksuite/affine/components/src/context-menu/menu-renderer.ts
# Check what popViewOptions does with the root parameter
echo -e "\n=== popViewOptions Implementation/Usage ==="
rg -n "popViewOptions|export.*popViewOptions" --type ts --type js -A 15 | head -80
# Check if there are any comments or tests explaining the root behavior
echo -e "\n=== Context around PopupTarget root usage ==="
rg -n "\.root" blocksuite/affine/components/src/context-menu/menu-renderer.ts -B 2 -A 2 | head -40Repository: toeverything/AFFiNE
Length of output: 5555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find getDefaultModalRoot implementation
echo "=== getDefaultModalRoot Implementation ==="
rg -n "getDefaultModalRoot|function getDefaultModalRoot|export.*getDefaultModalRoot" --type ts --type js -A 20 | head -60
# Check if there are shadow DOM or scoped root contexts in use
echo -e "\n=== Shadow DOM or scoped root patterns ==="
rg -n "shadowRoot|getRootNode" blocksuite/affine/components/src/context-menu --type ts --type js | head -20Repository: toeverything/AFFiNE
Length of output: 4138
Override modal container carefully to preserve scoped root logic.
Line 80 hardcodes the popup container to the global document.body, which bypasses the scoped root selection from getDefaultModalRoot (which already checks for editor-host and .data-view-popup-container contexts). This can cause placement/layering regressions in scoped or embedded document contexts. If forcing the body is required for the z-index fix, derive it from the computed root instead.
Proposed change
popViewOptions(
{
...target,
- root: document.body,
+ root: target.root.ownerDocument?.body ?? target.root,
},
this.dataViewLogic
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts`
around lines 77 - 83, The code is forcing the modal container to document.body
in the popViewOptions call which bypasses the scoped root selection from
getDefaultModalRoot; change the root passed into popViewOptions to be derived
from the computed default modal root instead of hardcoding document.body (e.g.,
compute const root = getDefaultModalRoot(target.root ?? document.body) or call
getDefaultModalRoot() and fall back to document.body only if that returns null),
then pass { ...target, root } and keep this.dataViewLogic; this preserves
scoped/embedded contexts while still allowing a fallback for z-index fixes.
There was a problem hiding this comment.
Current behavior is intentionally narrow: only the data-view “View settings” popup flow overrides root to document.body. We do that because the default scoped root resolves to the nearest editor-host, and editor-host creates an isolated stacking context, which is the reason this menu can render underneath sibling UI like the bi-directional links “Show” button. Re-deriving the root from getDefaultModalRoot(...) here would usually put the popup back into editor-host and reintroduce the original layering bug.
There was a problem hiding this comment.
@ibex088, understood — thanks for the detailed explanation. The isolated stacking context created by editor-host is exactly the kind of implicit constraint that's hard to infer from the code alone. Fixing the root to document.body specifically in this popup flow to escape that stacking context is the right targeted approach, and re-deriving via getDefaultModalRoot would indeed risk re-scoping to editor-host and reverting the fix.
I've noted this for future reviews.
( ˘▽˘)っ♨
✏️ Learnings added
Learnt from: ibex088
Repo: toeverything/AFFiNE PR: 14710
File: blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts:77-83
Timestamp: 2026-04-03T09:24:14.105Z
Learning: In `blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts`, the `openMoreAction` method in `DataViewHeaderToolsViewOptions` intentionally overrides `root` to `document.body` when calling `popViewOptions`. This is by design: the default scoped root resolves to the nearest `editor-host` element, which creates an isolated stacking context. That stacking context causes the "View settings" popup to render beneath sibling UI (e.g., the bi-directional links "Show/Hide" button). Hardcoding `document.body` as the root escapes that stacking context. Do NOT suggest replacing this with `getDefaultModalRoot()` or a derived root, as that would typically re-scope back to `editor-host` and reintroduce the original layering bug (GitHub issue `#14723`).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/frontend/core/src/desktop/dialogs/setting/index.tsx (1)
197-197:⚠️ Potential issue | 🔴 CriticalRemove undefined
currentLanguageKeyidentifier fromFrameworkScopekey prop.The identifier
currentLanguageKeyis referenced at line 197 but is neither declared nor imported in this file, causing a TypeScript compilation error. ThecurrentServerIdalone is sufficient for change detection on theFrameworkScopecomponent.Proposed fix
- <FrameworkScope - key={`setting-modal-${currentServerId}-${currentLanguageKey}`} - scope={currentServer.scope} - > + <FrameworkScope + key={`setting-modal-${currentServerId}`} + scope={currentServer.scope} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/core/src/desktop/dialogs/setting/index.tsx` at line 197, Remove the undefined identifier from the key prop on FrameworkScope: replace key={`setting-modal-${currentServerId}-${currentLanguageKey}`} with a key that uses only the declared currentServerId (e.g., key={`setting-modal-${currentServerId}`}) so the component no longer references the undeclared currentLanguageKey; update the usage in the FrameworkScope instantiation (FrameworkScope key prop) accordingly and run TypeScript to verify the compilation error is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/frontend/core/src/desktop/dialogs/setting/index.tsx`:
- Line 197: Remove the undefined identifier from the key prop on FrameworkScope:
replace key={`setting-modal-${currentServerId}-${currentLanguageKey}`} with a
key that uses only the declared currentServerId (e.g.,
key={`setting-modal-${currentServerId}`}) so the component no longer references
the undeclared currentLanguageKey; update the usage in the FrameworkScope
instantiation (FrameworkScope key prop) accordingly and run TypeScript to verify
the compilation error is resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 475dda9c-c051-486c-9cc8-821d23de1103
📒 Files selected for processing (1)
packages/frontend/core/src/desktop/dialogs/setting/index.tsx
darkskygit
force-pushed
the
fix/bi-directional-links-show-button-z-index
branch
from
d6ebb1e to
ce188b9
Compare
2 participants
Before
fixes #14723
Screen.Recording.2026-03-23.at.1.49.59.PM.mov
After
Screen.Recording.2026-03-23.at.1.49.28.PM.mov
Summary by CodeRabbit
Bug Fixes
Refactor