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:
📝 WalkthroughWalkthroughReplaces ProseMirror sync with a Yjs-based collaboration system: adds Yjs schema/tables, queries, mutations, internal mutations, utilities, cron cleanup, provider/hook/frontend integration; removes prosemirror-sync and related wiring; updates dependencies and CI/deploy/review config. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant Hook as useConvexYjsCollaboration
participant Provider as ConvexYjsProvider
participant Convex as Convex Backend
participant DB as Database
Client->>Hook: init(documentId, user, canEdit)
Hook->>Provider: create Y.Doc & ConvexYjsProvider
Hook->>Convex: query getUpdates(documentId)
Convex->>DB: fetch yjsUpdates by_document_seq
DB-->>Convex: return updates[]
Convex-->>Hook: updates[]
Hook->>Provider: applyRemoteUpdates(updates)
Provider->>Provider: apply to Y.Doc, emit sync
Client->>Provider: local edit -> Y.Doc update event
Provider->>Provider: buffer/merge updates (debounce)
Provider->>Convex: mutation pushUpdate(mergedUpdate)
Convex->>DB: insert yjsUpdates row (assign seq)
DB-->>Convex: ack(seq)
Convex-->>Provider: ack(seq)
sequenceDiagram
participant Provider as ConvexYjsProvider
participant Buffer as PendingBuffer
participant Convex as Convex Backend
Y.Doc->>Provider: 'update' event
Provider->>Buffer: append pendingUpdates
rect rgba(100, 150, 200, 0.5)
Note over Provider,Buffer: debounce / max-wait window
end
alt pushInFlight = false
Provider->>Convex: pushUpdate(merged)
Convex-->>Provider: ack(seq)
else pushInFlight = true
Provider->>Provider: mark hasPendingUpdates, reschedule
end
opt writable enabled
Provider->>Convex: periodic persistBlocks()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
wizard-archive | 9de96d9 | Commit Preview URL Branch Preview URL |
Apr 02 2026, 03:03 AM |
Preview Deployment
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
convex/yjsSync/functions/compactUpdates.ts (1)
17-30: Consider concurrent update handling during compaction.If a concurrent
pushUpdatewrites a new update (e.g., seq 21) while this compaction is running (processing seq 1-20), the new update won't be included in the snapshot. The resulting state has:
- Snapshot at seq 20
- Delta at seq 21
This should work correctly because Yjs uses state vectors for merging (not sequence numbers), but the seq ordering could appear non-intuitive. If compaction is scheduled via cron during active editing, consider documenting this behavior or adding a small delay after the last update before compacting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/yjsSync/functions/compactUpdates.ts` around lines 17 - 30, Compaction can miss concurrent updates written during its run (e.g., a new row with seq > maxSeq), so modify compact logic in compactUpdates to detect and handle races: after reconstructYDoc/doc encoding and before deleting/inserting, query for any updates for documentId with seq > maxSeq (using ctx.db) and if any exist either abort/retry compaction or recompute the encoded state (re-run reconstructYDoc) so the snapshot includes those newer updates; ensure deletes (ctx.db.delete on each row) and insert into 'yjsUpdates' happen only after this check to avoid losing concurrent updates.convex/yjsSync/queries.ts (1)
18-21: Consider explicit ordering for consistency withreconstructYDoc.Clients consuming these updates likely need them in
seqorder for correct Yjs state application. Adding.order('asc')ensures consistent ordering semantics across the codebase.♻️ Suggested improvement
const rows = await ctx.db .query('yjsUpdates') .withIndex('by_document_seq', (q) => q.eq('documentId', documentId)) + .order('asc') .collect()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/yjsSync/queries.ts` around lines 18 - 21, The query pulling Yjs updates does not specify an order, which can yield inconsistent seq ordering compared to reconstructYDoc; modify the query on ctx.db.query('yjsUpdates').withIndex('by_document_seq', (q) => q.eq('documentId', documentId)) to add an explicit ordering (e.g., .order('asc')) before calling .collect() so updates are returned in ascending seq order consistent with reconstructYDoc.convex/yjsSync/functions/reconstructYDoc.ts (1)
9-12: Optional: Add explicit.order('asc')for clarity.Convex guarantees that
withIndexqueries return results ordered by the indexed fields (in this case, byseqascending for a givendocumentId). The current code is correct. Adding.order('asc')is not required but makes the ordering intent explicit.♻️ Suggested improvement
const updates = await ctx.db .query('yjsUpdates') .withIndex('by_document_seq', (q) => q.eq('documentId', documentId)) + .order('asc') .collect()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/yjsSync/functions/reconstructYDoc.ts` around lines 9 - 12, The query that builds updates in reconstructYDoc currently relies on withIndex('by_document_seq', (q) => q.eq('documentId', documentId)) and should explicitly state ordering; update the chain that produces updates (the call starting with ctx.db.query('yjsUpdates').withIndex(...).collect()) to include .order('asc') before .collect() so the results are clearly ordered by seq ascending for the given documentId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/deploy.yml:
- Line 29: Replace the standalone "npx convex deploy" step so the Convex CLI
both deploys and builds the frontend with the exact deployed URL: invoke the
deploy command with the build command and URL injection flags (use the existing
"npx convex deploy" invocation, add --cmd 'npm run build' and
--cmd-url-env-var-name VITE_CONVEX_URL) and remove the separate build step or
the manual secrets-based VITE_CONVEX_URL injection so the frontend build uses
the deployment-generated Convex URL as the single source of truth.
In `@convex/yjsSync/functions/checkYjsAccess.ts`:
- Around line 36-45: Replace the coarse campaign-level check in Y.js handlers
with item-level permission checks: in the pushUpdate handler, call
checkYjsWriteAccess(ctx, documentId) instead of checkYjsMembership so only users
with EDIT permission can push edits; in the pushAwareness handler, call
checkYjsReadAccess(ctx, documentId) instead of checkYjsMembership so only users
with VIEW+ permission can broadcast presence; update imports to include
checkYjsWriteAccess and checkYjsReadAccess and remove or keep checkYjsMembership
only where still needed.
In `@convex/yjsSync/functions/createYjsDocument.ts`:
- Around line 21-28: The BlockNoteEditor created via BlockNoteEditor.create(...)
isn't being torn down; after creating the Y doc with blocksToYDoc(editor,
content) and calling doc.destroy(), also call the underlying TipTap editor
destroy method (editor._tiptapEditor.destroy()) to avoid leaking the headless
editor instance; ensure this call happens after blocksToYDoc and before or after
doc.destroy() as appropriate and handle if _tiptapEditor is undefined.
In `@convex/yjsSync/mutations.ts`:
- Around line 118-134: The handler currently applies the incoming encodedState
snapshot and persists blocks, which allows an older client snapshot to overwrite
newer server state; instead, keep checkYjsWriteAccess(ctx, documentId) but
replace creating a Y.Doc from the client-provided encodedState with
reconstructing the authoritative Y.Doc from the server yjs history (use the
existing reconstructYDoc function in convex/yjsSync/functions/reconstructYDoc.ts
or the compacted snapshot/yjsUpdates store), then call yDocToBlocks(editor,
reconstructedDoc) and pass those blocks into saveTopLevelBlocksForNote; target
the handler function, yDocToBlocks, and saveTopLevelBlocksForNote references
when making this change so persistence always uses the server-authoritative
Y.Doc.
In `@src/features/editor/components/viewer/note/note-editor.tsx`:
- Around line 195-211: The useEffect creating the BlockNoteEditor re-runs every
render because the `user` object prop is unstable; either memoize the `user`
object in the parent (`CollaborativeNote`) using useMemo so its reference only
changes when `userName` or `userColor` change, or change the dependency list of
the effect in `note-editor.tsx` to use stable primitives (e.g., `userName` and
`userColor`) instead of the whole `user` object; update references to
`BlockNoteEditor.create`/`instance` and keep the existing teardown
(`instance._tiptapEditor.destroy()`) intact.
- Around line 208-210: The cleanup currently only calls
instance._tiptapEditor.destroy(); update the unmount cleanup to also destroy the
Yjs provider and document (call provider.destroy() and doc.destroy()) to close
WebSocket connections and remove collaborators (these are the same resources
created by useConvexYjsCollaboration). Also avoid using the private
_tiptapEditor if BlockNoteEditor exposes a public destroy method—prefer calling
instance.destroy() when available; otherwise keep _tiptapEditor.destroy() but
still ensure provider.destroy() and doc.destroy() are invoked.
In `@src/features/editor/providers/convex-yjs-provider.ts`:
- Around line 131-137: In destroy(), do not clear the pushInFlight guard before
calling flushUpdates(): keep this.pushInFlight true (remove or skip the
this.pushInFlight = false line) so flushUpdates() and the existing pushUpdate()
.finally() path can serialize and flush any queued updates once the in-flight
mutation settles; ensure destroy() still sets this.destroyed = true and then
calls this.flushUpdates() so teardown waits for the existing push flow to finish
rather than starting a second push.
---
Nitpick comments:
In `@convex/yjsSync/functions/compactUpdates.ts`:
- Around line 17-30: Compaction can miss concurrent updates written during its
run (e.g., a new row with seq > maxSeq), so modify compact logic in
compactUpdates to detect and handle races: after reconstructYDoc/doc encoding
and before deleting/inserting, query for any updates for documentId with seq >
maxSeq (using ctx.db) and if any exist either abort/retry compaction or
recompute the encoded state (re-run reconstructYDoc) so the snapshot includes
those newer updates; ensure deletes (ctx.db.delete on each row) and insert into
'yjsUpdates' happen only after this check to avoid losing concurrent updates.
In `@convex/yjsSync/functions/reconstructYDoc.ts`:
- Around line 9-12: The query that builds updates in reconstructYDoc currently
relies on withIndex('by_document_seq', (q) => q.eq('documentId', documentId))
and should explicitly state ordering; update the chain that produces updates
(the call starting with ctx.db.query('yjsUpdates').withIndex(...).collect()) to
include .order('asc') before .collect() so the results are clearly ordered by
seq ascending for the given documentId.
In `@convex/yjsSync/queries.ts`:
- Around line 18-21: The query pulling Yjs updates does not specify an order,
which can yield inconsistent seq ordering compared to reconstructYDoc; modify
the query on ctx.db.query('yjsUpdates').withIndex('by_document_seq', (q) =>
q.eq('documentId', documentId)) to add an explicit ordering (e.g.,
.order('asc')) before calling .collect() so updates are returned in ascending
seq order consistent with reconstructYDoc.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b694043f-dd57-4928-a1e4-5b140f44c4a9
⛔ Files ignored due to path filters (2)
convex/_generated/api.d.tsis excluded by!**/_generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
.coderabbit.yaml.github/workflows/deploy.ymlconvex/_test/setup.helper.tsconvex/convex.config.tsconvex/crons.tsconvex/notes/functions/createNote.tsconvex/prosemirrorSync.tsconvex/schema.tsconvex/sidebarItems/functions/hardDeleteItem.tsconvex/yjsSync/constants.tsconvex/yjsSync/functions/checkYjsAccess.tsconvex/yjsSync/functions/compactUpdates.tsconvex/yjsSync/functions/createYjsDocument.tsconvex/yjsSync/functions/deleteYjsDocument.tsconvex/yjsSync/functions/reconstructYDoc.tsconvex/yjsSync/functions/uint8ToArrayBuffer.tsconvex/yjsSync/internalMutations.tsconvex/yjsSync/mutations.tsconvex/yjsSync/queries.tsconvex/yjsSync/schema.tspackage.jsonsrc/features/editor/components/viewer/note/note-editor.tsxsrc/features/editor/hooks/useConvexYjsCollaboration.tssrc/features/editor/providers/convex-yjs-provider.ts
💤 Files with no reviewable changes (3)
- convex/convex.config.ts
- convex/_test/setup.helper.ts
- convex/prosemirrorSync.ts
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
convex/yjsSync/__tests__/yjsSyncHelpers.test.ts (1)
12-22: Consider extractingmakeYjsUpdateto a shared test utility.This helper function is duplicated across multiple test files (
yjsSyncHelpers.test.ts,yjsSyncMutations.test.ts,yjsSyncInternalMutations.test.ts). Consider extracting it to a shared test helper file likeconvex/_test/yjs.helper.tsto reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/yjsSync/__tests__/yjsSyncHelpers.test.ts` around lines 12 - 22, The helper function makeYjsUpdate is duplicated across tests; extract it into a shared test utility and import it where needed. Create a new helper module exporting function makeYjsUpdate(): ArrayBuffer that contains the current implementation (creating Y.Doc, calling doc.getXmlFragment('document'), encoding state via Y.encodeStateAsUpdate, slicing to an ArrayBuffer, destroying the doc and returning it), then update tests (yjsSyncHelpers.test.ts, yjsSyncMutations.test.ts, yjsSyncInternalMutations.test.ts) to import { makeYjsUpdate } from the new helper instead of defining it inline.convex/yjsSync/__tests__/yjsSyncInternalMutations.test.ts (1)
175-201: Consider using the TTL constant instead of magic numbers.The tests use hardcoded values
31000and5000which are relative toAWARENESS_TTL_MS(30000ms). Using the constant would make the tests more maintainable if the TTL changes.♻️ Proposed improvement
+import { AWARENESS_TTL_MS } from '../constants' + describe('cleanupStaleAwareness', () => { // ... it('removes entries older than AWARENESS_TTL_MS', async () => { // ... await t.run(async (dbCtx) => { await dbCtx.db.insert('yjsAwareness', { documentId: noteId, clientId: 1, userId: ctx.dm.profile._id, state: new ArrayBuffer(4), - updatedAt: Date.now() - 31000, + updatedAt: Date.now() - AWARENESS_TTL_MS - 1000, }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/yjsSync/__tests__/yjsSyncInternalMutations.test.ts` around lines 175 - 201, Replace the hardcoded TTL-based numbers in the test with the AWARENESS_TTL_MS constant so the test remains correct if the TTL changes: in the test that inserts a stale yjsAwareness row and expects it removed by internal.yjsSync.internalMutations.cleanupStaleAwareness, compute the outdated updatedAt as Date.now() - (AWARENESS_TTL_MS + someDelta) (instead of 31000) and any boundary waits/expectations (instead of 5000) should reference AWARENESS_TTL_MS or a derived delta variable; locate the test around the yjsAwareness insert and the call to cleanupStaleAwareness to make these replacements.e2e/yjs-collaboration.spec.ts (2)
40-97: Potential test flakiness: concurrent typing may interleave unpredictably.Both editors type at the start position (
Home), and the order of text appearance depends on network timing. While the test correctly asserts both texts exist in both editors, consider adding a small delay between the two typing actions or using distinct positions to make the test more deterministic.♻️ Optional improvement for determinism
await editor1.click() await page1.keyboard.press('Home') await page1.keyboard.type(textA) + await page1.waitForTimeout(500) // Allow sync before second editor types await editor2.click() await page2.keyboard.press('Home') + await page2.keyboard.press('End') // Type at end to avoid interleaving await page2.keyboard.type(textB)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/yjs-collaboration.spec.ts` around lines 40 - 97, The test "concurrent typing in two tabs merges without conflict" can flake because both editors type at Home simultaneously; to fix make the typing order deterministic by adding a short explicit pause between the two typing actions or typing at distinct positions: after awaiting editor1.click() and page1.keyboard.type(textA) insert a small wait (e.g., page1.waitForTimeout(50) or shared browser-level wait) before performing editor2.click() and page2.keyboard.type(textB), or alternatively move one cursor to a different position (use pageX.keyboard.press('End') or similar) so textA and textB are typed into different known locations; update the test using the helpers getEditor, page1.keyboard.type, and page2.keyboard.type accordingly.
254-254: Fixed wait time may be fragile.Using
waitForTimeout(2000)to ensure offline typing completes before reconnecting is fragile. Consider polling for a condition or using a longer timeout in CI environments where timing may vary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/yjs-collaboration.spec.ts` at line 254, The fixed sleep using page1.waitForTimeout(2000) is fragile; replace it with a deterministic polling/wait-for-condition approach (e.g., use page1.waitForFunction or a loop that polls the editor/document state) to detect that offline typing has been applied before reconnecting; locate the waitForTimeout call on page1 in yjs-collaboration.spec.ts and change it to wait until the expected editor content/state (or presence of the typed text on page1/page2 or a sync flag) is observed before proceeding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/yjsSync/__tests__/yjsSyncMutations.test.ts`:
- Around line 594-602: The assertion using
expect(blocks.length).toBeGreaterThanOrEqual(0) is vacuous; update the test
inside the async t.run callback to assert the concrete expected number of block
records for this scenario: inspect how persistBlocks is invoked earlier in this
test and replace the weak assertion with either
expect(blocks.length).toBeGreaterThanOrEqual(1) if the pushed update should
create blocks, or expect(blocks.length).toBe(0) (or a specific expected count)
if an empty/nominal Yjs update should produce no blocks; reference the variables
and query used here (blocks and dbCtx.db.query('blocks')) to locate and change
the assertion accordingly.
In `@src/features/editor/components/viewer/note/note-editor.tsx`:
- Around line 237-239: The teardown currently calls the BlockNote internal API
instance._tiptapEditor.destroy(), which relies on a private field and may break
on BlockNote updates; update the cleanup to safely destroy the editor by
checking for the private editor before calling destroy (e.g., guard for
instance._tiptapEditor and typeof destroy === 'function') and keep Yjs cleanup
delegated to useConvexYjsCollaboration (provider/doc). Locate the teardown in
note-editor where instance._tiptapEditor.destroy() is called and replace it with
a safe conditional destroy call that avoids throwing if the internals are absent
or change in future.
In `@src/features/editor/hooks/useConvexYjsCollaboration.ts`:
- Around line 65-73: The effect currently only calls setAfterSeq(...) on the
first non-empty fetch, leaving getUpdates filtered by the initial watermark and
causing ever-growing payloads; change the useEffect in useConvexYjsCollaboration
to call setAfterSeq(state.provider.lastAppliedSeq) whenever
updatesResult.data.length > 0 (i.e., after calling
state.provider.applyRemoteUpdates) so afterSeq is advanced after every applied
batch; reference the useEffect that reads updatesResult.data and state, the call
state.provider.applyRemoteUpdates(...), the setter setAfterSeq(...), the
afterSeq filter used by getUpdates (gt('seq', afterSeq)), and use
state.provider.lastAppliedSeq as the new watermark.
In `@src/features/editor/providers/convex-yjs-provider.ts`:
- Around line 154-157: The persist() call runs too soon because flushUpdates()
is fire-and-forget and may return while pushUpdate() is still in-flight; change
the flow so persist() runs only after the final pushUpdate settles — e.g., make
flushUpdates() return a Promise that waits for any in-flight pushUpdate (or
expose the in-flight promise) and then replace the current block to await
this.flushUpdates() before calling this.persist(); update call-sites of
flushUpdates()/pushUpdate() accordingly to preserve behavior.
- Around line 159-166: Set this.destroyed = true before flushing and make
pushAwareness() no-op when destroyed to avoid any writes after teardown; then
await flushAwareness() to let in-flight pushes finish/cancel, and only after the
await call this.config.removeAwareness({ documentId: this.documentId, clientId:
this.doc.clientID }). Update pushAwareness() to check this.destroyed at the top
and return early so no new or late resolutions recreate presence after
removeAwareness().
- Around line 222-243: The code clears this.pendingUpdates before calling
this.config.pushUpdate, which drops a failed batch on transient errors; modify
flush logic so you capture the merged update (merged) into a local
variable/backup before mutating this.pendingUpdates, and on push failure (inside
.catch) re-queue that merged update into this.pendingUpdates (or prepend/append
consistently) and call this.scheduleFlush() as needed; keep pushInFlight and
lastAppliedSeq handling the same, and ensure you don't double-apply requeued
updates when .then updates seq.
---
Nitpick comments:
In `@convex/yjsSync/__tests__/yjsSyncHelpers.test.ts`:
- Around line 12-22: The helper function makeYjsUpdate is duplicated across
tests; extract it into a shared test utility and import it where needed. Create
a new helper module exporting function makeYjsUpdate(): ArrayBuffer that
contains the current implementation (creating Y.Doc, calling
doc.getXmlFragment('document'), encoding state via Y.encodeStateAsUpdate,
slicing to an ArrayBuffer, destroying the doc and returning it), then update
tests (yjsSyncHelpers.test.ts, yjsSyncMutations.test.ts,
yjsSyncInternalMutations.test.ts) to import { makeYjsUpdate } from the new
helper instead of defining it inline.
In `@convex/yjsSync/__tests__/yjsSyncInternalMutations.test.ts`:
- Around line 175-201: Replace the hardcoded TTL-based numbers in the test with
the AWARENESS_TTL_MS constant so the test remains correct if the TTL changes: in
the test that inserts a stale yjsAwareness row and expects it removed by
internal.yjsSync.internalMutations.cleanupStaleAwareness, compute the outdated
updatedAt as Date.now() - (AWARENESS_TTL_MS + someDelta) (instead of 31000) and
any boundary waits/expectations (instead of 5000) should reference
AWARENESS_TTL_MS or a derived delta variable; locate the test around the
yjsAwareness insert and the call to cleanupStaleAwareness to make these
replacements.
In `@e2e/yjs-collaboration.spec.ts`:
- Around line 40-97: The test "concurrent typing in two tabs merges without
conflict" can flake because both editors type at Home simultaneously; to fix
make the typing order deterministic by adding a short explicit pause between the
two typing actions or typing at distinct positions: after awaiting
editor1.click() and page1.keyboard.type(textA) insert a small wait (e.g.,
page1.waitForTimeout(50) or shared browser-level wait) before performing
editor2.click() and page2.keyboard.type(textB), or alternatively move one cursor
to a different position (use pageX.keyboard.press('End') or similar) so textA
and textB are typed into different known locations; update the test using the
helpers getEditor, page1.keyboard.type, and page2.keyboard.type accordingly.
- Line 254: The fixed sleep using page1.waitForTimeout(2000) is fragile; replace
it with a deterministic polling/wait-for-condition approach (e.g., use
page1.waitForFunction or a loop that polls the editor/document state) to detect
that offline typing has been applied before reconnecting; locate the
waitForTimeout call on page1 in yjs-collaboration.spec.ts and change it to wait
until the expected editor content/state (or presence of the typed text on
page1/page2 or a sync flag) is observed before proceeding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61cf3403-85ef-478e-8e37-c299dda1480c
📒 Files selected for processing (17)
convex/notes/__tests__/createNoteYjsIntegration.test.tsconvex/sidebarItems/__tests__/hardDeleteYjsCleanup.test.tsconvex/yjsSync/__tests__/yjsSyncHelpers.test.tsconvex/yjsSync/__tests__/yjsSyncInternalMutations.test.tsconvex/yjsSync/__tests__/yjsSyncMutations.test.tsconvex/yjsSync/__tests__/yjsSyncQueries.test.tsconvex/yjsSync/functions/checkYjsAccess.tsconvex/yjsSync/functions/createYjsDocument.tsconvex/yjsSync/functions/reconstructYDoc.tsconvex/yjsSync/mutations.tsconvex/yjsSync/queries.tse2e/yjs-collaboration.spec.tssrc/features/editor/components/viewer/note/note-editor.tsxsrc/features/editor/hooks/__tests__/useConvexYjsCollaboration.test.tssrc/features/editor/hooks/useConvexYjsCollaboration.tssrc/features/editor/providers/__tests__/convex-yjs-provider.test.tssrc/features/editor/providers/convex-yjs-provider.ts
✅ Files skipped from review due to trivial changes (2)
- convex/yjsSync/functions/reconstructYDoc.ts
- convex/yjsSync/tests/yjsSyncQueries.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- convex/yjsSync/functions/checkYjsAccess.ts
- convex/yjsSync/functions/createYjsDocument.ts
- convex/yjsSync/queries.ts
- convex/yjsSync/mutations.ts
There was a problem hiding this comment.
🧹 Nitpick comments (4)
convex/yjsSync/__tests__/makeYjsUpdate.helper.ts (1)
7-10: Reuse shareduint8ToArrayBufferhelper to avoid duplicated conversion logic.This conversion is already implemented in
convex/yjsSync/functions/uint8ToArrayBuffer.ts:1-6; reusing it keeps behavior centralized.♻️ Proposed refactor
import * as Y from 'yjs' +import { uint8ToArrayBuffer } from '../functions/uint8ToArrayBuffer' export function makeYjsUpdate(): ArrayBuffer { const doc = new Y.Doc() doc.getXmlFragment('document') const update = Y.encodeStateAsUpdate(doc) - const ab = update.buffer.slice( - update.byteOffset, - update.byteOffset + update.byteLength, - ) as ArrayBuffer + const ab = uint8ToArrayBuffer(update) doc.destroy() return ab }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/yjsSync/__tests__/makeYjsUpdate.helper.ts` around lines 7 - 10, The test currently manually converts a Uint8Array view to an ArrayBuffer via update.buffer.slice(...) — replace that duplication by importing and calling the shared uint8ToArrayBuffer helper (from convex/yjsSync/functions/uint8ToArrayBuffer) instead; locate the conversion in makeYjsUpdate.helper (variable ab) and swap the manual slice logic for a call like uint8ToArrayBuffer(update) so the project uses the centralized conversion function.e2e/yjs-collaboration.spec.ts (3)
248-260: Make the disconnected client author the offline edit.After Line 248,
context2is offline, butduringOfflineis still typed onpage1, which stays online. So this only verifies catch-up after reconnect; it won't catch regressions where offline edits from the disconnected tab fail to replay. Move the edit topage2and assert it reachespage1after Line 258.♻️ Example adjustment
- const duringOffline = `DuringOffline-${Date.now()}` - await editor1.click() - await page1.keyboard.press('End') - await page1.keyboard.press('Enter') - await page1.keyboard.type(duringOffline) - - await expect(editor1).toContainText(duringOffline, { timeout: 5000 }) + const duringOffline = `DuringOffline-${Date.now()}` + await editor2.click() + await page2.keyboard.press('End') + await page2.keyboard.press('Enter') + await page2.keyboard.type(duringOffline) + + await expect(editor2).toContainText(duringOffline, { timeout: 5000 }) await context2.setOffline(false) - await expect(editor2).toContainText(duringOffline, { timeout: 15000 }) + await expect(editor1).toContainText(duringOffline, { timeout: 15000 }) + await expect(editor2).toContainText(duringOffline, { timeout: 15000 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/yjs-collaboration.spec.ts` around lines 248 - 260, The test currently makes the offline edit from the online client; change it so the disconnected client (context2) is the author: after setting context2.setOffline(true) perform the click/End/Enter/type steps on page2 / editor2 (use editor2 and page2) to insert duringOffline, then wait for editor2 to contain the text locally and after context2.setOffline(false) assert editor1/page1 receives duringOffline; update assertions to check editor2 contains duringOffline before reconnection and editor1/page1 contains it after reconnection.
31-35: Surface cleanup failures instead of swallowing them silently.Because this suite shares a campaign across tests, a hidden delete failure can leave stale state behind with no debugging breadcrumb. Keeping cleanup best-effort is fine, but please log the campaign name and error here.
♻️ Example adjustment
- } catch { - /* best-effort */ + } catch (error) { + console.warn( + `Failed to delete E2E campaign "${campaignName}" during cleanup`, + error, + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/yjs-collaboration.spec.ts` around lines 31 - 35, The current try/catch around await deleteCampaign(page, campaignName) swallows errors; change the catch to capture the error (e.g., catch (err)) and log the campaignName and err so failures are visible (for example: console.error or the suite logger with a message like "Failed to delete campaign <campaignName>:" plus the error). Keep it best-effort (do not rethrow) but ensure deleteCampaign and campaignName appear in the log for debugging.
79-87: Drive both editors concurrently here.Line 83 makes this more serial, not more concurrent:
page2only starts afterpage1has already finished typing. That means this can pass while overlapping-insert merge behavior is still broken. Either start both edits together from the same initial state, or rename this to a sequential sync test.♻️ Example adjustment
- await editor1.click() - await page1.keyboard.press('Home') - await page1.keyboard.type(textA) - - await page1.waitForTimeout(100) - - await editor2.click() - await page2.keyboard.press('End') - await page2.keyboard.type(textB) + await editor1.click() + await page1.keyboard.press('End') + await editor2.click() + await page2.keyboard.press('End') + await Promise.all([ + page1.keyboard.type(textA), + page2.keyboard.type(textB), + ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/yjs-collaboration.spec.ts` around lines 79 - 87, The test currently sequences edits (click/type on page1 finishes before page2 starts), so it doesn't exercise concurrent overlapping inserts; to fix, position both cursors first (use editor1.click()/page1.keyboard.press('Home') and editor2.click()/page2.keyboard.press('End') or equivalent) and then start both typing operations concurrently (e.g., kick off both keyboard.type calls and await them together) so editor1 and editor2 perform their inserts from the same initial state at the same time; update the block around editor1/editor2, page1/page2, textA/textB to use concurrent typing instead of serial awaits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@convex/yjsSync/__tests__/makeYjsUpdate.helper.ts`:
- Around line 7-10: The test currently manually converts a Uint8Array view to an
ArrayBuffer via update.buffer.slice(...) — replace that duplication by importing
and calling the shared uint8ToArrayBuffer helper (from
convex/yjsSync/functions/uint8ToArrayBuffer) instead; locate the conversion in
makeYjsUpdate.helper (variable ab) and swap the manual slice logic for a call
like uint8ToArrayBuffer(update) so the project uses the centralized conversion
function.
In `@e2e/yjs-collaboration.spec.ts`:
- Around line 248-260: The test currently makes the offline edit from the online
client; change it so the disconnected client (context2) is the author: after
setting context2.setOffline(true) perform the click/End/Enter/type steps on
page2 / editor2 (use editor2 and page2) to insert duringOffline, then wait for
editor2 to contain the text locally and after context2.setOffline(false) assert
editor1/page1 receives duringOffline; update assertions to check editor2
contains duringOffline before reconnection and editor1/page1 contains it after
reconnection.
- Around line 31-35: The current try/catch around await deleteCampaign(page,
campaignName) swallows errors; change the catch to capture the error (e.g.,
catch (err)) and log the campaignName and err so failures are visible (for
example: console.error or the suite logger with a message like "Failed to delete
campaign <campaignName>:" plus the error). Keep it best-effort (do not rethrow)
but ensure deleteCampaign and campaignName appear in the log for debugging.
- Around line 79-87: The test currently sequences edits (click/type on page1
finishes before page2 starts), so it doesn't exercise concurrent overlapping
inserts; to fix, position both cursors first (use
editor1.click()/page1.keyboard.press('Home') and
editor2.click()/page2.keyboard.press('End') or equivalent) and then start both
typing operations concurrently (e.g., kick off both keyboard.type calls and
await them together) so editor1 and editor2 perform their inserts from the same
initial state at the same time; update the block around editor1/editor2,
page1/page2, textA/textB to use concurrent typing instead of serial awaits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ae8d179-8fbb-498a-82c9-4fd3bf13da54
📒 Files selected for processing (8)
convex/yjsSync/__tests__/makeYjsUpdate.helper.tsconvex/yjsSync/__tests__/yjsSyncHelpers.test.tsconvex/yjsSync/__tests__/yjsSyncInternalMutations.test.tsconvex/yjsSync/__tests__/yjsSyncMutations.test.tsconvex/yjsSync/__tests__/yjsSyncQueries.test.tse2e/yjs-collaboration.spec.tssrc/features/editor/hooks/useConvexYjsCollaboration.tssrc/features/editor/providers/convex-yjs-provider.ts
✅ Files skipped from review due to trivial changes (3)
- convex/yjsSync/tests/yjsSyncInternalMutations.test.ts
- src/features/editor/hooks/useConvexYjsCollaboration.ts
- convex/yjsSync/tests/yjsSyncHelpers.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- convex/yjsSync/tests/yjsSyncQueries.test.ts
- convex/yjsSync/tests/yjsSyncMutations.test.ts
- src/features/editor/providers/convex-yjs-provider.ts
Closed
ntietje1
added a commit
that referenced
this pull request
Apr 8, 2026
* add knip to CI * add preview deployments * fix site url env var * fix site_url for preview deps, also move e2e to separate job * attempt fix env var issue * remove duplicate comment * add back comment, move tooltip * add manual dispatch option to e2e test * improve preview deployments * prevent duplicate deployments to prod * fix sub-sub-domain issue * fix hardcoded url * fixes * add github repo env var * fix order of operations * attempt env var fix * fix * resolve type issue * Note yjs (#30) * add yjs live collab for notes * optimize collab performance * remove unused dep * fix missing env var * add yet another missing env var for preview * improvements * fix viewer strict mode issue * add tests, fix small issues * address comments * fixes * fix strict mode issue * fixes * resolve nits * add redo test * re-use convex preview * fix depreciated flag * add missing env var * fix env var handling * fix comments and undo bug * add canvases (#33) * add xy-flow * add live canvas features * add free-hand drawing * fix stroke impl, add more modes * add undo/redo * add color picker and selection improvements * add better zoom controls * refactor * selection fixes + minimap adjustment * improve multi select indicator * fix sidebar layout issue * improve color picker * add basic embedding and dnd * add resizing * add embedded content * add better prveiews with client side image generation for notes * add convex eslint plugin * fix ssr issue * add preview generation * fixes * improve canvas embeds * address comments * fixes * explicitly pass auth secret * fixes * more fixes * improvements * improvements * fix pre-mature throw * fix test * add resize awareness and fix canvas external file drop * address comments * fix flakey test * address comments again * fix claim token in tests * attempt fix env var issue * fix * add resend api key to preview dep
This was referenced Apr 12, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
Tests