fix(list): water-fill layout engine + anchor relation menus to click position#2239
fix(list): water-fill layout engine + anchor relation menus to click position#2239yasyuk wants to merge 4 commits into
Conversation
In list (and gallery/board) views, cells like Author and Status sit near the right edge of the row. With horizontal:Left, the menu computes the cell's left edge and flips leftward when it doesn't fit, placing the popup far from where the user clicked. For noInplace cells in non-grid views, recalcRect now returns the click's x coordinate so the menu always opens near the clicked value. The y coordinate still uses the cell's bounding rect so the menu appears just below the row. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
I have read the CLA Document and I hereby sign the CLA. |
requilence
left a comment
There was a problem hiding this comment.
Review focused on correctness, layout, and perf.
Scope mismatch: the title/description and test plan cover only the menu-anchoring change in cell/index.tsx (16 lines), but this commit also adds a ~110-line water-fill layout engine in row.tsx + new SCSS. The merge base (05dd906b25) has no such layout — it's entirely new here, undocumented and untested. The menu fix itself is correct and low-risk; recommend splitting the layout work into its own PR with its own description/test plan, or expanding this one to cover it.
Tests: none added. Per CLAUDE.md, menu/list changes should get /qa-engineer E2E coverage; SCSS changes should pass /dark-mode-check.
Inline comments below.
| // Cap each measured width at the CSS .name max-width (300px). | ||
| // Without this, text cells (description) measure at full text width and get | ||
| // assigned more space than they can display, creating an empty gap. | ||
| const naturalCapped = natural.map(w => Math.min(w, 300)); |
There was a problem hiding this comment.
naturalCapped (300px cap) is computed only for the total <= available early-return — the distribution loop below (L116-130) uses uncapped natural for both the fit-test and assigned[idx] = natural[idx]. So a cell with natural width in (300, share] still gets assigned its full width while .name { max-width: 300px } clips the content → the exact empty gap this comment says it prevents. Either drive the distribution from naturalCapped, or drop it as dead/misleading code.
|
|
||
| const sidesWidth = sidesEl.offsetWidth; | ||
| // Respect the CSS min-width:40% on the left side. | ||
| const effectiveLeft = Math.max(leftNatural, sidesWidth * 0.4); |
There was a problem hiding this comment.
Hardcoded 0.4 assumes the left side is ≥40%, but the non-grid branch renders s60 when left.length > 1, and list.scss pins .side.left.s60 { width: 60% }. When the name isn't the first relation, the real left side is 60% → available is overestimated → right cells are over-allocated and clipped by overflow:hidden. Branch on s60 (or read the rendered left width) instead of a fixed 0.4.
| }); | ||
| }; | ||
|
|
||
| useEffect(() => resize()); |
There was a problem hiding this comment.
No dependency array → resize() runs after every render of every row. In the non-inline path this is react-virtualized, which re-renders/recycles rows on each scroll tick and on AutoSizer width changes. Each run forces sync reflows and deep-clones the right-side subtree into <body> (L79). That's layout-thrashing on large lists / fast scroll. Gate with deps, a ResizeObserver on width, or debounce.
| return; | ||
| }; | ||
|
|
||
| leftSideEl.style.flex = '0 0 auto'; |
There was a problem hiding this comment.
Minor: this mutates live DOM (set/read/reset flex) to measure the left side, which both adds 2 synchronous reflows and contradicts the clone rationale at L74-77 ("never mutate live elements"). Also note sidesEl is used in U.Dom.select(..., sidesEl) above before its null check on L60.
| const rect = el?.getBoundingClientRect(); | ||
| return { | ||
| x: clickX, | ||
| y: rect ? rect.top + window.scrollY : clickY, |
There was a problem hiding this comment.
Mixed coordinate spaces: x: clickX is viewport-relative (no scrollX) while y: rect.top + window.scrollY is document-relative, and the clickY fallback is viewport-relative again. Harmless today only because this list scrolls an inner container (window.scrollY ≈ 0), but inconsistent with the surrounding fallback ox = elRect.left + window.scrollX and a latent bug if window scrolling ever applies. Align all three.
| .element { overflow: hidden; max-width: 100%; } | ||
| .tagItem { overflow: hidden; max-width: 100%; } | ||
| } | ||
| .element .iconObject { display: none; } |
There was a problem hiding this comment.
This hides object/relation icons (e.g. Author avatars) on the right side of list rows — an undescribed visual change. Per CLAUDE.md, design properties shouldn't change without an explicit spec. Confirm this is intended JS-9573 behavior.
- Fix coordinate spaces in cell recalcRect: align x/y to document coords - Replace live DOM flex mutation with leftSideEl.offsetWidth (also fixes TypeScript bug where leftSideEl.flex reset was a no-op) - Drive water-fill distribution entirely from naturalCapped (fixes over-allocation to cells whose visual cap is 300px) - Guard style.flex clear to only run when flex was previously set, preventing a spurious second ResizeObserver callback on every tick - Attach ResizeObserver to .sides container instead of the full row to avoid triggering on framer-motion animations and drag/selection changes - Move resizeRef update into useLayoutEffect for React 18 concurrent safety - Fix dead .side.left.s50 hover rule to .side.left.s60 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
I have read the CLA Document and I hereby sign the CLA. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
requilence
left a comment
There was a problem hiding this comment.
Re-reviewed 6863e53b63. All six findings from the previous round are resolved correctly:
- ✅ 300px cap now drives the distribution (
naturalCappedused in sort, fit-test and assignment) — no more over-allocation/gap. - ✅ 40%/60% mismatch removed;
availableis now derived from the realleftSideEl.offsetWidth(the CSSmin-width:40%enforces the floor naturally). Bonus: the dead.side.left.s50hover selector was corrected to.s60. - ✅ Perf: render-driven
useEffect(resize)replaced with a one-timeResizeObserver+resizeRef, plus a guard so the flex-clear doesn't re-trigger the observer. - ✅ Live-DOM mutation for left measurement gone;
sidesElnull-checked before use. - ✅ Coordinate spaces aligned (
+ window.scrollX/+ window.scrollY). - ✅
.element .iconObject { display:none }reverted.
One new trade-off to confirm + a nit inline. Still open from round 1: no tests (menu/list → /qa-engineer), and the PR description still only covers the menu fix while the layout engine is the bulk of the diff. The CLAUDE.md cleanup in e8fdb7a2bf is unrelated scope (benign).
LGTM once the ResizeObserver trade-off below is confirmed acceptable.
| return; | ||
| }; | ||
| const target = (U.Dom.select('.sides', node) as HTMLElement) || node; | ||
| const ro = new ResizeObserver(() => resizeRef.current()); |
There was a problem hiding this comment.
Trade-off to confirm: .sides is width:100% with a fixed row height, so the ResizeObserver only fires on container width changes — never on content changes inside a stable-width row. The previous useEffect(resize) re-ran on every render and so re-flowed after an in-place value edit (e.g. changing a Status/tag to a wider value). Now that won't recompute the water-fill until the next container resize, so flex-basis can go stale after editing a relation value in List/Gallery/Board. If that's a real scenario here, also re-run on the relevant record/relation changes. If in-place edits can't change right-cell widths in these views, ignore.
Nit: resizeRef.current() has no guard — resizeRef.current?.() is safer.
- Add useEffect keyed on right-side values so water-fill recomputes after in-place relation edits (ResizeObserver alone misses this since .sides width is stable when only cell content changes) - Fix resizeRef.current?.() guard in ResizeObserver callback Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Menu anchoring (JS-9573)
In list/gallery/board views, relation cells near the right edge of the row were computing the cell's left edge for menu placement (e.g. x=1394 for a Status cell), causing the popup to flip leftward ~400px away from the click. Added
recalcRectfornoInplacecells in non-grid views: horizontal placement uses the click x-coordinate, vertical uses the cell's bounding rect. Menu now opens near where the user clicked regardless of cell position in the flex layout.Also fixed coordinate space inconsistency:
x/yare now both document-relative (clientX + scrollX,rect.top + scrollY), consistent with the rest of the menu positioning code.Water-fill layout engine for List view right side
The right side of a List row can contain multiple relation cells (Status, Author, Tags, etc.) with no fixed widths. Without distribution, all cells were full-width and clipped or hidden by overflow. Added a water-fill algorithm that distributes the available right-side space proportionally:
.name { max-width: 300px })ResizeObserveron the.sidescontainer (fires on container width changes) and a separateuseEffectkeyed on right-side relation values (fires after in-place edits that change cell content but not container width)Performance and correctness fixes (post code-review)
leftSideEl.offsetWidthdirectly instead of temporarily mutatingleftSideEl.style.flexto measure (the mutation was also a TypeScript no-op:flexis not anHTMLElementproperty)naturalCapped— previouslynaturalCappedwas used only for the early-exit total check while the loop used uncapped values, over-allocating space to cells with a 300px visual capstyle.flex = ''clear to only run when flex was previously set, preventing a spurious second ResizeObserver callback on every tickResizeObserverto.sidesinstead of the full row to avoid triggering on framer-motion animations, drag handle, and selection state changesresizeRefupdate intouseLayoutEffect(post-commit, pre-paint) for React 18 concurrent mode safetysidesElnull check to occur before it is passed toU.Dom.select.side.left.s50hover selector →.s60(thes50class is never applied)resizeRef.current?.()in observer callbacksTest plan
🤖 Generated with Claude Code