Repository navigation
Put the boards on Supabase: accounts, persistence, live collaboration, sharing - #5
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe application now supports Supabase authentication, asynchronous persistent boards, realtime operation and cursor collaboration, invite-based sharing, row-level security policies, and local integration testing. Local storage and offline authentication remain available without Supabase configuration. ChangesSupabase collaboration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Store
participant Realtime
participant Repository
User->>App: Open board
App->>Repository: Load persisted board
Repository-->>App: Return board snapshot
App->>Store: Hydrate and replay local operations
User->>Store: Edit board
Store->>Realtime: Broadcast local operations
Realtime->>Store: Apply validated remote operations
Store-->>App: Render synchronized board
App->>Repository: Autosave when elected writer
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 review |
✅ Action performedReview finished.
|
Every repository method now returns a promise. Web Storage answers instantly
and could have stayed synchronous, but the contract is the one a server-backed
repository has to satisfy, and callers written against a synchronous seam are
callers that get rewritten the day the boards live on a server.
createApp stays synchronous — building the object graph needs no I/O — and
gains hydrate(), which does the round trip. The canvas mounts and accepts
edits while the board is still in flight, and destroy() cancels a hydrate that
has not landed yet: React unmounts a route far faster than a load completes,
and writing to the store afterwards would repopulate a torn-down app and start
an autosave with nothing left to stop it.
Two bugs the async seam exposed, both invisible against Web Storage and
routine against a network:
- a board title read at mount could resolve after the user had renamed the
board, putting the stale title back on screen
- the list page claimed "No boards yet" before the read had resolved, which
tells someone with boards that they have none
The list page now distinguishes "not read yet" from "no boards", and marks
reads and writes in flight so the browser tests wait for a settled list rather
than assuming a click has already landed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BmY2ZNmFSyP86hxGPPNuCX
boards and board_members, shaped by the BoardRepository contract rather than the other way round: list() is one indexed query, load() is a primary key lookup, save() is an upsert. Ownership is immutable, updated_at is stamped server-side — a client clock is not evidence of recency — and the document check mirrors isBoard(), so a malformed board is refused at the same place whichever repository is in front of it. Policies are exercised against a real Postgres in test/db/, every statement running as `authenticated` with a JWT claim the way PostgREST issues it. RLS is what keeps one person's boards out of another's, and it is not code that can be reviewed into correctness. board_role() is SECURITY DEFINER, and the reason is worth recording because a behavioural test cannot catch it. Called from the policies on boards, it also reads boards; under invoker rights that read is subject to the policy that called it, and Postgres unwinds it as `stack depth limit exceeded`. The schema survives without the keyword today only because every policy tests owner_id first and OR short-circuits before the call is reached — incidental, not structural, and one reordered disjunct from breaking. So the test asserts prosecdef directly rather than pretending behaviour covers it. test/db/ stays out of `npm test`: it needs a Postgres, not a browser, and skips itself when DATABASE_URL is unset. stub-auth.sql installs the auth surface a bare Postgres lacks, so the policies can be tested before anyone has Docker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BmY2ZNmFSyP86hxGPPNuCX
Generated by the CLI rather than written by hand, so the keys match the installed version instead of a guess at it. Three changes on top: the project id (init takes the working directory name, which here is the worktree), the site URL pointed at Vite rather than the static server, and anonymous sign-ins turned on. Anonymous sign-in is what lets a first visit get a real auth.users row without an account, so boards are server-backed from the first card instead of sitting in localStorage until someone signs up. Attaching an email later upgrades that same user, which is what stops the migration losing their work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BmY2ZNmFSyP86hxGPPNuCX
A first visit signs in anonymously. That is a real row in auth.users, so the
boards a guest makes are covered by the policies from the first card rather
than sitting outside them until someone decides to register; registering
attaches an email to that same user, which is what keeps their work. The gate
in RequireAccount is what stops a board list rendering before the session
settles — every read below it goes through row level security, and an empty
workspace on screen for a user who has boards is a lie the loading state does
not tell.
Everything above platform/auth.js works with an account — { id, email, guest }
— never a Supabase session, so the same shell runs against a project, against
nothing at all, and against a stub. A build with no VITE_SUPABASE_URL is the
local one and the published one: offline auth, no gate, no account menu.
start() memoises its promise because StrictMode mounts every effect twice, and
two anonymous sign-ins are two users — the second of which is the one that
sticks, with none of the first one's boards. A failed start drops the memo so
the retry is not permanently answering with the cached rejection.
Tested against a real Supabase rather than a fake one. The parts worth doubting
are exactly the ones a fake would have to invent — that a first visit produces
one guest, that registering keeps the same user id, that a rejected password
does not let anyone through — so test/run.js finds a local stack, serves a
second Vite origin that has been handed the project, and drives the gate there.
No stack, no suite: the account UI is then reported but left out of the
coverage total, with a line saying so, because averaging an unreachable file
into the number quietly lowers the bar for every other file. CI starts a stack,
so neither that suite nor the RLS one can skip before a merge.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
createSupabaseRepository satisfies the same contract as the Web Storage one — list / load / save / rename / remove, every call async, every call total — so nothing above shell/storage.js can tell which of the two it is holding. That was the point of making the contract async in the first commit of this branch, before there was anything asynchronous behind it. Access is not enforced in the client. list() is a select with no where clause, which is the correct way to ask for "my boards": the policies are what answer it, and a filter here would be a second, weaker copy of a rule that already exists in one place. save() updates and then inserts if there was nothing to update, rather than upserting. An upsert has to name owner_id, and an editor saving a board someone else owns would be sending their own id for a column freeze_board_owner refuses to let change; update-then-insert sends owner_id only on the path that really creates the row. Two tabs opening the same new board both fall through to the insert and one loses on the primary key, so that case retries the update it should have done — which is also, unchanged, how a viewer's save comes back false. Tested at both ends against a real stack. The contract tests sign in two anonymous users and drive them through PostgREST with exactly the authority the browser has, including the case that would be data loss rather than a refused request: a stranger's save falling through to an insert must not land on a board that already exists. The browser suite then proves the shell picked this repository at all — an edit flushed through autosave is still there after a reload, and the next account to sign in sees none of it. safeLocalStorage moves to its own module: the repository now depends on the account, so a helper shared with the auth client cannot live in either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
Every change was already a list of plain, serialisable ops with a computable
inverse; this is the commit that takes that literally. What a client applies
locally is exactly what it sends, and what it receives goes through the same
apply() the local UI uses — no second representation of a change, and so no
second implementation to keep honest.
Ops now carry an origin. A received one is applied with record: false, which
keeps somebody else's edit out of your undo stack, and marked REMOTE, which is
what stops it going straight back out.
The z-order op could not survive replication as it stood. `{ t: 'order' }`
names the whole array, and a sender cannot name an object they have not
received yet — applying their array literally dropped a concurrent addition out
of the order, out of all(), out of the render, and out of the next snapshot.
The object stayed in the map and left the document. An incoming order now
decides the relative depth of everything it mentions and leaves everything else
at the depth it already had, which also makes the op idempotent, which a
message that can arrive twice has to be. `add` is idempotent for the same
reason.
The channel is private, so joining it is authorised by row level security on
realtime.messages — the same board_role() that guards the table, with receive
and send split the way viewer and editor already are. That table has RLS on and
no policies by default, so the migration ships in the same change as the code
that joins it; without it every join is refused, which is the right way round.
Sending is throttled on a timer rather than batched to a frame. The first op of
a gesture goes out immediately and the rest at 50ms, so a drag is a handful of
messages instead of one per pointer move — and a timer keeps running in a tab
the compositor has stopped drawing, which requestAnimationFrame does not. That
was not a theory: the two-page browser test failed until it was a timer,
because the sending tab was in the background.
Two people editing at once still write their own snapshots on their own timers,
so the durable document is last-writer-wins until write authority lands. Ops
emitted between reading the snapshot and joining the channel are missed. Both
are in the README rather than implied.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
Ops kept the live documents identical, but the row is written by whole-document save and every editor had an autosave pointed at it. While everyone was in sync that was merely wasteful — the snapshots were identical — and when one of them was not, it wrote its stale document over work it had never seen. Broadcast made that both rarer and more expensive. Two halves, because neither is enough on its own. Election is the first. Presence names everyone on the board, the earliest joiner writes, ties break on id, and every client computes that from the same state and reaches the same answer without anyone deciding. A newcomer does not take the job from whoever is already doing it, and when the writer leaves the handover is announced so the client taking over saves what the last one may not have got round to. A board with nobody else on it, or one where the channel never came up, is a board this client writes. The version guard is the second, and it covers exactly what election cannot see: a client that has lost the channel elects itself and keeps saving. Its update names the version it last read, and there is no longer a row at that version. The counter is bumped by a trigger rather than chosen by the client — a writer that could pick the next number could pick one that keeps it winning — and it belongs to `doc` alone, so a rename is not a competing edit to the document and does not refuse the next honest save. Reading is what earns the right to write, which also fixes something that was wrong before: a board deleted while someone had it open used to be recreated by that client's next autosave, because a save that updated nothing fell through to an insert. A repository that has read the board no longer takes that path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
Cursors are broadcast rather than presence updates. Presence diffs a set and fans the whole thing out on every change, which is the right shape for a list of people and the wrong shape for something that moves at pointer rate — so presence keeps doing what it was already doing for the election, says who is here, and carries their name, and positions travel as their own throttled event. A name repeated on every message would be paid for hundreds of times a minute. Positions are in world coordinates, so a cursor points at the thing its owner is pointing at rather than at a place on their screen: two people at different zooms still agree about what is being pointed at, which is the only reason to draw someone else's pointer at all. They are drawn in the overlay, which is screen space, so the arrow and the name stay one size at any zoom — the same reasoning as the selection handles. The browser test pans the *receiving* page and expects the cursor to move with the board, because that is the difference between the two. A cursor leaves when its owner's pointer leaves, and again when presence says they have gone. A viewer's cursor is never seen: sending on the channel is an insert into realtime.messages, and the policy that stops a viewer broadcasting ops stops them broadcasting a position. Making that one visible means letting viewers write to the channel, and ops are applied by whoever receives them. Fixes a real bug found by the test for it. hydrate() replaced the document with the snapshot, so anything made while the board was still loading was discarded when it landed — and the canvas is deliberately interactive during exactly that window. Against Web Storage the window was a microtask and nobody ever saw it; against a network it is a round trip, and it is when an impatient person starts a card. Those ops are now kept and replayed onto the snapshot, which they can be because they are the same ops a remote client sends and the same apply() handles them: add is idempotent, del and set ignore what is not there, and order merges. Replayed unrecorded, so undo does not walk back past the load. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
board_members had policies and no way in. The obvious way in — invite by email
— cannot be built on what is here: auth.users is not readable by
`authenticated`, and most people on this app are anonymous and have no address
to be invited at. A link needs neither, and the person following one needs no
account beyond the guest session anonymous sign-in already gave them.
One live link per board, and it says what it grants. Changing the role changes
what the outstanding link is worth rather than minting a new one, because a
link already pasted into a chat should start granting the new role rather than
quietly dying. Revoking deletes it and the people who joined stay: they are
rows in board_members now, and the link is not what holds them there.
The token is the whole secret, so it is 122 bits from gen_random_uuid() rather
than anything derived from the board, and the invite row is readable only by
the owner — an editor is on the board and still cannot see how to hand it out.
Redeeming is a SECURITY DEFINER function because the point is to act on a row
the caller cannot see; it takes no identity, adds only the caller, and only at
the role the owner chose. Redeeming twice is a no-op rather than an error, and
`do nothing` rather than an upsert, so following a viewer link cannot cost an
editor their access. Every reason a token might not work gives the same answer.
board_people() is the other definer function, and it is the one worth being
careful about: it resolves user ids to addresses, so it is bounded to the owner
asking about their own board. There is no way to ask it about an address, only
about a board — it is a members list, not a lookup.
The test harness gains isolated browser contexts, and needed them. Two tabs on
one origin share Web Storage, so signing the second one in took the first one's
session away — the owner's own page quietly became the invitee, and the share
dialog correctly told it the board was not its own. A second person needs a
second browser context, which is now what `openApp({ isolated: true })` gives.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
The policy already allowed it — board_members_delete has always said an owner may take anyone off a board and anyone may take themselves off one. What was missing was any way to ask, and something worse: a shared board's card offered Delete, which the policies refuse. remove() answers true for a delete that matched nothing, so the board came back on the next read and looked like a bug rather than a refusal. So the card offers what is actually available. That needs the list to know whose board it is looking at, which is one more column on a query it was already making — asking board_role() per row would have been a round trip each. The local repository reports every board as owned, because nobody else can reach a browser's own storage, and a caller should not have to know which repository it is holding to read a summary. leave() is its own method rather than remove(boardId, myOwnId): it is a different thing to want, and nobody should have to look up their own id to do it. It is also the one place a `select` on a delete earns its keep — leaving a board you were never on has to be false, and a delete that matched nothing is otherwise indistinguishable from one that did. An owner cannot leave. They have no membership row to hand back, and a board with no owner is one nobody can share, delete, or ever get back — deleting your own board is what Delete is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
a553da6 to
fc8de56
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (21)
src/shell/storage.js-10-14 (1)
10-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify second-device access for anonymous accounts.
The PR flow starts users with anonymous accounts. Registration attaches an email identity to that account. Change “reachable from a second device” to state that the user must register and sign in with the same account first.
🤖 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/shell/storage.js` around lines 10 - 14, Update the storage behavior documentation near the shared auth decision to clarify that boards become accessible on a second device only after the user registers and signs in with the same account, replacing the vague “reachable from a second device” wording.test/node/sharing.test.js-125-134 (1)
125-134: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe "owner is listed first" claim holds by alphabetical accident.
board_peopleorders byrole desc, so the position of the owner depends on the role strings. Descending,viewersorts beforeowner, andownersorts beforeeditor. This test uses aneditormember, so['owner', 'editor']passes — but the same expectation with aviewermember would produce['viewer', 'owner']. The test name states a general invariant that the query does not provide.Either add a
viewercase to pin the real ordering, or narrow the test name to the roles under test.🤖 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/sharing.test.js` around lines 125 - 134, The test name claims a general invariant that "the owner is listed first", but the test only verifies this with an editor role. Since the database query orders by role descending (where viewer > owner > editor alphabetically), this invariant would fail with a viewer role. Either add a second assertion case using a viewer role alongside the existing editor case to pin the actual ordering behavior, or rename the test to specify that it only tests the owner-and-editor scenario instead of making a general claim about owner ordering.test/node/sharing.test.js-104-104 (1)
104-104: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
board({ objects: [] })passes an argument that the helper ignores.
boardis declared at line 42 as() => ({ v: 1, order: [], objects: [] })and takes no parameters. The{ objects: [] }argument is discarded, so this call is identical toboard(). The editor test at line 114 builds its payload inline, which confirms the helper has no override support. Drop the argument, so the call does not suggest a payload that it does not produce.💚 Proposed fix
- assert.equal(await bob.repository.save(id, board({ objects: [] })), false); + assert.equal(await bob.repository.save(id, board()), false);🤖 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/sharing.test.js` at line 104, Update the save call in the sharing test to invoke the zero-argument board helper directly, removing the ignored `{ objects: [] }` argument. Keep the existing board helper behavior and assertion unchanged.src/routes/JoinPage.jsx-22-35 (1)
22-35: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winA rejected
redeemleaves this page loading forever.
sharing.redeem(token)has no rejection handler. If the call rejects — offline, DNS failure, a 5xx that the client surfaces as a thrown error — neithernavigatenorsetFailedruns. The page then shows "Opening the board…" permanently, and the loading branch renders noLink, so the user has no way out of this route. The failure branch is already the correct answer for a token that cannot be redeemed.🐛 Proposed fix
- sharing.redeem(token).then((boardId) => { - if (!live) return; - if (boardId) navigate(`/b/${boardId}`, { replace: true }); - else setFailed(true); - }); + sharing + .redeem(token) + .then((boardId) => { + if (!live) return; + if (boardId) navigate(`/b/${boardId}`, { replace: true }); + else setFailed(true); + }) + // A link that could not be redeemed and a link that failed to answer + // are the same thing from here: ask for a new one. + .catch(() => { + if (live) setFailed(true); + });🤖 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/routes/JoinPage.jsx` around lines 22 - 35, Update the redeem flow in the useEffect around sharing.redeem so rejected promises also transition to the existing failure state. Add a rejection handler that respects the live cancellation guard before calling setFailed(true), while preserving the current successful navigation and cleanup behavior.src/routes/BoardPage.jsx-66-76 (1)
66-76: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReport or revert a rename that the repository refuses.
setTitle(trimmed)applies the new title optimistically. Ifrepository.renamereturnsfalseand the fallbackrepository.savealso returnsfalse, the code discards that answer. A viewer on a shared board hits exactly this path: the policies refuse both writes, and the bar keeps showing a title that was never stored until the next load. Restore the previous title, or surface an error, when both writes fail.🐛 Proposed fix to restore the committed title on refusal
setTitle(trimmed); renamed.current = true; if (await repository.rename(boardId, trimmed)) return; // A board seeded on a first visit has no stored record until autosave // fires, and rename() cannot touch what does not exist. Write one now — // otherwise the next autosave stores it under the default name and the // rename silently disappears. save() without a title keeps this one. - await repository.save(boardId, app.current?.store.toJSON() ?? EMPTY_BOARD, { title: trimmed }); + if (await repository.save(boardId, app.current?.store.toJSON() ?? EMPTY_BOARD, { title: trimmed })) return; + + // Neither write landed — the board is not this account's to rename. Put + // the stored name back rather than showing one that does not exist. + setTitle(title); + setDraft(title); + renamed.current = false;🤖 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/routes/BoardPage.jsx` around lines 66 - 76, Update the rename handler around repository.rename and the fallback repository.save to track whether either write succeeds; when both return false, restore the previously committed title or surface the existing error state instead of retaining the optimistic setTitle value. Preserve the current fallback save behavior for boards without stored records.src/routes/BoardListPage.jsx-43-51 (1)
43-51: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
mutatereports nothing when a mutation fails or throws.
erroris set only bycreate.remove,leave, andrenameroute throughmutate, which ignores the return value and has nocatch. Two consequences:
- A refused write returns
falseand the list refresh shows the board unchanged, with no explanation.ShareDialogsets an error for the samesharing.leaverefusal, so the two surfaces behave differently.- If
fn()orrefresh()rejects, the rejection escapes the click handler unhandled, and the list keeps stale rows.Consider having
mutateaccept a failure message and seterroronfalseor on rejection.♻️ Proposed shape
- const mutate = async (fn) => { - setBusy(true); - try { - await fn(); - await refresh(); - } finally { - setBusy(false); - } - }; + const mutate = async (fn, failure) => { + setBusy(true); + setError(null); + try { + if ((await fn()) === false) setError(failure); + } catch { + setError(failure); + } finally { + // The list is re-read either way: a refused write still leaves the + // truth on the server, and that is what should be on screen. + await refresh().catch(() => {}); + setBusy(false); + } + };🤖 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/routes/BoardListPage.jsx` around lines 43 - 51, Update the mutate function so callers provide an operation-specific failure message, set error when the mutation returns false or when fn or refresh rejects, and preserve the busy-state cleanup in finally. Update remove, leave, and rename call sites to supply appropriate messages, ensuring failed operations do not leave stale rows or unhandled rejections.src/styles/shell.css-103-106 (1)
103-106: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStrengthen the focus indicator on the account inputs.
Line 104 removes the browser outline. The replacement is a border colour change only. A colour-only change is hard to see, and it disappears for users with forced colours or reduced colour perception. These inputs are on the sign-in gate, so keyboard users need a clear focus state.
Add a visible ring instead of removing the outline entirely.
♻️ Proposed focus style
.account-form input:focus { - outline: none; + outline: 2px solid var(--accent); + outline-offset: 1px; border-color: color-mix(in srgb, var(--accent) 55%, var(--line)); }🤖 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/styles/shell.css` around lines 103 - 106, The .account-form input:focus rule removes the outline and relies solely on a border-color change for focus indication, which fails accessibility requirements for keyboard users and those with reduced color perception. Remove the outline: none declaration and instead add a visible ring property (such as outline or box-shadow) that provides a clear, non-color-dependent focus indicator alongside the border-color change. This ensures the focus state remains visible under forced colors and for all users navigating via keyboard.test/node/supabase-repository.test.js-156-165 (1)
156-165: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis ordering assertion has no tie-break margin.
The assertion at Line 164 depends on
firstreceiving a strictly laterupdated_atthansecond. Thelist()query orders byupdated_atonly, with no secondary sort key. If two saves land on the same timestamp, Postgres returns those rows in an arbitrary order and the test fails intermittently.Make the claim explicit by asserting the timestamps, or separate the two saves so the stamps cannot collide.
💚 Proposed fix
const ids = (await alice.repository.list()).map((b) => b.id); + const stamps = new Map((await alice.repository.list()).map((b) => [b.id, b.updatedAt])); + assert.ok( + stamps.get(first) > stamps.get(second), + 'the two saves landed on the same updated_at, so the order is arbitrary', + ); assert.ok(ids.indexOf(first) < ids.indexOf(second), 'updated_at was not restamped');A cheaper alternative is a secondary sort key in
list(), which also makes the production order stable.As per path instructions: "flag assertions that could be flaky — fixed sleeps standing in for a real readiness condition, or probabilistic claims without margin."
🤖 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/supabase-repository.test.js` around lines 156 - 165, The test assertion in the save-moves-board-back-to-front test depends on two saves receiving different updated_at timestamps without any guarantee of separation, causing flaky failures when they land on the same timestamp. Either add a small time delay between the alice.repository.save calls for the second save and the subsequent save of first (so their timestamps cannot collide), or replace the indexOf comparison assertion with an explicit timestamp assertion that directly compares the updated_at values of the first and second boards from the list result.Source: Path instructions
src/platform/supabase-repository.js-59-72 (1)
59-72: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn an empty list when
auth.current()is null instead of marking all boards as not owned.When
auth.current()returns null before the account is populated,list()setsmetoundefined. The comparisonrow.owner_id === methen evaluates to false for all rows, causingBoardListPageto show a "Leave" button for boards the caller actually owns. Thesave()method at line 145 already checks for null account explicitly (if (!ownerId) return false;), confirming this condition is reachable. Return an empty list when there is no account to prevent incorrect UI labels.Proposed fix
const me = auth.current()?.id; + if (!me) return []; return data.map((row) => ({ ...toSummary(row), owned: row.owner_id === me }));🤖 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/supabase-repository.js` around lines 59 - 72, Update the list() method to check the result of auth.current() before mapping board rows; when no account is present, return an empty list immediately. Preserve the existing owned calculation and summary mapping for authenticated callers.test/browser/auth.test.js-32-35 (1)
32-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
uniqueEmailcan collide across runs.The address is derived from
process.pidand a counter that restarts at 0 on every run. The operating system reuses pids. If a later run gets a pid that an earlier run already used, the first generated address matches a registration row that the earlier run left behind. The registration test then fails against an existing account, and the failure looks like a bug in the sign-up path.Add a per-run value that does not repeat.
test/node/sync.test.jsline 62 already usesDate.now()for the same reason.🔧 Proposed fix
- const uniqueEmail = () => `peerthink-${process.pid}-${counter++}`@example.com``; + const run = `${process.pid.toString(36)}${Date.now().toString(36)}`; + const uniqueEmail = () => `peerthink-${run}-${counter++}`@example.com``; let counter = 0;🤖 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/browser/auth.test.js` around lines 32 - 35, Update the uniqueEmail generator to include a per-run Date.now() value alongside the process ID and counter, ensuring generated registration addresses remain unique even when process IDs are reused. Keep the existing counter-based uniqueness within the run.Source: Path instructions
test/node/sync.test.js-194-206 (1)
194-206: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThis test leaves Alice's channel subscribed.
Every other test destroys both sides. This one destroys
his.synconly, sohers.syncstays subscribed untilafter()callsremoveAllChannels(). The comment at lines 100-102 states why an open realtime handle matters. Destroyhers.syncat the end of the test.🔧 Proposed fix
assert.equal(his.store.has('c1'), false, 'a destroyed sync was still listening'); + + await hers.sync.destroy(); });🤖 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/sync.test.js` around lines 194 - 206, Update the “destroy stops sending, and stops receiving” test to destroy hers.sync after the assertion, ensuring both sync instances created by join are cleaned up before the test completes while preserving the existing assertion behavior.test/browser/boards.test.js-353-354 (1)
353-354: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the flush result here, as line 106 does.
flush()returns whether the write was accepted. This call discards it. If the write is refused, the test continues and fails later at line 362 while waiting for the card to reach the other page. The reported failure then points at sharing instead of at the refused save.🔧 Proposed fix
await page.waitFor('Boolean(window.app.autosave)'); - await page.eval('window.app.autosave.flush()'); + assert.equal(await page.eval('window.app.autosave.flush()'), true, 'the write was refused');🤖 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/browser/boards.test.js` around lines 353 - 354, Update the page.eval call at line 354 that invokes window.app.autosave.flush() to capture and assert its return value, matching the pattern used at line 106. The flush method returns a boolean indicating whether the write was accepted, and currently this result is discarded. Asserting the return value ensures the test fails immediately if the save is refused instead of continuing and failing later with a misleading error message at line 362.Source: Path instructions
src/core/scheduler.js-37-55 (1)
37-55: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
throttledoes not provide cancellation, so pending runs may execute afterdestroy().The implementation returns only a trigger function. When
destroy()setsstopped = true, the pending timer still fires. The callbacks insync.jsinclude guards (if (!next || stopped) return;) that prevent sending, but they do not prevent timer execution or queue mutation.The test at lines 194-206 verifies that a destroyed receiver stops listening, but does not cover the case where a destroyed sender still has a queued throttle. Add a
cancel()method to the returned trigger that callsclearTimeout(timer)and resets state, or verify that all callers handle a pending run after teardown.♻️ Optional: expose a cancel handle
throttle(fn, ms) { let timer = null; let queued = false; const run = () => { fn(); timer = setTimeout(() => { timer = null; if (!queued) return; queued = false; run(); }, ms); }; - return () => { + const trigger = () => { if (timer) queued = true; else run(); }; + trigger.cancel = () => { + clearTimeout(timer); + timer = null; + queued = false; + }; + return trigger; },🤖 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/scheduler.js` around lines 37 - 55, Update throttle in the scheduler to expose cancellation on the returned trigger, with cancel clearing any pending timer and resetting queued/timer state so no deferred run executes after teardown. Update destroy() in the relevant receiver/sender flow to invoke this cancel handle, preserving existing trigger behavior before destruction.README.md-308-308 (1)
308-308: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove share links from "Not built yet".
This change ships share links. The Sharing section on lines 231-259 documents them, and
supabase/migrations/20260804193000_board_invites.sqlimplements them.📝 Proposed fix
-Share links and export. +Export.🤖 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 `@README.md` at line 308, Update the README feature-status list by removing “Share links” from the “Not built yet” entry, while preserving the existing Sharing documentation and other unfinished features.supabase/migrations/20260804193000_board_invites.sql-136-136 (1)
136-136: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
order by 3 descputsvieweraboveowner.Column 3 is the role text. Descending text order is
viewer,owner,editor, so the owner appears between the two other roles instead of first. Sort on the role's meaning rather than its spelling.🐛 Proposed fix
- order by 3 desc, 2 nulls last; + order by (case role when 'owner' then 0 when 'editor' then 1 else 2 end), email nulls last;🤖 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 `@supabase/migrations/20260804193000_board_invites.sql` at line 136, Update the ORDER BY clause in the board invite query to sort the role column by semantic priority, ensuring owner appears first, followed by the other roles in the intended order, while preserving nulls-last behavior.src/core/store.js-38-48 (1)
38-48: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
mergeOrdercan put the same id inordertwice.
keptfiltersincomingby object membership but does not deduplicate, andmergedstarts as a copy ofkept. Anorderop whose array repeats an id therefore yields a repeated id inthis.order.The consequences outlast the message.
all()returns the object twice, the renderer draws it twice, andtoJSON()writes the duplicate into the saveddoc. A laterdelremoves only the first occurrence, becausethis.order.splice(this.order.indexOf(op.id), 1)on line 124 matches once, soorderkeeps an id with no object andall()then yieldsundefined.Local
orderops come fromcore/board.jsand are well formed, but a remote one is applied straight from a peer broadcast insrc/platform/sync.js(line 133).🐛 Proposed fix
export function mergeOrder(current, incoming, objects) { - const kept = incoming.filter((id) => objects.has(id)); - const named = new Set(kept); - const merged = kept.slice(); + const named = new Set(); + // an id named twice is one position, not two: a duplicate in `order` is an + // object drawn twice and saved twice + const kept = incoming.filter((id) => objects.has(id) && !named.has(id) && named.add(id)); + const merged = kept.slice();🤖 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 38 - 48, Update mergeOrder to deduplicate incoming ids while retaining only ids present in objects, and build merged from that unique sequence. Preserve the existing current-order insertion behavior for valid ids so the resulting order contains each object id at most once.supabase/migrations/20260804154000_board_broadcast.sql-20-28 (1)
20-28: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAllow viewers to publish presence only.
board_broadcast_sendrejects a viewer’schannel.track(...), so other clients omit that viewer frompresenceState()and the member list. The viewer still subscribes and receives ops; a failedtrackdoes not changesync.readyfromSUBSCRIBED. Add anINSERTpolicy forextension = 'presence'whenboard_role(...) is not null, and restrict the existing send policy toextension = 'broadcast'.🤖 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 `@supabase/migrations/20260804154000_board_broadcast.sql` around lines 20 - 28, Create a new INSERT policy on realtime.messages that allows authenticated users with any valid board_role to insert presence messages by checking extension = 'presence', and update the existing board_broadcast_send policy to add a filter for extension = 'broadcast' so that it only restricts broadcast message inserts to users with 'owner' or 'editor' roles while allowing viewers to publish presence.src/app.js-195-195 (1)
195-195: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
commands.fit()discards a pan or zoom made during the load.The canvas is interactive from the first frame, and the load is now a network round trip. If the user pans or zooms in that window, this call resets the viewport when the snapshot lands. That is the same class of loss the
earlyop buffer was added to prevent, applied to the viewport instead of the document. Consider skipping the fit if the viewport has already been moved by the user.🤖 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` at line 195, Update the load completion flow around commands.fit() to avoid resetting the viewport when the user has already panned or zoomed during the network request. Track or reuse the existing viewport-interaction state and only call fit for an untouched viewport, preserving user camera changes when the snapshot arrives.test/db/apply.js-14-14 (1)
14-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the
pgimport.
test/**permits only Vite from npm, and only intest/helpers/vite.js. AdevDependencydeclaration does not makepgpermitted.🤖 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/db/apply.js` at line 14, Remove the import statement for pg from the top of the test file. The pg module import violates the constraint that only Vite is permitted as an npm import in test files outside of test/helpers/vite.js.Source: Path instructions
test/browser/app.test.js-26-26 (1)
26-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe doc comment names a binding the body never receives.
The body is called with
{ second, host, elements, hydrated }. There is noapp.📝 Proposed change
- * `body` is evaluated with `second`, `host`, `hydrated` and `app` in scope. + * `body` is evaluated with `second`, `host`, `elements` and `hydrated` in scope.🤖 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/browser/app.test.js` at line 26, Update the doc comment describing the body callback scope to list the actual bindings passed by the call—second, host, elements, and hydrated—and remove the nonexistent app binding.test/db/rls.test.js-12-12 (1)
12-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a comment explaining the
pgdependency exception in test/db/, or update the test layer guideline.The coding guidelines restrict test/* to Node built-ins with Vite only in test/helpers/vite.js. Both
test/db/rls.test.jsandtest/db/apply.jsimportpg, which violates this constraint.The practical justification is present: row-level security testing requires transaction control (
begin/commit/rollback), parameterized queries ($1,$2), and per-transaction role switching—all of which need a full PostgreSQL client. Raw wire protocol, HTTP endpoints, or CLI invocation viachild_processwould lose transaction boundaries or programmatic control.Either add a code comment documenting why
pgis necessary despite the guideline, or clarify the guideline to permit database clients in test infrastructure code.🤖 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/db/rls.test.js` at line 12, Add a concise comment near the pg import in the database test infrastructure explaining that pg is intentionally required for transaction control, parameterized queries, and per-transaction role switching, despite the test dependency guideline; apply the same documentation to the related apply.js usage if needed.Source: Path instructions
🧹 Nitpick comments (20)
test/node/sharing.test.js (1)
239-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename this test to match what it builds.
The test constructs a sharing service, not a repository, and it reuses alice's authenticated client — so what it pins is the client-side
auth.current()guard insidecreateSharing, not a database policy. A name such as'sharing with no account in hand is refused'states that.🤖 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/sharing.test.js` around lines 239 - 242, Rename the test case around createSharing to describe the client-side no-account guard rather than a repository policy, reflecting that it uses alice.client with auth.current() returning null and verifies sharing is refused.src/routes/BoardPage.jsx (2)
105-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove focus into the share dialog and return it on close.
ShareDialogrendersrole="dialog"but nothing moves focus into it. Keyboard focus stays on the Share button behind the backdrop, and Tab continues through the board underneath. On close, focus is not restored either. Add an initial focus target inside the dialog and a focus trap, then return focus to the Share button whenshowSharebecomes false.🤖 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/routes/BoardPage.jsx` around lines 105 - 114, Update the ShareDialog integration in BoardPage to provide an initial focus target and trap keyboard focus within the dialog while showShare is true; when the dialog closes, restore focus to the Share button that opened it. Use refs and the existing showShare lifecycle, preserving the current onClose and onLeave behavior.
27-44: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider handling a rejected title read.
titleOfawaitsrepository.load, which now performs a network call.createSupabaseRepository.loadconverts query errors intonull, but a transport-level failure still rejects the promise. This effect has no rejection handler, so such a failure produces an unhandled rejection and the field keepsDEFAULT_TITLEwith no signal.♻️ Proposed fix to swallow the read failure explicitly
- titleOf(boardId).then((current) => { - // Two ways this result is no longer wanted: the route moved on, or the - // user renamed the board while the read was still in flight. Applying it - // in either case puts a stale title back on screen — invisible against - // Web Storage, routine once the repository is a network away. - if (!live || renamed.current) return; - setTitle(current); - setDraft(current); - }); + titleOf(boardId) + .then((current) => { + // Two ways this result is no longer wanted: the route moved on, or the + // user renamed the board while the read was still in flight. Applying it + // in either case puts a stale title back on screen — invisible against + // Web Storage, routine once the repository is a network away. + if (!live || renamed.current) return; + setTitle(current); + setDraft(current); + }) + // A title that could not be read leaves the default in place; there is + // nothing to recover and nothing to report. + .catch(() => {});🤖 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/routes/BoardPage.jsx` around lines 27 - 44, Add explicit rejection handling to the titleOf promise in the useEffect for BoardPage, swallowing transport-level read failures without updating the title or draft and preventing an unhandled rejection. Preserve the existing live and renamed guards for successful results.test/node/supabase-repository.test.js (1)
233-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe first doc block describes a different test.
Two JSDoc blocks stack before
describe('write authority'). The first block describes two tabs racing one new board, which is the test at Line 339, not this suite. Move it to that test so each block documents what follows it.♻️ Proposed fix
- /** - * Two tabs, one new board, no row yet: both updates find nothing and both - * insert. One loses on the primary key and has to notice that the row it - * wanted now exists rather than reporting a failed save. - */ /** * The half of write authority that election cannot provide.Then place the removed block directly above the test at Line 339.
🤖 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/supabase-repository.test.js` around lines 233 - 245, Move the first JSDoc block describing two tabs racing to insert a new board from above describe('write authority') to directly above the corresponding test near the two-tab insertion race, while leaving the write-authority documentation immediately before describe('write authority').test/helpers/supabase.js (1)
28-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOn Windows this call fails as "no stack" instead of finding one.
execFileSyncwithout a shell cannot runnpx, becausenpxis a.cmdshim on Windows. The spawn error is caught at Line 38, so a running local stack is reported as absent and the Supabase suites skip silently.test/helpers/vite.jsalready handles Windows paths, so the project appears to support that platform.If Windows support matters here, resolve the platform-specific binary name.
♻️ Proposed platform-aware command
- const out = execFileSync('npx', ['supabase', 'status', '-o', 'json'], { + const npx = process.platform === 'win32' ? 'npx.cmd' : 'npx'; + const out = execFileSync(npx, ['supabase', 'status', '-o', 'json'], { cwd: ROOT,🤖 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/supabase.js` around lines 28 - 33, Update the Supabase status invocation in the helper’s stack-detection function to select the Windows-compatible npx command name when running on Windows, while retaining the existing command for other platforms. Keep the current execFileSync arguments and error handling unchanged..github/workflows/ci.yml (1)
47-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the Supabase CLI to an exact version in devDependencies.
The current devDependency declaration uses
"^2.111.0"(caret range), which resolves to any version from 2.111.0 to 2.999.x at run time. Between CI runs,npx supabase startmay pull different CLI versions, causing unpredictable changes to the stack version and migration behavior. Change the dependency to an exact version pin (e.g.,"2.111.0").Additionally, add
set -o pipefailto this step. Without it, a failure innpx supabase statusis masked by thejqpipe, leavingDATABASE_URLempty instead of failing the step.♻️ Proposed step hardening
- name: Start Supabase run: | + set -euo pipefail npx supabase start echo "DATABASE_URL=$(npx supabase status -o json | jq -r .DB_URL)" >> "$GITHUB_ENV"🤖 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 @.github/workflows/ci.yml around lines 47 - 50, Pin the Supabase CLI devDependency to an exact version such as 2.111.0 instead of a caret range, and update the “Start Supabase” workflow step to enable pipefail before the status-to-jq pipeline so failures propagate and prevent writing an empty DATABASE_URL.test/browser/auth.test.js (1)
203-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe comment does not describe what the test does.
The comment says the fresh guest "wants their own account instead". The test then submits the same
passwordand asserts that.account-emailshows that address. That is a sign-in to the existing account, which matches the test title. Correct the comment so it states that the second guest signs in to an account that already exists.🤖 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/browser/auth.test.js` around lines 203 - 213, Update the comment at the start of this test block to accurately describe the behavior: instead of saying the fresh guest "wants their own account instead," clarify that the second guest is signing in to an account that already exists using the same email and password. Ensure the comment aligns with the test title and the submitAccountForm call that uses the existing email and password credentials.test/node/autosave.test.js (1)
80-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
gatedduplicatesharness.
gatedrepeats the store, scheduler, recording repository, andcreateAutosavecall fromharnessat lines 10-23. The only difference is thecanWriteargument. Giveharnessan optionalcanWriteparameter and call it from both places.♻️ Proposed refactor
-function harness() { +function harness(canWrite) { const store = new Store(); const scheduler = createManualScheduler(); const saved = []; const repository = { load: async () => null, async save(boardId, board) { saved.push({ boardId, board }); return true; }, }; - const autosave = createAutosave({ store, repository, boardId: 'alpha', scheduler }); + const autosave = createAutosave({ store, repository, boardId: 'alpha', scheduler, canWrite }); return { store, scheduler, saved, autosave }; }Then replace the
gatedbody withconst gated = (canWrite) => harness(canWrite);.🤖 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/autosave.test.js` around lines 80 - 93, Add an optional canWrite parameter to the harness function defined at lines 10-23, and update the createAutosave call within harness to pass this parameter. Then replace the gated function body with a simple delegation that calls harness(canWrite), removing the duplicated store, scheduler, repository, and createAutosave logic.src/components/ShareDialog.jsx (1)
107-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
aria-modaland initial focus.The dialog closes on Escape and on the backdrop, but it does not tell assistive technology that the content behind it is inert, and it does not move focus into itself. A screen reader user keeps hearing the board list, and a keyboard user must tab in from wherever focus happened to be.
♻️ Proposed change
- <div className="share" role="dialog" aria-label={`Share ${title}`}> + <div className="share" role="dialog" aria-modal="true" aria-label={`Share ${title}`}> <header className="share-header"> <h2>Share “{title}”</h2> - <button type="button" data-action="close-share" onClick={onClose} aria-label="Close">✕</button> + <button + type="button" + data-action="close-share" + onClick={onClose} + aria-label="Close" + // the first thing a keyboard user reaches, and the safe default + autoFocus + >✕</button>A focus trap is a larger change and can follow separately.
🤖 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/components/ShareDialog.jsx` around lines 107 - 111, Add the aria-modal attribute to the main dialog container (the div with className="share" and role="dialog") to inform assistive technology that content behind the dialog is inert. Additionally, set initial focus to the close button (the button with data-action="close-share") when the dialog opens so keyboard and screen reader users immediately interact with the dialog rather than the underlying page content.supabase/migrations/20260804101204_boards.sql (2)
147-149: 🚀 Performance & Scalability | 🔵 Trivial
list()cost grows with the wholeboardstable, not with the caller's boards.
list()sends nowhereclause, soboards_selectdecides the row set. Theowner_iddisjunct is served byboards_owner_updated_idx, but every row that fails it reachespublic.board_role(id), which runs two lookups per row. As the table grows across all users, one person's board list pays for rows they will never see.If the list query becomes slow, add an explicit membership predicate on the client side, for example
owner_id = <uid>unioned with aninlist fromboard_members, so the index does the filtering and the policy only confirms it.🤖 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 `@supabase/migrations/20260804101204_boards.sql` around lines 147 - 149, The boards_select policy evaluates public.board_role(id) for every row that does not match owner_id, causing the cost to grow with the entire boards table rather than just the caller's accessible boards. Update the list() function or query to add explicit membership filtering using a WHERE clause that checks owner_id = authenticated user's uid OR id in a subquery joining board_members by user_id, so the index handles the row filtering before the policy evaluates access. This ensures expensive lookups in public.board_role(id) only run on rows that have already been narrowed to the caller's boards.
93-130: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSecurity Misconfiguration (CWE-426): Untrusted Search Path
Exploitability: Theoretical
Pin
search_pathon all board trigger functions.Set
search_path = pg_catalog, public, pg_tempontouch_updated_at(),freeze_board_owner(), andbump_board_version(). This prevents caller-selected search paths from affecting unqualified function and operator lookups. The migrations do not grantauthenticatedschema-creation privileges, so this remains defense-in-depth.🤖 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 `@supabase/migrations/20260804101204_boards.sql` around lines 93 - 130, Set the function-level search_path to pg_catalog, public, pg_temp on touch_updated_at(), freeze_board_owner(), and bump_board_version(). Preserve each trigger function’s existing behavior while pinning lookup resolution against caller-controlled search paths.test/db/apply.js (1)
46-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProtect the migration error from a failing
rollback.If
client.query('rollback')rejects, that rejection replaces the original migration error. The failing migration name is then lost, which defeats the stated purpose of the per-file transaction. Swallow the rollback failure.♻️ Proposed change
} catch (error) { - await client.query('rollback'); + await client.query('rollback').catch(() => {}); error.message = `${name}: ${error.message}`; throw error; }🤖 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/db/apply.js` around lines 46 - 50, The rollback call inside the catch block can reject and replace the original migration error. Wrap the client.query('rollback') call in its own try-catch block to swallow any rollback failures, then continue with throwing the original error that already has the migration name prepended in its message. This preserves the original migration error details even when rollback itself fails.test/coverage.js (1)
29-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider failing when a
SUPABASE_ONLYentry no longer exists.The list holds hardcoded paths. If a file is renamed or removed, the stale entry stays silent and the intent is lost.
report()could assert that everyunmeasuredpath exists on disk, or at least appears inrows. That keeps the gate honest as the file set changes.🤖 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/coverage.js` around lines 29 - 41, Update the report() function to validate that every entry in the SUPABASE_ONLY array either exists on disk or appears in the rows collection. If any SUPABASE_ONLY entry cannot be found in either location, fail with a clear error message indicating which stale paths need to be removed or updated. This ensures the list stays synchronized as files are renamed or removed during development.src/app.js (1)
108-108: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake
hydrate()refuse a second call.
hydrateis part of the publicappsurface and has no re-entry guard. A second call loads the board again, then builds a second autosave and a second sync. The first autosave is never stopped andapp.autosaveis overwritten, sodestroy()releases only the last one.createBoardSyncalso documents that a second channel for the same topic throws. A single flag keeps the lifecycle honest.♻️ Proposed guard
let autosave = null; let sync = null; let cursors = null; let destroyed = false; + let hydrating = null;- async function hydrate() { + function hydrate() { + // One board, one load: a second call would build a second autosave and a + // second channel, and only the last of each would be torn down. + hydrating ??= load(); + return hydrating; + } + + async function load() {🤖 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` at line 108, Add a re-entry guard flag to the hydrate function to prevent it from being called more than once. At the start of hydrate, check if a flag (such as hydrated or hasHydrated) has already been set to true and return early if so; otherwise set the flag and proceed with the existing hydrate logic. This prevents duplicate board loads, prevents creating multiple autosave and sync instances, and ensures app.autosave is not overwritten and destroy() releases the correct instance.test/browser/shell.test.js (1)
110-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test rarely observes the state it names.
The assertion races the board read. If the read has already resolved,
seen.resolvedistrue, soseen.empty && !seen.resolvedisfalsewhatever the empty state was. The test then passes without ever reaching the loading state, and the comment above it describes an invariant that stays unverified. The test cannot fail spuriously, but it also cannot catch a regression.Hold the read open instead of racing it — inject a repository whose
list()resolves on command, assert[data-loading]is present and[data-empty]is absent, then resolve and assert the settled empty state.As per path instructions: "flag assertions that could be flaky — fixed sleeps standing in for a real readiness condition, or probabilistic claims without margin."
🤖 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/browser/shell.test.js` around lines 110 - 125, The test 'says nothing about being empty until the boards have been read' races the board read instead of holding it open, so it frequently passes without observing the loading state it claims to verify. Replace the racing approach by injecting a repository whose list() method resolves on command rather than immediately, then assert that [data-loading] is present and [data-empty] is absent while the read is in flight, resolve the injected list() promise, and finally assert the settled empty state using showsEmptyState(). This ensures the loading-state invariant described in the comment is actually tested rather than skipped.Source: Path instructions
src/shell/sync.js (1)
9-24: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe presence label is captured once and does not follow a later sign-in.
auth.current()runs whencreateSyncis called, which is duringhydrate(). The PR lets an anonymous user attach an email afterwards. If the user registers while a board is open, presence keeps reportingGuestuntil the board is rejoined. Passing a function instead of a resolved string, or re-tracking on an auth change, would keep the label current.🤖 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/shell/sync.js` around lines 9 - 24, The identity.label in createSync is evaluated once when createSync is called, capturing the user's auth state at that moment. If a user later signs in after initially being a guest, the label never updates. Update the identity.label to use a function that dynamically returns the current auth state (passing a function like () => auth.current()?.email ?? 'Guest' instead of evaluating auth.current() immediately), or alternatively set up a listener that re-tracks the presence when the user's auth state changes.test/browser/app.test.js (1)
174-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTwo tests re-inline the whole
withSecondAppscaffold.Lines 174-192 and lines 230-248 each rebuild the host element, the markup and the
elementsmap thatwithSecondAppalready builds. They opt out only because they needdestroy()at a chosen moment rather than in thefinally. An option on the helper —destroy: false, leaving teardown to the body — would let both tests use it and remove about forty duplicated lines.🤖 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/browser/app.test.js` around lines 174 - 183, The two test blocks around lines 174-183 and lines 230-248 are duplicating the host element creation, markup setup, and elements map initialization that the withSecondApp helper already provides. Add a configuration option to the withSecondApp helper (such as destroy: false or manualDestroy: true) to defer the destroy() call from the finally block, allowing callers to invoke destroy() manually at their chosen moment. Then refactor both duplicated test blocks to use withSecondApp with this new option enabled, and have each test explicitly call destroy() when ready instead of relying on the finally cleanup. This removes the forty lines of duplicated scaffold code while preserving the tests' control over destroy timing.src/components/BoardCanvas.jsx (1)
48-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueA failed load leaves no signal to the user.
The
catchdiscards the error. The canvas stays empty, which is safe:hydraterejects before it seeds or builds autosave, so nothing overwrites the stored board. But the user sees an empty board and cannot tell it apart from a new one, and has no way to retry other than a reload. Consider surfacing the failure throughonReadyor route state.🤖 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/components/BoardCanvas.jsx` around lines 48 - 50, Update the error handling in the app.hydrate().catch() block to communicate the failure to the user instead of silently discarding it. Use either the onReady callback or route state to surface the error, allowing the user to understand that the board failed to load and potentially retry without needing a full page reload. Preserve the existing safe behavior of not seeding or overwriting stored data when hydrate rejects.src/components/AccountMenu.jsx (1)
34-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
aria-expandedto the disclosure button.The button toggles the popover, but it does not report its state. A screen reader announces it as a plain button, so the user cannot tell whether the account form is open.
♻️ Proposed change
- <button type="button" data-action="save-account" onClick={() => setOpen((was) => !was)}> + <button + type="button" + data-action="save-account" + aria-expanded={open} + onClick={() => setOpen((was) => !was)} + > Save your boards </button>🤖 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/components/AccountMenu.jsx` around lines 34 - 45, Update the disclosure button in the AccountMenu component to include aria-expanded bound to the open state, so assistive technologies can identify whether the account popover is expanded or collapsed.test/node/auth.test.js (1)
291-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe assertion does not test the stated behavior.
The test name states that a client without storage keeps its session in memory. Both assertions only check that
client.authis truthy, which is true for any constructed client. The test passes even ifpersistSessionis wired backwards.Assert the observable difference instead: with a storage adapter supplied, a
setItemcall reaches theMapafter a session is written; with none supplied, theMapstays empty. If that is not reachable without a live project, assert the configuration thatcreateSupabaseClientproduces from each branch.🤖 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/auth.test.js` around lines 291 - 304, The test around createSupabaseClient must verify session persistence behavior rather than merely checking that auth exists. Exercise a session write and assert that the supplied storage adapter’s Map receives it, while the no-storage branch does not write to that Map; if session writing requires unavailable network access, assert the corresponding persistSession configuration produced by each client branch.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/BoardPage.jsx (1)
50-76: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
commit()has no failure path for a rejected rename or save.
repository.rename()andrepository.save()returnfalseon a refused write, and the code handles that. Against Supabase they can also reject on a network failure.commit()runs fromonBlur, so a rejection escapes as an unhandled rejection.Line 66 also sets the committed title before the write is confirmed. If both the rename and the save fail, the header shows a name that was never persisted, and
renamed.currentstaystrue, so the reload path at line 36 will not correct it.🛡️ Proposed fix
setTitle(trimmed); renamed.current = true; - if (await repository.rename(boardId, trimmed)) return; - - // A board seeded on a first visit has no stored record until autosave - // fires, and rename() cannot touch what does not exist. Write one now — - // otherwise the next autosave stores it under the default name and the - // rename silently disappears. save() without a title keeps this one. - await repository.save(boardId, app.current?.store.toJSON() ?? EMPTY_BOARD, { title: trimmed }); + try { + if (await repository.rename(boardId, trimmed)) return; + + // A board seeded on a first visit has no stored record until autosave + // fires, and rename() cannot touch what does not exist. Write one now — + // otherwise the next autosave stores it under the default name and the + // rename silently disappears. save() without a title keeps this one. + if (await repository.save(boardId, app.current?.store.toJSON() ?? EMPTY_BOARD, { title: trimmed })) return; + } catch { + // fall through: a rename that did not land must not stay on screen + } + + setTitle(title); + setDraft(title); + renamed.current = false;🤖 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/routes/BoardPage.jsx` around lines 50 - 76, Update commit() to handle rejected promises from repository.rename() and repository.save() with a failure path that restores the persisted title state, resets renamed.current, and avoids unhandled onBlur rejections. Only keep the new title committed after the write succeeds; ensure refused writes and thrown network errors both leave the header and reload behavior consistent.
♻️ Duplicate comments (4)
src/components/AccountForm.jsx (1)
21-42: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset
busyin afinallyblock and handle a rejected auth call.
submit()still awaitsauth.register()andauth.signIn()without acatch. A rejection skips line 30, so the submit button stays disabled and no error appears.🛡️ Proposed fix
const submit = async (event) => { event.preventDefault(); setBusy(true); setError(null); - const result = registering - ? await auth.register({ email, password }) - : await auth.signIn({ email, password }); - - setBusy(false); + let result; + try { + result = registering + ? await auth.register({ email, password }) + : await auth.signIn({ email, password }); + } catch { + setError('Something went wrong. Try again.'); + return; + } finally { + setBusy(false); + } + if (!result.ok) {🤖 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/components/AccountForm.jsx` around lines 21 - 42, Update the submit function to wrap the auth.register/auth.signIn await and result handling in try/catch/finally, display a suitable error when the auth call rejects, and always reset busy in finally so rejected requests cannot leave the form disabled.Source: Linters/SAST tools
test/node/sync.test.js (2)
81-88: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMake
waitForthrow on timeout.
waitForstill returnsfalsewhen the deadline passes, and lines 138, 287, 335, 355 and 357 discard the result. At line 138 a timeout means Bob never received the op, and the assertions at lines 145-146 then pass against an empty store. Throw inside the helper so every call site fails loudly.🔧 Proposed fix
const waitFor = async (predicate, label, timeout = 5000) => { const deadline = Date.now() + timeout; while (Date.now() < deadline) { if (predicate()) return true; await new Promise((r) => setTimeout(r, 20)); } - return false; + throw new Error(`timed out after ${timeout}ms waiting for ${label}`); };🤖 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/sync.test.js` around lines 81 - 88, Update the waitFor helper to throw an error containing the provided label when the timeout expires instead of returning false, while preserving its successful true result so all existing call sites fail loudly on unmet conditions.Source: Path instructions
90-90: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
settle()is a fixed sleep behind every negative assertion.Lines 143, 203, 224 and 245 wait 250 ms and then claim a message did not arrive. On a loaded runner the message can still be in flight, so the assertion proves nothing and the suite stays green while echo suppression,
destroy()or the channel policy is broken.Anchor each negative claim to a positive readiness condition: send a marker op after the op under test, wait for the marker to arrive, then assert the forbidden op is absent.
As per path instructions: "flag assertions that could be flaky — fixed sleeps standing in for a real readiness condition".
🤖 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/sync.test.js` at line 90, The settle() function provides only a fixed 250ms sleep before negative assertions instead of verifying actual readiness, causing flaky tests on loaded runners. Replace the fixed-sleep pattern at each negative assertion location (lines 143, 203, 224, and 245) with a marker-based readiness check: after the operation under test, send a marker operation, await its arrival as a positive condition confirming in-flight messages have completed, then assert the forbidden operation is absent.Source: Path instructions
src/components/ShareDialog.jsx (1)
33-43: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGive the initial load a failure path.
refresh()still has no rejection handler.Promise.allrejects ifsharing.inviteorsharing.peoplerejects.peoplethen staysnull, so the dialog shows "Loading…" indefinitely and the rejection is unhandled.🛡️ Proposed fix
useEffect(() => { let live = true; - refresh().then(({ link, members }) => { - if (!live) return; - setInvite(link); - setPeople(members); - }); + refresh().then( + ({ link, members }) => { + if (!live) return; + setInvite(link); + setPeople(members); + }, + () => { + if (!live) return; + setError('Could not load sharing for this board.'); + }, + ); return () => { live = false; }; }, [refresh]);🤖 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/components/ShareDialog.jsx` around lines 33 - 43, Update the initial loading effect around refresh in ShareDialog so rejected refresh promises are handled: preserve the live guard, set an appropriate non-loading error state or fallback when loading fails, and prevent the rejection from remaining unhandled. Keep successful link and member updates unchanged.
🧹 Nitpick comments (2)
test/node/autosave.test.js (1)
80-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
gated()duplicatesharness().
gated(canWrite)repeats the store, scheduler,savedarray and repository fromharness()at lines 10-23. OnlycanWritediffers. Giveharness()an optionalcanWriteparameter and call it from both places.♻️ Proposed refactor
-function harness() { +function harness({ canWrite } = {}) { const store = new Store(); const scheduler = createManualScheduler(); const saved = []; const repository = { load: async () => null, async save(boardId, board) { saved.push({ boardId, board }); return true; }, }; - const autosave = createAutosave({ store, repository, boardId: 'alpha', scheduler }); + const autosave = createAutosave({ store, repository, boardId: 'alpha', scheduler, canWrite }); return { store, scheduler, saved, autosave }; }
gatedthen becomesconst gated = (canWrite) => harness({ canWrite });.🤖 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/autosave.test.js` around lines 80 - 93, Refactor the test setup so the existing harness function accepts an optional canWrite parameter and passes it to createAutosave. Replace the duplicated setup inside gated with a call to harness({ canWrite }), preserving the existing defaults and returned test fixtures.src/platform/sync.js (1)
80-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
nowa required injectable dependency.
now = () => Date.now()defaults to a global in a browser adapter. This violates the requirement that every dependency arrive via factory argument. Remove the default and update call sites insrc/shell/sync.jsand test files to passnowexplicitly.🤖 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/sync.js` at line 80, Make the now dependency required in the sync factory by removing its Date.now default, then update all callers in the sync shell and tests to pass an explicit now implementation. Preserve existing timing behavior by using a Date.now-based function at production call sites and controlled functions in tests.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 `@README.md`:
- Around line 306-308: Update the README.md “Not built yet” section to remove
“Share links” and leave only “export” listed, preserving the existing
documentation that describes link-based sharing as available.
In `@src/platform/supabase-repository.js`:
- Around line 80-90: Update load() to delete the entry from versions when the
query confirms the board row is absent, while preserving the entry for query
errors or invalid data. Update remove() to request the deleted row and clear
versions only when that specific row was successfully deleted; retain the
existing version guard after errors or unauthorized zero-row deletions.
In `@src/platform/sync.js`:
- Around line 103-110: Update the flush callback in scheduler.throttle so
operation batches are removed from queue only after channel.send confirms
delivery with acknowledgements enabled. Handle “error” and “timed out” outcomes
by restoring the unsent ops to queue for retry after reconnection, while
preserving the stopped and empty-queue guards.
In `@src/routes/BoardListPage.jsx`:
- Around line 25-51: Handle rejected repository operations in the BoardListPage
loading and mutation flows: catch the initial repository.list() failure in the
useEffect and set the existing error message so boards no longer remains null or
produces an unhandled rejection; update mutate to catch failures from fn() or
refresh(), set error, and keep the rejection from escaping click handlers; clear
error after a successful mutation, and apply the same rejection handling to the
create handler’s repository.save call.
In `@test/helpers/supabase.js`:
- Around line 28-33: Add shell: true to the options object passed by the
execFileSync call in the Supabase status helper so npx resolves through npx.cmd
on Windows; preserve the existing command, working directory, output handling,
timeout, and catch behavior.
In `@test/node/sharing.test.js`:
- Around line 157-164: Update the “a member cannot remove anyone else” test to
add Carol as a board member before Bob’s removal attempt, then have Bob target
Carol’s member ID instead of alice.id. Assert that the board’s people() result
still includes Carol after the attempted removal, ensuring the unauthorized
deletion targets an actual board_members row.
- Line 4: Update test/node/sharing.test.js and all other test/** files with
direct Supabase npm imports to use the permitted createSupabaseClient export
from src/platform/supabase.js. In the member-removal test, create or add a
second non-owner member and target that member’s board_members row, preserving
the test’s authorization assertions while ensuring it exercises removal of a
real member.
In `@test/node/sync.test.js`:
- Around line 180-188: Replace the deliveries <= 2 assertion in the burst test
with a stable throttle-owned check: count outbound messages produced by the
throttle and assert fewer than the three applied operations, or assert
deliveries < 3 if that is the available observable. Update the assertion message
to describe the invariant as fewer messages than ops, avoiding an exact
wire-delivery ceiling.
---
Outside diff comments:
In `@src/routes/BoardPage.jsx`:
- Around line 50-76: Update commit() to handle rejected promises from
repository.rename() and repository.save() with a failure path that restores the
persisted title state, resets renamed.current, and avoids unhandled onBlur
rejections. Only keep the new title committed after the write succeeds; ensure
refused writes and thrown network errors both leave the header and reload
behavior consistent.
---
Duplicate comments:
In `@src/components/AccountForm.jsx`:
- Around line 21-42: Update the submit function to wrap the
auth.register/auth.signIn await and result handling in try/catch/finally,
display a suitable error when the auth call rejects, and always reset busy in
finally so rejected requests cannot leave the form disabled.
In `@src/components/ShareDialog.jsx`:
- Around line 33-43: Update the initial loading effect around refresh in
ShareDialog so rejected refresh promises are handled: preserve the live guard,
set an appropriate non-loading error state or fallback when loading fails, and
prevent the rejection from remaining unhandled. Keep successful link and member
updates unchanged.
In `@test/node/sync.test.js`:
- Around line 81-88: Update the waitFor helper to throw an error containing the
provided label when the timeout expires instead of returning false, while
preserving its successful true result so all existing call sites fail loudly on
unmet conditions.
- Line 90: The settle() function provides only a fixed 250ms sleep before
negative assertions instead of verifying actual readiness, causing flaky tests
on loaded runners. Replace the fixed-sleep pattern at each negative assertion
location (lines 143, 203, 224, and 245) with a marker-based readiness check:
after the operation under test, send a marker operation, await its arrival as a
positive condition confirming in-flight messages have completed, then assert the
forbidden operation is absent.
---
Nitpick comments:
In `@src/platform/sync.js`:
- Line 80: Make the now dependency required in the sync factory by removing its
Date.now default, then update all callers in the sync shell and tests to pass an
explicit now implementation. Preserve existing timing behavior by using a
Date.now-based function at production call sites and controlled functions in
tests.
In `@test/node/autosave.test.js`:
- Around line 80-93: Refactor the test setup so the existing harness function
accepts an optional canWrite parameter and passes it to createAutosave. Replace
the duplicated setup inside gated with a call to harness({ canWrite }),
preserving the existing defaults and returned test fixtures.
🪄 Autofix
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: 5933e4c1-8d17-41db-a9b6-b605e25e44cd
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (58)
.github/workflows/ci.yml.gitignoreREADME.mdpackage.jsonsrc/App.jsxsrc/app.jssrc/components/AccountForm.jsxsrc/components/AccountMenu.jsxsrc/components/BoardCanvas.jsxsrc/components/ShareDialog.jsxsrc/core/autosave.jssrc/core/scheduler.jssrc/core/store.jssrc/platform/auth.jssrc/platform/cursors.jssrc/platform/sharing.jssrc/platform/storage.jssrc/platform/supabase-repository.jssrc/platform/supabase.jssrc/platform/sync.jssrc/routes/BoardListPage.jsxsrc/routes/BoardPage.jsxsrc/routes/JoinPage.jsxsrc/shell/RequireAccount.jsxsrc/shell/auth.jssrc/shell/sharing.jssrc/shell/storage.jssrc/shell/sync.jssrc/shell/web-storage.jssrc/styles/canvas.csssrc/styles/shell.csssupabase/.gitignoresupabase/config.tomlsupabase/migrations/20260804101204_boards.sqlsupabase/migrations/20260804154000_board_broadcast.sqlsupabase/migrations/20260804171500_board_version.sqlsupabase/migrations/20260804193000_board_invites.sqltest/browser/app.test.jstest/browser/auth.test.jstest/browser/boards.test.jstest/browser/shell.test.jstest/coverage.jstest/db/apply.jstest/db/rls.test.jstest/db/stub-auth.sqltest/helpers/browser.jstest/helpers/cdp.jstest/helpers/supabase.jstest/helpers/vite.jstest/node/auth.test.jstest/node/autosave.test.jstest/node/scheduler.test.jstest/node/sharing.test.jstest/node/storage.test.jstest/node/store.test.jstest/node/supabase-repository.test.jstest/node/sync.test.jstest/run.js
🚧 Files skipped from review as they are similar to previous changes (43)
- .github/workflows/ci.yml
- supabase/migrations/20260804154000_board_broadcast.sql
- src/shell/sharing.js
- src/routes/JoinPage.jsx
- src/components/AccountMenu.jsx
- src/shell/auth.js
- src/shell/web-storage.js
- src/shell/sync.js
- src/components/BoardCanvas.jsx
- src/App.jsx
- .gitignore
- src/shell/RequireAccount.jsx
- src/platform/auth.js
- supabase/config.toml
- src/core/autosave.js
- src/core/store.js
- src/platform/sharing.js
- src/platform/cursors.js
- src/platform/storage.js
- src/styles/shell.css
- supabase/.gitignore
- src/shell/storage.js
- src/styles/canvas.css
- test/helpers/cdp.js
- package.json
- supabase/migrations/20260804101204_boards.sql
- test/browser/shell.test.js
- src/platform/supabase.js
- test/node/scheduler.test.js
- test/helpers/vite.js
- src/app.js
- test/node/storage.test.js
- test/node/auth.test.js
- test/db/stub-auth.sql
- supabase/migrations/20260804193000_board_invites.sql
- test/browser/auth.test.js
- supabase/migrations/20260804171500_board_version.sql
- test/db/apply.js
- test/node/store.test.js
- test/browser/app.test.js
- test/coverage.js
- test/node/supabase-repository.test.js
- test/browser/boards.test.js
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 17 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (14)
src/platform/auth.js (1)
77-83: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRelease the fulfilled startup memo before a later guest retry.
After a successful
start(),startingremains a fulfilled{ ok: true }promise. IfonAuthStateChangelater publishesnullfor an external sign-out, the retry effect inRequireAccountreceives cached success. It does not callsignInAnonymously, so the user remains without an account.Cache only the in-flight attempt, or clear the memo after it settles. Add coverage that emits
nullafter a successful start and then callsstart()again. This path is separate from the previously reported rejected-start path.Proposed fix
start() { - starting ??= startOnce().then((result) => { - if (!result.ok) starting = null; - return result; - }); + if (!starting) { + const attempt = startOnce().catch((error) => + failed(error, 'Could not start a session.'), + ); + starting = attempt; + void attempt.finally(() => { + if (starting === attempt) starting = null; + }); + } return starting; }🤖 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/auth.js` around lines 77 - 83, Update the startup memoization in start() so starting retains only the in-flight attempt and is cleared after both successful and failed settlement, allowing a later start() after an external null auth state to invoke startOnce() again. Add coverage for a successful start followed by a null auth-state emission and a second start() call, verifying the retry performs anonymous sign-in rather than returning cached success.test/db/apply.js (1)
59-59: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
import.meta.mainis undefined before Node 22.2.The project declares
"engines": { "node": ">=22" }, which admits Node 22.0 and 22.1.import.meta.mainlanded in 22.2.0. On 22.0 and 22.1 this condition is always false, andnode test/db/apply.js --stubexits without applying any migration and without reporting anything.
fileURLToPathis already imported at line 12, so the version-agnostic check costs nothing.🐛 Proposed fix
-if (import.meta.main) { +if (process.argv[1] === fileURLToPath(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/db/apply.js` at line 59, Replace the unsupported import.meta.main condition in test/db/apply.js with a Node 22.0-compatible entry-point check using the existing fileURLToPath import, so direct execution still invokes the migration logic while preserving module-import behavior.test/node/sync.test.js (3)
186-188: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
deliveries <= 2is a wire claim, not a throttle claim.The three ops share one throttle window on a manual scheduler, so the local split is deterministic. Delivery is not: the leading message and the trailed message are two hops, and Realtime can split or coalesce them. The ceiling of 2 therefore encodes an assumption about the network.
Assert the property you own. Count the outbound messages the throttle produced, or assert
deliveries < 3and state the reason as fewer messages than ops.As per path instructions: "flag ... probabilistic claims without margin".
🤖 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/sync.test.js` around lines 186 - 188, Replace the `deliveries <= 2` assertion with a deterministic assertion about the throttle’s outbound message count, or use `deliveries < 3` only if that is the owned behavior; update the assertion message to explain that fewer messages than operations are expected, without assuming Realtime’s exact delivery count.Source: Path instructions
81-88: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMake
waitForthrow when the deadline passes.
waitForreturnsfalseon timeout. Lines 138, 287, 335, 355, and 357 discard the return value. At line 138 a timeout means Bob never received the op, and the assertions at lines 145-146 then pass against an empty store, so the test reports success while delivery is broken.Throw inside the helper. The
assert.ok(await waitFor(...))wrappers stay valid, and their messages become unreachable rather than wrong.As per path instructions: "flag assertions that could be flaky".
🔧 Proposed fix
const waitFor = async (predicate, label, timeout = 5000) => { const deadline = Date.now() + timeout; while (Date.now() < deadline) { if (predicate()) return true; await new Promise((r) => setTimeout(r, 20)); } - return false; + throw new Error(`timed out after ${timeout}ms waiting for ${label}`); };🤖 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/sync.test.js` around lines 81 - 88, Update the waitFor helper to throw an error when its timeout deadline is reached instead of returning false; retain the existing successful true return and polling behavior so current assert.ok(await waitFor(...)) calls remain valid.Source: Path instructions
90-90: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
settle()is a fixed sleep behind every negative assertion.
settle()waits 250 ms, and lines 143, 203, 224, and 245 then claim a message did not arrive. On a loaded runner the message is still in flight after 250 ms, so the assertion proves nothing and the suite stays green while echo suppression,destroy(), or the channel policy is broken.Anchor each negative claim to a positive readiness condition. Send a marker op after the op under test, wait for the marker to arrive, then assert the forbidden op is absent.
As per path instructions: "flag assertions that could be flaky — fixed sleeps standing in for a real readiness condition".
🤖 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/sync.test.js` at line 90, Replace the fixed-sleep pattern used with settle() at the negative assertion checks by implementing a marker-based readiness condition. For each negative assertion (the checks claiming a message did not arrive), send a marker operation after the operation under test, wait for the marker to arrive, then assert that the forbidden operation is absent. This anchors each negative claim to a positive readiness signal, ensuring the assertion is reliable rather than depending on whether 250ms is sufficient on a loaded test runner.Source: Path instructions
src/platform/sync.js (3)
113-125: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRegister the store listener after the channel exists.
store.onOps(...)at line 113 runs beforeclient.channel(...)at line 123. Ifclient.channel()throws,createBoardSyncnever returns, so nothing can calldestroy()andunsubscribeStoreis unreachable. The listener stays on the store, and the next local op entersflush, which dereferences the uninitialisedchannelbinding inside a scheduler callback.Move the
store.onOpsregistration below theclient.channelcall.🤖 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/sync.js` around lines 113 - 125, Move the store.onOps registration and its unsubscribeStore assignment below the client.channel call in createBoardSync, ensuring the channel is successfully created before the listener can enqueue operations or invoke flush. Preserve the existing stopped and LOCAL checks and callback behavior.
103-111: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not drop op batches before delivery is confirmed.
queue.spliceremoves the ops beforechannel.sendresolves, andbroadcast.ackis not enabled, so a failed send discards the batch silently. Retain the batch and re-queue it when the send reports"error"or"timed out".🤖 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/sync.js` around lines 103 - 111, Update the flush callback and channel.send handling so queued ops are not permanently removed before delivery succeeds. Retain the batch from queue, and re-queue it when the send result reports “error” or “timed out”; preserve successful delivery and stopped-session behavior while avoiding reliance on broadcast.ack.
127-134: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate each remote op before you apply the batch.
Line 129 checks only that
payload.opsis an array.store.applythen readsop.obj.id,Object.keys(op.patch), andop.order.filterwithout guards. A malformed element throws mid-loop, after earlier ops in the batch have already mutatedobjectsandorderand beforeemitruns, so the store and the DOM diverge.The cursor handler at lines 136-142 already validates field by field. Apply the same standard here.
🤖 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/sync.js` around lines 127 - 134, Validate every element of message.payload.ops before calling store.apply in the broadcast handler: require the same per-field structure used by the cursor handler, including a valid op.obj.id, patch object, and order array. If any remote op is malformed, reject the entire batch before mutation; otherwise preserve the existing store.apply(ops, false, REMOTE) behavior.src/platform/supabase-repository.js (2)
116-146: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard returned rows before you read
lengthor index them.Line 121 reads
data.lengthand line 144 readsinserted.data[0].version. A successful PostgREST response can carrynullor an empty array when the read-back after a write is not permitted by the select policy. Either line then throws aTypeError, andsave()must never throw.The same shape appears at line 71 (
data.map) and line 165 (data.length), so fix all four together.🛡️ Proposed fix
- if (error) return []; + if (error || !data) return []; @@ - if (error || !data.length) return { error, wrote: false }; + if (error || !data?.length) return { error, wrote: false }; versions.set(id, data[0].version); @@ - if (!inserted.error) { + if (!inserted.error && inserted.data?.length) { versions.set(id, inserted.data[0].version); return true; } @@ - return !error && data.length > 0; + return !error && (data?.length ?? 0) > 0;🤖 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/supabase-repository.js` around lines 116 - 146, Update save() and the related read paths so nullable Supabase data is validated before use: guard data before data.map, require a non-empty array before checking data.length, and require inserted.data to contain a row before reading inserted.data[0].version. Ensure these guards return the existing failure result rather than throwing, preserving save()’s no-throw behavior across all four locations.
80-92: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear
versionswhen the row is confirmed absent or deleted.
load()keeps a stale version after the board disappears, andremove()never clears one. A latersave()then seesexpected !== undefined, finds nothing to update, and returnsfalseat line 137 instead of recreating the board the caller seeded.Also applies to: 174-177
🤖 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/supabase-repository.js` around lines 80 - 92, Update the repository’s version-tracking lifecycle: clear the entry for id from versions in load() whenever the row is absent, invalid/deleted, or otherwise confirmed missing, and also clear it in remove() after deletion. Preserve version assignment for successfully loaded boards so a later save can recreate a missing board instead of using a stale expected version.test/helpers/supabase.js (1)
28-33: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
execFileSync('npx', ...)still fails on Windows.On Windows,
npxisnpx.cmd.execFileSynccannot run a.cmdshim withoutshell: true; the call throwsENOENTorEINVAL. Thecatchon line 38 turns that intonull, so every Supabase-backed suite skips silently even when a local stack is running.🐛 Proposed fix
const out = execFileSync('npx', ['supabase', 'status', '-o', 'json'], { cwd: ROOT, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'], timeout: 60_000, + shell: process.platform === 'win32', });Node.js execFileSync npx.cmd Windows shell option required🤖 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/supabase.js` around lines 28 - 33, The execFileSync call with npx fails on Windows because it cannot execute the npx.cmd shim without shell support. Add shell: true to the options object passed to execFileSync alongside the existing cwd, encoding, stdio, and timeout properties to enable the command to run properly on Windows systems while preserving the existing behavior on other platforms.test/node/sharing.test.js (2)
157-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe unauthorized-removal test cannot fail.
Alice is the owner and has no row in
board_members. Bob's delete therefore affects zero rows even if the delete policy wrongly allowed him to remove any member. Add a third member and let Bob target that member.💚 Proposed test change
const id = await aliceBoard(); const { token } = await alice.sharing.share(id, 'editor'); await bob.sharing.redeem(token); + const carol = await signIn(); + await carol.sharing.redeem(token); - await bob.sharing.remove(id, alice.id); - assert.equal((await alice.sharing.people(id)).length, 2, 'an editor removed the owner'); + assert.equal(await bob.sharing.remove(id, carol.id), false); + assert.equal( + (await alice.sharing.people(id)).some((p) => p.id === carol.id), + true, + 'an editor removed another member', + );The
falseassertion depends on theremovefix insrc/platform/sharing.js.🤖 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/sharing.test.js` around lines 157 - 164, The test cannot fail because Alice as owner has no row in board_members, so Bob's remove operation affects zero rows regardless of authorization. Add a third member to the board (neither Alice nor Bob), then have Bob attempt to remove that third member instead of Alice. Verify the final member count still shows that Bob's unauthorized removal did not succeed.
4-4: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winTwo test files import
@supabase/supabase-jsdirectly. The path instructions fortest/**permit only the Vite import. Both files build a client themselves instead of going through the app's client boundary,createSupabaseClientinsrc/platform/supabase.js.
test/node/sharing.test.js#L4: replace thecreateClientimport withcreateSupabaseClientfromsrc/platform/supabase.jsand build the client through it insignIn.test/node/supabase-repository.test.js#L4: apply the same replacement in its ownsignIn.As per path instructions: "Vite is the one permitted npm import and only inside test/helpers/vite.js … Flag any other added npm dependency."
🤖 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/sharing.test.js` at line 4, In test/node/sharing.test.js at line 4, replace the import of createClient from `@supabase/supabase-js` with an import of createSupabaseClient from src/platform/supabase.js, then update the signIn function to build the client through createSupabaseClient instead of createClient directly. Apply the identical replacement in test/node/supabase-repository.test.js at line 4, updating its signIn function to use createSupabaseClient in the same way. This ensures both test files respect the path instructions by routing Supabase client creation through the app's client boundary rather than importing directly from an npm dependency.Source: Path instructions
src/platform/sharing.js (1)
51-54: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
revokestill reports success when nothing was deleted.
revokereturns!error. Under row level security a refused delete affects zero rows and raises no error.test/db/rls.test.jsline 309 shows exactly this: an editor's delete onboard_invitescompletes without an error and removes nothing.removeon lines 69-77 has the same problem.leaveon line 90 already returns the row count, so use that pattern in both.🐛 Proposed fix
async revoke(boardId) { - const { error } = await invites().delete().eq('board_id', boardId); - return !error; + const { data, error } = await invites().delete().eq('board_id', boardId).select('board_id'); + return !error && (data?.length ?? 0) > 0; },async remove(boardId, userId) { - const { error } = await client + const { data, error } = await client .from('board_members') .delete() .eq('board_id', boardId) - .eq('user_id', userId); + .eq('user_id', userId) + .select('board_id'); - return !error; + return !error && (data?.length ?? 0) > 0; },Line 101 reads
data.lengthdirectly. Usedata?.length ?? 0there too.Also applies to: 69-77
🤖 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/sharing.js` around lines 51 - 54, The revoke method (lines 51-54) and remove method (lines 69-77) both return success based only on the absence of an error, but under row level security a refused delete raises no error and affects zero rows. Update both methods to verify that rows were actually deleted by checking the returned data count (use the pattern from the leave method which already returns the row count), ensuring the methods return true only when the delete operation actually removed records. Additionally, update line 101 to safely access data length using the optional chaining pattern data?.length ?? 0 to handle cases where data may be undefined.
🧹 Nitpick comments (6)
test/node/store.test.js (1)
252-261: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for the inverse of a merged
orderop.The
ordercase inapplyrecords{ t: 'order', order: this.order.slice() }as its inverse and then rewritesthis.orderthroughmergeOrder. The inverse is replayed throughmergeOrdertoo, so undo restores the exact prior order only because the inverse array names every known id. No test in this suite covers that path.Add a recorded
orderop that the merge actually changes, then assert thatundo()reproduces the original order string.💚 Suggested test
+ test('undoing a merged order op restores the order it replaced', () => { + store.apply([{ t: 'add', obj: card('a') }, { t: 'add', obj: card('b') }]); + store.apply([{ t: 'add', obj: card('mine') }]); + const before = order(); + + // recorded, and naming less than the document holds + store.apply([{ t: 'order', order: ['b', 'a'] }]); + assert.notEqual(order(), before); + + assert.equal(store.undo(), true); + assert.equal(order(), before); + });🤖 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/store.test.js` around lines 252 - 261, Add a test in the order-operation section that applies an order operation whose result is modified by merging with the existing order, records the original order string, calls undo(), and asserts the order is restored exactly. Reuse the existing store setup, order helper, and REMOTE constants while ensuring the test exercises the recorded inverse through mergeOrder.test/browser/shell.test.js (1)
110-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMake loading-state observation deterministic.
The local
list()method resolves without awaiting an asynchronous operation. The test can therefore capture only the settled DOM, and its assertions allow that result. Add a delayed-read fixture, then assert that[data-loading]was observed before[data-empty].🤖 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/browser/shell.test.js` around lines 110 - 125, Update the browser test around the local list() fixture so its read operation is delayed and remains pending long enough to observe the loading state. Capture/assert that [data-loading] is present before releasing or completing the delayed read, then assert that [data-empty] appears afterward; replace the current race-tolerant assertions in “says nothing about being empty until the boards have been read” with this deterministic sequence.test/db/rls.test.js (1)
244-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up
caroleven when an assertion fails.Line 255 deletes carol only after the two assertions. If either assertion fails, the row stays in
auth.usersfor the rest of the run and for later runs. Delete the row in afinallyblock, or register the id with theafterhook that already cleans up alice and bob.🤖 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/db/rls.test.js` around lines 244 - 256, Update the test named “an owner sees every member; a member sees only themselves” so the temporary carol auth user is always deleted, even when an assertion fails. Move the cleanup into a finally block or register carol with the existing after-hook cleanup for alice and bob, while preserving the current assertions.test/browser/auth.test.js (1)
32-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the test address collision-proof across hosts.
uniqueEmailderives uniqueness fromprocess.pidand a counter. That is unique within one run on one machine. IfSUPABASE_URLpoints at a shared stack, two machines can produce the same pid and the same counter, and the registration then fails against a row that already exists. Add a random component.♻️ Proposed change
+import { randomUUID } from 'node:crypto'; + + const run = randomUUID().slice(0, 8); - const uniqueEmail = () => `peerthink-${process.pid}-${counter++}`@example.com``; + const uniqueEmail = () => `peerthink-${run}-${counter++}`@example.com``;🤖 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/browser/auth.test.js` around lines 32 - 35, Update the uniqueEmail generator to include a random component in addition to process.pid and counter, ensuring registration addresses remain collision-proof across hosts sharing the same SUPABASE_URL.test/node/supabase-repository.test.js (1)
233-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the first doc comment to the test it describes.
Two doc comments sit back to back above
describe('write authority'). The first one describes two tabs racing one new board, which is the test at line 339, not this describe block. Move it there so each comment sits above the code it explains.🤖 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/supabase-repository.test.js` around lines 233 - 245, Move the first JSDoc comment describing two tabs racing to insert a new board from above describe('write authority') to the specific test around the concurrent new-board updates at line 339, leaving the write-authority comment above its describe block.supabase/migrations/20260804101204_boards.sql (1)
94-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrigger functions are created without a fixed
search_path.board_role()pinssearch_path = public, pg_temp, but the three trigger functions added by these migrations do not. All three run with invoker rights, so the exposure is small. Pinning removes the dependency on the caller'ssearch_pathand makes the migrations consistent.
supabase/migrations/20260804101204_boards.sql#L94-L102: addset search_path = pg_catalog, public, pg_temptotouch_updated_at(), and the same tofreeze_board_owner()at lines 116-126.supabase/migrations/20260804171500_board_version.sql#L22-L37: add the same clause tobump_board_version().🤖 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 `@supabase/migrations/20260804101204_boards.sql` around lines 94 - 102, The trigger functions touch_updated_at(), freeze_board_owner(), and bump_board_version() lack a fixed search_path setting, creating inconsistency with board_role() which pins search_path to public, pg_temp. Add the clause `set search_path = pg_catalog, public, pg_temp` to each trigger function definition to ensure they always execute with a consistent search path regardless of the caller's settings. In supabase/migrations/20260804101204_boards.sql, add this clause to the touch_updated_at() function (lines 94-102) and to the freeze_board_owner() function (lines 116-126). In supabase/migrations/20260804171500_board_version.sql, add the same clause to the bump_board_version() function (lines 22-37).
🤖 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 `@package.json`:
- Around line 28-29: Remove the newly added pg and supabase dependencies from
package.json and replace the integration tests with the permitted test/**
toolchain: node:test, raw CDP, and Node’s built-in WebSocket, using Vite only
where allowed. If these dependencies are required, obtain and record an explicit
approved exception instead of leaving them unapproved.
In `@src/app.js`:
- Around line 145-167: Move the createAutosave initialization earlier in the
restore flow: keep it after store.load(saved.board) when saved data exists, but
before store.apply(early, false) or seed(board). Preserve the existing autosave
options and metadata assignments while ensuring replayed hydration edits and
seeded-board changes are observed and scheduled for saving.
In `@src/components/BoardCanvas.jsx`:
- Around line 48-50: Update the app.hydrate() failure handling in BoardCanvas so
a rejected migrateLegacy() or load() does not leave the canvas editable without
persistence. Show a visible error with a retry action, or establish a safe
persistence fallback before enabling edits; do not silently swallow the
rejection with an empty catch.
In `@src/core/autosave.js`:
- Around line 20-22: Update createAutosave’s flush and debounced save flow so a
rejected repository.save or a false result does not clear the board’s dirty
state; retain the unsaved snapshot and retry it when scheduling or
reconciliation occurs, including handling version-refused saves. Only mark the
board clean after repository.save resolves successfully, while preserving the
existing canWrite check and debounce behavior.
In `@src/platform/auth.js`:
- Around line 85-127: Handle promise rejections in signIn, register, and
signOut: wrap signInWithPassword, signUp, and updateUser calls in try-catch and
return failed(error, ...) using the existing operation-specific messages, while
preserving resolved error handling and success results. In signOut, move
publish(null) and starting = null into a finally block so local state resets
even when client.auth.signOut rejects. Add rejection coverage for all four auth
operations.
In `@src/platform/supabase.js`:
- Around line 35-38: The authentication flow is incomplete because disabling
detectSessionInUrl leaves confirmation codes unprocessed. Add a callback route
in the router that reads the URL’s code parameter and invokes
client.auth.exchangeCodeForSession(code), integrating the resulting
session/error handling with the existing auth.js flow; alternatively, restore
automatic URL detection until that callback exists.
- Line 1: Remove the createClient import from src/platform/supabase.js and move
it to src/shell/auth.js. Update the createSupabaseClient function (or whichever
factory function uses createClient) to accept createClient as an injected
parameter instead of importing it directly. Update the call site in
src/shell/auth.js to pass the imported createClient when invoking the factory
function.
- Around line 17-20: Update readSupabaseConfig to parse VITE_SUPABASE_URL and
accept only HTTPS endpoints, while allowing an explicit loopback-only exception
for local development over HTTP. Return null for malformed URLs or non-HTTPS,
non-loopback endpoints, and preserve the existing trimmed URL/anonKey result for
valid configurations.
In `@supabase/migrations/20260804193000_board_invites.sql`:
- Around line 125-136: The board_people query orders roles lexically, allowing
viewers to precede the owner. Wrap the UNION in a subquery with an explicit rank
column, order by rank first, then role and email, while keeping the declared
return columns limited to user_id, email, and role; add coverage for a viewer
member.
In `@test/browser/app.test.js`:
- Around line 260-269: Replace the fixed 30 ms timeout after await hydrated in
the test with an injected manual scheduler tied to the autosave timing
mechanism. Flush the scheduler’s pending timers after hydration resolves, then
assert that writes remains empty, preserving the existing result checks and
destroy-during-load scenario.
In `@test/db/apply.js`:
- Line 14: Clarify the `pg` dependency and execution model in the
`test/db/apply.js` utility: verify whether `pg` is already present in
`package.json`, and if it is new, document why raw SQL via `pg.Client` is
required instead of `@supabase/supabase-js` or the Supabase CLI, including the
Node-context limitation if applicable. Because this utility resides under
`test/**` but is not a test, either move it outside that constrained path or add
an explicit documented exception explaining the direct npm import.
In `@test/db/rls.test.js`:
- Line 12: The import of pg at the top of the rls.test.js file violates the path
instruction that only Vite is permitted as an npm import within test files.
Either remove the pg import and replace direct database calls with equivalent
operations through the existing Supabase client using PostgREST, or document a
clear justification in the PR explaining why pg is necessary and add it to the
exception list in the path instructions.
In `@test/helpers/cdp.js`:
- Around line 90-113: Update newIsolatedPage so any rejection during
Target.createBrowserContext, Target.createTarget, or the page CDPSession.connect
closes the already-open browser session and disposes the browser context when it
has been created. Wrap setup in failure cleanup while preserving the returned
dispose behavior for successful initialization.
---
Duplicate comments:
In `@src/platform/auth.js`:
- Around line 77-83: Update the startup memoization in start() so starting
retains only the in-flight attempt and is cleared after both successful and
failed settlement, allowing a later start() after an external null auth state to
invoke startOnce() again. Add coverage for a successful start followed by a null
auth-state emission and a second start() call, verifying the retry performs
anonymous sign-in rather than returning cached success.
In `@src/platform/sharing.js`:
- Around line 51-54: The revoke method (lines 51-54) and remove method (lines
69-77) both return success based only on the absence of an error, but under row
level security a refused delete raises no error and affects zero rows. Update
both methods to verify that rows were actually deleted by checking the returned
data count (use the pattern from the leave method which already returns the row
count), ensuring the methods return true only when the delete operation actually
removed records. Additionally, update line 101 to safely access data length
using the optional chaining pattern data?.length ?? 0 to handle cases where data
may be undefined.
In `@src/platform/supabase-repository.js`:
- Around line 116-146: Update save() and the related read paths so nullable
Supabase data is validated before use: guard data before data.map, require a
non-empty array before checking data.length, and require inserted.data to
contain a row before reading inserted.data[0].version. Ensure these guards
return the existing failure result rather than throwing, preserving save()’s
no-throw behavior across all four locations.
- Around line 80-92: Update the repository’s version-tracking lifecycle: clear
the entry for id from versions in load() whenever the row is absent,
invalid/deleted, or otherwise confirmed missing, and also clear it in remove()
after deletion. Preserve version assignment for successfully loaded boards so a
later save can recreate a missing board instead of using a stale expected
version.
In `@src/platform/sync.js`:
- Around line 113-125: Move the store.onOps registration and its
unsubscribeStore assignment below the client.channel call in createBoardSync,
ensuring the channel is successfully created before the listener can enqueue
operations or invoke flush. Preserve the existing stopped and LOCAL checks and
callback behavior.
- Around line 103-111: Update the flush callback and channel.send handling so
queued ops are not permanently removed before delivery succeeds. Retain the
batch from queue, and re-queue it when the send result reports “error” or “timed
out”; preserve successful delivery and stopped-session behavior while avoiding
reliance on broadcast.ack.
- Around line 127-134: Validate every element of message.payload.ops before
calling store.apply in the broadcast handler: require the same per-field
structure used by the cursor handler, including a valid op.obj.id, patch object,
and order array. If any remote op is malformed, reject the entire batch before
mutation; otherwise preserve the existing store.apply(ops, false, REMOTE)
behavior.
In `@test/db/apply.js`:
- Line 59: Replace the unsupported import.meta.main condition in
test/db/apply.js with a Node 22.0-compatible entry-point check using the
existing fileURLToPath import, so direct execution still invokes the migration
logic while preserving module-import behavior.
In `@test/helpers/supabase.js`:
- Around line 28-33: The execFileSync call with npx fails on Windows because it
cannot execute the npx.cmd shim without shell support. Add shell: true to the
options object passed to execFileSync alongside the existing cwd, encoding,
stdio, and timeout properties to enable the command to run properly on Windows
systems while preserving the existing behavior on other platforms.
In `@test/node/sharing.test.js`:
- Around line 157-164: The test cannot fail because Alice as owner has no row in
board_members, so Bob's remove operation affects zero rows regardless of
authorization. Add a third member to the board (neither Alice nor Bob), then
have Bob attempt to remove that third member instead of Alice. Verify the final
member count still shows that Bob's unauthorized removal did not succeed.
- Line 4: In test/node/sharing.test.js at line 4, replace the import of
createClient from `@supabase/supabase-js` with an import of createSupabaseClient
from src/platform/supabase.js, then update the signIn function to build the
client through createSupabaseClient instead of createClient directly. Apply the
identical replacement in test/node/supabase-repository.test.js at line 4,
updating its signIn function to use createSupabaseClient in the same way. This
ensures both test files respect the path instructions by routing Supabase client
creation through the app's client boundary rather than importing directly from
an npm dependency.
In `@test/node/sync.test.js`:
- Around line 186-188: Replace the `deliveries <= 2` assertion with a
deterministic assertion about the throttle’s outbound message count, or use
`deliveries < 3` only if that is the owned behavior; update the assertion
message to explain that fewer messages than operations are expected, without
assuming Realtime’s exact delivery count.
- Around line 81-88: Update the waitFor helper to throw an error when its
timeout deadline is reached instead of returning false; retain the existing
successful true return and polling behavior so current assert.ok(await
waitFor(...)) calls remain valid.
- Line 90: Replace the fixed-sleep pattern used with settle() at the negative
assertion checks by implementing a marker-based readiness condition. For each
negative assertion (the checks claiming a message did not arrive), send a marker
operation after the operation under test, wait for the marker to arrive, then
assert that the forbidden operation is absent. This anchors each negative claim
to a positive readiness signal, ensuring the assertion is reliable rather than
depending on whether 250ms is sufficient on a loaded test runner.
---
Nitpick comments:
In `@supabase/migrations/20260804101204_boards.sql`:
- Around line 94-102: The trigger functions touch_updated_at(),
freeze_board_owner(), and bump_board_version() lack a fixed search_path setting,
creating inconsistency with board_role() which pins search_path to public,
pg_temp. Add the clause `set search_path = pg_catalog, public, pg_temp` to each
trigger function definition to ensure they always execute with a consistent
search path regardless of the caller's settings. In
supabase/migrations/20260804101204_boards.sql, add this clause to the
touch_updated_at() function (lines 94-102) and to the freeze_board_owner()
function (lines 116-126). In
supabase/migrations/20260804171500_board_version.sql, add the same clause to the
bump_board_version() function (lines 22-37).
In `@test/browser/auth.test.js`:
- Around line 32-35: Update the uniqueEmail generator to include a random
component in addition to process.pid and counter, ensuring registration
addresses remain collision-proof across hosts sharing the same SUPABASE_URL.
In `@test/browser/shell.test.js`:
- Around line 110-125: Update the browser test around the local list() fixture
so its read operation is delayed and remains pending long enough to observe the
loading state. Capture/assert that [data-loading] is present before releasing or
completing the delayed read, then assert that [data-empty] appears afterward;
replace the current race-tolerant assertions in “says nothing about being empty
until the boards have been read” with this deterministic sequence.
In `@test/db/rls.test.js`:
- Around line 244-256: Update the test named “an owner sees every member; a
member sees only themselves” so the temporary carol auth user is always deleted,
even when an assertion fails. Move the cleanup into a finally block or register
carol with the existing after-hook cleanup for alice and bob, while preserving
the current assertions.
In `@test/node/store.test.js`:
- Around line 252-261: Add a test in the order-operation section that applies an
order operation whose result is modified by merging with the existing order,
records the original order string, calls undo(), and asserts the order is
restored exactly. Reuse the existing store setup, order helper, and REMOTE
constants while ensuring the test exercises the recorded inverse through
mergeOrder.
In `@test/node/supabase-repository.test.js`:
- Around line 233-245: Move the first JSDoc comment describing two tabs racing
to insert a new board from above describe('write authority') to the specific
test around the concurrent new-board updates at line 339, leaving the
write-authority comment above its describe block.
🪄 Autofix
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: d903dbda-6ced-4376-9474-2cf9236ffbe5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (58)
.github/workflows/ci.yml.gitignoreREADME.mdpackage.jsonsrc/App.jsxsrc/app.jssrc/components/AccountForm.jsxsrc/components/AccountMenu.jsxsrc/components/BoardCanvas.jsxsrc/components/ShareDialog.jsxsrc/core/autosave.jssrc/core/scheduler.jssrc/core/store.jssrc/platform/auth.jssrc/platform/cursors.jssrc/platform/sharing.jssrc/platform/storage.jssrc/platform/supabase-repository.jssrc/platform/supabase.jssrc/platform/sync.jssrc/routes/BoardListPage.jsxsrc/routes/BoardPage.jsxsrc/routes/JoinPage.jsxsrc/shell/RequireAccount.jsxsrc/shell/auth.jssrc/shell/sharing.jssrc/shell/storage.jssrc/shell/sync.jssrc/shell/web-storage.jssrc/styles/canvas.csssrc/styles/shell.csssupabase/.gitignoresupabase/config.tomlsupabase/migrations/20260804101204_boards.sqlsupabase/migrations/20260804154000_board_broadcast.sqlsupabase/migrations/20260804171500_board_version.sqlsupabase/migrations/20260804193000_board_invites.sqltest/browser/app.test.jstest/browser/auth.test.jstest/browser/boards.test.jstest/browser/shell.test.jstest/coverage.jstest/db/apply.jstest/db/rls.test.jstest/db/stub-auth.sqltest/helpers/browser.jstest/helpers/cdp.jstest/helpers/supabase.jstest/helpers/vite.jstest/node/auth.test.jstest/node/autosave.test.jstest/node/scheduler.test.jstest/node/sharing.test.jstest/node/storage.test.jstest/node/store.test.jstest/node/supabase-repository.test.jstest/node/sync.test.jstest/run.js
Ten findings from review, and a trap found while checking them. `start()` promised an answer and one path did not keep the promise. A client that *throws* — a transport that never came back — left the account gate on "Loading…" with no retry, because the caller is a .then() with nowhere to put a rejection. `revoke` and `remove` reported success for a delete that changed nothing. A delete the policies refuse matches no rows and reports no error, so a member revoking their owner's link came back true and the dialog cleared a link that was still live. Both now ask which rows went, the way leave() already did. A remote batch is validated before it is applied. `apply` trusts what it is handed, which is right in-process and wrong over a wire — an `add` with no `obj` threw on the way in and took the receiving session with it. The check lives in store.js, where the vocabulary is defined, rather than in the transport. The whole batch is dropped rather than the bad op filtered out: a sender emitting one op this client cannot read is a sender whose other ops mean something it does not understand either. The stub's auth.uid() is created only if absent. `--stub` is documented for a bare Postgres, but nothing stopped it being pointed at a real project, where `create or replace` would swap the function every RLS policy in the database reads. Verified against a real GoTrue: the definition is untouched. waitFor answered false on timeout, which made every bare `await waitFor(...)` a no-op — the thing never arrived, the test carried on, and whatever it asserted next passed for the wrong reason. It throws now. The fixed 250ms sleep behind the negative assertions is gone too: a message that *is* expected is sent after the one that is not, and the assertion is made once that has landed, so the thing being denied has demonstrably had its turn on the wire. Also: import.meta.main is undefined before Node 22.19 and package.json allows 22.0; both entry-point checks compare URLs instead. Guards on an insert that returns no row, and on the two forms, whose only way out of `busy` was the happy path. And the trap. Vite reads .env.local, so a developer who has run the app against a local stack was serving a Supabase build to the origin the offline suites test — 26 failures with nothing in the diff to explain them. test/run.js blanks the two variables for that server, so it is offline because this file says so rather than because of what is on disk, and a test now notices if it stops. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
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)
src/core/store.js (1)
131-139: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the overwritten object in the inverse.
If
op.obj.idalready exists, Lines 131-138 replace the object but retain its order entry. Line 139 records adelinverse. Undo then deletes the previous object instead of restoring it.When replacing an existing ID, record a cloned
addinverse with the prior object and its current index. Usedelonly for a newly inserted ID.Proposed fix
case 'add': { const obj = structuredClone(op.obj); + const previous = this.objects.get(obj.id); + const existingIndex = this.order.indexOf(obj.id); const index = op.index ?? this.order.length; this.objects.set(obj.id, obj); - if (!this.order.includes(obj.id)) this.order.splice(index, 0, obj.id); - inverse.unshift({ t: 'del', id: obj.id }); + if (existingIndex === -1) { + this.order.splice(index, 0, obj.id); + inverse.unshift({ t: 'del', id: obj.id }); + } else { + inverse.unshift({ + t: 'add', + obj: structuredClone(previous), + index: existingIndex, + }); + }As per path instructions: "Store mutations must go through apply(ops) with a correct inverse."
🤖 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 131 - 139, Update the mutation handling around the object replacement logic in apply(ops) so an existing obj.id records an inverse add containing the previously stored object clone and its current order index; retain the existing order entry. Record the del inverse only when the ID is newly inserted, preserving correct undo behavior for both cases.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 `@src/core/store.js`:
- Around line 33-34: Update the `isOp` validation for the `'set'` case to reject
patches with an own `id` property while preserving validation of string IDs and
object patches. Add a regression test covering a remote batch with an
ID-changing set patch, verifying the batch is rejected and store object IDs,
`objects`, and `order` remain consistent.
---
Outside diff comments:
In `@src/core/store.js`:
- Around line 131-139: Update the mutation handling around the object
replacement logic in apply(ops) so an existing obj.id records an inverse add
containing the previously stored object clone and its current order index;
retain the existing order entry. Record the del inverse only when the ID is
newly inserted, preserving correct undo behavior for both cases.
🪄 Autofix
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: dfc368cf-0011-4d2c-9e71-573ba9359876
📒 Files selected for processing (16)
src/components/AccountForm.jsxsrc/components/ShareDialog.jsxsrc/core/store.jssrc/platform/auth.jssrc/platform/sharing.jssrc/platform/supabase-repository.jssrc/platform/sync.jstest/browser/shell.test.jstest/coverage.jstest/db/apply.jstest/db/stub-auth.sqltest/node/auth.test.jstest/node/sharing.test.jstest/node/store.test.jstest/node/sync.test.jstest/run.js
🚧 Files skipped from review as they are similar to previous changes (10)
- test/run.js
- src/platform/supabase-repository.js
- src/platform/auth.js
- test/node/sharing.test.js
- test/db/apply.js
- src/components/AccountForm.jsx
- src/platform/sharing.js
- test/node/store.test.js
- test/node/auth.test.js
- src/platform/sync.js
The review arrived in three waves and I had triaged only the first. These are the substantive findings from the other two. board_people ordered by the role *text*, descending — which puts 'viewer' above 'owner'. Any board with a viewer listed them before its owner. Both existing tests used an editor, where 'owner' happens to sort higher, so they passed on a coincidence; the new one uses a viewer, which is what exposes it. The union is wrapped so the ordering has named columns to sort on, because across a UNION `order by` cannot see an expression over the output list at all. The replay added in the previous commit never scheduled a save. Autosave subscribes after the load, deliberately, so a restore does not write itself straight back — which meant the ops replayed onto the snapshot happened with nobody listening, and a card made while the board was loading persisted only if the user happened to edit again. It flushes explicitly now. A failed save left no trace. The debounced path discarded both rejections and a `false` result, so a write refused because someone else's version is current — or dropped because the network was — was never retried, and the session lost edits it was still holding. The board stays dirty until a write lands, and the retry rides the next settled edit rather than a timer, so a board nobody is touching does not hammer a server that just said no. The rest of the auth port got the rejection handling start() got last time: signIn, register and signOut all promised an answer and could reject instead. And two tests that could not fail. The unauthorized-removal test aimed Bob at Alice, who is the *owner* and has no row in board_members — so his delete matched nothing whatever the policy said, and it passed with the policy dropped entirely. It goes through Carol now. The isolated-page helper leaked its browser-level socket when setup failed part-way, turning a setup error into a hang with a different symptom than its cause. Also: npx is npx.cmd on Windows, where execFileSync would not have found it and the failure would have looked exactly like "no stack" — every suite skipping rather than reporting anything. And share links are built, so they are no longer in "Not built yet". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
`isOp` let `{ t: 'set', id: 'a', patch: { id: 'b' } }` through. `apply` assigns
the patch over the object, so the object's id changed while the key in
`objects` and the entry in `order` kept the old one — an object nothing can
address any more, and the old id freed for a later `add` to duplicate.
Not a crash, which is why it is worth a test: the document goes on working and
quietly stops making sense. The test asserts the damage `apply` does when asked
directly, then that the transport refuses to ask.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
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/helpers/supabase.js (1)
28-36: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
execFileSync('npx.cmd', ...)still fails on Windows withoutshell: true.The explicit
npx.cmdselection does not fix the underlying problem. Node'sexecFileSyncrequiresshell: trueto execute.cmd/.batfiles on Windows, even when the.cmdextension is given explicitly. Without it, the call throwsEINVAL, thecatchblock converts this tonull, and every Supabase-backed suite skips silently on Windows, exactly the failure mode the surrounding comment warns about.Add
shell: process.platform === 'win32'to the options object.🐛 Proposed fix
const out = execFileSync(process.platform === 'win32' ? 'npx.cmd' : 'npx', ['supabase', 'status', '-o', 'json'], { cwd: ROOT, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'], timeout: 60_000, + shell: process.platform === 'win32', });Does Node.js execFileSync require shell:true to run npx.cmd on Windows even when the .cmd extension is specified explicitly?🤖 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/supabase.js` around lines 28 - 36, Update the execFileSync options in the Supabase status invocation to include shell: process.platform === 'win32', while preserving the existing platform-specific npx executable selection and other options.
🤖 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 `@src/core/autosave.js`:
- Around line 29-37: The flush() async function does not protect the dirty state
invariant when repository.save() rejects. Wrap the repository.save() call (and
the following dirty assignment) in a try-catch block so that if the save fails,
dirty is set to true before rethrowing the error. This ensures the dirty
invariant—that dirty means the screen document is ahead of the stored one—is
maintained regardless of whether the caller at the public API entry points (such
as lines 174 and 193 in src/app.js) handles the rejection or not.
---
Outside diff comments:
In `@test/helpers/supabase.js`:
- Around line 28-36: Update the execFileSync options in the Supabase status
invocation to include shell: process.platform === 'win32', while preserving the
existing platform-specific npx executable selection and other options.
🪄 Autofix
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: 15f82fd8-a790-4f8c-9b19-f7e88bae7341
📒 Files selected for processing (9)
README.mdsrc/app.jssrc/core/autosave.jssrc/platform/auth.jssupabase/migrations/20260804193000_board_invites.sqltest/db/rls.test.jstest/helpers/cdp.jstest/helpers/supabase.jstest/node/sharing.test.js
🚧 Files skipped from review as they are similar to previous changes (6)
- supabase/migrations/20260804193000_board_invites.sql
- README.md
- test/node/sharing.test.js
- src/platform/auth.js
- test/db/rls.test.js
- src/app.js
Only the debounced wrapper set `dirty` on a rejection, so the two callers that use `flush()` directly — the replay after a load, and the save on taking over write authority — left a board that had failed to write looking clean. Nothing would then retry it, which is the whole point of the flag. It is set inside `flush` now and rethrown, so a caller that awaited the write still learns it failed while the retry is armed either way. The debounced path swallows, as before: it fires on every settled edit with nobody waiting. Tested through both paths, and for a refused write as well as a rejected one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
Two findings that were malformed on their first posting and re-stated as real ones. readSupabaseConfig accepted any non-empty URL, and every request the client makes carries the user's access token — so an `http:` project URL puts that token on the wire in clear text. It now parses the URL and requires https, with loopback the exception because there is no wire: that is the stack on this machine, which is how the tests and `npm run dev` reach it. A malformed URL is refused for the same reason a missing one is — the app runs on Web Storage instead, which is a working app rather than one making requests nobody can vouch for. And the node suites were assembling their own Supabase client, which is both a vendor import the test rules do not permit outside test/helpers/vite.js and a test of a client the app does not use. They go through createSupabaseClient now, so the adapter under test is the one that ships — and the URL guard above covers them too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
Three corrections to "Not built yet". A refused save is retried now — on the next settled edit, which is the part worth stating, because a board whose last edit was refused and then left alone still holds changes that are not stored. The confirmation-link flow and the unenforced repository contract were both raised in review, deferred deliberately, and were nowhere in the README. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
Boards move from Web Storage to Postgres, and gain accounts, live collaboration
and sharing. Ten commits, each one a working step.
Based on
react-shell, notmain— the board-scopedBoardRepositorythisbuilds on lives there and is still unmerged. Review that one first, or read this
one as stacked on it.
What changed
auth.usersrow, so a guest's boards are covered by the policies from the first card. Registering attaches an email to that same user, so the boards come along.createSupabaseRepositorysatisfies the same contract as the Web Storage one. Nothing aboveshell/storage.jsknows which it is holding.platform/sync.jstakes that literally. What a client applies locally is exactly what it sends.A build with no
VITE_SUPABASE_URLis unchanged — Web Storage, no accounts,no gate. That is what the published GitHub Pages site is, and what most of the
test suite runs against.
Things worth a reviewer's attention
board_role()must staySECURITY DEFINER. No behavioural test catchesits removal — every policy tests
owner_idfirst and OR short-circuits aheadof the call, so the recursion it would cause is one reordered disjunct away.
test/db/rls.test.jsassertsprosecdefdirectly for that reason.save()updates then inserts, rather than upserting. An upsert must nameowner_id, and an editor saving someone else's board would send their own idfor a column
freeze_board_ownerrefuses to let change.orderop merges rather than replaces. A sender cannot name an objectthey have not received yet, and taking their array literally dropped a
concurrent addition out of the document entirely.
requestAnimationFrame. rAFstops in a background tab; the two-page test failed until this changed.
board id, and the invite row is readable only by the owner. Redeeming is a
definer function because the point is to act on a row the caller cannot see.
board_people()resolves user ids to email addresses, so it is bounded tothe owner asking about their own board. It is a members list, not a lookup.
Bugs found and fixed on the way
hydrate()replaced the document with the snapshot, discarding anything madewhile the board was still loading — and the canvas is deliberately interactive
during exactly that window. Invisible against Web Storage; a round trip against
a network.
autosave, because a save that updated nothing fell through to an insert.
refusal looked like a board that came back.
Tests
one skip.
are reported but excluded from the total when it is absent, with a line saying
so — averaging an unreachable file in would quietly lower the bar for every
other file.
anonymous users and talk to real GoTrue, PostgREST and Realtime, so a policy
that would refuse the app refuses the tests.
npx supabase startin the main job, so neither those suites northe RLS ones can skip before a merge (~1–3 min of image pulls per run).
openApp({ isolated: true })gives a page its own browser context. Two tabs onone origin share Web Storage, so signing the second in took the first's session
away — a sharing test would have quietly become one person sharing with
themselves.
Not done
configured. Nothing is at risk yet — the deployed build has no
VITE_SUPABASE_*— but this has to be decided before the backend is switchedon in production.
persisting until it reads the board again.
until a reload.
🤖 Generated with Claude Code
https://claude.ai/code/session_013RLXDDxiJBHFCCMR9uekuN
Summary by CodeRabbit