[WIP]feat(core): markdown-diff & patch apply #12844
Conversation
WalkthroughThis update introduces new utilities for diffing and patching markdown documents at the block level, including functions to compute and apply patches. It adds comprehensive tests for these features, refactors test utility imports, and consolidates test utilities under a single entry point. Dependency and export configurations are also updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Diff as diffMarkdown
participant Patch as applyPatchToDoc
participant Store as Document Store
Test->>Diff: Provide old and new markdown
Diff->>Diff: Parse markdown into blocks
Diff->>Diff: Compare block lists, generate PatchOps
Diff-->>Test: Return PatchOps
Test->>Patch: Provide Store and PatchOps
Patch->>Store: Locate note blocks
Patch->>Store: For each PatchOp, modify blocks (insert, delete, replace)
Patch-->>Test: Store updated
Suggested labels
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
docs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
blocksuite/affine/shared/src/__tests__/commands/model-crud/replace-selected-text-with-blocks.unit.spec.ts (1)
4-5: Simplify side-effect importThe only purpose of this import is to register custom matchers.
You can rely on the barrel to pull inaffine-test-utilsinternally and avoid addressing the file directly:-import '../../../test-utils/affine-test-utils'; +import '../../../test-utils';This makes the import resilient to future file moves inside
test-utils.packages/frontend/core/src/blocksuite/ai/utils/apply-model/apply-patch-to-doc.ts (1)
15-20: Scope limited to the first note – is that intentional?
applyPatchToDocsilently ignores every note block after the first.
If multiple notes are allowed in a page, patches targeting the others will never be applied. Clarify via docs or throw whenpatchrefers to out-of-scope blocks.packages/frontend/core/src/__tests__/ai/utils/apply-model/apply-patch-to-doc.spec.ts (1)
78-83:new_blockincludestypebut it is never assertedThe test constructs
new_block.type = 'affine:paragraph'yet later only checks for a paragraph element in the expected template.
If future ops require non-paragraph types this test will not guard against regressions.
Consider parameterising the test to cover multiple block types.packages/frontend/core/src/blocksuite/ai/utils/apply-model/markdown-diff.ts (1)
64-86: Algorithm ignores block re-ordering
diffBlockListsemits nothing when a block keeps the same ID/content but moves position.
Applying the patch will therefore preserve the old order.
If ordering matters you’ll need a move/insert+delete strategy.blocksuite/affine/shared/src/test-utils/affine-template.ts (1)
81-86: Inconsistent store initialisation API
affine()now callsdoc.getStore({ extensions, provider: container.provider() }),
butblock()(line 176) still uses the old signaturedoc.getStore({ extensions }).
Keep both paths consistent to avoid subtle test-only discrepancies.packages/frontend/core/src/__tests__/ai/utils/apply-model/markdown-diff.spec.ts (1)
208-221: Patch order assertion may hide real-world ordering bugsThe test expects
insert_atbefore the correspondingdelete.
Depending on how the patch is applied, deleting first could be safer (avoids index drift).
Consider adding a second test asserting the alternative order or relaxing order usingexpect.arrayContaining.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
blocksuite/affine/shared/package.json(2 hunks)blocksuite/affine/shared/src/__tests__/commands/block-crud/get-first-block.unit.spec.ts(1 hunks)blocksuite/affine/shared/src/__tests__/commands/block-crud/get-last-block.unit.spec.ts(1 hunks)blocksuite/affine/shared/src/__tests__/commands/model-crud/replace-selected-text-with-blocks.unit.spec.ts(1 hunks)blocksuite/affine/shared/src/__tests__/commands/selection/is-nothing-selected.unit.spec.ts(1 hunks)blocksuite/affine/shared/src/__tests__/test-utils/affine-template.unit.spec.ts(1 hunks)blocksuite/affine/shared/src/test-utils/affine-template.ts(2 hunks)blocksuite/affine/shared/src/test-utils/affine-test-utils.ts(1 hunks)blocksuite/affine/shared/src/test-utils/create-test-host.ts(1 hunks)blocksuite/affine/shared/src/test-utils/index.ts(1 hunks)packages/frontend/core/package.json(1 hunks)packages/frontend/core/src/__tests__/ai/utils/apply-model/apply-patch-to-doc.spec.ts(1 hunks)packages/frontend/core/src/__tests__/ai/utils/apply-model/markdown-diff.spec.ts(1 hunks)packages/frontend/core/src/blocksuite/ai/utils/apply-model/apply-patch-to-doc.ts(1 hunks)packages/frontend/core/src/blocksuite/ai/utils/apply-model/markdown-diff.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: test-build-mobile-app / build-android-web
- GitHub Check: test-build-mobile-app / build-ios-web
- GitHub Check: loom thread test
- GitHub Check: E2E Test (7)
- GitHub Check: Run native tests
- GitHub Check: fuzzing
- GitHub Check: Build AFFiNE native (x86_64-pc-windows-msvc)
- GitHub Check: Build AFFiNE native (aarch64-pc-windows-msvc)
- GitHub Check: Build AFFiNE native (x86_64-apple-darwin)
- GitHub Check: Analyze (javascript, blocksuite)
- GitHub Check: Build Server native
- GitHub Check: Analyze (typescript, blocksuite)
- GitHub Check: Analyze (typescript, affine)
- GitHub Check: Analyze (javascript, affine)
🔇 Additional comments (14)
blocksuite/affine/shared/package.json (2)
13-14: Add new workspace dependencies
Introducing@blocksuite/affineand@blocksuite/affine-ext-loaderas workspace dependencies. Ensure these packages are at compatible versions and verify the monorepo build/bundling picks up the updates without conflict.
68-69: Expose new export paths for adapters and test-utils
Adding"./adapters"and"./test-utils"exports consolidates utilities for downstream imports. Confirm that these paths resolve correctly in TS builds and that the target files (src/adapters/index.ts,src/test-utils/index.ts) exist and re-export as intended.blocksuite/affine/shared/src/test-utils/affine-test-utils.ts (1)
66-67: Refactor loop to usefor...ofwith destructuring
Switching tofor (const [i, child] of actual.children.entries())is more concise and avoids index-based access errors. Implementation looks correct and improves readability.packages/frontend/core/package.json (1)
22-22: Add@blocksuite/affine-sharedas a workspace dependency
Linking the frontend core to the newly extended shared package is necessary for the markdown-diff and patch features. Ensure that all imports from@blocksuite/affine-sharedresolve and that CI builds/tests pass.blocksuite/affine/shared/src/test-utils/create-test-host.ts (1)
243-243: Clarify@ts-expect-erroras a dev-only exception
Updating the comment to indicate a "dev-only" exception improves context for future maintainers. This change is purely cosmetic and safe.blocksuite/affine/shared/src/__tests__/commands/block-crud/get-last-block.unit.spec.ts (1)
7-7: Update import path to consolidatedtest-utilsmodule
Changing fromhelpers/affine-templateto the newtest-utilsentry point aligns all tests to use the unified utilities. Verify no residual legacy imports remain.blocksuite/affine/shared/src/__tests__/commands/block-crud/get-first-block.unit.spec.ts (1)
7-8: Centralisedtest-utilsimport looks correctThe new relative path resolves cleanly to
src/test-utilsand removes duplicate helper paths – ✅ from my side.blocksuite/affine/shared/src/__tests__/commands/selection/is-nothing-selected.unit.spec.ts (1)
9-10: Good move to the barrel fileImporting from the barrel
../../../test-utilskeeps the tests in sync with the new public export. No issues spotted.blocksuite/affine/shared/src/__tests__/test-utils/affine-template.unit.spec.ts (1)
4-4: Barrel import ✅The switch to
../../test-utilsmaintains correct depth and benefits maintainability.blocksuite/affine/shared/src/__tests__/commands/model-crud/replace-selected-text-with-blocks.unit.spec.ts (1)
10-11: Barrel file gives youaffine/blockalready – niceNo further action needed – import is correct.
blocksuite/affine/shared/src/test-utils/index.ts (1)
1-3: Consider export order where side-effects matter
affine-test-utilsregisters Vitest matchers via side-effects.
If in the future eitheraffine-templateorcreate-test-hostalso mutate globals, order may become significant.
Document (or enforce) the expected load order to avoid hard-to-trace issues.No changes required right now.
packages/frontend/core/src/blocksuite/ai/utils/apply-model/apply-patch-to-doc.ts (1)
22-25:blockIdMapbecomes stale after mutations
blockIdMapis built once and never updated.
If a later patch op references a block that was inserted or re-created by an earlier op in the same batch, the lookup will fail.
Either rebuild the map (cheap, couple of dozen nodes) inside the loop or switch todoc.getBlock(id)on demand.packages/frontend/core/src/__tests__/ai/utils/apply-model/apply-patch-to-doc.spec.ts (1)
87-97: Test relies on deterministic IDs that the store may not guarantee
expectedhard-codesparagraph-3, butinsertFromMarkdowngenerates a fresh ID internally.
If the underlying ID generator changes these assertions will break while the feature still works.
Prefer structural assertions (e.g. compare text & order, ignore IDs) or fetch the inserted block ID from the store before asserting.blocksuite/affine/shared/src/test-utils/affine-template.ts (1)
10-12:StoreExtensionManager.get('store')might be asyncIf
getreturns a promise (common for dynamic loaders) the current code will break at runtime.
Double-check the API andawaitif required.
| await insertFromMarkdown(undefined, op.new_content, doc, parentId, index); | ||
| } else if (op.op === 'insert_at') { | ||
| // Insert new block | ||
| const parentId = note.id; | ||
| const index = op.index; | ||
| await insertFromMarkdown( | ||
| undefined, | ||
| op.new_block.content, | ||
| doc, | ||
| parentId, | ||
| index | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
insert_at ignores the requested block type
The insert_at branch passes only op.new_block.content to insertFromMarkdown, silently discarding op.new_block.type.
If the diff asks for something other than a paragraph (e.g. a title), the inserted block will default to a paragraph and the resulting document will diverge from the desired state.
- await insertFromMarkdown(
- undefined,
- op.new_block.content,
- doc,
- parentId,
- index
- );
+ const markdown =
+ op.new_block.type === 'paragraph'
+ ? op.new_block.content
+ : `# ${op.new_block.content}`; // minimal example – build markdown per type
+ await insertFromMarkdown(undefined, markdown, doc, parentId, index);Consider centralising the type ➜ markdown mapping instead of the quick-hack above.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await insertFromMarkdown(undefined, op.new_content, doc, parentId, index); | |
| } else if (op.op === 'insert_at') { | |
| // Insert new block | |
| const parentId = note.id; | |
| const index = op.index; | |
| await insertFromMarkdown( | |
| undefined, | |
| op.new_block.content, | |
| doc, | |
| parentId, | |
| index | |
| ); | |
| await insertFromMarkdown(undefined, op.new_content, doc, parentId, index); | |
| } else if (op.op === 'insert_at') { | |
| // Insert new block | |
| const parentId = note.id; | |
| const index = op.index; | |
| const markdown = | |
| op.new_block.type === 'paragraph' | |
| ? op.new_block.content | |
| : `# ${op.new_block.content}`; // minimal example – build markdown per type | |
| await insertFromMarkdown(undefined, markdown, doc, parentId, index); |
🤖 Prompt for AI Agents
In
packages/frontend/core/src/blocksuite/ai/utils/apply-model/apply-patch-to-doc.ts
around lines 40 to 51, the insert_at case calls insertFromMarkdown with only the
content, ignoring the block type from op.new_block.type, causing all inserted
blocks to default to paragraphs. To fix this, modify the call to
insertFromMarkdown to include the block type by converting op.new_block.type to
the appropriate markdown format and passing it along with the content, ensuring
the inserted block matches the requested type. Consider centralizing the
type-to-markdown conversion logic to maintain consistency and avoid duplication.
| for (const line of lines) { | ||
| const match = line.match(/^<!--\s*block_id=(.*?)\s+type=(.*?)\s*-->/); | ||
| if (match) { | ||
| // If there is a block being collected, push it into blocks first |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Regex misses comments with leading whitespace
/^<!-- … -->/ fails when the comment is indented (common inside markdown lists).
Trim the line or allow optional leading space:
- const match = line.match(/^<!--\s*block_id=(.*?)\s+type=(.*?)\s*-->/);
+ const match = line.match(/^\s*<!--\s*block_id=(.*?)\s+type=(.*?)\s*-->/);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const line of lines) { | |
| const match = line.match(/^<!--\s*block_id=(.*?)\s+type=(.*?)\s*-->/); | |
| if (match) { | |
| // If there is a block being collected, push it into blocks first | |
| for (const line of lines) { | |
| const match = line.match(/^\s*<!--\s*block_id=(.*?)\s+type=(.*?)\s*-->/); | |
| if (match) { | |
| // If there is a block being collected, push it into blocks first |
🤖 Prompt for AI Agents
In packages/frontend/core/src/blocksuite/ai/utils/apply-model/markdown-diff.ts
around lines 21 to 24, the regex used to match HTML comments does not account
for leading whitespace, causing it to miss indented comments. Modify the regex
to allow optional leading whitespace before the comment start, or trim the line
before applying the regex, so that indented comments inside markdown lists are
correctly matched.
Summary by CodeRabbit