fix(common): flush pending resize frame on pointer up - #1161
Conversation
Deploying plait with
|
| Latest commit: |
2edddef
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://37bcb5a1.plait.pages.dev |
| Branch Preview URL: | https://xingxing-flush-pending-resiz.plait.pages.dev |
Deploying plait-docs with
|
| Latest commit: |
2edddef
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9098909a.plait-docs.pages.dev |
| Branch Preview URL: | https://xingxing-flush-pending-resiz.plait-docs.pages.dev |
|
Please help me review @nightt5879,I have a question about this PR , we can discuss it on #1162 |
nightt5879
left a comment
nightt5879
left a comment
There was a problem hiding this comment.
The direction looks good overall. I left two inline concerns: one lifecycle issue around flushing a stale resize callback, and one public callback compatibility issue. Once those are handled and the lifecycle case is covered by an integration test, this should be good to go.
|
|
||
| board.globalPointerUp = (event: PointerEvent) => { | ||
| if (isResizing(board)) { | ||
| flushThrottleRAF(board, RESIZE_RAF_KEY); |
There was a problem hiding this comment.
[P1] Could we avoid flushing after the resize target has been invalidated?
The target can be removed or replaced while the last move is still pending. Flushing here would then run onResize with a stale path, which can update the element that shifted into that path or throw before the resize cleanup runs. I think we need a cancel/invalidation check here, plus a regression test for delete/replace before pointer-up.
| value[key] = null; | ||
| BOARD_TO_RAF.set(board, value); | ||
| PlaitBoard.isAlive(board) && fn(); | ||
| PlaitBoard.isAlive(board) && task.callback(); |
There was a problem hiding this comment.
[P2] Could we preserve the callback’s original call semantics here?
This used to invoke the callback directly with fn(). Calling it as task.callback() now binds this to the internal task object, which is observable for non-arrow callbacks accepted by this public API. Could we detach the callback before invoking it? The same applies to the flush path below.
|
Thanks for the thorough review. After reconsidering the scope, I do not think this PR is appropriate to merge at this time. The original case requires At the moment, we do not have a confirmed, user-facing Plait defect showing that this timing issue occurs in practice. It is also difficult for a user to trigger Therefore, I do not recommend merging this PR. Going forward, I also suggest that we avoid adding similar RAF-flush handling unless there is a concrete, reproducible defect with user impact. This keeps the work focused on verified problems. Thank you again for surfacing the lifecycle and API-compatibility risks. |
|
got it, thanks for the clarification. i agree that Iimay have overthought this edge case. we can close this PR for now. |
2 participants
Summary
throttleRAFcallbacksRoot Cause
withResizeschedules resize work throughrequestAnimationFrame. IfglobalPointerUpruns before the pending frame, it clearsstartPointandresizeRef; the queued callback then observes empty resize state and drops the user's final pointer move.Impact
The last resize position is now applied before the resize lifecycle is finalized. This keeps the behavior generic for every consumer of
withResizeand avoids table-specific state management. This was identified while reviewing #1160.Verification
./node_modules/.bin/tsc -p packages/core/tsconfig.spec.json --noEmitnpm run build:corenpm run build:commonnpm run test:core -- --watch=false --browsers=ChromeHeadlessCI --progress=false --include=packages/core/src/utils/common.spec.ts(blocked during the Angular/esbuild build phase by an esbuild deadlock, exit 134; specs did not start)