Repository navigation
Add a React shell with routing and a board list - #4
Conversation
Step 2 of supporting multiple boards. React owns the shell — routing, the board list, page chrome — and nothing else. The canvas stays imperative. Pan, zoom and drag fire at frame rate, so putting board objects into React state would mean reconciling hundreds of components per pointer move. BoardCanvas renders the stage markup once and hands the refs to createApp; destroy() is the effect cleanup. That works only because elements were already an injected argument, so the framework integration is three lines rather than a rewrite. createApp now requires `elements` — the getElementById fallback had no callers left. Routes are / for the list and /b/:boardId for a board, so a board is linkable. Boards created from the list start empty; only a first-ever visit gets the starter board, which is a tour rather than content anyone asked for. Stack: React 19, Vite 8, react-router 8. The router is v8 rather than v7's react-router-dom because that package carries a high-severity RSC CSRF advisory — unreachable in a client-only SPA, but not worth keeping in the tree. `npm audit --omit=dev` is clean and CI now fails on any high-severity production advisory. Routing is hash-based because GitHub Pages has no SPA rewrite. Swapping to BrowserRouter is one line once hosting moves. Sources moved from public/js/** to src/**; public/ is now Vite's static asset directory and the site builds to dist/. CI installs dependencies, runs the suite, then builds the artifact; Deploy publishes dist/. The browser suite runs against Vite's dev server rather than a bundle: unbundled modules are served at their source paths, so V8 coverage attributes to src/core/board.js instead of one minified chunk. The shipped build is verified separately in CI. Two test-harness bugs fixed along the way. goto() paired Page.navigate with an immediate Page.reload, which aborted the in-flight navigation and left every page on about:blank; it now bounces through about:blank instead. And server.test.js hardcoded port 3999 and assumed the old public/ layout — it now uses a free port and a checked-in fixture site, so it no longer depends on a build having happened. 266 tests, 99.7% line coverage.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe application moved from a static browser entry point to a React/Vite architecture. It adds routed board management, a React-mounted canvas, source-based core and platform modules, local persistence, browser integration tests, and Vite build and deployment workflows. ChangesReact/Vite application migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (6)
src/core/board.js (1)
163-182: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueNote the quadratic cost of envelope expansion.
Each pass compares every id in
outagainst every object inall, for up toMAX_NESTINGpasses. The worst case is8 × n²containment tests. The method runs once per drag start, not per frame, so the current cost is acceptable for typical board sizes.If board sizes grow, index objects by envelope or sort by area so each pass only tests plausible children. No change is required now.
🤖 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 `@src/core/board.js` around lines 163 - 182, No implementation change is required for withEnvelopeChildren; retain the current envelope expansion logic and its existing behavior.src/platform/input.js (2)
404-416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
focusItemSoonbypasses the injected scheduler.
createAppbuilds aclockfrom the injectedschedulerand passes it tocreateRenderer. It givescreateInputthe rawwindowinstead.focusItemSoontherefore callswindow.requestAnimationFramedirectly, andcreateAppexposes a comparable helperfocusNewthat usesclock.nextFrame.Two consequences follow. A test that injects a fake scheduler cannot drive this path. The callback can also run after
destroy(), becausedestroy()removes listeners but does not cancel the frame. The callback is safe today, sincelayer.querySelectorreturnsnullafter React unmounts the subtree.Pass the scheduler into
createInputand usenextFramefor consistency with the renderer.🤖 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 `@src/platform/input.js` around lines 404 - 416, Update createInput and its caller createApp to accept the injected scheduler, then change focusItemSoon to schedule through the scheduler’s nextFrame method instead of window.requestAnimationFrame. Preserve the existing focus and selection behavior while ensuring tests can control the frame callback through the injected scheduler.
360-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the deprecated API usage.
Add a short comment explaining that
document.execCommand('insertText')is retained for cross-browsercontenteditableinsertion and native undo integration. No standardized cross-browser replacement provides the same behavior.🤖 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 `@src/platform/input.js` around lines 360 - 364, Add a concise comment immediately before the document.execCommand('insertText') call in onPaste, documenting that this deprecated API is intentionally retained for cross-browser contenteditable insertion and native undo integration because no standardized replacement provides equivalent behavior.src/platform/toolbar.js (1)
7-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
listenhelper is duplicated.
src/platform/input.jsdefines the same helper at lines 34-37. That copy also accepts anoptionsargument, which this copy omits. Two near-identical copies will drift.Extract one helper into a shared module, for example
src/platform/listeners.js, and import it in both files.♻️ Proposed extraction
New file
src/platform/listeners.js:/** Collect event listeners so a module can remove all of them at once. */ export function createListenerBag() { const offs = []; return { listen(target, type, fn, options) { target.addEventListener(type, fn, options); offs.push(() => target.removeEventListener(type, fn, options)); }, add: (off) => offs.push(off), removeAll() { for (const off of offs) off(); offs.length = 0; }, }; }Then in this file:
+import { createListenerBag } from './listeners.js'; + export function createToolbar({ window, elements, store, viewport, commands }) { const { toolbar, zoomLabel } = elements; - const listeners = []; - const listen = (target, type, fn) => { - target.addEventListener(type, fn); - listeners.push(() => target.removeEventListener(type, fn)); - }; + const bag = createListenerBag(); + const listen = bag.listen;🤖 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 `@src/platform/toolbar.js` around lines 7 - 11, Extract the duplicated listen helper into a shared listener-bag module, preserving support for the options argument and centralized cleanup behavior. Update the toolbar module’s listener setup and the corresponding helper in input.js to import and use the shared createListenerBag implementation, removing their local copies.src/core/viewport.js (1)
78-84: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
fitcan exceed the instance scale limit.The clamp uses the
maxScaleoption, which defaults to1.5. It does not considerthis.maxScale. Every other scale path (zoomAt,setScaleAt) respectsthis.maxScale. If a caller constructsViewportwithmaxScalebelow1.5,fitcan set a scale above that limit.The default construction in
src/app.jsusesMAX_SCALE = 6, so the current behavior is correct. Bound the option to the instance limit to keep the invariant true for any construction.♻️ Proposed change
const scale = clamp( Math.min((w - padding * 2) / rect.w, (h - padding * 2) / rect.h), this.minScale, - maxScale, + Math.min(maxScale, this.maxScale), );🤖 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 `@src/core/viewport.js` around lines 78 - 84, Update the scale clamp in Viewport.fit so its upper bound cannot exceed this.maxScale: combine the maxScale option with the instance limit before passing it to clamp. Preserve the existing default option and ensure fit respects the same instance-wide maximum enforced by zoomAt and setScaleAt.test/helpers/vite.js (1)
13-14: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
fileURLToPathfor filesystem paths.If contributors run this test helper on Windows,
.pathnamecan produce/C:/...instead of a platform filesystem path. Convert both URLs withfileURLToPath, or document Linux-only support.Proposed fix
import { createServer } from 'vite'; +import { fileURLToPath } from 'node:url'; export async function startDevServer() { const server = await createServer({ - configFile: new URL('../../vite.config.js', import.meta.url).pathname, - root: new URL('../../', import.meta.url).pathname, + configFile: fileURLToPath(new URL('../../vite.config.js', import.meta.url)), + root: fileURLToPath(new URL('../../', import.meta.url)),🤖 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 `@test/helpers/vite.js` around lines 13 - 14, Update the Vite configuration in the test helper to convert both URL values used by configFile and root through fileURLToPath instead of reading .pathname directly, preserving the existing target paths while supporting Windows filesystem path conventions.
🤖 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 `@index.html`:
- Line 5: Update the viewport meta tag in index.html to remove the maximum-scale
and user-scalable directives, leaving width=device-width and initial-scale=1
unchanged so browser page zoom remains available.
In `@README.md`:
- Around line 6-11: Update both fenced command blocks in README.md, including
the blocks near the npm command examples, to specify the sh language identifier
after the opening fence. Leave the command contents unchanged.
In `@src/App.jsx`:
- Around line 9-10: Ensure BoardPage synchronizes board-specific state when the
boardId route parameter changes, rather than retaining the previous board’s
title. Update the title input from defaultValue to controlled state or remount
BoardPage using boardId, and add coverage for navigating directly between two
board routes.
In `@src/core/ids.js`:
- Around line 5-9: Correct the explanatory comment above the 12-character ID
width to use the birthday-bound figures: 48 bits yields approximately 1.8 × 10⁻³
collision probability at one million IDs (about 1 in 560), while the
below-one-in-a-million threshold is around 24,000 IDs; preserve the existing
rationale for choosing 12 hex characters without changing implementation.
In `@src/core/store.js`:
- Around line 137-143: Update the storage validation in the load path around
store.load and the board record checks to accept only supported v values and
object entries that are valid records with usable ids; reject malformed data
before constructing the Map. Ensure invalid input is handled as a rejected load
rather than allowing load to throw while reading o.id.
In `@src/platform/input.js`:
- Around line 52-58: Update onPointerDown to end any existing gesture before
starting a new one, ensuring startPan, startDrag, and startMarquee cannot
overwrite active gesture state. Extract the shared active-gesture teardown into
a helper and reuse it from onPointerUp, preserving each gesture’s end() behavior
such as marquee cleanup and drag history recording.
In `@src/routes/BoardListPage.jsx`:
- Around line 23-26: Update create in BoardListPage so it checks the boolean
result of repository.save before navigating; on failure, show a storage error
and remain on the board list, while preserving navigation only for successful
saves.
In `@src/routes/BoardPage.jsx`:
- Around line 11-19: Update rename in BoardPage so the displayed title is
changed only after repository.rename succeeds; when rename returns false for an
unstored board, persist the current board snapshot with the trimmed title
instead. Reuse the existing onReady canvas-app state rather than removing or
altering handleReady, and report completion only after either repository write
succeeds.
In `@test/browser/shell.test.js`:
- Around line 35-40: Remove the fixed page.sleep call from clickOn and keep that
helper limited to dispatching clicks. Update each affected test to use
page.waitFor with the specific expected route, canvas readiness, or rendered
title before assertions, preserving the existing test flow while eliminating
timing-based waits.
In `@test/helpers/browser.js`:
- Around line 74-86: Update the browser helper’s goto readiness selection so the
default ready expression is derived from the target route `to`, while preserving
an explicitly supplied `readyWhen`. Ensure the initial navigation passes the
custom readiness condition explicitly, and subsequent calls such as
`goto(LIST_PATH)` wait for the readiness condition associated with that
destination rather than the initial path.
In `@test/node/server.test.js`:
- Around line 23-25: Update the listen helper to reject when server.listen emits
an error, while continuing to resolve with the localhost URL on successful
listening. Attach the error handler before starting the server and remove it
once the listening callback succeeds.
- Around line 127-129: Update the cleanup logic in the test’s finally block to
attach the child process exit listener before calling child.kill('SIGINT'),
while also resolving immediately if the process has already exited via exitCode
or signalCode. Ensure cleanup cannot hang when startup fails.
- Around line 116-124: Update the startup wait around the child.stdout listener
and banner promise to accumulate chunks until the complete expected “PeerThink →
http://localhost:${port}” banner is present before resolving. Also reject the
promise when the child process emits an error or exits before the banner
arrives, while preserving the existing timeout and assertion.
- Around line 99-100: Strengthen the production-server test around the “defaults
to serving the build output” case: ensure the dist build is available before
spawning server.js, then assert the response status is exactly 200 and verify
the expected index content rather than only checking basename(PUBLIC_DIR) or
accepting any positive status. Preserve the existing server startup flow and use
the test’s existing build/server helpers.
In `@test/run.js`:
- Line 44: Update the test setup flow around startDevServer so Chrome startup
and test execution are wrapped in a finally block that always awaits
vite.close(). Track whether Chrome started before calling chrome.kill(), and
only kill it when initialization succeeded.
---
Nitpick comments:
In `@src/core/board.js`:
- Around line 163-182: No implementation change is required for
withEnvelopeChildren; retain the current envelope expansion logic and its
existing behavior.
In `@src/core/viewport.js`:
- Around line 78-84: Update the scale clamp in Viewport.fit so its upper bound
cannot exceed this.maxScale: combine the maxScale option with the instance limit
before passing it to clamp. Preserve the existing default option and ensure fit
respects the same instance-wide maximum enforced by zoomAt and setScaleAt.
In `@src/platform/input.js`:
- Around line 404-416: Update createInput and its caller createApp to accept the
injected scheduler, then change focusItemSoon to schedule through the
scheduler’s nextFrame method instead of window.requestAnimationFrame. Preserve
the existing focus and selection behavior while ensuring tests can control the
frame callback through the injected scheduler.
- Around line 360-364: Add a concise comment immediately before the
document.execCommand('insertText') call in onPaste, documenting that this
deprecated API is intentionally retained for cross-browser contenteditable
insertion and native undo integration because no standardized replacement
provides equivalent behavior.
In `@src/platform/toolbar.js`:
- Around line 7-11: Extract the duplicated listen helper into a shared
listener-bag module, preserving support for the options argument and centralized
cleanup behavior. Update the toolbar module’s listener setup and the
corresponding helper in input.js to import and use the shared createListenerBag
implementation, removing their local copies.
In `@test/helpers/vite.js`:
- Around line 13-14: Update the Vite configuration in the test helper to convert
both URL values used by configFile and root through fileURLToPath instead of
reading .pathname directly, preserving the existing target paths while
supporting Windows filesystem path conventions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d2486ec-4132-46e2-9167-07725fc963b3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (53)
.github/workflows/ci.yml.github/workflows/deploy.yml.gitignoreREADME.mdindex.htmlpackage.jsonpublic/index.htmlpublic/js/main.jsserver.jssrc/App.jsxsrc/app.jssrc/components/BoardCanvas.jsxsrc/core/autosave.jssrc/core/board.jssrc/core/geometry.jssrc/core/ids.jssrc/core/scheduler.jssrc/core/seed.jssrc/core/selection.jssrc/core/store.jssrc/core/viewport.jssrc/main.jsxsrc/platform/input.jssrc/platform/renderer.jssrc/platform/storage.jssrc/platform/toolbar.jssrc/platform/views.jssrc/routes/BoardListPage.jsxsrc/routes/BoardPage.jsxsrc/shell/storage.jssrc/styles/canvas.csssrc/styles/shell.csstest/browser/app.test.jstest/browser/shell.test.jstest/coverage.jstest/fixtures/site/assets/app.jstest/fixtures/site/index.htmltest/fixtures/site/style.csstest/helpers/browser.jstest/helpers/vite.jstest/node/autosave.test.jstest/node/board.test.jstest/node/geometry.test.jstest/node/ids.test.jstest/node/scheduler.test.jstest/node/seed.test.jstest/node/selection.test.jstest/node/server.test.jstest/node/storage.test.jstest/node/store.test.jstest/node/viewport.test.jstest/run.jsvite.config.js
💤 Files with no reviewable changes (2)
- public/js/main.js
- public/index.html
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 15
🧹 Nitpick comments (6)
src/core/board.js (1)
163-182: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueNote the quadratic cost of envelope expansion.
Each pass compares every id in
outagainst every object inall, for up toMAX_NESTINGpasses. The worst case is8 × n²containment tests. The method runs once per drag start, not per frame, so the current cost is acceptable for typical board sizes.If board sizes grow, index objects by envelope or sort by area so each pass only tests plausible children. No change is required now.
🤖 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 `@src/core/board.js` around lines 163 - 182, No implementation change is required for withEnvelopeChildren; retain the current envelope expansion logic and its existing behavior.src/platform/input.js (2)
404-416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
focusItemSoonbypasses the injected scheduler.
createAppbuilds aclockfrom the injectedschedulerand passes it tocreateRenderer. It givescreateInputthe rawwindowinstead.focusItemSoontherefore callswindow.requestAnimationFramedirectly, andcreateAppexposes a comparable helperfocusNewthat usesclock.nextFrame.Two consequences follow. A test that injects a fake scheduler cannot drive this path. The callback can also run after
destroy(), becausedestroy()removes listeners but does not cancel the frame. The callback is safe today, sincelayer.querySelectorreturnsnullafter React unmounts the subtree.Pass the scheduler into
createInputand usenextFramefor consistency with the renderer.🤖 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 `@src/platform/input.js` around lines 404 - 416, Update createInput and its caller createApp to accept the injected scheduler, then change focusItemSoon to schedule through the scheduler’s nextFrame method instead of window.requestAnimationFrame. Preserve the existing focus and selection behavior while ensuring tests can control the frame callback through the injected scheduler.
360-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the deprecated API usage.
Add a short comment explaining that
document.execCommand('insertText')is retained for cross-browsercontenteditableinsertion and native undo integration. No standardized cross-browser replacement provides the same behavior.🤖 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 `@src/platform/input.js` around lines 360 - 364, Add a concise comment immediately before the document.execCommand('insertText') call in onPaste, documenting that this deprecated API is intentionally retained for cross-browser contenteditable insertion and native undo integration because no standardized replacement provides equivalent behavior.src/platform/toolbar.js (1)
7-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
listenhelper is duplicated.
src/platform/input.jsdefines the same helper at lines 34-37. That copy also accepts anoptionsargument, which this copy omits. Two near-identical copies will drift.Extract one helper into a shared module, for example
src/platform/listeners.js, and import it in both files.♻️ Proposed extraction
New file
src/platform/listeners.js:/** Collect event listeners so a module can remove all of them at once. */ export function createListenerBag() { const offs = []; return { listen(target, type, fn, options) { target.addEventListener(type, fn, options); offs.push(() => target.removeEventListener(type, fn, options)); }, add: (off) => offs.push(off), removeAll() { for (const off of offs) off(); offs.length = 0; }, }; }Then in this file:
+import { createListenerBag } from './listeners.js'; + export function createToolbar({ window, elements, store, viewport, commands }) { const { toolbar, zoomLabel } = elements; - const listeners = []; - const listen = (target, type, fn) => { - target.addEventListener(type, fn); - listeners.push(() => target.removeEventListener(type, fn)); - }; + const bag = createListenerBag(); + const listen = bag.listen;🤖 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 `@src/platform/toolbar.js` around lines 7 - 11, Extract the duplicated listen helper into a shared listener-bag module, preserving support for the options argument and centralized cleanup behavior. Update the toolbar module’s listener setup and the corresponding helper in input.js to import and use the shared createListenerBag implementation, removing their local copies.src/core/viewport.js (1)
78-84: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
fitcan exceed the instance scale limit.The clamp uses the
maxScaleoption, which defaults to1.5. It does not considerthis.maxScale. Every other scale path (zoomAt,setScaleAt) respectsthis.maxScale. If a caller constructsViewportwithmaxScalebelow1.5,fitcan set a scale above that limit.The default construction in
src/app.jsusesMAX_SCALE = 6, so the current behavior is correct. Bound the option to the instance limit to keep the invariant true for any construction.♻️ Proposed change
const scale = clamp( Math.min((w - padding * 2) / rect.w, (h - padding * 2) / rect.h), this.minScale, - maxScale, + Math.min(maxScale, this.maxScale), );🤖 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 `@src/core/viewport.js` around lines 78 - 84, Update the scale clamp in Viewport.fit so its upper bound cannot exceed this.maxScale: combine the maxScale option with the instance limit before passing it to clamp. Preserve the existing default option and ensure fit respects the same instance-wide maximum enforced by zoomAt and setScaleAt.test/helpers/vite.js (1)
13-14: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
fileURLToPathfor filesystem paths.If contributors run this test helper on Windows,
.pathnamecan produce/C:/...instead of a platform filesystem path. Convert both URLs withfileURLToPath, or document Linux-only support.Proposed fix
import { createServer } from 'vite'; +import { fileURLToPath } from 'node:url'; export async function startDevServer() { const server = await createServer({ - configFile: new URL('../../vite.config.js', import.meta.url).pathname, - root: new URL('../../', import.meta.url).pathname, + configFile: fileURLToPath(new URL('../../vite.config.js', import.meta.url)), + root: fileURLToPath(new URL('../../', import.meta.url)),🤖 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 `@test/helpers/vite.js` around lines 13 - 14, Update the Vite configuration in the test helper to convert both URL values used by configFile and root through fileURLToPath instead of reading .pathname directly, preserving the existing target paths while supporting Windows filesystem path conventions.
🤖 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 `@index.html`:
- Line 5: Update the viewport meta tag in index.html to remove the maximum-scale
and user-scalable directives, leaving width=device-width and initial-scale=1
unchanged so browser page zoom remains available.
In `@README.md`:
- Around line 6-11: Update both fenced command blocks in README.md, including
the blocks near the npm command examples, to specify the sh language identifier
after the opening fence. Leave the command contents unchanged.
In `@src/App.jsx`:
- Around line 9-10: Ensure BoardPage synchronizes board-specific state when the
boardId route parameter changes, rather than retaining the previous board’s
title. Update the title input from defaultValue to controlled state or remount
BoardPage using boardId, and add coverage for navigating directly between two
board routes.
In `@src/core/ids.js`:
- Around line 5-9: Correct the explanatory comment above the 12-character ID
width to use the birthday-bound figures: 48 bits yields approximately 1.8 × 10⁻³
collision probability at one million IDs (about 1 in 560), while the
below-one-in-a-million threshold is around 24,000 IDs; preserve the existing
rationale for choosing 12 hex characters without changing implementation.
In `@src/core/store.js`:
- Around line 137-143: Update the storage validation in the load path around
store.load and the board record checks to accept only supported v values and
object entries that are valid records with usable ids; reject malformed data
before constructing the Map. Ensure invalid input is handled as a rejected load
rather than allowing load to throw while reading o.id.
In `@src/platform/input.js`:
- Around line 52-58: Update onPointerDown to end any existing gesture before
starting a new one, ensuring startPan, startDrag, and startMarquee cannot
overwrite active gesture state. Extract the shared active-gesture teardown into
a helper and reuse it from onPointerUp, preserving each gesture’s end() behavior
such as marquee cleanup and drag history recording.
In `@src/routes/BoardListPage.jsx`:
- Around line 23-26: Update create in BoardListPage so it checks the boolean
result of repository.save before navigating; on failure, show a storage error
and remain on the board list, while preserving navigation only for successful
saves.
In `@src/routes/BoardPage.jsx`:
- Around line 11-19: Update rename in BoardPage so the displayed title is
changed only after repository.rename succeeds; when rename returns false for an
unstored board, persist the current board snapshot with the trimmed title
instead. Reuse the existing onReady canvas-app state rather than removing or
altering handleReady, and report completion only after either repository write
succeeds.
In `@test/browser/shell.test.js`:
- Around line 35-40: Remove the fixed page.sleep call from clickOn and keep that
helper limited to dispatching clicks. Update each affected test to use
page.waitFor with the specific expected route, canvas readiness, or rendered
title before assertions, preserving the existing test flow while eliminating
timing-based waits.
In `@test/helpers/browser.js`:
- Around line 74-86: Update the browser helper’s goto readiness selection so the
default ready expression is derived from the target route `to`, while preserving
an explicitly supplied `readyWhen`. Ensure the initial navigation passes the
custom readiness condition explicitly, and subsequent calls such as
`goto(LIST_PATH)` wait for the readiness condition associated with that
destination rather than the initial path.
In `@test/node/server.test.js`:
- Around line 23-25: Update the listen helper to reject when server.listen emits
an error, while continuing to resolve with the localhost URL on successful
listening. Attach the error handler before starting the server and remove it
once the listening callback succeeds.
- Around line 127-129: Update the cleanup logic in the test’s finally block to
attach the child process exit listener before calling child.kill('SIGINT'),
while also resolving immediately if the process has already exited via exitCode
or signalCode. Ensure cleanup cannot hang when startup fails.
- Around line 116-124: Update the startup wait around the child.stdout listener
and banner promise to accumulate chunks until the complete expected “PeerThink →
http://localhost:${port}” banner is present before resolving. Also reject the
promise when the child process emits an error or exits before the banner
arrives, while preserving the existing timeout and assertion.
- Around line 99-100: Strengthen the production-server test around the “defaults
to serving the build output” case: ensure the dist build is available before
spawning server.js, then assert the response status is exactly 200 and verify
the expected index content rather than only checking basename(PUBLIC_DIR) or
accepting any positive status. Preserve the existing server startup flow and use
the test’s existing build/server helpers.
In `@test/run.js`:
- Line 44: Update the test setup flow around startDevServer so Chrome startup
and test execution are wrapped in a finally block that always awaits
vite.close(). Track whether Chrome started before calling chrome.kill(), and
only kill it when initialization succeeded.
---
Nitpick comments:
In `@src/core/board.js`:
- Around line 163-182: No implementation change is required for
withEnvelopeChildren; retain the current envelope expansion logic and its
existing behavior.
In `@src/core/viewport.js`:
- Around line 78-84: Update the scale clamp in Viewport.fit so its upper bound
cannot exceed this.maxScale: combine the maxScale option with the instance limit
before passing it to clamp. Preserve the existing default option and ensure fit
respects the same instance-wide maximum enforced by zoomAt and setScaleAt.
In `@src/platform/input.js`:
- Around line 404-416: Update createInput and its caller createApp to accept the
injected scheduler, then change focusItemSoon to schedule through the
scheduler’s nextFrame method instead of window.requestAnimationFrame. Preserve
the existing focus and selection behavior while ensuring tests can control the
frame callback through the injected scheduler.
- Around line 360-364: Add a concise comment immediately before the
document.execCommand('insertText') call in onPaste, documenting that this
deprecated API is intentionally retained for cross-browser contenteditable
insertion and native undo integration because no standardized replacement
provides equivalent behavior.
In `@src/platform/toolbar.js`:
- Around line 7-11: Extract the duplicated listen helper into a shared
listener-bag module, preserving support for the options argument and centralized
cleanup behavior. Update the toolbar module’s listener setup and the
corresponding helper in input.js to import and use the shared createListenerBag
implementation, removing their local copies.
In `@test/helpers/vite.js`:
- Around line 13-14: Update the Vite configuration in the test helper to convert
both URL values used by configFile and root through fileURLToPath instead of
reading .pathname directly, preserving the existing target paths while
supporting Windows filesystem path conventions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d2486ec-4132-46e2-9167-07725fc963b3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (53)
.github/workflows/ci.yml.github/workflows/deploy.yml.gitignoreREADME.mdindex.htmlpackage.jsonpublic/index.htmlpublic/js/main.jsserver.jssrc/App.jsxsrc/app.jssrc/components/BoardCanvas.jsxsrc/core/autosave.jssrc/core/board.jssrc/core/geometry.jssrc/core/ids.jssrc/core/scheduler.jssrc/core/seed.jssrc/core/selection.jssrc/core/store.jssrc/core/viewport.jssrc/main.jsxsrc/platform/input.jssrc/platform/renderer.jssrc/platform/storage.jssrc/platform/toolbar.jssrc/platform/views.jssrc/routes/BoardListPage.jsxsrc/routes/BoardPage.jsxsrc/shell/storage.jssrc/styles/canvas.csssrc/styles/shell.csstest/browser/app.test.jstest/browser/shell.test.jstest/coverage.jstest/fixtures/site/assets/app.jstest/fixtures/site/index.htmltest/fixtures/site/style.csstest/helpers/browser.jstest/helpers/vite.jstest/node/autosave.test.jstest/node/board.test.jstest/node/geometry.test.jstest/node/ids.test.jstest/node/scheduler.test.jstest/node/seed.test.jstest/node/selection.test.jstest/node/server.test.jstest/node/storage.test.jstest/node/store.test.jstest/node/viewport.test.jstest/run.jsvite.config.js
💤 Files with no reviewable changes (2)
- public/js/main.js
- public/index.html
🛑 Comments failed to post (3)
src/core/ids.js (1)
5-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the collision-probability claim.
The comment states that 48 bits keeps a collision "below one in a million out past a million objects". The birthday bound gives a different result. For
nids and2^48values, the collision probability is approximatelyn² / 2^49. Atn = 1,000,000that is about1.8 × 10⁻³, or roughly 1 in 560. A probability below one in a million is reached at about 24,000 ids.The 12-character width can still be the right choice. Update the justification so the stated margin matches the arithmetic.
📝 Proposed comment correction
- * 12 hex characters is 48 bits. Ids are minted independently by every client — - * there is no coordinator to reject a duplicate — so the width has to survive - * the birthday bound rather than the object count: 48 bits keeps a collision - * below one in a million out past a million objects, where 32 bits would start - * colliding in the tens of thousands. + * 12 hex characters is 48 bits. Ids are minted independently by every client — + * there is no coordinator to reject a duplicate — so the width has to survive + * the birthday bound rather than the object count. At 48 bits the collision + * probability is about n² / 2^49: under one in a million up to ~24k ids, and + * still under 0.2% at a million. 32 bits would pass 1% before 10k ids.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.* 12 hex characters is 48 bits. Ids are minted independently by every client — * there is no coordinator to reject a duplicate — so the width has to survive * the birthday bound rather than the object count. At 48 bits the collision * probability is about n² / 2^49: under one in a million up to ~24k ids, and * still under 0.2% at a million. 32 bits would pass 1% before 10k ids.🤖 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 `@src/core/ids.js` around lines 5 - 9, Correct the explanatory comment above the 12-character ID width to use the birthday-bound figures: 48 bits yields approximately 1.8 × 10⁻³ collision probability at one million IDs (about 1 in 560), while the below-one-in-a-million threshold is around 24,000 IDs; preserve the existing rationale for choosing 12 hex characters without changing implementation.src/core/store.js (1)
137-143: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Inspect the storage repository for board-payload validation before store.load. set -euo pipefail fd -t f 'storage.js' src | while IFS= read -r f; do echo "===== $f =====" cat -n "$f" done echo "===== load callers =====" rg -nP -C 4 '\bstore\.load\s*\(|\bload\s*\(\s*boardId' --glob '*.js' --glob '*.jsx'Repository: boubou666/peerthink
Length of output: 7241
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "===== candidate app and store files =====" fd -t f -e js -e jsx src | sort | while IFS= read -r f; do if rg -q 'saved\.board|store\.load|repository\.load|function createApp|BoardCanvas|class .*Store|load\(data\)' "$f"; then echo "===== $f =====" rg -n -C 8 'saved\.board|store\.load|repository\.load|function createApp|BoardCanvas|class .*Store|load\(data\)' "$f" fi done echo "===== all repository load and store load references =====" rg -n -C 5 '\.(load|list)\s*\(|\bload\s*\(\s*data\s*\)' src --glob '*.js' --glob '*.jsx' echo "===== tests and package metadata =====" fd -t f . | rg '(^|/)(package\.json|.*test.*|.*spec.*|.*store.*|.*storage.*)$' | sortRepository: boubou666/peerthink
Length of output: 7332
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "===== src/app.js =====" cat -n src/app.js | sed -n '1,125p' echo "===== src/platform/storage.js validation and version references =====" rg -n -C 6 'RECORD_VERSION|isBoard|JSON\.parse|return read|function createLocalStorageRepository|load\(id\)' src/platform/storage.js echo "===== storage tests =====" cat -n test/node/storage.test.js | sed -n '1,260p' echo "===== version and board-shape references =====" rg -n -C 3 '\b(v|RECORD_VERSION|objects|order)\b' src test --glob '*.js' --glob '*.jsx' | head -n 260 echo "===== standalone validation probe =====" node - <<'JS' const isBoard = (board) => Array.isArray(board?.objects) && Array.isArray(board?.order); const records = [ null, {}, { board: {} }, { board: { objects: [], order: [] } }, { v: 0, board: { objects: [], order: [] } }, { v: 1, board: { objects: [], order: [] } }, { v: 2, board: { objects: [], order: [] } }, ]; for (const record of records) { console.log(JSON.stringify(record), '=>', isBoard(record?.board) ? 'accepted by read' : 'rejected by read'); } JSRepository: boubou666/peerthink
Length of output: 32662
🏁 Script executed:
#!/bin/bash set -euo pipefail node - <<'JS' const isBoard = (board) => Array.isArray(board?.objects) && Array.isArray(board?.order); const load = (data) => { const objects = new Map(data.objects.map((o) => [o.id, structuredClone(o)])); return data.order.filter((id) => objects.has(id)); }; const records = [ { label: 'missing version', record: { board: { objects: [], order: [] } } }, { label: 'newer version', record: { v: 2, board: { objects: [], order: [] } } }, { label: 'null object', record: { v: 1, board: { objects: [null], order: [] } } }, { label: 'primitive object', record: { v: 1, board: { objects: [7], order: [] } } }, { label: 'valid object', record: { v: 1, board: { objects: [{ id: 'a' }], order: ['a'] } } }, ]; for (const { label, record } of records) { const accepted = isBoard(record.board); let result = 'not passed to load'; if (accepted) { try { result = `load completed with ${load(record.board).length} ordered object(s)`; } catch (error) { result = `${error.name}: ${error.message}`; } } console.log(`${label}: ${accepted ? 'accepted by repository predicate' : 'rejected'}; ${result}`); } JSRepository: boubou666/peerthink
Length of output: 621
Reject malformed object entries before
store.load.src/platform/storage.jschecks only thatboard.objectsandboard.orderare arrays. A record withobjects: [null]passes this check, thenloadthrows when it readso.id. The repository also accepts unsupportedvvalues. Validate the record version and object entries, or makeloadreject invalid data without throwing.🤖 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 `@src/core/store.js` around lines 137 - 143, Update the storage validation in the load path around store.load and the board record checks to accept only supported v values and object entries that are valid records with usable ids; reject malformed data before constructing the Map. Ensure invalid input is handled as a rejected load rather than allowing load to throw while reading o.id.src/platform/input.js (1)
52-58: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A second pointerdown replaces the active gesture without ending it.
startPan,startDrag, andstartMarqueeeach assigngesturedirectly.onPointerDowndoes not check whether a gesture is already active. A second button press during an active gesture discards the first gesture object without calling itsend().The marquee case leaves visible state behind.
startMarqueeappends#marqueetooverlayand removes it only inend(). Reproduce it as follows:
- Press the left button on empty space. The marquee starts.
- Press the right button without releasing the left button.
onPointerDowncallsstartPan, which overwritesgesture.- Release.
onPointerUpruns the panend(), which is empty.The
#marqueeelement stays in the overlay until the next marquee gesture appends a second one. The drag case is less visible but also drops thepushHistorycall in itsend(), so the move is not undoable.Multi-button presses and multi-touch both reach this path. End the active gesture before starting a new one.
🐛 Proposed fix
Add a helper that ends any active gesture, then call it at the top of
onPointerDown.function onPointerDown(e) { + endGesture(e); // a second press must not orphan the active gesture if (e.button === 1 || e.button === 2 || spaceDown) { e.preventDefault(); startPan(e); return; }Extract the shared teardown so
onPointerUpreuses it:+ function endGesture(e) { + if (!gesture) return; + const active = gesture; + gesture = null; + active.end(e); + stage.classList.remove('panning'); + } + function onPointerUp(e) { - if (!gesture) return; - const active = gesture; - gesture = null; - active.end(e); - stage.classList.remove('panning'); + endGesture(e); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function endGesture(e) { if (!gesture) return; const active = gesture; gesture = null; active.end(e); stage.classList.remove('panning'); } function onPointerDown(e) { endGesture(e); // a second press must not orphan the active gesture if (e.button === 1 || e.button === 2 || spaceDown) { e.preventDefault(); startPan(e); return; } if (e.button !== 0) return; } function onPointerUp(e) { endGesture(e); }🤖 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 `@src/platform/input.js` around lines 52 - 58, Update onPointerDown to end any existing gesture before starting a new one, ensuring startPan, startDrag, and startMarquee cannot overwrite active gesture state. Extract the shared active-gesture teardown into a helper and reuse it from onPointerUp, preserving each gesture’s end() behavior such as marquee cleanup and drag history recording.
BoardPage never resynced when only :boardId changed. The router reuses the component across that navigation, so a title read once at mount and pinned through defaultValue kept showing the previous board's name over the new board's canvas. The field is now controlled, with a draft/committed split, and both follow the parameter — an uncontrolled input ignores defaultValue after mount, which is why the first attempt at this fix did not work. BoardListPage navigated after a save that returned false. That opens a route with no stored record, which createApp reads as a first visit and seeds — so a failed creation handed back a board full of starter content and then autosaved it. It now reports the failure and stays on the list. Renaming a board that had never been stored lost the name. On a first visit the seeded board lives only in memory until autosave fires, so rename() has no record to touch and returned false while the UI updated anyway; the next autosave then wrote the default title back. A failed rename now writes the record itself. goto() picked its readiness condition from the path the session was opened with rather than the path it was navigating to, so a canvas session moving to the list would have waited for window.app on a route that deliberately destroys it. Readiness now comes from the target. Also: browser zoom is no longer disabled — the canvas has its own zoom, the board list does not, and pinch on the stage is already handled by touch-action. Remaining fixed sleeps in the shell tests are gone, and the title-field tests drive real key input, which a controlled field requires. 273 tests, 99.6% line coverage.
The spawn test resolved on the first stdout chunk and then asserted the whole banner matched it. A data event is not a line boundary, so a server that started perfectly well could fail the assertion on a split write. It now buffers until the expected text appears, and rejects on early exit or spawn error rather than waiting out the timeout. Cleanup killed the child before attaching the exit listener, so an already-dead child would never fire it and the finally block would hang. The listener is attached first, and resolves immediately when exitCode or signalCode is already set. The same test only checked that PUBLIC_DIR ends in dist and that the status was positive — which a 404 satisfies. It now builds dist when it is missing, since npm test runs before npm run build in CI, and asserts a real 200 whose body contains the application root. listen() resolved on success but never rejected, so a bind failure hung the setup instead of failing it. test/run.js had no try/finally around the run, so a throw leaked Chromium and its profile lock — which surfaces on the *next* run as a message about the debugging port that says nothing about the real cause. Verified with dist/ deleted first, so the build-if-missing path runs rather than being assumed. 273 tests, 99.6% line coverage.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (1)
test/run.js (1)
44-44: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPut resource acquisition inside the cleanup scope.
startDevServer()runs at Line 44, before thetryat Line 71. A failure during Chromium startup or other setup can skip both cleanup paths. Thefinallyblock also assumes thatchromeexists. Acquire Vite and Chromium inside an outertry/finally, initialize their handles tonull, and close only initialized resources. This is the same unresolved cleanup issue reported in the previous review.Also applies to: 67-83
🤖 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 `@test/run.js` at line 44, Move the startDevServer() acquisition and Chromium setup into an outer try/finally cleanup scope, initialize vite, appBase, and chrome handles to null, and close each resource only when it was successfully initialized. Ensure setup failures still reach cleanup while preserving the existing test execution flow.
🧹 Nitpick comments (6)
test/node/server.test.js (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep Vite out of the test runtime.
Line 8 imports an npm package from a
test/**file. Lines 155-160 use it to prepare test input. Builddist/in a pre-test script or CI step, then keep this suite on Node built-ins. If that is not possible, document an approved exception.As per path instructions, “test/**: Zero-dependency suite: node:test plus raw CDP over Node's built-in WebSocket. Flag added npm dependencies.”
Also applies to: 155-160
🤖 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 `@test/node/server.test.js` at line 8, Remove the Vite import and the corresponding build usage in the test setup around the affected preparation logic, keeping test/node/server.test.js dependent only on Node built-ins. Move dist/ generation into the existing pre-test script or CI workflow; if that cannot be done, document the approved exception instead.Source: Path instructions
src/core/autosave.js (1)
9-9: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider surfacing a failed autosave.
repository.savereturnsfalsewhen storage is full or unavailable.flushdrops that result, so the user gets no signal that edits stopped persisting.BoardListPagealready reports a failed board creation, so the shell has a place to show this.An
onErrorcallback oncreateAutosavewould letBoardCanvasorBoardPagerender the same warning.🤖 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 `@src/core/autosave.js` at line 9, Update createAutosave and its flush function to inspect the boolean result from repository.save and invoke an optional onError callback when saving fails. Thread this callback through the BoardCanvas or BoardPage integration so the shell displays the existing failed-operation warning consistently with BoardListPage.src/platform/storage.js (1)
95-106: 🗄️ Data Integrity & Integration | 🔵 TrivialConsider how two tabs on one board interact.
savereads the existing record, then writes a complete replacement. Web Storage offers no transaction, so two tabs open on the same board each write their own full snapshot, and the later write discards the other tab's edits. Autosave makes this automatic rather than user-driven.The
storageevent is the usual mitigation: a tab listens for changes to its own key and either reloads the board or warns the user. That work belongs in the shell rather than in this file, so treat this as a follow-up for the persistence design instead of a change to this diff.🤖 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 `@src/platform/storage.js` around lines 95 - 106, Keep save in src/platform/storage.js unchanged; treat cross-tab concurrent writes as a follow-up persistence concern. Implement any storage-event listener and reload or warning behavior in the shell layer, outside save and this storage module.src/core/board.js (1)
72-81: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRegenerate list item ids in
duplicate.
structuredClone(src)copies theitemsarray of a list, including each item id. The clone receives a new object id, but its rows keep the source rows' ids. Two list objects then hold rows with identical ids.No current caller breaks, because
toggleItem,onInput, andfocusItemSoonall scope the row lookup by object id first. The duplicate rows still violate the uniqueness thatnewItemestablishes, so any later code that indexes rows across objects would mix them up.♻️ Proposed fix to mint new row ids
duplicate(offset = 24) { if (!this.selection.size) return []; const clones = this.selection.list().map((id) => { const src = this.store.get(id); - return { ...structuredClone(src), id: this.newId(), x: src.x + offset, y: src.y + offset }; + const clone = { ...structuredClone(src), id: this.newId(), x: src.x + offset, y: src.y + offset }; + if (Array.isArray(clone.items)) { + clone.items = clone.items.map((item) => ({ ...item, id: this.newId() })); + } + return clone; });🤖 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 `@src/core/board.js` around lines 72 - 81, Update Board.duplicate to regenerate ids for every item in a cloned list, rather than retaining the ids copied by structuredClone(src). Preserve each item’s other fields while assigning a fresh id through the same uniqueness mechanism used by newItem, and ensure the cloned object’s items contain only the newly minted row ids.src/platform/input.js (1)
359-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
execCommand('insertText')compatibility assumption.
execCommandis deprecated, butinsertTextremains supported in current Chrome, Firefox, and Safari and preserves the native undo stack. Add a short comment explaining why this call remains. The repository has no declared browser matrix, and CI tests only Chromium; document the tested browser target or define the supported browsers.🤖 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 `@src/platform/input.js` around lines 359 - 364, Add a concise comment immediately before the execCommand('insertText') call in onPaste explaining that insertText is retained for current Chrome, Firefox, and Safari compatibility and native undo-stack preservation, and note the tested Chromium target or repository-supported browser scope. Leave the paste behavior unchanged.src/app.js (1)
20-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate
elementsat thecreateAppboundary. Checkelementsand its six required refs before constructing the renderer, input, and toolbar. The only source caller and all test callers passelements; remaininggetElementByIdcalls are unrelated DOM queries, not fallback usage.🤖 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 `@src/app.js` around lines 20 - 26, Update createApp to validate elements and all six required refs immediately at its boundary, before constructing the renderer, input, or toolbar. Reject missing or invalid elements explicitly, while preserving the existing caller-provided refs and unrelated getElementById queries.
🤖 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 @.github/workflows/deploy.yml:
- Around line 3-4: Update the deployment comment near the workflow
trigger/condition to document both automatic publishing after successful CI and
manual publishing via workflow_dispatch, unless the intended policy requires CI
success for every deployment, in which case remove workflow_dispatch from the
condition.
- Around line 42-49: Update the actions/checkout@v4 step in the deployment
workflow to set its ref to `${{ github.event.workflow_run.head_sha || github.sha
}}`. Keep the existing build and Pages artifact steps unchanged so workflow_run
deployments build the commit validated by CI.
In `@README.md`:
- Line 160: Update the React Router entry in the README dependency table to
remove the broad “v7 carries a CSRF advisory” rationale or replace it with a
precise reference to GHSA-qwww-vcr4-c8h2, its unstable RSC APIs, affected
version ranges, and patched versions 7.18.2 and 8.3.0.
In `@src/core/autosave.js`:
- Around line 8-13: Update the debounce flow used by createAutosave so
scheduler.debounce exposes a cancel handle for its pending timer, and have the
returned stop function remove the store listener and cancel that timer. Preserve
the existing flush behavior and return shape in createAutosave, while ensuring
stopped autosaves cannot execute a previously scheduled save.
In `@src/platform/input.js`:
- Around line 100-106: Update the pointer event wiring near the existing
pointerdown, pointermove, and pointerup registrations to also register
pointercancel with onPointerUp, reusing that handler’s gesture cleanup so
cancelled gestures clear gesture, end the active interaction, and remove the
panning state and marquee overlay.
- Around line 420-457: Update onKeyDown to return before board shortcut handling
when e.target is a form control, including input, textarea, select, or a
content-editable element. Preserve the existing Space handling only for non-form
targets, while retaining shortcuts for canvas and other non-editable targets.
In `@src/platform/renderer.js`:
- Around line 29-52: Guard the views[obj.type] lookup in createElement and
updateElement, and make createElement return null for unknown types instead of
throwing. Update sync to skip objects whose createElement result is null, while
preserving normal creation and update behavior for recognized types.
In `@src/platform/toolbar.js`:
- Around line 27-32: Update the keydown handler around the existing
e.target.isContentEditable guard to also ignore standard form controls,
including input elements such as the board title, before evaluating the ! and )
shortcuts. Preserve shortcut handling for canvas and other non-form targets.
In `@src/routes/BoardPage.jsx`:
- Around line 58-63: Update the title input’s onKeyDown handler so the Escape
branch marks the edit as cancelled before calling e.currentTarget.blur(),
allowing the nested onBlur handler to skip persisting the cancelled title;
preserve the existing draft reset and Enter behavior.
In `@test/browser/shell.test.js`:
- Around line 47-49: Update the browser test teardown wait condition around
onList and newBoard so it waits until window.app is absent and `#stage` no longer
exists before asserting unmount. Keep the existing .shell presence check only
where needed to confirm the list view, and ensure the unmount assertions run
after canvas teardown completes.
In `@test/coverage.js`:
- Around line 15-16: Update the coverage path handling around isMeasured so
paths from relative() are normalized by replacing backslashes with forward
slashes before filtering. Preserve the existing server.js and src JavaScript/JSX
matching rules after normalization.
In `@test/helpers/browser.js`:
- Around line 81-94: Update the goto method’s about:blank navigation flow to
wait until the blank document is committed, using the Page.frameNavigated event
or an equivalent committed-document condition, before issuing the appBase + to
navigation. Keep the existing readiness polling and timeout behavior unchanged.
In `@test/helpers/vite.js`:
- Around line 13-14: Update the Vite configuration options in the test helper to
convert both new URL values through fileURLToPath() instead of reading .pathname
directly. Apply this to configFile and root, preserving their existing URL
targets while producing native paths on Windows and decoding percent-encoded
characters.
- Line 1: Document Vite as an explicit orchestration exception for the test
suite, covering the dependencies in test/helpers/vite.js, test/run.js, and
test/node/server.test.js; alternatively, move the Vite-dependent code outside
test/** while preserving the existing test behavior.
- Around line 19-23: Update the Vite server setup flow after server.listen so a
missing server.resolvedUrls?.local?.[0] closes the server before throwing the
existing error. Preserve the successful return path in the helper that returns {
server, base }.
In `@test/node/server.test.js`:
- Around line 46-74: Update waitForOutput so child-process errors use a
dedicated handler that calls cleanup() before rejecting, rather than passing
reject directly to child.on('error'). Ensure teardown also resolves or completes
when the child emits error or close, preventing the timeout and pending teardown
from remaining active after a spawn failure.
In `@test/run.js`:
- Around line 72-79: Update the Promise wrapping child-process creation to
accept both resolve and reject, and register a one-time error handler on proc
that calls reject when spawn fails. Preserve the existing exit resolution while
ensuring the surrounding finally cleanup runs for rejected spawn attempts.
---
Duplicate comments:
In `@test/run.js`:
- Line 44: Move the startDevServer() acquisition and Chromium setup into an
outer try/finally cleanup scope, initialize vite, appBase, and chrome handles to
null, and close each resource only when it was successfully initialized. Ensure
setup failures still reach cleanup while preserving the existing test execution
flow.
---
Nitpick comments:
In `@src/app.js`:
- Around line 20-26: Update createApp to validate elements and all six required
refs immediately at its boundary, before constructing the renderer, input, or
toolbar. Reject missing or invalid elements explicitly, while preserving the
existing caller-provided refs and unrelated getElementById queries.
In `@src/core/autosave.js`:
- Line 9: Update createAutosave and its flush function to inspect the boolean
result from repository.save and invoke an optional onError callback when saving
fails. Thread this callback through the BoardCanvas or BoardPage integration so
the shell displays the existing failed-operation warning consistently with
BoardListPage.
In `@src/core/board.js`:
- Around line 72-81: Update Board.duplicate to regenerate ids for every item in
a cloned list, rather than retaining the ids copied by structuredClone(src).
Preserve each item’s other fields while assigning a fresh id through the same
uniqueness mechanism used by newItem, and ensure the cloned object’s items
contain only the newly minted row ids.
In `@src/platform/input.js`:
- Around line 359-364: Add a concise comment immediately before the
execCommand('insertText') call in onPaste explaining that insertText is retained
for current Chrome, Firefox, and Safari compatibility and native undo-stack
preservation, and note the tested Chromium target or repository-supported
browser scope. Leave the paste behavior unchanged.
In `@src/platform/storage.js`:
- Around line 95-106: Keep save in src/platform/storage.js unchanged; treat
cross-tab concurrent writes as a follow-up persistence concern. Implement any
storage-event listener and reload or warning behavior in the shell layer,
outside save and this storage module.
In `@test/node/server.test.js`:
- Line 8: Remove the Vite import and the corresponding build usage in the test
setup around the affected preparation logic, keeping test/node/server.test.js
dependent only on Node built-ins. Move dist/ generation into the existing
pre-test script or CI workflow; if that cannot be done, document the approved
exception instead.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 49940a2d-4368-47cf-9a8f-967686c48951
⛔ Files ignored due to path filters (5)
dist/assets/index-CGKfOYFM.jsis excluded by!**/dist/**dist/assets/index-CGKfOYFM.js.mapis excluded by!**/dist/**,!**/*.mapdist/assets/index-DUTNQzWv.cssis excluded by!**/dist/**dist/index.htmlis excluded by!**/dist/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (53)
.github/workflows/ci.yml.github/workflows/deploy.yml.gitignoreREADME.mdindex.htmlpackage.jsonpublic/index.htmlpublic/js/main.jsserver.jssrc/App.jsxsrc/app.jssrc/components/BoardCanvas.jsxsrc/core/autosave.jssrc/core/board.jssrc/core/geometry.jssrc/core/ids.jssrc/core/scheduler.jssrc/core/seed.jssrc/core/selection.jssrc/core/store.jssrc/core/viewport.jssrc/main.jsxsrc/platform/input.jssrc/platform/renderer.jssrc/platform/storage.jssrc/platform/toolbar.jssrc/platform/views.jssrc/routes/BoardListPage.jsxsrc/routes/BoardPage.jsxsrc/shell/storage.jssrc/styles/canvas.csssrc/styles/shell.csstest/browser/app.test.jstest/browser/shell.test.jstest/coverage.jstest/fixtures/site/assets/app.jstest/fixtures/site/index.htmltest/fixtures/site/style.csstest/helpers/browser.jstest/helpers/vite.jstest/node/autosave.test.jstest/node/board.test.jstest/node/geometry.test.jstest/node/ids.test.jstest/node/scheduler.test.jstest/node/seed.test.jstest/node/selection.test.jstest/node/server.test.jstest/node/storage.test.jstest/node/store.test.jstest/node/viewport.test.jstest/run.jsvite.config.js
💤 Files with no reviewable changes (2)
- public/index.html
- public/js/main.js
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 17
♻️ Duplicate comments (1)
test/run.js (1)
44-44: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPut resource acquisition inside the cleanup scope.
startDevServer()runs at Line 44, before thetryat Line 71. A failure during Chromium startup or other setup can skip both cleanup paths. Thefinallyblock also assumes thatchromeexists. Acquire Vite and Chromium inside an outertry/finally, initialize their handles tonull, and close only initialized resources. This is the same unresolved cleanup issue reported in the previous review.Also applies to: 67-83
🤖 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 `@test/run.js` at line 44, Move the startDevServer() acquisition and Chromium setup into an outer try/finally cleanup scope, initialize vite, appBase, and chrome handles to null, and close each resource only when it was successfully initialized. Ensure setup failures still reach cleanup while preserving the existing test execution flow.
🧹 Nitpick comments (6)
test/node/server.test.js (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep Vite out of the test runtime.
Line 8 imports an npm package from a
test/**file. Lines 155-160 use it to prepare test input. Builddist/in a pre-test script or CI step, then keep this suite on Node built-ins. If that is not possible, document an approved exception.As per path instructions, “test/**: Zero-dependency suite: node:test plus raw CDP over Node's built-in WebSocket. Flag added npm dependencies.”
Also applies to: 155-160
🤖 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 `@test/node/server.test.js` at line 8, Remove the Vite import and the corresponding build usage in the test setup around the affected preparation logic, keeping test/node/server.test.js dependent only on Node built-ins. Move dist/ generation into the existing pre-test script or CI workflow; if that cannot be done, document the approved exception instead.Source: Path instructions
src/core/autosave.js (1)
9-9: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider surfacing a failed autosave.
repository.savereturnsfalsewhen storage is full or unavailable.flushdrops that result, so the user gets no signal that edits stopped persisting.BoardListPagealready reports a failed board creation, so the shell has a place to show this.An
onErrorcallback oncreateAutosavewould letBoardCanvasorBoardPagerender the same warning.🤖 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 `@src/core/autosave.js` at line 9, Update createAutosave and its flush function to inspect the boolean result from repository.save and invoke an optional onError callback when saving fails. Thread this callback through the BoardCanvas or BoardPage integration so the shell displays the existing failed-operation warning consistently with BoardListPage.src/platform/storage.js (1)
95-106: 🗄️ Data Integrity & Integration | 🔵 TrivialConsider how two tabs on one board interact.
savereads the existing record, then writes a complete replacement. Web Storage offers no transaction, so two tabs open on the same board each write their own full snapshot, and the later write discards the other tab's edits. Autosave makes this automatic rather than user-driven.The
storageevent is the usual mitigation: a tab listens for changes to its own key and either reloads the board or warns the user. That work belongs in the shell rather than in this file, so treat this as a follow-up for the persistence design instead of a change to this diff.🤖 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 `@src/platform/storage.js` around lines 95 - 106, Keep save in src/platform/storage.js unchanged; treat cross-tab concurrent writes as a follow-up persistence concern. Implement any storage-event listener and reload or warning behavior in the shell layer, outside save and this storage module.src/core/board.js (1)
72-81: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRegenerate list item ids in
duplicate.
structuredClone(src)copies theitemsarray of a list, including each item id. The clone receives a new object id, but its rows keep the source rows' ids. Two list objects then hold rows with identical ids.No current caller breaks, because
toggleItem,onInput, andfocusItemSoonall scope the row lookup by object id first. The duplicate rows still violate the uniqueness thatnewItemestablishes, so any later code that indexes rows across objects would mix them up.♻️ Proposed fix to mint new row ids
duplicate(offset = 24) { if (!this.selection.size) return []; const clones = this.selection.list().map((id) => { const src = this.store.get(id); - return { ...structuredClone(src), id: this.newId(), x: src.x + offset, y: src.y + offset }; + const clone = { ...structuredClone(src), id: this.newId(), x: src.x + offset, y: src.y + offset }; + if (Array.isArray(clone.items)) { + clone.items = clone.items.map((item) => ({ ...item, id: this.newId() })); + } + return clone; });🤖 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 `@src/core/board.js` around lines 72 - 81, Update Board.duplicate to regenerate ids for every item in a cloned list, rather than retaining the ids copied by structuredClone(src). Preserve each item’s other fields while assigning a fresh id through the same uniqueness mechanism used by newItem, and ensure the cloned object’s items contain only the newly minted row ids.src/platform/input.js (1)
359-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
execCommand('insertText')compatibility assumption.
execCommandis deprecated, butinsertTextremains supported in current Chrome, Firefox, and Safari and preserves the native undo stack. Add a short comment explaining why this call remains. The repository has no declared browser matrix, and CI tests only Chromium; document the tested browser target or define the supported browsers.🤖 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 `@src/platform/input.js` around lines 359 - 364, Add a concise comment immediately before the execCommand('insertText') call in onPaste explaining that insertText is retained for current Chrome, Firefox, and Safari compatibility and native undo-stack preservation, and note the tested Chromium target or repository-supported browser scope. Leave the paste behavior unchanged.src/app.js (1)
20-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate
elementsat thecreateAppboundary. Checkelementsand its six required refs before constructing the renderer, input, and toolbar. The only source caller and all test callers passelements; remaininggetElementByIdcalls are unrelated DOM queries, not fallback usage.🤖 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 `@src/app.js` around lines 20 - 26, Update createApp to validate elements and all six required refs immediately at its boundary, before constructing the renderer, input, or toolbar. Reject missing or invalid elements explicitly, while preserving the existing caller-provided refs and unrelated getElementById queries.
🤖 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 @.github/workflows/deploy.yml:
- Around line 3-4: Update the deployment comment near the workflow
trigger/condition to document both automatic publishing after successful CI and
manual publishing via workflow_dispatch, unless the intended policy requires CI
success for every deployment, in which case remove workflow_dispatch from the
condition.
- Around line 42-49: Update the actions/checkout@v4 step in the deployment
workflow to set its ref to `${{ github.event.workflow_run.head_sha || github.sha
}}`. Keep the existing build and Pages artifact steps unchanged so workflow_run
deployments build the commit validated by CI.
In `@README.md`:
- Line 160: Update the React Router entry in the README dependency table to
remove the broad “v7 carries a CSRF advisory” rationale or replace it with a
precise reference to GHSA-qwww-vcr4-c8h2, its unstable RSC APIs, affected
version ranges, and patched versions 7.18.2 and 8.3.0.
In `@src/core/autosave.js`:
- Around line 8-13: Update the debounce flow used by createAutosave so
scheduler.debounce exposes a cancel handle for its pending timer, and have the
returned stop function remove the store listener and cancel that timer. Preserve
the existing flush behavior and return shape in createAutosave, while ensuring
stopped autosaves cannot execute a previously scheduled save.
In `@src/platform/input.js`:
- Around line 100-106: Update the pointer event wiring near the existing
pointerdown, pointermove, and pointerup registrations to also register
pointercancel with onPointerUp, reusing that handler’s gesture cleanup so
cancelled gestures clear gesture, end the active interaction, and remove the
panning state and marquee overlay.
- Around line 420-457: Update onKeyDown to return before board shortcut handling
when e.target is a form control, including input, textarea, select, or a
content-editable element. Preserve the existing Space handling only for non-form
targets, while retaining shortcuts for canvas and other non-editable targets.
In `@src/platform/renderer.js`:
- Around line 29-52: Guard the views[obj.type] lookup in createElement and
updateElement, and make createElement return null for unknown types instead of
throwing. Update sync to skip objects whose createElement result is null, while
preserving normal creation and update behavior for recognized types.
In `@src/platform/toolbar.js`:
- Around line 27-32: Update the keydown handler around the existing
e.target.isContentEditable guard to also ignore standard form controls,
including input elements such as the board title, before evaluating the ! and )
shortcuts. Preserve shortcut handling for canvas and other non-form targets.
In `@src/routes/BoardPage.jsx`:
- Around line 58-63: Update the title input’s onKeyDown handler so the Escape
branch marks the edit as cancelled before calling e.currentTarget.blur(),
allowing the nested onBlur handler to skip persisting the cancelled title;
preserve the existing draft reset and Enter behavior.
In `@test/browser/shell.test.js`:
- Around line 47-49: Update the browser test teardown wait condition around
onList and newBoard so it waits until window.app is absent and `#stage` no longer
exists before asserting unmount. Keep the existing .shell presence check only
where needed to confirm the list view, and ensure the unmount assertions run
after canvas teardown completes.
In `@test/coverage.js`:
- Around line 15-16: Update the coverage path handling around isMeasured so
paths from relative() are normalized by replacing backslashes with forward
slashes before filtering. Preserve the existing server.js and src JavaScript/JSX
matching rules after normalization.
In `@test/helpers/browser.js`:
- Around line 81-94: Update the goto method’s about:blank navigation flow to
wait until the blank document is committed, using the Page.frameNavigated event
or an equivalent committed-document condition, before issuing the appBase + to
navigation. Keep the existing readiness polling and timeout behavior unchanged.
In `@test/helpers/vite.js`:
- Around line 13-14: Update the Vite configuration options in the test helper to
convert both new URL values through fileURLToPath() instead of reading .pathname
directly. Apply this to configFile and root, preserving their existing URL
targets while producing native paths on Windows and decoding percent-encoded
characters.
- Line 1: Document Vite as an explicit orchestration exception for the test
suite, covering the dependencies in test/helpers/vite.js, test/run.js, and
test/node/server.test.js; alternatively, move the Vite-dependent code outside
test/** while preserving the existing test behavior.
- Around line 19-23: Update the Vite server setup flow after server.listen so a
missing server.resolvedUrls?.local?.[0] closes the server before throwing the
existing error. Preserve the successful return path in the helper that returns {
server, base }.
In `@test/node/server.test.js`:
- Around line 46-74: Update waitForOutput so child-process errors use a
dedicated handler that calls cleanup() before rejecting, rather than passing
reject directly to child.on('error'). Ensure teardown also resolves or completes
when the child emits error or close, preventing the timeout and pending teardown
from remaining active after a spawn failure.
In `@test/run.js`:
- Around line 72-79: Update the Promise wrapping child-process creation to
accept both resolve and reject, and register a one-time error handler on proc
that calls reject when spawn fails. Preserve the existing exit resolution while
ensuring the surrounding finally cleanup runs for rejected spawn attempts.
---
Duplicate comments:
In `@test/run.js`:
- Line 44: Move the startDevServer() acquisition and Chromium setup into an
outer try/finally cleanup scope, initialize vite, appBase, and chrome handles to
null, and close each resource only when it was successfully initialized. Ensure
setup failures still reach cleanup while preserving the existing test execution
flow.
---
Nitpick comments:
In `@src/app.js`:
- Around line 20-26: Update createApp to validate elements and all six required
refs immediately at its boundary, before constructing the renderer, input, or
toolbar. Reject missing or invalid elements explicitly, while preserving the
existing caller-provided refs and unrelated getElementById queries.
In `@src/core/autosave.js`:
- Line 9: Update createAutosave and its flush function to inspect the boolean
result from repository.save and invoke an optional onError callback when saving
fails. Thread this callback through the BoardCanvas or BoardPage integration so
the shell displays the existing failed-operation warning consistently with
BoardListPage.
In `@src/core/board.js`:
- Around line 72-81: Update Board.duplicate to regenerate ids for every item in
a cloned list, rather than retaining the ids copied by structuredClone(src).
Preserve each item’s other fields while assigning a fresh id through the same
uniqueness mechanism used by newItem, and ensure the cloned object’s items
contain only the newly minted row ids.
In `@src/platform/input.js`:
- Around line 359-364: Add a concise comment immediately before the
execCommand('insertText') call in onPaste explaining that insertText is retained
for current Chrome, Firefox, and Safari compatibility and native undo-stack
preservation, and note the tested Chromium target or repository-supported
browser scope. Leave the paste behavior unchanged.
In `@src/platform/storage.js`:
- Around line 95-106: Keep save in src/platform/storage.js unchanged; treat
cross-tab concurrent writes as a follow-up persistence concern. Implement any
storage-event listener and reload or warning behavior in the shell layer,
outside save and this storage module.
In `@test/node/server.test.js`:
- Line 8: Remove the Vite import and the corresponding build usage in the test
setup around the affected preparation logic, keeping test/node/server.test.js
dependent only on Node built-ins. Move dist/ generation into the existing
pre-test script or CI workflow; if that cannot be done, document the approved
exception instead.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 49940a2d-4368-47cf-9a8f-967686c48951
⛔ Files ignored due to path filters (5)
dist/assets/index-CGKfOYFM.jsis excluded by!**/dist/**dist/assets/index-CGKfOYFM.js.mapis excluded by!**/dist/**,!**/*.mapdist/assets/index-DUTNQzWv.cssis excluded by!**/dist/**dist/index.htmlis excluded by!**/dist/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (53)
.github/workflows/ci.yml.github/workflows/deploy.yml.gitignoreREADME.mdindex.htmlpackage.jsonpublic/index.htmlpublic/js/main.jsserver.jssrc/App.jsxsrc/app.jssrc/components/BoardCanvas.jsxsrc/core/autosave.jssrc/core/board.jssrc/core/geometry.jssrc/core/ids.jssrc/core/scheduler.jssrc/core/seed.jssrc/core/selection.jssrc/core/store.jssrc/core/viewport.jssrc/main.jsxsrc/platform/input.jssrc/platform/renderer.jssrc/platform/storage.jssrc/platform/toolbar.jssrc/platform/views.jssrc/routes/BoardListPage.jsxsrc/routes/BoardPage.jsxsrc/shell/storage.jssrc/styles/canvas.csssrc/styles/shell.csstest/browser/app.test.jstest/browser/shell.test.jstest/coverage.jstest/fixtures/site/assets/app.jstest/fixtures/site/index.htmltest/fixtures/site/style.csstest/helpers/browser.jstest/helpers/vite.jstest/node/autosave.test.jstest/node/board.test.jstest/node/geometry.test.jstest/node/ids.test.jstest/node/scheduler.test.jstest/node/seed.test.jstest/node/selection.test.jstest/node/server.test.jstest/node/storage.test.jstest/node/store.test.jstest/node/viewport.test.jstest/run.jsvite.config.js
💤 Files with no reviewable changes (2)
- public/index.html
- public/js/main.js
🛑 Comments failed to post (5)
src/core/autosave.js (1)
8-13: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
stop()leaves a pending debounced save armed.
stoponly removes the store listener. The timer thatscheduler.debouncecreated stays live, so a save scheduled in the lastdelaymilliseconds still runs aftercreateApp().destroy().
BoardCanvasdestroys the app on every route change, so this fires on normal navigation. The capturedboardIdandstorekeep the write itself correct, but the ordering is not safe: if the user returns to the same board and edits inside the window, the stale timer can write its older snapshot over the newer record.Give
debouncea cancel handle, then cancel or flush it instop.🐛 Proposed fix to cancel the pending save
In
src/core/scheduler.js, expose a cancel function:debounce(fn, ms) { let timer = null; - return (...a) => { + const run = (...a) => { clearTimeout(timer); timer = setTimeout(() => fn(...a), ms); }; + run.cancel = () => { + clearTimeout(timer); + timer = null; + }; + return run; },Then use it here:
export function createAutosave({ store, repository, boardId, scheduler, delay = 400 }) { const flush = () => repository.save(boardId, store.toJSON()); const save = scheduler.debounce(flush, delay); - const stop = store.on(save); - return { stop, flush, boardId }; + const off = store.on(save); + const stop = () => { + off(); + save.cancel?.(); + flush(); // persist the last edits instead of dropping them + }; + return { stop, flush, boardId }; }🤖 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 `@src/core/autosave.js` around lines 8 - 13, Update the debounce flow used by createAutosave so scheduler.debounce exposes a cancel handle for its pending timer, and have the returned stop function remove the store listener and cancel that timer. Preserve the existing flush behavior and return shape in createAutosave, while ensuring stopped autosaves cannot execute a previously scheduled save.src/platform/input.js (2)
100-106: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle
pointercancelso a gesture cannot get stuck.
onPointerUpis the only path that clearsgesture. The wiring at Line 466 registerspointerdown,pointermove, andpointerup, but notpointercancel.The browser fires
pointercancelinstead ofpointerupwhen it takes over the pointer, which happens on touch devices during a scroll or a gesture interruption.gesturethen stays non-null, so laterpointermoveevents keep dragging, resizing, or extending the marquee with no button pressed. A marquee element also stays in the overlay.Route
pointercancelthrough the same cleanup.🐛 Proposed fix to end the gesture on `pointercancel`
listen(window, 'pointerup', onPointerUp); + listen(window, 'pointercancel', onPointerUp);🤖 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 `@src/platform/input.js` around lines 100 - 106, Update the pointer event wiring near the existing pointerdown, pointermove, and pointerup registrations to also register pointercancel with onPointerUp, reusing that handler’s gesture cleanup so cancelled gestures clear gesture, end the active interaction, and remove the panning state and marquee overlay.
420-457: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Board shortcuts fire while the user types in the React title input.
onKeyDownis registered onwindowat Line 479. The only guards areeditingIdande.target.isContentEditable.editingIdis set byfocusinonlayer, so it staysnullfor any element outside the canvas. A plain<input>is notcontentEditable.
src/routes/BoardPage.jsxrenders<input className="board-bar-title" aria-label="Board title">outsidelayer. With focus in that field:
- Line 424 calls
preventDefault()forSpace, so the user cannot type a space in a board title.- Line 454 creates a card, envelope, or list when the user types
c,e, orl.- Line 440 deletes the current selection on
DeleteorBackspace.- Line 446 blocks caret movement with the arrow keys and moves the selected objects instead.
- Line 434 and Line 437 hijack Ctrl+A and Ctrl+D.
src/routes/BoardListPage.jsxuseswindow.prompt, which is not affected, but any future React form field has the same problem.Ignore key events that target a form control.
🐛 Proposed fix to skip editable form controls
+ /** True for anything the browser is already routing text into. */ + const isTextTarget = (target) => + target?.isContentEditable || + /^(INPUT|TEXTAREA|SELECT)$/.test(target?.tagName ?? ''); + function onKeyDown(e) { - if (e.code === 'Space' && !spaceDown && !editingId) { + if (isTextTarget(e.target)) return; + + if (e.code === 'Space' && !spaceDown && !editingId) { spaceDown = true; stage.classList.add('space'); e.preventDefault(); } - if (editingId || e.target.isContentEditable) return; + if (editingId) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** True for anything the browser is already routing text into. */ const isTextTarget = (target) => target?.isContentEditable || /^(INPUT|TEXTAREA|SELECT)$/.test(target?.tagName ?? ''); function onKeyDown(e) { if (isTextTarget(e.target)) return; if (e.code === 'Space' && !spaceDown && !editingId) { spaceDown = true; stage.classList.add('space'); e.preventDefault(); } if (editingId) return; const mod = e.metaKey || e.ctrlKey; const key = e.key.toLowerCase(); if (mod && key === 'z') { e.preventDefault(); e.shiftKey ? store.redo() : store.undo(); } else if (mod && key === 'a') { e.preventDefault(); selection.set(store.order); } else if (mod && key === 'd') { e.preventDefault(); board.duplicate(); } else if (e.key === 'Delete' || e.key === 'Backspace') { if (!selection.size) return; e.preventDefault(); board.deleteSelected(); } else if (e.key === 'Escape') { selection.clear(); } else if (e.key.startsWith('Arrow')) { if (!selection.size) return; e.preventDefault(); const step = e.shiftKey ? 20 : 1; board.nudgeSelection( ((e.key === 'ArrowRight') - (e.key === 'ArrowLeft')) * step, ((e.key === 'ArrowDown') - (e.key === 'ArrowUp')) * step, ); } else if (!mod && e.key.length === 1 && 'cel'.includes(key)) { commands.addAtCenter({ c: 'card', e: 'envelope', l: 'list' }[key]); } }🤖 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 `@src/platform/input.js` around lines 420 - 457, Update onKeyDown to return before board shortcut handling when e.target is a form control, including input, textarea, select, or a content-editable element. Preserve the existing Space handling only for non-form targets, while retaining shortcuts for canvas and other non-editable targets.src/platform/renderer.js (1)
29-52: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the
views[obj.type]lookup against an unknown object type.
createElementreadsviews[obj.type].create()andupdateElementreadsviews[obj.type].update(...). Neither checks that the type exists. An object with an unrecognizedtypethrows a TypeError insidesync, and the whole board fails to render.That input is reachable.
read()insrc/platform/storage.jsaccepts any record whoseboard.objectsandboard.orderare arrays; it does not validate each object'stype. A record written by an older version, or edited by hand, therefore reaches this code. The storage layer states that it tolerates junk from an older version, so the render path should not turn that record into a hard failure.Skip unknown types in
syncso one bad object cannot take down the board.🐛 Proposed fix to skip unknown object types
function createElement(obj) { + if (!views[obj.type]) return null; const el = views[obj.type].create(); el.dataset.id = obj.id; el.dataset.type = obj.type;Then handle the
nullinsync:let el = els.get(id); if (!el) { el = createElement(obj); + if (!el) continue; els.set(id, el); ids?.add(id); }🤖 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 `@src/platform/renderer.js` around lines 29 - 52, Guard the views[obj.type] lookup in createElement and updateElement, and make createElement return null for unknown types instead of throwing. Update sync to skip objects whose createElement result is null, while preserving normal creation and update behavior for recognized types.src/platform/toolbar.js (1)
27-32: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ignore canvas shortcuts in standard form controls.
Line 28 excludes only contenteditable targets. The board title is an
<input>. Typing!or)in that input also runscommands.fit()orcommands.resetZoom().Proposed fix
listen(window, 'keydown', (e) => { - if (e.target.isContentEditable) return; + const target = e.target; + if (target.isContentEditable || ['INPUT', 'TEXTAREA', 'SELECT'].includes(target.tagName)) return; // "!" and ")" are shift+1 / shift+0 on a US layout; the code covers the rest📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.listen(window, 'keydown', (e) => { const target = e.target; if (target.isContentEditable || ['INPUT', 'TEXTAREA', 'SELECT'].includes(target.tagName)) return; // "!" and ")" are shift+1 / shift+0 on a US layout; the code covers the rest if (e.key === '!' || (e.shiftKey && e.code === 'Digit1')) commands.fit(); else if (e.key === ')' || (e.shiftKey && e.code === 'Digit0')) commands.resetZoom(); });🤖 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 `@src/platform/toolbar.js` around lines 27 - 32, Update the keydown handler around the existing e.target.isContentEditable guard to also ignore standard form controls, including input elements such as the board title, before evaluating the ! and ) shortcuts. Preserve shortcut handling for canvas and other non-form targets.
Twelve findings from the second review pass. The one a user could hit: Escape in the title field restored the draft and called blur() — but blur() runs onBlur synchronously, before React re-renders, so commit() still read the typed-and-abandoned value and persisted exactly what had just been rejected. A ref flags the intent so commit() can bail. The test fails without the fix; the neighbouring Enter path had no coverage at all, so it gets a test too. CI and deploy: - Checkout now pins `github.event.workflow_run.head_sha`. A workflow_run event checks out the default branch, which need not be the commit CI just passed — the deploy could ship something that was never tested. - The header describes both entry points rather than implying CI gates every deployment; workflow_dispatch is deliberately ungated as the escape hatch. - README no longer claims v7 broadly "carries a CSRF advisory". GHSA-qwww-vcr4-c8h2 is scoped to the unstable RSC APIs, affects >=7.12.0 <7.18.2 and >=8.0.0 <8.3.0, and both lines are patched. The ^8.3.0 pin is on the patched side either way. Process and navigation lifecycles — the paths the last round's fixes left open: - test/run.js: a spawn failure emits 'error' and never 'exit', so the promise never settled and the finally that tears down Chromium never ran. - server.test.js: waitForOutput passed `reject` straight to 'error', skipping cleanup and leaving the timer live; teardown now settles on 'close' or 'error', since a child that failed to spawn has no exit to wait for. - helpers/vite.js: close the dev server when listen() succeeds but no local URL comes back, or the caller gets an exception instead of a handle to clean up. - helpers/browser.js: Page.navigate resolving means the request was accepted, not that the document committed. goto() now waits for the main frame to land on about:blank before navigating on, so the second navigation cannot replace an uncommitted blank and leave the old app state in place. CDPSession grew off()/once() for it, and #dispatch iterates a copy so a one-shot handler can unsubscribe mid-dispatch. Windows paths, which were wrong in two places and would have failed quietly: - coverage.js normalises relative() output to forward slashes; backslashes make isMeasured() reject every src/ file, emptying the report rather than failing. - fileURLToPath instead of URL.pathname in helpers/vite.js and helpers/browser.js — pathname keeps percent-encoding and yields /C:/... paths. Also: onList waited only for `.shell`, so the unmount assertions raced a teardown still in flight; it now waits for the canvas to actually be gone. Vite's role as the one framework dependency test/** may import is documented where it is imported. 275 tests, 99.6% line coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MKSLjnqN7rKPVNYjbg5J6H
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/node/server.test.js (1)
4-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the Vite API dependency from this test.
npm testruns beforenpm run buildin CI, so this test callsbuild()fromvitewhendist/index.htmlis absent. Builddistbeforenpm test, and keeptest/node/server.test.jslimited to Node built-ins.🤖 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 `@test/node/server.test.js` around lines 4 - 14, Remove the Vite build import and any build() usage from test/node/server.test.js, keeping the test dependent only on Node built-ins and the server exports. Update the test/build workflow so dist is generated before npm test runs, preserving the existing server test behavior without an in-test build fallback.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 @.github/workflows/deploy.yml:
- Around line 37-40: Update the checkout step in the deployment workflow to set
persist-credentials to false, and constrain workflow_dispatch deployments to
trusted refs while preserving the workflow_run head_sha selection. Use the
existing trigger and checkout configuration rather than changing unrelated
deployment behavior.
---
Outside diff comments:
In `@test/node/server.test.js`:
- Around line 4-14: Remove the Vite build import and any build() usage from
test/node/server.test.js, keeping the test dependent only on Node built-ins and
the server exports. Update the test/build workflow so dist is generated before
npm test runs, preserving the existing server test behavior without an in-test
build fallback.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 06be2d2a-c363-4ff5-867a-7e11b81579b3
📒 Files selected for processing (10)
.github/workflows/deploy.ymlREADME.mdsrc/routes/BoardPage.jsxtest/browser/shell.test.jstest/coverage.jstest/helpers/browser.jstest/helpers/cdp.jstest/helpers/vite.jstest/node/server.test.jstest/run.js
🚧 Files skipped from review as they are similar to previous changes (5)
- test/helpers/vite.js
- test/run.js
- test/coverage.js
- README.md
- test/helpers/browser.js
actions/checkout persists GITHUB_TOKEN into .git/config by default. Both jobs then run `npm ci` and `npm run build`, which execute code from the checked-out ref — lifecycle scripts, the vite config — with that token sitting in the workspace. Deploy is the one that matters, since it holds pages:write and id-token:write, but CI runs fork code on pull_request, so both get it. Neither job runs an authenticated git command, so this costs nothing. Not done: restricting workflow_dispatch to an allowlist of refs. Dispatching already requires write access, and the ref picker only offers refs in this repo — anyone who can choose a ref can push one to main instead. An allowlist would remove the escape hatch without closing a path that is open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MKSLjnqN7rKPVNYjbg5J6H
The one outside-diff finding: test/node/server.test.js imported `build` from vite to create dist/ when it was missing. The repo's own test/** instruction says node:test plus raw CDP, and a test file reaching for a bundler to manufacture its own fixture is exactly what that rule is about. Building dist/ moves to test/helpers/vite.js — already the documented Vite exception — and test/run.js calls it before spawning the suite. Vite is now imported in exactly one file under test/. The production-server test skips, with a message saying what to run, only on a bare `npm run test:node` against a never-built tree; under `npm test` dist/ always exists, so it never skips. Two things found on the way: - .coderabbit.yaml still pointed its layering rules at public/js/core/** and public/js/platform/**, which this PR moved to src/. Those two instructions — the ones the config calls the thing worth policing — have been matching nothing since the move. Repointed. - `npm run test:node` was broken before this branch: Node 24 will not take a bare directory for --test, so it died resolving `test/node` as a module without running anything. Now a quoted glob. The test/** instruction itself said "zero-dependency suite", which stopped being true the moment this PR adopted Vite. It now states the actual rule — Vite only in test/helpers/vite.js, a test file importing it is a finding — so it is enforceable instead of merely aspirational. 275 tests, 99.6% line coverage. Verified with dist/ removed, for both runners. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MKSLjnqN7rKPVNYjbg5J6H
|
Re: the outside-diff comment on You were right, and the rule you were applying is the repo's own. Building Verified with Two things surfaced while doing it, both worth a look:
I also rewrote the |
dist/ is in .gitignore, but dist/index.html was tracked anyway, and none of the assets it references were. A fresh checkout therefore got an index.html asking for ./assets/index-BUMevTdn.js and ./assets/index-C3A6Oezp.css, neither of which existed in the repository. That is not cosmetic. ensureBuild() skips when dist/index.html is present, so CI never built, and the production-server test — the one hardened last round to assert a real 200 containing the application root rather than `status > 0` — passed against that stale index. Verified against a clean `git archive` of the previous commit: the test goes green while / returns 200 and both assets it references return 404. The suite was proving the server could serve a file, not that it could serve the application. With dist/ untracked, a checkout has no dist/ at all, ensureBuild() builds it, and the assertion is made against an artifact that was actually produced from the source in the tree. Deploy already builds from source, so nothing there changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MKSLjnqN7rKPVNYjbg5J6H
Step 2 of supporting multiple boards. React owns the shell — routing, the board list, page chrome — and nothing else.
The canvas stays imperative
Pan, zoom and drag fire at frame rate, so putting board objects into React state would mean reconciling hundreds of components per pointer move.
BoardCanvasrenders the stage markup once and hands the refs tocreateApp;destroy()is the effect cleanup:That is the entire integration, and it only works because
elementswas already an injected argument.createAppnow requires it — thegetElementByIdfallback had no callers left.Routes
/for the board list,/b/:boardIdfor a board, so a board is linkable. Boards created from the list start empty; only a first-ever visit gets the starter board, which is a tour rather than content anyone asked for.Stack
React 19, Vite 8,
react-router8.The router is v8 rather than v7's
react-router-dombecause that package carries a high-severity RSC CSRF advisory (GHSA-qwww-vcr4-c8h2, affecting 7.12–8.2). It is not reachable in a client-only SPA, but it is not worth keeping in the tree.npm audit --omit=devis clean, and CI now fails on any high-severity production advisory so this cannot recur silently.Routing is hash-based because GitHub Pages has no SPA rewrite. Swapping to
BrowserRouteris one line once hosting moves.Layout and pipeline
Sources moved
public/js/**→src/**;public/is now Vite's static asset directory and the site builds todist/.dist/instead ofpublic/.Tests
The browser suite runs against Vite's dev server, not a bundle. Unbundled modules are served at their source paths, so V8 coverage attributes to
src/core/board.jsrather than one minified chunk; a sourcemap remapper would be a project of its own. CI builds the production artifact separately so the shipped output is still verified.Two harness bugs fixed along the way:
goto()pairedPage.navigatewith an immediatePage.reload, which aborted the in-flight navigation and left every page onabout:blank. It now bounces throughabout:blankinstead.server.test.jshardcoded port 3999 and assumed the oldpublic/layout. It now uses a free port and a checked-in fixture site, so it no longer depends on a build having happened.270 tests, 99.7% line coverage. Every React component and route is at 100%.
Known gaps
The toolbar is still imperative rather than a React component — converting it needs
useSyncExternalStoreover the store and viewport, which would have doubled this diff.Summary by CodeRabbit