Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughMigrates blocks from flat Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Convex as Convex Mutation
participant Yjs as Yjs Converter
participant DB as Database
Client->>Convex: persistNoteBlocks(noteId, yjsUpdate)
Convex->>Yjs: yDocToBlocks(doc, 'document')
Yjs-->>Convex: Array<CustomBlock>
Convex->>Convex: flattenBlocks -> dedupe by blockNoteId
Convex->>DB: updateBlock / insertBlock for each flattened block
DB-->>Convex: ack
Convex->>DB: delete orphaned blockShares and blocks
DB-->>Convex: ack
Convex-->>Client: mutation result
sequenceDiagram
participant Client
participant Convex as Convex Mutation
participant DB as Database
Client->>Convex: shareBlocks(blockNoteIds, member)
Convex->>Convex: for each blockNoteId -> findBlockOrThrow
Convex->>DB: updateBlock(shareStatus) / insert share rows
DB-->>Convex: ack
Convex-->>Client: result / errors (NOT_FOUND if missing)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
wizard-archive | a2636e4 | Apr 13 2026, 03:27 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/features/sharing/components/share-menu-content.tsx (1)
73-77:⚠️ Potential issue | 🟠 MajorKeep an explicit disabled path for unshareable selections.
After this change,
unsharableMessageis only explanatory; it no longer prevents the checkbox actions. That re-enables share toggles for selections the caller already knows are not shareable, which is a UX regression and can trigger invalid/no-op mutations.Suggested fix
interface ShareMenuContentProps { label?: string isPending: boolean isMutating: boolean + isDisabled?: boolean shareItems: Array<ShareItem> onToggleShareWithMember: (memberId: Id<'campaignMembers'>) => Promise<void> unsharableMessage?: string } export function ShareMenuContent({ label = 'Share with', isPending, isMutating, + isDisabled = false, shareItems, onToggleShareWithMember, unsharableMessage, }: ShareMenuContentProps) { return ( @@ shareItems.map((shareItem) => ( <ShareMenuItem key={shareItem.key} shareItem={shareItem} isMutating={isMutating} + isDisabled={isDisabled} onToggle={onToggleShareWithMember} /> )) )} </ContextMenuGroup> ) } interface ShareMenuItemProps { shareItem: ShareItem isMutating: boolean + isDisabled?: boolean onToggle: (memberId: Id<'campaignMembers'>) => Promise<void> } -function ShareMenuItem({ shareItem, isMutating, onToggle }: ShareMenuItemProps) { +function ShareMenuItem({ + shareItem, + isMutating, + isDisabled = false, + onToggle, +}: ShareMenuItemProps) { @@ <ContextMenuCheckboxItem checked={isChecked} - disabled={isMutating} + disabled={isMutating || isDisabled}Also applies to: 85-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sharing/components/share-menu-content.tsx` around lines 73 - 77, The change removed an explicit disabled path so items that are flagged unsharable can still be toggled; restore an explicit disabled behavior by passing a boolean (e.g., disabled or isDisabled) into ShareMenuItem (and into its internal checkbox) when the selection is not shareable (use the existing unsharable flag/prop on shareItem or compute shareItem.unsharable) and ensure onToggleShareWithMember early-returns when called for disabled/unsharable items; apply the same fix for the other instances in the 85-105 block so the UI and handler both prevent no-op/invalid mutations.convex/_test/factories.helper.ts (1)
378-392:⚠️ Potential issue | 🟠 MajorDon’t default child blocks to
depth: 0.If a caller provides
parentBlockIdbut omitsdepth, this helper still inserts a root-level depth. That creates impossible nested fixtures and can hide traversal/query bugs in the new hierarchical model. RequiredepthwheneverparentBlockIdis set, or derive it before insert.💡 Fail fast on inconsistent nested fixtures
const data = { ...defaults, ...overrides } + if (data.parentBlockId !== null && overrides?.depth === undefined) { + throw new Error('createBlock requires `depth` when `parentBlockId` is set') + } const blockDbId = await t.run(async (ctx) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_test/factories.helper.ts` around lines 378 - 392, The helper currently sets depth: 0 in defaults which leads to inconsistent fixtures when a caller supplies parentBlockId but omits depth; update the logic around defaults/data (the const defaults and the data = { ...defaults, ...overrides } merge in this file) to either: 1) require callers to provide depth when parentBlockId is present (throw an error if parentBlockId is set and overrides.depth is undefined) or 2) derive depth from the parent (lookup or call a helper to compute parent depth+1) before insertion; implement this check/derivation immediately prior to constructing data so nested fixtures are consistent and impossible states are prevented.convex/blockShares/functions/blockShareMutations.ts (1)
181-193:⚠️ Potential issue | 🔴 CriticalDo not downgrade
all_sharedblocks when removing one member share.When a block is globally shared,
remainingSharesis usually empty because access is not coming from per-member rows. This branch will still flip the block toNOT_SHARED, so unsharing one member can revoke visibility for every player.🔧 Proposed fix
- if (!remainingShares) { + if (!remainingShares && block.shareStatus === SHARE_STATUS.INDIVIDUALLY_SHARED) { await updateBlock(ctx, { blockDbId: block._id, shareStatus: SHARE_STATUS.NOT_SHARED, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/blockShares/functions/blockShareMutations.ts` around lines 181 - 193, The current logic flips blocks to SHARE_STATUS.NOT_SHARED whenever no per-member remainingShares row exists, which incorrectly downgrades blocks that are globally shared; update the check after querying remainingShares to only call updateBlock(..., shareStatus: SHARE_STATUS.NOT_SHARED) when remainingShares is null AND the block is not globally shared (check the block.all_shared / block.allShared flag on the block object), leaving globally-shared blocks unchanged; keep references to remainingShares, updateBlock, block._id, block.campaignId and the by_campaign_block_member index so the change is made in the same function and branch.
🤖 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/_test/factories.helper.ts`:
- Around line 36-39: testBlockNoteId currently only uses the first 12 bytes of
the label which causes collisions; change testBlockNoteId to hash the full label
(e.g., use Node's crypto.createHash('sha256').update(label).digest('hex')) and
then use slices of that hex digest to build the UUID-like string (keep the
existing slice layout and the version/variant nibble inserts like the "4" and
"8" positions). Update the implementation in function testBlockNoteId to derive
hex from the full-label hash rather than Buffer.from(label.padEnd(12,...)) so
different labels with the same prefix produce distinct ids.
In `@convex/blocks/__tests__/flatBlockValidator.test.ts`:
- Around line 5-15: Replace the single generic fixture { type: blockType, props:
{} } with a per-type minimal-valid-fixtures map keyed by block type (use the
same editorBlockTypes and customBlockSpecs to drive it) and for the test
"accepts every block type from the editor schema" look up the minimal valid
payload for each blockType and validate that with
flatBlockContentSchema.safeParse; ensure the map covers special cases like
'heading' (e.g., include level) and any other block types that require specific
props or content so the test asserts true only for genuinely valid minimal
examples.
In `@convex/blocks/__tests__/reconstructBlockTree.test.ts`:
- Around line 199-223: The test currently reuses the same blockNoteId for two
entries which tests duplicate IDs rather than a real parent-cycle; update the
test data in reconstructBlockTree.test.ts to create distinct blocks that form a
real cycle (e.g., use makeFlatBlock with testBlockNoteId('a') and
testBlockNoteId('b') and set parentBlockId so a -> b -> a), keep the root block
as before, retain the warnSpy on console.warn and the
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('Cycle detected'))
assertion, and adjust any child existence assertions as needed so the test
verifies that reconstructBlockTree breaks the cycle and logs the warning
(reference functions: reconstructBlockTree, makeFlatBlock, testBlockNoteId, and
the warnSpy).
In `@convex/blocks/functions/insertBlock.ts`:
- Around line 23-30: The insertBlock logic currently only checks params.depth >=
0; add a joint validation before calling ctx.db.insert in the insertBlock
function to reject mismatched parentBlockId and depth: if params.parentBlockId
is null/undefined then require params.depth === 0, and if params.parentBlockId
is non-null require params.depth > 0; when the check fails
throwClientError(ERROR_CODE.VALIDATION_FAILED, ...) with a clear message
referencing parentBlockId and depth so invalid combinations (e.g.,
parentBlockId: null with depth: 2 or parentBlockId: someId with depth: 0) are
rejected before calling ctx.db.insert.
In `@convex/blocks/functions/reconstructBlockTree.ts`:
- Around line 18-55: The current reconstruction only starts traversal from
parentBlockId === null so blocks in cycles with no root (e.g., A->B->A) are
silently dropped; update reconstructBlockTree to detect and report
unreachable/cyclic components by performing a secondary pass over flatBlocks:
for each block not reached by the initial buildChildren traversal, run a DFS
(reusing the same cycle-detection logic from buildChildren or a small helper
like detectCycle) against childrenMap to determine if it participates in a cycle
or is simply orphaned, and log a clear warning listing the involved block ids
(use symbols block.blockNoteId, buildChildren, childrenMap, orphanedBlocks and
the visited sets) before dropping or skipping them so corrupt note data is
surfaced.
In `@convex/blocks/functions/saveAllBlocksForNote.ts`:
- Around line 28-59: The flatten/upsert loop currently processes every flattened
block and can upsert the same blockNoteId multiple times; before calling
asyncMap/use of updateBlock/insertBlock, deduplicate flatBlocks by blockNoteId
(e.g., build a Map from flat.blockNoteId -> first flat entry) and iterate that
deduplicated collection (checking existingBlocksMap.get(blockNoteId) as now) so
each blockNoteId is inserted/updated exactly once; keep existing calls to
updateBlock and insertBlock and preserve fields (blockDbId, parentBlockId,
depth, position, type, props, inlineContent, plainText, shareStatus).
- Around line 72-73: The db.delete calls are using the old API without table
names; update the two calls in saveAllBlocksForNote (the asyncMap over
blockShares and the subsequent delete of block._id) to pass the explicit table
name as first arg (e.g., ctx.db.delete("blocks", share._id) and
ctx.db.delete("blocks", block._1d) using the same table string used elsewhere in
this codebase), keeping the existing identifiers asyncMap, blockShares,
share._id, block._id and ctx.db.delete.
In `@convex/blocks/functions/updateBlock.ts`:
- Around line 23-29: The update path allows negative depths to be persisted via
updates.depth; add the same validation used in insertBlock to reject negative
depths on update: inside updateBlock (the code block that assigns to
updates.depth), check if fields.depth is defined and if fields.depth < 0 then
throw an error (or return a failed validation) instead of assigning to
updates.depth; keep the existing behavior for undefined values and apply the
guard before updates.depth = fields.depth to ensure negative depths cannot be
saved.
In `@convex/folders/__tests__/getFolderContentsForDownload.test.ts`:
- Around line 42-72: The test should be extended to include a nested-block
scenario so the rebuilt block tree preserves descendants: after createNote and
the top-level createBlock, add another createBlock whose parentId is the first
block's id (creating a child block), then call
dmAuth.query(api.folders.queries.getFolderContentsForDownload) and assert the
returned note's content preserves the nesting (e.g., the root content contains
the parent block and that block has a non-empty children array or the expected
child block content), preventing descendants from being dropped or lifted; apply
the same change to the other similar test case referenced around lines 274-309.
In `@convex/folders/functions/getFolderContentsForDownload.ts`:
- Around line 70-76: The current approach filters out unauthorized blocks before
rebuilding the tree, which loses ancestry needed to place permitted child
blocks; instead, call reconstructBlockTree(allBlocks) to build the full tree
first, then traverse that tree and prune or redact nodes that fail
enforceBlockSharePermissionsOrNull (or use its result) so that readable children
remain attached via their preserved ancestors; alternatively, compute the
ancestor chain for each permitted block (using block.parentId or tree helpers)
and include those ancestors in permittedBlocks before calling
reconstructBlockTree — update the logic around
enforceBlockSharePermissionsOrNull, permittedBlocks, and reconstructBlockTree to
follow one of these two strategies.
In `@convex/notes/blocknote.ts`:
- Around line 16-17: The finally block calls _tiptapEditor.destroy() unguarded
which can throw and mask the original conversion error; change the cleanup to
either (a) avoid calling _tiptapEditor.destroy() and use a headless server
editor (consider ServerBlockNoteEditor from `@blocknote/server-util`) if lifecycle
handling is unnecessary, or (b) if cleanup is required keep using _tiptapEditor
but guard the call with optional chaining and a try/catch that swallows only
cleanup errors (do not rethrow) so the original exception from the conversion
code is preserved; update both occurrences of _tiptapEditor.destroy()
accordingly and document why you chose ServerBlockNoteEditor or retained
destroy().
In `@src/features/editor/hooks/useScrollToHeading.ts`:
- Around line 57-59: The dynamic CSS selector built in useScrollToHeading uses
raw target.blockNoteId in document.querySelector which can throw for special
characters; update the selector construction to escape the blockNoteId (use
CSS.escape if available with a safe fallback) before interpolating into the
`[data-id="..."]` string, then call document.querySelector with the escaped
value and keep the existing optional chaining and scrollIntoView call intact;
key symbols: useScrollToHeading, target.blockNoteId, and the
document.querySelector(`[data-id="${...}"]`) expression.
In `@src/features/editor/utils/__tests__/heading-utils.test.ts`:
- Around line 10-33: Replace the double-cast pattern by making the test builder
return a properly typed object rather than using "as unknown as CustomBlock":
update the heading(...) and paragraph(...) helpers to construct values that
satisfy CustomBlock (use the TypeScript "satisfies CustomBlock" operator or
change the function return type to CustomBlock and ensure all required fields
match the CustomBlock interface), and mirror this change for the similar "as
unknown as CustomBlock['props']" usage in
convex/blocks/__tests__/flattenBlocks.test.ts so tests remain type-safe and no
runtime-unsafe casts are used.
In `@src/features/editor/utils/__tests__/text-to-blocks.test.ts`:
- Around line 692-700: The test currently treats loss of "<angle>" as expected;
instead ensure the import/export preserves user content by updating the
assertion in the test that uses convertTextContentToBlocks and
convertBlocksToMarkdown: remove the expect(md).not.toContain('angle') check and
replace it with an assertion that the angle text is preserved (e.g.,
expect(md).toContain('angle') or expect(md).toMatch(/(<angle>|<angle>)/)
to accept either escaped or literal forms), leaving the existing checks for
quotes and apostrophes intact; reference the functions
convertTextContentToBlocks and convertBlocksToMarkdown to locate the relevant
test lines.
In `@src/features/editor/utils/text-to-blocks.ts`:
- Around line 38-50: convertTextContentToBlocks currently calls the async
BlockNote parsers without awaiting them, passing Promises into
flattenLinksToBlocks; change convertTextContentToBlocks to be async, update its
return type to Promise<Array<CustomBlock>>, and add await to both
editor.tryParseMarkdownToBlocks(...) and editor.tryParseHTMLToBlocks(...) before
passing results into flattenLinksToBlocks; also update the call site
convertTextToBlocks(...) to await convertTextContentToBlocks(...) so callers
receive the resolved blocks.
In `@src/features/sharing/hooks/useBlocksShare.ts`:
- Around line 58-64: The aggregate share computation and subsequent mutations
must be blocked until the query has full data: use the existing hasCompleteData
boolean (which checks query.data?.blocks and blockNoteIds.every(...)) as a guard
before computing aggregateShareStatus and before calling setBlocksShareStatus or
shareBlocks; ensure any place that currently falls back to
SHARE_STATUS.NOT_SHARED when query.data is incomplete (the blocks.map using
blockInfoMap.get(...), and the mutation calls around aggregateShareStatus and
the later branches that invoke setBlocksShareStatus/shareBlocks) early-return or
skip running until hasCompleteData is true so you never derive or dispatch state
from incomplete blockInfoMap data.
---
Outside diff comments:
In `@convex/_test/factories.helper.ts`:
- Around line 378-392: The helper currently sets depth: 0 in defaults which
leads to inconsistent fixtures when a caller supplies parentBlockId but omits
depth; update the logic around defaults/data (the const defaults and the data =
{ ...defaults, ...overrides } merge in this file) to either: 1) require callers
to provide depth when parentBlockId is present (throw an error if parentBlockId
is set and overrides.depth is undefined) or 2) derive depth from the parent
(lookup or call a helper to compute parent depth+1) before insertion; implement
this check/derivation immediately prior to constructing data so nested fixtures
are consistent and impossible states are prevented.
In `@convex/blockShares/functions/blockShareMutations.ts`:
- Around line 181-193: The current logic flips blocks to SHARE_STATUS.NOT_SHARED
whenever no per-member remainingShares row exists, which incorrectly downgrades
blocks that are globally shared; update the check after querying remainingShares
to only call updateBlock(..., shareStatus: SHARE_STATUS.NOT_SHARED) when
remainingShares is null AND the block is not globally shared (check the
block.all_shared / block.allShared flag on the block object), leaving
globally-shared blocks unchanged; keep references to remainingShares,
updateBlock, block._id, block.campaignId and the by_campaign_block_member index
so the change is made in the same function and branch.
In `@src/features/sharing/components/share-menu-content.tsx`:
- Around line 73-77: The change removed an explicit disabled path so items that
are flagged unsharable can still be toggled; restore an explicit disabled
behavior by passing a boolean (e.g., disabled or isDisabled) into ShareMenuItem
(and into its internal checkbox) when the selection is not shareable (use the
existing unsharable flag/prop on shareItem or compute shareItem.unsharable) and
ensure onToggleShareWithMember early-returns when called for disabled/unsharable
items; apply the same fix for the other instances in the 85-105 block so the UI
and handler both prevent no-op/invalid mutations.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: b3dc3beb-b2a5-47bb-8217-0e38e23b32e8
⛔ 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 (73)
README.mdconvex/_test/factories.helper.tsconvex/blockShares/__tests__/blockShares.test.tsconvex/blockShares/__tests__/blockSharesWithNestedBlocks.test.tsconvex/blockShares/__tests__/blockSharingWorkflows.test.tsconvex/blockShares/functions/blockShareMutations.tsconvex/blockShares/functions/setBlocksShareStatus.tsconvex/blockShares/functions/shareBlocks.tsconvex/blockShares/functions/unshareBlocks.tsconvex/blockShares/mutations.tsconvex/blockShares/types.tsconvex/blocks/__tests__/blockNoteValidator.test.tsconvex/blocks/__tests__/blockQueryEdgeCases.test.tsconvex/blocks/__tests__/extractPlainText.test.tsconvex/blocks/__tests__/flatBlockValidator.test.tsconvex/blocks/__tests__/flattenBlocks.test.tsconvex/blocks/__tests__/reconstructBlockTree.test.tsconvex/blocks/__tests__/saveAllBlocksForNote.test.tsconvex/blocks/blockSchemas.tsconvex/blocks/functions/extractPlainText.tsconvex/blocks/functions/findBlockByBlockNoteId.tsconvex/blocks/functions/flattenBlocks.tsconvex/blocks/functions/getAllBlocksByNote.tsconvex/blocks/functions/getBlockWithShares.tsconvex/blocks/functions/getBlocksWithShares.tsconvex/blocks/functions/insertBlock.tsconvex/blocks/functions/reconstructBlockTree.tsconvex/blocks/functions/removeBlockIfNotNeeded.tsconvex/blocks/functions/saveAllBlocksForNote.tsconvex/blocks/functions/saveTopLevelBlocksForNote.tsconvex/blocks/functions/updateBlock.tsconvex/blocks/queries.tsconvex/blocks/schema.tsconvex/blocks/types.tsconvex/campaigns/__tests__/campaignDeletionCascade.test.tsconvex/campaigns/__tests__/memberRemovalCascade.test.tsconvex/documentSnapshots/__tests__/rollbackEdgeCases.test.tsconvex/documentSnapshots/__tests__/snapshot.test.tsconvex/folders/__tests__/getFolderContentsForDownload.test.tsconvex/folders/functions/getFolderContentsForDownload.tsconvex/notes/__tests__/noteSoftDeleteCascade.test.tsconvex/notes/__tests__/noteWorkflows.test.tsconvex/notes/__tests__/persistBlocks.test.tsconvex/notes/blocknote.tsconvex/notes/functions/createNote.tsconvex/notes/functions/enhanceNote.tsconvex/notes/mutations.tsconvex/notes/schema.tsconvex/schema.tsconvex/sidebarItems/__tests__/bulkTrashOperations.test.tsconvex/sidebarItems/__tests__/trashRestorePurgeWithShares.test.tsconvex/yjsSync/__tests__/makeYjsUpdate.helper.tspackage.jsonsrc/features/context-menu/__tests__/predicates.test.tssrc/features/context-menu/components/editor-context-menu-provider.tsxsrc/features/context-menu/menu-registry.tssrc/features/context-menu/predicates.tssrc/features/context-menu/types.tssrc/features/editor/components/extensions/blocknote-context-menu/blocknote-context-menu-handler.tsxsrc/features/editor/components/extensions/side-menu/share/share-side-menu-button.tsxsrc/features/editor/components/extensions/wiki-link/wiki-link-autocomplete.tsxsrc/features/editor/components/viewer/note/note-editor.tsxsrc/features/editor/contexts/blocknote-context-menu-context.tsxsrc/features/editor/hooks/useBlockNoteContextMenu.tssrc/features/editor/hooks/useScrollToHeading.tssrc/features/editor/utils/__tests__/heading-utils.test.tssrc/features/editor/utils/__tests__/text-to-blocks.test.tssrc/features/editor/utils/heading-utils.tssrc/features/editor/utils/text-to-blocks.tssrc/features/file-upload/utils/__tests__/folder-reader.test.tssrc/features/sharing/components/share-menu-content.tsxsrc/features/sharing/hooks/useBlocksShare.tssrc/features/sidebar/hooks/useCreateSidebarItem.ts
💤 Files with no reviewable changes (3)
- convex/blockShares/types.ts
- convex/blocks/functions/removeBlockIfNotNeeded.ts
- convex/blocks/functions/saveTopLevelBlocksForNote.ts
👮 Files not reviewed due to content moderation or server errors (8)
- convex/blocks/functions/extractPlainText.ts
- src/features/editor/hooks/useBlockNoteContextMenu.ts
- convex/notes/functions/enhanceNote.ts
- convex/blocks/queries.ts
- convex/blocks/functions/getBlockWithShares.ts
- src/features/editor/components/extensions/side-menu/share/share-side-menu-button.tsx
- convex/blockShares/tests/blockSharingWorkflows.test.ts
- convex/blocks/functions/getBlocksWithShares.ts
Comment on lines
+10
to
+33
| function heading( | ||
| id: string, | ||
| text: string, | ||
| level: number, | ||
| children: Array<CustomBlock> = [], | ||
| ): CustomBlock { | ||
| return { | ||
| id, | ||
| type: 'heading', | ||
| props: { level }, | ||
| content: [{ type: 'text', text, styles: {} }], | ||
| children, | ||
| } as unknown as CustomBlock | ||
| } | ||
|
|
||
| function paragraph(id: string, text: string, children: Array<CustomBlock> = []): CustomBlock { | ||
| return { | ||
| id, | ||
| type: 'paragraph', | ||
| props: {}, | ||
| content: [{ type: 'text', text, styles: {} }], | ||
| children, | ||
| } as unknown as CustomBlock | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether this double-cast pattern appears elsewhere in tests.
rg -nP --type=ts 'as unknown as CustomBlock'Repository: ntietje1/wizard-archive
Length of output: 1122
Consider replacing double-cast pattern with type-safe alternatives.
The as unknown as CustomBlock casts at lines 22 and 32 bypass compile-time checks. This pattern also appears with similar as unknown as CustomBlock['props'] form in convex/blocks/__tests__/flattenBlocks.test.ts. Refactor test builders to use satisfies or strongly-typed factories instead to maintain type safety across the test suite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/editor/utils/__tests__/heading-utils.test.ts` around lines 10 -
33, Replace the double-cast pattern by making the test builder return a properly
typed object rather than using "as unknown as CustomBlock": update the
heading(...) and paragraph(...) helpers to construct values that satisfy
CustomBlock (use the TypeScript "satisfies CustomBlock" operator or change the
function return type to CustomBlock and ensure all required fields match the
CustomBlock interface), and mirror this change for the similar "as unknown as
CustomBlock['props']" usage in convex/blocks/__tests__/flattenBlocks.test.ts so
tests remain type-safe and no runtime-unsafe casts are used.
Comment on lines
+692
to
+700
| it('documents that angle brackets in markdown are interpreted as HTML and lost', () => { | ||
| const input = 'Text with <angle> & "quotes" and \'apostrophes\'' | ||
| const blocks = convertTextContentToBlocks(input, 'test.md', 'text/markdown') | ||
| const md = convertBlocksToMarkdown(blocks) | ||
| // Angle brackets are parsed as HTML tags by the markdown parser — this is expected markdown behavior | ||
| expect(md).not.toContain('angle') | ||
| // But quotes and ampersands survive | ||
| expect(md).toContain('quotes') | ||
| expect(md).toContain('apostrophes') |
There was a problem hiding this comment.
Don’t codify angle-bracket data loss as expected behavior.
This test currently treats dropping <angle> as correct, but that’s silent user-content loss in an export/import path. Locking it into the suite will make a future preservation/escaping fix look like a regression instead of a bug fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/editor/utils/__tests__/text-to-blocks.test.ts` around lines 692
- 700, The test currently treats loss of "<angle>" as expected; instead ensure
the import/export preserves user content by updating the assertion in the test
that uses convertTextContentToBlocks and convertBlocksToMarkdown: remove the
expect(md).not.toContain('angle') check and replace it with an assertion that
the angle text is preserved (e.g., expect(md).toContain('angle') or
expect(md).toMatch(/(<angle>|<angle>)/) to accept either escaped or
literal forms), leaving the existing checks for quotes and apostrophes intact;
reference the functions convertTextContentToBlocks and convertBlocksToMarkdown
to locate the relevant test lines.
Comment on lines
+38
to
+50
| export function convertTextContentToBlocks( | ||
| textContent: string, | ||
| fileName: string, | ||
| mimeType: string, | ||
| ): Array<CustomBlock> { | ||
| const editor = BlockNoteEditor.create({ schema: editorSchema }) | ||
|
|
||
| if (isMarkdownFile(fileName, mimeType)) { | ||
| return flattenLinksToBlocks(editor.tryParseMarkdownToBlocks(textContent)) | ||
| } else { | ||
| const html = convertTextToHTML(textContent) | ||
| return editor.tryParseHTMLToBlocks(html) | ||
| return flattenLinksToBlocks(editor.tryParseHTMLToBlocks(html)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In @blocknote/core, what are the return types of BlockNoteEditor.tryParseMarkdownToBlocks(...) and BlockNoteEditor.tryParseHTMLToBlocks(...)? Are they Promise-based in the version used by this repo?
💡 Result:
In @blocknote/core, BlockNoteEditor.tryParseMarkdownToBlocks(markdown: string) returns Promise<Block[]> and BlockNoteEditor.tryParseHTMLToBlocks(html: string) returns Promise<Block[]>. Both methods are asynchronous (Promise-based) in current versions, including the latest v0.47.3 (as of 2026-03-25). This is confirmed across official documentation, examples, and code usage. Official docs show synchronous signatures but examples consistently use 'await', indicating the docs may be simplified/omitting Promise. No version uses synchronous returns; always Promise-based. Since the repo uses @blocknote/core (latest available), yes, they are Promise-based.
Citations:
- 1: https://www.blocknotejs.org/examples/interoperability/converting-blocks-from-md
- 2: https://blocknotejs.org/docs/features/import/markdown
- 3: https://www.blocknotejs.org/examples/interoperability/converting-blocks-from-html
- 4: https://github.com/TypeCellOS/BlockNote/blob/ba03b7d2/packages/core/src/editor/BlockNoteEditor.ts
- 5: MD parser broken TypeCellOS/BlockNote#1711
- 6: https://blocknotejs.org/docs/features/import/html
🏁 Script executed:
# First, check the file content around the mentioned lines
cat -n src/features/editor/utils/text-to-blocks.ts | head -60Repository: ntietje1/wizard-archive
Length of output: 2471
🏁 Script executed:
# Check the blocknote version in dependencies
cat package.json | grep -A 5 -B 5 "@blocknote/core"Repository: ntietje1/wizard-archive
Length of output: 443
🏁 Script executed:
# Look at lines 102-104 mentioned in "Also applies to"
cat -n src/features/editor/utils/text-to-blocks.ts | sed -n '95,115p'Repository: ntietje1/wizard-archive
Length of output: 854
🏁 Script executed:
# Find all callers of convertTextContentToBlocks and convertTextToBlocks
rg "convertTextContentToBlocks|convertTextToBlocks" --type typescript --type javascriptRepository: ntietje1/wizard-archive
Length of output: 101
🏁 Script executed:
# Try finding callers without the file type flag
rg "convertTextContentToBlocks|convertTextToBlocks" -A 2 -B 2Repository: ntietje1/wizard-archive
Length of output: 12162
🏁 Script executed:
# Check blocknote core package details in node_modules or documentation
ls -la node_modules/@blocknote/core 2>/dev/null | head -20 || echo "node_modules not available"Repository: ntietje1/wizard-archive
Length of output: 49
🏁 Script executed:
# Check the test file to see if tests are awaiting or if they expect sync behavior
head -50 src/features/editor/utils/__tests__/text-to-blocks.test.tsRepository: ntietje1/wizard-archive
Length of output: 1912
🏁 Script executed:
# Look for any tsconfig or jest config that might reveal how tests run
find . -maxdepth 2 -name "tsconfig*.json" -o -name "jest.config.*" -o -name "vitest.config.*" | head -5Repository: ntietje1/wizard-archive
Length of output: 151
🏁 Script executed:
# Check if there are any type errors or async handling in the tests
grep -n "async\|await" src/features/editor/utils/__tests__/text-to-blocks.test.ts | head -20Repository: ntietje1/wizard-archive
Length of output: 49
🏁 Script executed:
# Look at how tests are calling convertTextContentToBlocks
grep -n "convertTextContentToBlocks" src/features/editor/utils/__tests__/text-to-blocks.test.ts | head -20Repository: ntietje1/wizard-archive
Length of output: 1579
🏁 Script executed:
# Check specific test case to see if it's sync or async
sed -n '50,80p' src/features/editor/utils/__tests__/text-to-blocks.test.tsRepository: ntietje1/wizard-archive
Length of output: 1090
🏁 Script executed:
# Look for any test that might show it returning a Promise
sed -n '50,150p' src/features/editor/utils/__tests__/text-to-blocks.test.ts | grep -A 5 "md ="Repository: ntietje1/wizard-archive
Length of output: 49
🏁 Script executed:
# Check if there's a build or type check script
cat package.json | grep -A 10 '"scripts"'Repository: ntietje1/wizard-archive
Length of output: 461
🏁 Script executed:
# Look at the vitest config to understand test setup
cat vitest.config.tsRepository: ntietje1/wizard-archive
Length of output: 575
🏁 Script executed:
# Check if the function is actually returning a Promise in practice by looking at the full test implementation
sed -n '330,360p' src/features/editor/utils/__tests__/text-to-blocks.test.tsRepository: ntietje1/wizard-archive
Length of output: 1316
🏁 Script executed:
# Check if there's any type annotation or assertion on the md variable that might indicate it's a Promise
sed -n '330,440p' src/features/editor/utils/__tests__/text-to-blocks.test.ts | head -80Repository: ntietje1/wizard-archive
Length of output: 3126
🏁 Script executed:
# Let's check if there are any type errors reported in the codebase
# Look for TypeScript errors or any suppression comments
grep -n "ts-ignore\|@ts-expect-error" src/features/editor/utils/text-to-blocks.tsRepository: ntietje1/wizard-archive
Length of output: 49
🏁 Script executed:
# Check if tests are actually running - look at test output or configuration
# Try running a simple check to see current state
head -100 src/features/editor/utils/__tests__/text-to-blocks.test.ts | tail -50Repository: ntietje1/wizard-archive
Length of output: 1664
🏁 Script executed:
# Verify the actual blocknote version being used more thoroughly
grep -A 2 "@blocknote/core" package-lock.json | head -10Repository: ntietje1/wizard-archive
Length of output: 117
🏁 Script executed:
# Check if package.json or yarn.lock exists
ls -la | grep -E "package|yarn|lock"Repository: ntietje1/wizard-archive
Length of output: 198
🏁 Script executed:
# Let's search for where BlockNoteEditor is imported and check if there are any type definitions
grep -r "BlockNoteEditor" src/ --include="*.ts" --include="*.tsx" | head -20Repository: ntietje1/wizard-archive
Length of output: 2703
🏁 Script executed:
# Check the blocknote core version more carefully in package.json
cat package.json | grep -A 20 '"dependencies"' | grep blocknoteRepository: ntietje1/wizard-archive
Length of output: 171
🏁 Script executed:
# Check if there are any TypeScript errors or type assertions in the actual implementation
sed -n '38,51p' src/features/editor/utils/text-to-blocks.tsRepository: ntietje1/wizard-archive
Length of output: 518
🏁 Script executed:
# Let's look at the flattenLinksToBlocks signature to understand what it expects
sed -n '54,100p' src/features/editor/utils/text-to-blocks.tsRepository: ntietje1/wizard-archive
Length of output: 1648
🏁 Script executed:
# Check if tests are actually passing - look at the package.json test script
cat package.json | grep -A 2 '"test"'Repository: ntietje1/wizard-archive
Length of output: 49
🏁 Script executed:
# Let's check what the actual test command is
grep -E '"test|"check' package.jsonRepository: ntietje1/wizard-archive
Length of output: 645
🏁 Script executed:
# Let's see if there's a TypeScript or build check that would catch this
ls -la convex/ | head -20Repository: ntietje1/wizard-archive
Length of output: 1205
🏁 Script executed:
# Verify the blocknote imports are actually coming from the right package
cat -n src/features/editor/utils/text-to-blocks.ts | head -10Repository: ntietje1/wizard-archive
Length of output: 624
Await the BlockNote parsers before flattening their output.
BlockNoteEditor.tryParseMarkdownToBlocks(...) and tryParseHTMLToBlocks(...) are async APIs that return Promise<Block[]>. Passing their unresolved promises to flattenLinksToBlocks(...) creates a type mismatch—the function expects Array<ParsedBlock> but receives a Promise. Make convertTextContentToBlocks() async, add await to both parser calls, update the return type to Promise<Array<CustomBlock>>, and add await when calling it from convertTextToBlocks() (line 104).
🔧 Proposed fix
-export function convertTextContentToBlocks(
+export async function convertTextContentToBlocks(
textContent: string,
fileName: string,
mimeType: string,
-): Array<CustomBlock> {
+): Promise<Array<CustomBlock>> {
const editor = BlockNoteEditor.create({ schema: editorSchema })
if (isMarkdownFile(fileName, mimeType)) {
- return flattenLinksToBlocks(editor.tryParseMarkdownToBlocks(textContent))
+ return flattenLinksToBlocks(await editor.tryParseMarkdownToBlocks(textContent))
} else {
const html = convertTextToHTML(textContent)
- return flattenLinksToBlocks(editor.tryParseHTMLToBlocks(html))
+ return flattenLinksToBlocks(await editor.tryParseHTMLToBlocks(html))
}
}
export async function convertTextToBlocks(file: File): Promise<Array<CustomBlock>> {
const textContent = await file.text()
- return convertTextContentToBlocks(textContent, file.name, file.type)
+ return await convertTextContentToBlocks(textContent, file.name, file.type)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/editor/utils/text-to-blocks.ts` around lines 38 - 50,
convertTextContentToBlocks currently calls the async BlockNote parsers without
awaiting them, passing Promises into flattenLinksToBlocks; change
convertTextContentToBlocks to be async, update its return type to
Promise<Array<CustomBlock>>, and add await to both
editor.tryParseMarkdownToBlocks(...) and editor.tryParseHTMLToBlocks(...) before
passing results into flattenLinksToBlocks; also update the call site
convertTextToBlocks(...) to await convertTextContentToBlocks(...) so callers
receive the resolved blocks.
Comment on lines
+58
to
64
| const hasCompleteData = query.data?.blocks && blockNoteIds.every((id) => blockInfoMap.has(id)) | ||
|
|
||
| const aggregateShareStatus: AggregateShareStatus = (() => { | ||
| if (!hasCompleteData || topLevelBlocks.length === 0) return AGGREGATE_SHARE_STATUS.NOT_SHARED | ||
| if (!hasCompleteData || blocks.length === 0) return AGGREGATE_SHARE_STATUS.NOT_SHARED | ||
|
|
||
| const statuses = topLevelBlocks.map( | ||
| const statuses = blocks.map( | ||
| (b) => blockInfoMap.get(b.id)?.shareStatus ?? SHARE_STATUS.NOT_SHARED, |
There was a problem hiding this comment.
Guard share mutations until block share data is complete.
These actions currently run off fallback NOT_SHARED values when query.data is still incomplete. On first load, that can send setBlocksShareStatus / shareBlocks for blocks that are already shared and mutate them into the wrong state.
🔧 Proposed fix
const toggleShareStatus = async () => {
- if (!campaignData?._id || !isNote(item) || isMutating || blocks.length === 0) return
+ if (
+ !campaignData?._id ||
+ !isNote(item) ||
+ isMutating ||
+ blocks.length === 0 ||
+ !hasCompleteData
+ ) {
+ return
+ }
try {
const blocksToUpdate = unsharedBlocks.length > 0 ? unsharedBlocks : blocks
const newStatus =
unsharedBlocks.length > 0 ? SHARE_STATUS.ALL_SHARED : SHARE_STATUS.NOT_SHARED
@@
const toggleShareWithMember = async (memberId: Id<'campaignMembers'>) => {
- if (!campaignData?._id || !isNote(item) || isMutating || blocks.length === 0) return
+ if (
+ !campaignData?._id ||
+ !isNote(item) ||
+ isMutating ||
+ blocks.length === 0 ||
+ !hasCompleteData
+ ) {
+ return
+ }
try {
if (getShareState(memberId) === 'all') {Also applies to: 78-80, 103-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/sharing/hooks/useBlocksShare.ts` around lines 58 - 64, The
aggregate share computation and subsequent mutations must be blocked until the
query has full data: use the existing hasCompleteData boolean (which checks
query.data?.blocks and blockNoteIds.every(...)) as a guard before computing
aggregateShareStatus and before calling setBlocksShareStatus or shareBlocks;
ensure any place that currently falls back to SHARE_STATUS.NOT_SHARED when
query.data is incomplete (the blocks.map using blockInfoMap.get(...), and the
mutation calls around aggregateShareStatus and the later branches that invoke
setBlocksShareStatus/shareBlocks) early-return or skip running until
hasCompleteData is true so you never derive or dispatch state from incomplete
blockInfoMap data.
This was referenced Apr 13, 2026
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
Tests
Documentation
Chores