Fix shapes jumping to center after drawing and cursor drift under zoom/pan#35
Conversation
Drawing any shape made it jump to the screen center ~0.5s later: auto-save published a new currentBoard into the store, BoardCanvas re-fed the re-themed elements array into Plait's Wrapper as value, and Wrapper treats a value/children mismatch as an external board replacement and calls fitViewport. Seed Wrapper's value once per board via a keyed inner component so store echoes for the same board never flow back into the live Plait board. Keep board.children in sync when applying remote collab changes so the same refit cannot fire there.
Regression test for the viewport refit: draws a rectangle, waits past the 500ms auto-save debounce, and asserts the rendered bounding box has not moved.
Plait's viewport has no offsetX/offsetY, so remote cursors drifted under zoom and pan. Replace the offset-based math with origination-based screenToDocument/documentToScreen transforms, resolve the viewport container via getViewportContainerElement instead of the scrolling SVG host, and update tests to the new semantics, including a cross-client zoom/pan/window-size case.
Add top-level rules and concrete guidance for zero-comment code, simplicity over cleverness, naming, and atomic commits.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR renames viewport offset fields to origination-based coordinates across collaboration utilities, hooks, and components, updating coordinate conversion logic and container resolution throughout cursor tracking. Separately, BoardCanvas gains remote sync fixes and a lazy state initializer, a new e2e stability test is added, and CLAUDE.md is expanded with stricter conventions. ChangesViewport Origination Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Board Canvas and Testing Updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Documentation Conventions Update
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant Pointer
participant useCursorTracking
participant getViewportContainerElement
participant screenToDocument
Pointer->>useCursorTracking: pointermove event
useCursorTracking->>getViewportContainerElement: resolve container(board)
getViewportContainerElement-->>useCursorTracking: container element
useCursorTracking->>screenToDocument: convert clientX/clientY with viewport
screenToDocument-->>useCursorTracking: documentX, documentY
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
❌ Test Results
How to view coverage report
Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/unit/viewport.test.ts (1)
281-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSecond half of the cross-client test is tautological.
screenB's expected value is computed by re-applyingdocumentToScreen's exact formula inline (usingboardPointinstead ofwire), rather than an independently-derived pixel value. Since the first assertion already proveswire ≈ boardPoint, this second assertion would still pass even ifdocumentToScreen's math were subtly wrong — it isn't an independent oracle.Consider asserting against literal expected numbers instead, e.g. for
viewportB(zoom 1.5, origination 350/220) andrectB(left 60, top 40): expectedscreenB.x + rectB.left === 285andscreenB.y + rectB.top === 160.✅ Suggested hardened assertion
const screenB = documentToScreen(wire.x, wire.y, viewportB); - expect(screenB.x + rectB.left).toBeCloseTo( - (boardPoint.x - viewportB.originationX) * viewportB.zoom + rectB.left, - 10 - ); - expect(screenB.y + rectB.top).toBeCloseTo( - (boardPoint.y - viewportB.originationY) * viewportB.zoom + rectB.top, - 10 - ); + expect(screenB.x + rectB.left).toBeCloseTo(285, 10); + expect(screenB.y + rectB.top).toBeCloseTo(160, 10);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/viewport.test.ts` around lines 281 - 309, The second half of the cross-client consistency test is tautological because it reuses the same `documentToScreen` math inline instead of checking against an independent expected value. Update the `cross-client consistency` test in `viewport.test.ts` so the `screenB` assertions in `documentToScreen` compare against fixed expected screen coordinates for `viewportB` and `rectB` (using the existing `screenB` result), rather than recomputing the formula from `boardPoint`.packages/collaboration/src/utils/viewport.ts (1)
44-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc to this new exported package API.
getViewportContainerElementis a new exported function frompackages/collaboration. As per path instructions,packages/**/*.{ts,tsx}: "Use JSDoc only on exported package APIs in packages/*; do not add JSDoc to internal functions."📝 Suggested JSDoc
+/** + * Resolves the DOM element representing the Plait board's viewport container, + * used as the reference frame for screen/document coordinate conversions. + */ export function getViewportContainerElement(board: PlaitBoard | null): Element | null {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/collaboration/src/utils/viewport.ts` around lines 44 - 56, Add JSDoc to the exported package API getViewportContainerElement in the viewport utility so it is documented as part of packages/collaboration; keep the comment on this exported function only and do not add JSDoc to any internal helpers. Describe what the function returns for a given PlaitBoard or null input, and make sure the doc block sits directly above getViewportContainerElement so it remains attached if the implementation moves.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@features/board/components/BoardCanvas.tsx`:
- Line 100: The direct assignment to board.children in BoardCanvas should be
treated as an intentional Plait mutation and suppressed for the
react-hooks/immutability lint rule, matching the pattern already used in
collaborative-board. Add the immutability disable comment at the board.children
= normalized update site in BoardCanvas so the lint check does not fail on this
deliberate mutation.
In `@packages/collaboration/src/utils/viewport.ts`:
- Around line 44-56: The fallback in getViewportContainerElement is using
document.querySelector, which can pick the first board in the page instead of
the active one. Update the fallback to resolve the viewport container from the
provided PlaitBoard instance or its own container scope, keeping the lookup
board-specific when PlaitBoard.getViewportContainer fails. Use
getViewportContainerElement and PlaitBoard.getViewportContainer as the key
places to adjust the logic.
---
Nitpick comments:
In `@packages/collaboration/src/utils/viewport.ts`:
- Around line 44-56: Add JSDoc to the exported package API
getViewportContainerElement in the viewport utility so it is documented as part
of packages/collaboration; keep the comment on this exported function only and
do not add JSDoc to any internal helpers. Describe what the function returns for
a given PlaitBoard or null input, and make sure the doc block sits directly
above getViewportContainerElement so it remains attached if the implementation
moves.
In `@tests/unit/viewport.test.ts`:
- Around line 281-309: The second half of the cross-client consistency test is
tautological because it reuses the same `documentToScreen` math inline instead
of checking against an independent expected value. Update the `cross-client
consistency` test in `viewport.test.ts` so the `screenB` assertions in
`documentToScreen` compare against fixed expected screen coordinates for
`viewportB` and `rectB` (using the existing `screenB` result), rather than
recomputing the formula from `boardPoint`.
🪄 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: feffcceb-e53b-4ccc-97b1-c023a4a1afe4
📒 Files selected for processing (15)
CLAUDE.mdfeatures/board/components/BoardCanvas.tsxpackages/collaboration/src/components/cursor-overlay.tsxpackages/collaboration/src/cursor-manager.tspackages/collaboration/src/hooks/use-cursor-tracking.tspackages/collaboration/src/hooks/use-sync.tspackages/collaboration/src/hooks/use-viewport.tspackages/collaboration/src/utils/index.tspackages/collaboration/src/utils/viewport.tstests/e2e/shapes.spec.tstests/integration/cursor-collaboration.test.tstests/unit/cursor-manager.test.tstests/unit/cursor-overlay.test.tsxtests/unit/use-cursor-tracking.test.tsxtests/unit/viewport.test.ts
| const unsubscribe = syncBusContext.syncBus.subscribeToRemoteChanges((elements: BoardElement[]) => { | ||
| const normalized = normalizeElements(elements); | ||
| onElementsChange(normalized); | ||
| board.children = normalized; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether react-hooks/immutability rule is enabled and how the sibling file guards the same mutation
fd -e cjs -e js -e json -e mjs -e ts eslint.config -H | xargs -r rg -n "immutability|react-hooks"
rg -n "board.children\s*=" -C2Repository: Kripu77/thinkix
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ESLint config files ==\n'
fd -H -a 'eslint.config.*' . || true
fd -H -a '.eslintrc*' . || true
printf '\n== Search for react-hooks/immutability ==\n'
rg -n --hidden -S "react-hooks/immutability|immutability" . || true
printf '\n== Relevant files ==\n'
fd -H -a 'BoardCanvas.tsx' features || true
fd -H -a 'collaborative-board.tsx' features || true
printf '\n== BoardCanvas mutation context ==\n'
rg -n -C 4 "board\.children\s*=" features/board/components/BoardCanvas.tsx || true
printf '\n== Collaborative board mutation context ==\n'
rg -n -C 6 "board\.children\s*=" features/collaboration/components/collaborative-board.tsx || trueRepository: Kripu77/thinkix
Length of output: 2306
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== eslint.config.mjs relevant rules ==\n'
rg -n -C 3 "react-hooks/immutability|immutability" eslint.config.mjs || true
printf '\n== BoardCanvas surrounding context ==\n'
sed -n '1,140p' features/board/components/BoardCanvas.tsx
printf '\n== Collaborative board surrounding context ==\n'
sed -n '80,120p' features/collaboration/components/collaborative-board.tsxRepository: Kripu77/thinkix
Length of output: 6205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== eslint.config.mjs ==\n'
sed -n '1,260p' eslint.config.mjs
printf '\n== package.json eslint-related deps ==\n'
rg -n '"eslint|react-hooks|typescript-eslint|prettier"' package.json
printf '\n== any repo-wide react-hooks config imports ==\n'
rg -n "react-hooks|eslint-plugin-react-hooks|reactHooks" . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'Repository: Kripu77/thinkix
Length of output: 4263
Add the immutability suppression in features/board/components/BoardCanvas.tsx:100. board.children = normalized uses the same direct Plait mutation already guarded in features/collaboration/components/collaborative-board.tsx; without the react-hooks/immutability disable, this file will fail the lint rule.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@features/board/components/BoardCanvas.tsx` at line 100, The direct assignment
to board.children in BoardCanvas should be treated as an intentional Plait
mutation and suppressed for the react-hooks/immutability lint rule, matching the
pattern already used in collaborative-board. Add the immutability disable
comment at the board.children = normalized update site in BoardCanvas so the
lint check does not fail on this deliberate mutation.
| export function getViewportContainerElement(board: PlaitBoard | null): Element | null { | ||
| if (board) { | ||
| try { | ||
| const container = PlaitBoard.getViewportContainer(board); | ||
| if (container) return container; | ||
| } catch { | ||
| } | ||
| } | ||
| return ( | ||
| document.querySelector('.plait-board-container .viewport-container') ?? | ||
| document.querySelector('.plait-board-container') | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does @plait/core0.92.1 export a staticPlaitBoard.getViewportContainer method, and what does it return when the board has no viewport container yet?
💡 Result:
Based on the available documentation and codebase for @plait/core 0.92.1, there is no static PlaitBoard.getViewportContainer method [1][2][3]. The framework does, however, provide a standalone function named initializeViewportContainer, which is exported by @plait/core and used to initialize the viewport container [4]. In the Plait framework's architecture, viewport and board management are typically handled through specific utility functions rather than static methods on the PlaitBoard class [4][5]. Since the method PlaitBoard.getViewportContainer does not exist in the public API of @plait/core 0.92.1, it does not return any value for a board that has no viewport container [4][5]. If you are looking for the viewport container of a board, the framework expects the container to be managed and initialized via functions like initializeViewportContainer [4].
Citations:
- 1: https://registry.npmjs.org/@plait/core
- 2: https://libraries.io/npm/@plait%2Fcore
- 3: https://github.com/plait-board/x-plait
- 4: plait-board/drawnix@da405d3
- 5: https://context7.com/worktile/plait/llms.txt?tokens=10000
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and find its call sites/usages.
git ls-files 'packages/collaboration/src/utils/viewport.ts' 'packages/collaboration/src/**' | sed -n '1,120p'
echo
echo '--- outline viewport.ts ---'
ast-grep outline packages/collaboration/src/utils/viewport.ts --view expanded
echo
echo '--- references to getViewportContainerElement ---'
rg -n "getViewportContainerElement|getViewportContainer" packages/collaboration/src packages -g '!**/dist/**' -g '!**/build/**'
echo
echo '--- files mentioning viewport-container selector ---'
rg -n "\.plait-board-container|viewport-container" packages -g '!**/dist/**' -g '!**/build/**'Repository: Kripu77/thinkix
Length of output: 4939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- use-cursor-tracking.ts excerpt ---'
sed -n '1,180p' packages/collaboration/src/hooks/use-cursor-tracking.ts
echo
echo '--- use-sync.ts excerpt ---'
sed -n '1,180p' packages/collaboration/src/hooks/use-sync.ts
echo
echo '--- cursor-overlay.tsx excerpt ---'
sed -n '1,220p' packages/collaboration/src/components/cursor-overlay.tsx
echo
echo '--- viewport.ts excerpt ---'
sed -n '1,120p' packages/collaboration/src/utils/viewport.tsRepository: Kripu77/thinkix
Length of output: 14792
Scope the viewport fallback to the active board container.
document.querySelector('.plait-board-container ...') returns the first matching board in the document, so if PlaitBoard.getViewportContainer() throws or is unavailable, cursor math can attach to the wrong board when more than one is mounted. Use a board-scoped lookup instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/collaboration/src/utils/viewport.ts` around lines 44 - 56, The
fallback in getViewportContainerElement is using document.querySelector, which
can pick the first board in the page instead of the active one. Update the
fallback to resolve the viewport container from the provided PlaitBoard instance
or its own container scope, keeping the lookup board-specific when
PlaitBoard.getViewportContainer fails. Use getViewportContainerElement and
PlaitBoard.getViewportContainer as the key places to adjust the logic.
1 participant
What
Wrapperas a newvalue, which Plait treats as an external board replacement and re-fits the viewport.BoardCanvasnow seedsvalueonce per board (keyed remount on board switch), and remote sync keepsboard.childrenin step so the refit can't fire there either.viewport.offsetX/offsetY, which Plait doesn't have. Cursor transforms now use viewport origination via sharedscreenToDocument/documentToScreenhelpers and resolve the viewport container instead of the scrolling SVG host.Testing
shapes+board-managemente2e specs, and typecheck all pass.Summary by CodeRabbit