feat: implement movable and resizable map tiles (#904)#1397
feat: implement movable and resizable map tiles (#904)#1397lspassos1 wants to merge 2 commits intokoala73:mainfrom
Conversation
|
@lspassos1 is attempting to deploy a commit to the Elie Team on Vercel. A member of the Team first needs to authorize it. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
koala73
left a comment
There was a problem hiding this comment.
Focused review on the resize-refactor portion of PR #1397
The refactoring direction is sound: extracting ResizeHandler reduces Panel.ts complexity and makes the resize logic reusable. However, there are three behavioral regressions that need to be fixed before merging.
Summary of findings
| Priority | File | Issue |
|---|---|---|
| P1 | resize-handler.ts |
Mouseup can persist the wrong span (rAF race) |
| P1 | Panel.ts |
Async import() leaves resize handlers unavailable during setup |
| P1 | Panel.ts |
Column resize no longer respects current grid width while dragging |
| P2 | resize-handler.ts |
getStartSpan API contract is misleading |
| P2 | MapContainer.ts / Panel.ts |
New public API surface is unused |
|
|
||
| if (this.rafId !== null) { | ||
| cancelAnimationFrame(this.rafId); | ||
| this.rafId = null; |
There was a problem hiding this comment.
P1: Mouseup can persist the wrong span
handleMove() defers DOM updates into requestAnimationFrame, but endResize() cancels any pending frame before reading the final span back from the element. If the user releases immediately after the last move, the last span change is dropped and the old span is what gets saved/tracked.
The previous implementation updated synchronously, so this is a behavioral regression.
Fix: Flush the pending resize before teardown (execute the rAF callback synchronously), or remove the rAF indirection entirely. The old code did not need it, and span clamping is cheap enough to run on every mousemove.
| this.resizeHandle?.classList.remove('active'); | ||
| if (this.onRowMouseMove) { | ||
| document.removeEventListener('mousemove', this.onRowMouseMove); | ||
| import('../utils/resize-handler').then(({ ResizeHandler }) => { |
There was a problem hiding this comment.
P1: Async setup leaves resize handlers unavailable
setupResizeHandlers() now waits on a dynamic import(). Until that promise resolves, neither resize handler nor the dblclick reset listeners exist, so early user interaction is silently dropped.
There is also a destroy race: if destroy() runs before the promise settles, the .then() callback still instantiates handlers and attaches listeners on an already-destroyed panel.
This code is always needed and should be statically imported:
import { ResizeHandler } from '../utils/resize-handler';| type: 'col', | ||
| getStartSpan: () => clampColSpan(getColSpan(this.element), getMaxColSpan(this.element)), | ||
| setSpanClass: (span) => setColSpanClass(this.element, span), | ||
| maxSpan: 3, |
There was a problem hiding this comment.
P1: Column resize no longer respects current grid width while dragging
The new column handler is constructed with maxSpan: 3, so ResizeHandler clamps against a fixed upper bound. The old code recomputed getMaxColSpan(this.element) on every mouse move and mouse up, which mattered on 1- or 2-column responsive layouts.
This regression allows impossible col-span-3 states during drag on narrow viewports and only snaps/clamps at persistence time.
Fix: Either pass a getMaxSpan callback instead of a static number, or compute maxSpan dynamically inside handleMove.
| document.body.classList.remove('panel-resize-active'); | ||
| this.options.handle.classList.remove('active'); | ||
|
|
||
| const finalSpan = this.options.getStartSpan(); // get current span |
There was a problem hiding this comment.
P2: API contract is internally inconsistent
endResize() calls getStartSpan() to obtain the final span. Today the callback happens to read live DOM state, but the name and call site disagree about its contract. If a consumer ever adds caching or side effects to getStartSpan (since it sounds like it should return the value from drag start), this breaks silently.
Suggestion: Rename the callback to getCurrentSpan, or track the last computed span inside ResizeHandler and pass it to onResizeEnd directly.
| if (this.useDeckGL) { this.deckGLMap?.setIsResizing(isResizing); } else { this.svgMap?.setIsResizing(isResizing); } | ||
| } | ||
|
|
||
| public getIsResizing(): boolean { |
There was a problem hiding this comment.
P2: New public API surface is unused
getIsResizing(), getIsPinned(), and setIsPinned() have no in-tree callers. Same for Panel.setEnabled() (which just delegates to toggle()). This expands the public contract with no demonstrated need.
These should either be removed here or introduced in the PR that actually consumes them.
koala73
left a comment
There was a problem hiding this comment.
Thanks for the refactor, @lspassos1! Extracting the resize logic into shared utilities is a solid direction that cuts ~400 lines from Panel.ts. A few issues to address:
BLOCKING: RAF race condition loses final span on resize end
In resize-handler.ts, handleMove() uses requestAnimationFrame to apply span changes, but endResize() cancels the pending RAF (line 762) before reading the final span (line 771). If the user releases the mouse between a handleMove call and its RAF executing, the last span update is cancelled and never applied, but onResizeEnd fires with the stale value. Fix: apply the final span synchronously in endResize() before firing onResizeEnd.
HIGH: Dynamic import creates a setup race condition
setupResizeHandlers() uses import('../utils/resize-handler').then(...). Resize handlers aren't attached synchronously, so if a user interacts before the chunk loads, nothing happens. The dblclick reset handlers are also delayed. The old code set these up synchronously. This regression could cause "resize doesn't work" reports on slow connections. Consider using a synchronous import instead.
HIGH: onDocMouseUp unconditionally removes panel-resize-active
The old code checked if (!this.isResizing && !this.isColResizing) before removing the body class. The new code removes it unconditionally. If a resize is in-progress (in this or another panel), the body class gets prematurely removed, breaking the pointer-events: none protection on iframes during resize.
HIGH: PR scope doesn't match title
Title says "implement movable and resizable map tiles" but there's no drag-to-move functionality. The actual change is a refactor of existing resize logic. getIsPinned()/setIsPinned() on MapContainer and setEnabled() on Panel are added but never called anywhere in this PR (dead code). Consider removing unused methods or deferring them to the actual "movable tiles" PR, and updating the title to reflect this is a resize refactor.
MEDIUM: getStartSpan is misnamed
ResizeHandler calls this.options.getStartSpan() in endResize() to get the final span, but the option is named getStartSpan. It's actually "get current span from DOM." Confusing for maintainers. Consider getCurrentSpan instead.
What's good:
- Clean 1:1 extraction of utility functions into
resize-utils.ts ResizeHandlerproperly handles both mouse and touch with bound methods and cleandestroy()- RAF-based move handling is a nice performance improvement (once the race is fixed)
- Concurrent resize deduplication is well structured
Please fix the RAF race, switch to synchronous import, and restore the mouseup guard. Thanks again for the contribution!
|
Thanks for the fix commit, @lspassos1! The switch to sync imports and the primary button guard are solid improvements. A few remaining items to address: BLOCKING: RAF race in BLOCKING: HIGH: HIGH: Unused public API surface MEDIUM: MEDIUM: LOW: Two empty lines at top of The refactor direction is great. Once the RAF race and mouseup guard are fixed, this should be good to go. |
|
Cheers for the eagle eye on the resize handler, @koala73. You're spot on about that race condition - it's been a bit flaky when moving the mouse quickly. I'm also refactoring the async logic in Panel.ts now so we don't have that lag during the initial load. I'll get the unused API bits cleared out and fix the grid width logic today. Should have a fresh push for you to look at shortly. |
1 similar comment
|
Cheers for the eagle eye on the resize handler, @koala73. You're spot on about that race condition - it's been a bit flaky when moving the mouse quickly. I'm also refactoring the async logic in Panel.ts now so we don't have that lag during the initial load. I'll get the unused API bits cleared out and fix the grid width logic today. Should have a fresh push for you to look at shortly. |
|
These PRs are just too complicated with all drifts, and no followup. Closing. Hoping some contributor can spend time on this |
This PR introduces a dedicated utility to allow panels and the main map to be moved and resized.
Key changes:
Closes #904