fix: md editor doing round-trip html-md-html on save#442
Closed
edgarsskore wants to merge 25 commits intomainfrom
Closed
fix: md editor doing round-trip html-md-html on save#442edgarsskore wants to merge 25 commits intomainfrom
edgarsskore wants to merge 25 commits intomainfrom
Conversation
Let markdown previews open linked notes, follow headings, and switch into a fullscreen editing workspace so note updates can happen without leaving the preview.
…tent height, reset view on fullscreen exit - Scope min-height to .markdown-workspace--edit only so preview mode shrinks to content - Set --content-height to 920px inline, 89vh in fullscreen - Reset editorView to 'markdown' when exiting fullscreen so inline doesn't stay in raw mode Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolved conflicts in src/ui/file-preview/src/app.ts where the markdown workspace state on the feature branch overlapped with the directory-tree browser added on main: - Kept both directoryBackPayload (main) and the markdown editor state vars (feature branch) at module scope. - Renamed feature branch's local parseDirectoryEntries helper to splitListingLines to avoid clashing with main's directory-tree parser of the same name. - Combined the renderApp panel-topbar additions: keeps backButton from main and hideSummaryRow conditional from the feature branch. - Removed a duplicate stripReadStatusLine that already comes via workspace-controller import on the feature branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…p stale state leaking across files Three related fixes for the markdown workspace inside the file preview: 1. Markdown link clicks were dead in edit mode. The old handler queried `.markdown-doc` synchronously, but in edit mode that element is created lazily inside `mountMarkdownEditor`, so the query returned null and the handler never attached. Switch to event delegation on the stable `.panel-content-wrapper` and scope to `.markdown-doc` via `closest`. The Cmd/Ctrl gate is no longer needed because preventDefault plus delegation handles contentEditable cleanly. 2. The 120ms artificial delay before the first render was a leftover from a removed cache-restore race. Render fresh `tool_result` payloads immediately so opens stop flashing a loading state. 3. Sidebar file switches showed the previous file's content because the sessionStorage cache (shared across all preview iframes that sit on the same parent origin, e.g. dc-app) was eagerly restored on init and beat the host's fresh `tool_result`. Replace the eager restore with a targeted `ontoolinput` match: stash the cached payload at `onConnected`, then in `ontoolinput` only render it if the host's announced file path matches the cache. Fresh `tool_result` always wins. Reopens of the same document still feel instant; switches to a different file no longer flash stale content. The 8s "Preview unavailable" fallback continues to surface an error if the host never re-sends `tool_result`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…istency and partial read handling Major restructuring of the file preview UI: - Refactor markdown workspace into modular files under markdown/ directory - Extract document-layout, panel-actions, file-type-handlers, model, and payload-utils into standalone modules - Unify toolbar into a single consistent bar with conditional button visibility instead of separate code paths for markdown vs non-markdown files - Make all action bar buttons icon-only with tooltips for consistency - Fix button ordering: copy before open-in-folder across all file types Fix partial markdown read handling: - Partial reads show preview mode with expand/copy/open-in-folder buttons - Expanding to fullscreen loads full document and enters edit mode - Exiting fullscreen restores the original line range view - Re-reads the same line range from disk on exit so edits are reflected - refreshFromDisk now respects the original line range instead of always loading the full file (which was overwriting partial reads) Fix fullscreen/inline transitions: - Entering fullscreen from partial read triggers requestEditMode to load full document - Exiting fullscreen from full file read preserves saved content instead of reverting to original tool result - refreshFromDisk on initial load ensures content reflects disk state after page refresh for both partial and full reads Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The markdown WYSIWYG editor's HTML-to-markdown serialization corrupted badge images, links and formatting in the README. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…and preview after write/edit Make file preview work after write_file/edit_block by adding structuredContent to their responses and auto-fetching content when needed. Remove read-only preview mode so markdown always opens in the editor. Replace htmlToMarkdown serializer with block-aware patchMarkdownFromHtml that preserves original source via text diffing. Add source-level formatting, link hover popover with edit/open actions, and collapsible TOC sidebar. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…iew centering and link editing - Intercept Enter keydown to insert \n\n directly into raw source at cursor position, avoiding DOM serialization that could lose formatting or content - Fix edit_block preview centering: use known search position instead of indexOf(replacement) which could match wrong occurrence - Fix formatting/link insertion targeting wrong occurrence by restricting indexOf to the block containing the selection - Fix link edit creating duplicates: replace full [text](old-url) instead of just the visible text - Expose maybeAutosave on controller and call before display mode changes so edits save when clicking expand Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Tiptap The contenteditable + HTML-to-markdown serialization approach was fundamentally unsound for the mixed content in our READMEs — serializeNode lost images inside anchor links, code fences, and nested HTML; LCS-based block alignment misfired and duplicated or dropped content on complex documents. Swap the markdown-view branch of mountMarkdownEditor to Tiptap (ProseMirror) with tiptap-markdown for round-tripping. Source of truth becomes a structured document instead of a rendered-HTML-we-reverse-engineer, so edits go through typed transactions that can only produce valid document states. Trade-off: first save normalizes the doc to tiptap-markdown's canonical form (asterisk vs underscore emphasis, bullet markers, paragraph reflow). After that, the saved file round-trips to itself and subsequent edits are stable. Also: - Remove the "newer version on disk" conflict prompt: refreshFromDisk only runs at mount (no file watcher), so disk-vs-payload mismatch can only mean the host sent a stale payload — just reload silently. - Remove unused @ts-expect-error in parser.ts (markdown-it types now present transitively via tiptap-markdown). - Enable minify in build-ui-runtime.cjs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…after Tiptap swap
Post-Tiptap cleanup pass, mostly from a code review:
- controller.ts: refreshFromDisk was calling isMissingFileErrorResult(null)
after the !freshPayload guard, so the "file deleted" branch was dead.
Introduce callReadFile returning { rawResult, payload } and pass the raw
tool result to the error check; readPayload delegates to it.
- model.ts / controller.ts / test: pendingExternalPayload is only ever set
to null now (the sole writer was the removed disk-conflict notice branch).
Drop the field, the dead branch in revertEditing, and the extra clause in
isUndoAvailable. Remove the stale test assertion.
- linking.ts: add restoreWikiLinks as the inverse of rewriteWikiLinks, so
the mcp-wiki: title contract lives in one file. editor.ts uses it
directly; drop the trivial preprocessForTiptap alias.
- editor.ts: syncHeadingIds now skips no-op id/data-heading-id writes so
identical slugs don't dirty the style engine on every keystroke.
- editor.ts: drop redundant tiptap.commands.focus() calls in handleFormatClick
and handleBlockStyleChange — .chain().focus() already focuses.
- editor.ts: name the popover mouseenter/mouseleave, linkMode file/url click,
and popover hover handlers and remove them in destroy(). Fixes a listener
leak on Raw↔Preview view toggles.
- editor.ts: drop three WHAT comments.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…wrapper
Two fixes from code review:
- app.ts: needsContentRead now short-circuits on http(s):// paths. URL
payloads from read_file(isUrl:true) have no [Reading ...] marker, which
previously looked like "content missing — re-fetch" and triggered a
read_file({ path: url }) call without isUrl, producing a guaranteed-
failed tool round-trip on every URL render. Error was swallowed by
the catch but the round-trip was wasted work.
- controller.ts + app.ts: remove maybeAutosave wrapper and the autosave
trigger on display-mode changes. Clicking a mode-change button blurs
the editor first, and the editor's onBlur now calls saveDocument
directly (which self-gates on dirty/saving/fileDeleted). One save
path, no race with the editor remount that follows a mode change.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wire 5 events through the existing ui-event-tracker so we can measure whether the new markdown editor is actually being used: - markdown_edit_started: first dirty onChange per mount (engagement) - markdown_saved: successful saveDocument; params: blocks count - markdown_save_failed: save catch path; params: reloaded_from_disk - markdown_view_toggled: raw ↔ preview switch - markdown_reverted: revertEditing All events carry file_extension via the shared analytics helper, matching the convention used elsewhere in file-preview. trackUiEvent is added to MarkdownControllerDependencies and wired from app.ts with a lazy closure — the controller is built at module load but the tracker is set up later inside bootstrapApp. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Commit 8fd8f94 ("feat(file-preview): always-edit markdown, source-preserving WYSIWYG, and preview after write/edit") changed handleEditBlock's exact-match path to return a file preview (status line + snippet of the edited region) plus structuredContent carrying fileName/filePath/fileType for the preview UI, replacing the old "Successfully applied N edit(s) to <path>" text message. Both test-edit-block-line-endings.js and test-file-handlers.js still asserted on the old string, so they failed after the refactor even though the edit itself was working correctly. Updates: - test-edit-block-line-endings.js: add assertEditBlockSuccess() helper checking content[0].type === 'text', structuredContent.filePath present, and the [Reading N lines from ...] preview header. Replace all 11 assertion sites. - test-file-handlers.js (Test 10): same check inline. The file-read-back assertion on the next line already verifies the edit landed, so the old text-match assertion was redundant signal anyway.
edit_block deliberately uses soft-failure returns (no isError flag) for cases the LLM should recover from — "Search content not found", fuzzy match below threshold, "Expected N occurrences but found M". These look like success from the MCP protocol level; the server returns isError: undefined and the client's existing assertSuccessfulEditBlockResult only threw on isError === true. Symptom: open a preview, something else modifies the file on disk, edit and save in the preview. Every edit_block call soft-fails (search string no longer on disk), but the editor reports "Saved", clears the dirty flag, advances fullDocumentContent to match the draft, and fires markdown_saved telemetry. Disk is untouched. Subsequent saves diff against the phantom fullDocumentContent and keep failing silently. Fix: successful edit_block always carries structuredContent with fileName/filePath/fileType (per commit 8fd8f94). Absence is a reliable signal of a soft-failure return. assertSuccessfulEditBlockResult now also throws when structuredContent is missing, surfacing the server's message as the error. Throwing routes these through saveDocument's existing catch, which already does the right thing: re-reads disk, compares to our fullDocumentContent, and if they differ reloads the payload with keepDraft: true and shows "Save failed. Reloaded the file from disk." If disk matches (non-staleness failure — e.g. expected_replacements mismatch on unchanged content), surfaces the server's message verbatim. Either way: draft preserved, dirty not cleared, save button remains actionable. Verified with a probe against the built dist: success still passes, "Search content not found" throws, count mismatch throws, disk untouched on soft-fail.
…tected on save
When edit_block fails because the file changed on disk (e.g. a concurrent
agent tool call or the user editing in another app), the previous fix
surfaced this as a small inline notice "Save failed. Reloaded the file
from disk." That message was easy to miss, and the passive swap of
content underneath the editor meant users often didn't realize what had
happened — they'd keep typing against content that had changed beneath
them.
Add a modal that appears specifically when a save attempt detected an
external change, giving the user two concrete actions:
Use disk version — discard draft, accept external changes
(syncStateFromContent without keepDraft).
Save my changes — re-run saveDocument. The earlier fix already
synced sourceContent to disk with keepDraft: true,
so computeEditBlocks now diffs draft vs. the
fresh disk content. Non-overlapping edits merge
in automatically; overlapping edits win over
disk on just the lines the user touched, leaving
the rest of the external change intact.
Dismiss (Esc / ✕) — neither action. Leaves the draft dirty and shows
a lighter inline note so the save button remains
intelligible.
The dialog only opens when reloadedFromDisk === true in saveDocument's
catch. Other save failures (e.g. expected_replacements count mismatch
on unchanged disk — not actually a conflict) keep the existing inline
error path — a modal would be overkill there.
Implementation notes:
- conflict-dialog.ts: self-contained module modeled after config-editor's
array-modal pattern. renderConflictDialogMarkup() emits hidden HTML;
createConflictDialogController() wires events and returns { open,
close, isOpen }. Minimal focus trap between the three buttons,
Escape/✕ cancel, click-outside deliberately does not dismiss
(conflict resolution is not something users should blow past by
accident), default focus on "Save my changes" as the non-destructive
intent.
- Markup is mounted once at document.body in bootstrapApp so that
position: fixed z-index works and app re-renders don't wipe it while
it's open.
- Callbacks guard against the user having navigated away during the
conflict: if workspaceState no longer matches the captured state,
mutations are skipped. Telemetry still fires.
- Three new ui events for funnel analysis:
markdown_save_conflict_shown
markdown_save_conflict_resolved (action: use_disk | save_mine | dismissed)
- CSS uses existing file-preview theme variables (--panel, --text,
--border, --text-secondary, --accent) so it inherits light/dark mode
without further work.
…ocks
Test 12 ("successful saves reset the undo baseline") was failing
because its edit_block mock returned only { content: [...] } — no
structuredContent. After commit d9c15a3 (detect edit_block soft-failures
by structuredContent absence), a success mock without structuredContent
looks indistinguishable from a soft-failure to the client, so the save
went through the catch branch instead of the success branch and the
draft never advanced to become the new baseline.
Fix: add structuredContent to the success mocks in Tests 11 and 12 to
match what the real handleEditBlock has returned since commit 8fd8f94.
Test 11 was also relying on the old lax contract — its first edit_block
mock didn't have structuredContent either. It was passing by accident
because the mock's side effect (diskContent = nextContent) ran before
the new assertion threw, so the observable final state happened to
match expectations anyway. Fixed proactively so the coincidence doesn't
unravel on the next change.
…st outcome
Previously, saveDocument ran all hunks in a single for-loop and threw on
the first edit_block that soft-failed. That aborted the rest of the
loop but the earlier hunks had already been written to disk. The UI
showed "Save failed. Reloaded the file from disk." — which was
misleading: disk had actually been partially overwritten, external
changes on the successful hunks were gone, and the user could not tell
which of their edits had landed.
Now each hunk is wrapped in its own try/catch. Failures are collected
as skippedHunks instead of bailing out. Post-loop:
appliedCount === total -> full success, as before.
appliedCount > 0 && skipped > 0 -> partial success. Tell the user
honestly: "N of M edits saved. K edits did not apply because that
text changed on disk — your draft still has them; save again to
merge." Status badge reads "Saved (partial)" instead of
"Save failed". Draft stays dirty; next save re-tries only the
remaining hunks against the fresh disk baseline.
appliedCount === 0 && skipped > 0 -> total failure and disk did
change. This is the case the conflict dialog was actually designed
for — nothing landed, so the keep-mine / use-disk choice is real.
The partial-success branch reuses the existing syncStateFromContent
pass (keepDraft: true) that already re-reads disk in the catch, so
fullDocumentContent reflects what disk actually has after the partial
writes, not what the editor wishfully thought it had.
New telemetry event markdown_save_partial { applied, skipped, total }
so we can see how often this happens in practice. Existing
markdown_save_failed now also carries applied/skipped/total.
app.ts: harden showConflictDialog dependency against the (unlikely)
case where the lambda fires before bootstrapApp has initialized the
controller — log a warning and invoke options.onCancel so the editor
still surfaces an inline note instead of silently no-op'ing.
test(markdown-preview) Test 11: the simulated-failure scenario now
resolves to partial-success instead of total-failure, so the assertion
was relaxed from exact 'File changed on disk' to the shared substring
'changed on disk' that both the partial-success message and the
onCancel/total-failure messages contain.
Add a 1-second debounced autosave in addition to the existing onBlur save. onChange in the markdown editor schedules a save 1s after the last keystroke; rapid typing resets the timer, so a single save runs once the user pauses. onBlur still fires save immediately and cancels any pending debounce. saveDocument cancels the debounce at start to cover non-debounce callers (conflict-dialog "save my changes", external saveDocument()). disposeHandles clears the timer so a pending save can't fire into a torn-down editor after remount. The AUTOSAVE_DEBOUNCE_MS constant is local to createMarkdownController so tuning the interval later is a one-line change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
📝 WalkthroughWalkthroughThis PR replaces the TipTap/markdown-it markdown editor and preview stack with CodeMirror + Lezer, removes wiki-link rewrite/restore and markdown-it rendering utilities, migrates outline extraction to Lezer, changes controller save/dirty/partial-payload flows to be race-aware and debounced, updates CSS for the new editor, and adjusts tests and package dependencies accordingly. Changes
Sequence DiagramsequenceDiagram
participant User
participant Editor as CodeMirror (EditorView)
participant Controller
participant Outline as Lezer Outline Extractor
participant Save as Save Handler
participant Disk
User->>Editor: Edit markdown
Editor->>Controller: onChange (docChanged, may suppress)
Controller->>Controller: Schedule debounced outline refresh
loop Debounce window
User->>Editor: More edits
Editor->>Controller: onChange
end
Controller->>Outline: extractMarkdownOutline(draftContent)
Note right of Outline: Lezer GFM parse tree traversal
Outline-->>Controller: heading items (with line numbers)
Controller->>Controller: compare/update TOC & activeHeadingId
User->>Editor: Trigger save (autosave/explicit)
Editor->>Controller: requestSave
Controller->>Save: capture savedContent snapshot, compute diff
Save->>Disk: write content
Disk-->>Save: success
Controller->>Controller: detect edits during save
alt No new edits
Controller-->>User: mark as saved
else New edits occurred
Controller->>Controller: mark dirty, schedule autosave
Controller-->>User: keep dirty state
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
codeant-ai
Bot
added
the
size:XXL
edgarsskore
changed the title
Contributor
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+237
to
+240
| outlineRefreshTimer = setTimeout(() => { | ||
| outlineRefreshTimer = null; | ||
| applyOutlineUpdate(state, extractMarkdownOutline(state.draftContent)); | ||
| }, OUTLINE_REFRESH_DEBOUNCE_MS); |
Contributor
There was a problem hiding this comment.
Suggestion: The debounced outline refresh captures a specific state object and later applies it unconditionally, which can refresh the TOC with stale data after state transitions. If workspace state changes before the timer fires, this callback can update outline/UI from an outdated draft. Gate the callback to the current workspace instance (or cancel when state changes) before applying updates. [race condition]
Severity Level: Major ⚠️
- ⚠️ Document outline headings can lag behind editor content.
- ⚠️ Active heading highlight may not match current cursor section.Steps of Reproduction ✅
1. With a markdown file open as in steps 1–2 above, `attachHandlers(payload)` mounts the
editor in `controller.ts:947-989`. The `onChange` handler at `controller.ts:961-982`
updates `state.draftContent` for the current `workspaceState` and calls
`scheduleOutlineRefresh(state)` (`controller.ts:974`), passing the current `state` object
by reference.
2. `scheduleOutlineRefresh` at `controller.ts:214-221` debounces outline updates by
clearing any existing `outlineRefreshTimer` and setting a new timeout
(`outlineRefreshTimer = setTimeout(...)` at line 237). The closure captures the specific
`state` object passed from `attachHandlers`, and after `OUTLINE_REFRESH_DEBOUNCE_MS` it
will call `applyOutlineUpdate(state, extractMarkdownOutline(state.draftContent))` at
`controller.ts:239-240`.
3. While this debounce timer is pending, `workspaceState` can be replaced by
`getState(payload)` when a new `RenderPayload` for the same file path but with different
content arrives (see autosave/save flow in suggestion 1): `buildBody(payload)` at
`controller.ts:338-369` calls `getState(payload)`; if `workspaceState.fullDocumentContent
!== cleanedContent`, `getState` reinitializes `workspaceState` to a new object at
`controller.ts:311-329`, leaving the previously captured `state` reference from
`scheduleOutlineRefresh` stale.
4. When the outline refresh timer eventually fires (`controller.ts:237-241`), it executes
`applyOutlineUpdate(state, extractMarkdownOutline(state.draftContent))` using the stale
`state` object instead of the current `workspaceState`. `applyOutlineUpdate`
(`controller.ts:222-230`) mutates that stale state's `outline` and `activeHeadingId` and
then calls `markdownTocHandle?.refresh(state.outline, state.activeHeadingId)`, causing the
document outline UI (attached in `attachHandlers` at `controller.ts:1013-1026`) to be
refreshed with headings derived from the outdated draft rather than the current editor
content and active state.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/markdown/controller.ts
**Line:** 237:240
**Comment:**
*Race Condition: The debounced outline refresh captures a specific `state` object and later applies it unconditionally, which can refresh the TOC with stale data after state transitions. If workspace state changes before the timer fires, this callback can update outline/UI from an outdated draft. Gate the callback to the current workspace instance (or cancel when state changes) before applying updates.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+310
to
317
| if (!workspaceState || workspaceState.filePath !== payload.filePath || workspaceState.fullDocumentContent !== cleanedContent) { | ||
| const outline = extractMarkdownOutline(cleanedContent); | ||
| workspaceState = { | ||
| filePath: payload.filePath, | ||
| sourceContent: cleanedContent, | ||
| fullDocumentContent: cleanedContent, | ||
| draftContent: cleanedContent, | ||
| outline, | ||
| mode: 'edit', | ||
| dirty: false, |
Contributor
There was a problem hiding this comment.
Suggestion: Reinitializing workspace state whenever incoming payload content differs from fullDocumentContent will discard in-memory draft edits and reset dirty to false on rerender. If the host sends a refreshed payload while the user has unsaved changes, this branch recreates state from disk content and loses the draft. Preserve the existing state when there are unsaved edits instead of recreating it purely on content mismatch. [logic error]
Severity Level: Critical 🚨
- ❌ Markdown editor can drop unsaved text during autosave refresh.
- ⚠️ Users may lose recent keystrokes without any conflict warning.Steps of Reproduction ✅
1. Open a markdown file in the File Preview app so the host calls the `read_file` tool and
delivers a `RenderPayload` to `app.ontoolresult` at
`src/ui/file-preview/src/app.ts:247-258`. This resolves into
`renderAndSync(getEffectiveIncomingPayload(payload))`, which calls `renderApp(container,
payload, ...)` at `app.ts:143-151`.
2. In `renderApp` (`app.ts:201-239`), when `payload.fileType === 'markdown'`, the markdown
body is rendered via `renderPayloadBody` (`file-type-handlers.ts:115-122`), which in turn
calls `markdownController.buildBody(payload)` (`file-type-handlers.ts:69-80`). `buildBody`
in `controller.ts:338-369` calls `getState(payload)` at `controller.ts:339`, initializing
`workspaceState` with `fullDocumentContent` and `draftContent` equal to the cleaned
payload content (`controller.ts:311-318`).
3. The editor DOM is then wired up by `markdownController.attachHandlers(payload)` in
`renderApp` at `app.ts:241-243`, which calls `attachHandlers` in `controller.ts:940-1032`.
Inside `attachHandlers`, `mountMarkdownEditor` is invoked (`controller.ts:950-988`) with
an `onChange` handler at `controller.ts:961-982` that updates `state.draftContent`, sets
`state.dirty = value !== state.fullDocumentContent`, and calls `scheduleAutosave()`
(`controller.ts:198-205`), creating unsaved in-memory edits (`dirty === true`,
`draftContent !== fullDocumentContent`).
4. After the debounce period, `scheduleAutosave`'s timer fires and calls `saveDocument()`
at `controller.ts:699`. Inside `saveDocument`, per hunk, the code issues
`dependencies.callTool('edit_block', ...)` at `controller.ts:729-735`. Each `edit_block`
result is handled globally by `app.ontoolresult` (`app.ts:247-260`), which detects that
the tool result text lacks a read status line and therefore calls `readAndResolvePayload`
(`app.ts:164-181`). `readAndResolvePayload` uses
`markdownController.readPayload(filePath)` (`controller.ts:285-287`) to invoke the
`read_file` tool again; the resulting `RenderPayload` (with updated on-disk content for
the same `filePath`) is passed back into `renderAndSync(getEffectiveIncomingPayload(p))`
(`app.ts:508-513`), which re-invokes `renderApp` and then
`markdownController.buildBody(payload)` for the updated payload. At this point
`workspaceState` still refers to the previous in-memory state and is still `dirty`, but
the incoming `cleanedContent = stripReadStatusLine(payload.content)` now reflects the
newer on-disk content. The condition at `controller.ts:311`
(`workspaceState.fullDocumentContent !== cleanedContent`) is true, so `getState`
reinitializes `workspaceState` with `draftContent: cleanedContent` and `dirty: false`
(`controller.ts:311-318`), discarding any unsaved edits held only in the previous
`workspaceState` (including keystrokes typed while autosave/saveDocument was in progress).Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/markdown/controller.ts
**Line:** 310:317
**Comment:**
*Logic Error: Reinitializing workspace state whenever incoming payload content differs from `fullDocumentContent` will discard in-memory draft edits and reset `dirty` to false on rerender. If the host sends a refreshed payload while the user has unsaved changes, this branch recreates state from disk content and loses the draft. Preserve the existing state when there are unsaved edits instead of recreating it purely on content mismatch.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+1195
to
+1197
| if (button.id === 'link-popover-open') { | ||
| void options.onOpenLink?.(link.href); | ||
| return; |
Contributor
There was a problem hiding this comment.
Suggestion: Opening links directly from editor popover forwards raw href values (including non-http schemes) to the navigation path without scheme validation. Combined with external-link handling, this allows dangerous URLs like javascript: to be executed. Restrict allowed schemes (e.g., http/https/mailto) before invoking link opening. [security]
Severity Level: Critical 🚨
- ❌ File preview can open javascript: links from markdown content.
- ❌ Host executes attacker-controlled URLs without scheme allowlisting.
- ⚠️ Untrusted repositories can weaponize markdown links for abuse.Steps of Reproduction ✅
1. Open the File Preview app (`src/ui/file-preview/src/app.ts:1-200`) on any markdown
payload so that `markdownController` is constructed at `app.ts:116-144` and used to render
the document via `renderPayloadBody()` (`file-type-handlers.ts:16-23`).
2. Ensure the markdown content being rendered contains a link with a dangerous scheme,
e.g. `[Click me](javascript:alert(1))`. This content is loaded into the markdown editor
when `attachHandlers()` in `src/ui/file-preview/src/markdown/controller.ts:41-88` calls
`mountMarkdownEditor({ value: state.draftContent, ... })` at `controller.ts:51-58`.
3. In the rendered markdown editor (CodeMirror preview), hover the mouse over the `Click
me` link so the link popover is shown, then click the "Open link" button in that popover.
This triggers `handleLinkPopoverClick()` in
`src/ui/file-preview/src/markdown/editor.ts:1180-1203`, which, when `button.id ===
'link-popover-open'` (`editor.ts:1195-1197`), calls `options.onOpenLink(link.href)` with
the raw `href` value `javascript:alert(1)`.
4. The `onOpenLink` callback was wired in `attachHandlers()` (`controller.ts:59-61`) to
`navigateLink(payload, href)`. `navigateLink()` in
`src/ui/file-preview/src/markdown/controller.ts:12-31` passes this `href` to
`resolveMarkdownLink()` in `src/ui/file-preview/src/markdown/linking.ts:138-62`, which
uses `isExternalHref()` at `linking.ts:120-121` to treat any `scheme:` (including
`javascript:`) as external. For `javascript:alert(1)`, `resolvedLink.kind === 'external'`
and `navigateLink()` calls `dependencies.openExternalLink(resolvedLink.url)` or falls back
to `window.open(resolvedLink.url, '_blank', 'noopener')` (`controller.ts:25-29`), causing
the unsafe `javascript:` URL to be executed/handed off without any scheme allowlist.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/markdown/editor.ts
**Line:** 1195:1197
**Comment:**
*Security: Opening links directly from editor popover forwards raw `href` values (including non-http schemes) to the navigation path without scheme validation. Combined with external-link handling, this allows dangerous URLs like `javascript:` to be executed. Restrict allowed schemes (e.g., http/https/mailto) before invoking link opening.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
codeant-ai
Bot
added
size:XXL
Contributor
Sequence DiagramThis PR replaces the WYSIWYG markdown pipeline with a source-backed editor so edits, copies, outlines, and saves all operate on the original markdown text while autosave preserves newer drafts even if a save is in flight. sequenceDiagram
participant User
participant UI as File preview UI
participant Controller as Markdown controller
participant Editor as Source editor
participant Tool as Edit tool
User->>UI: Open markdown note
UI->>Controller: Request markdown workspace for payload
Controller->>Editor: Mount editor with markdown source
User->>Editor: Edit markdown text
Editor-->>Controller: Report draft content changes
Controller->>Controller: Mark dirty and debounce autosave and outline refresh
Controller->>Tool: On autosave, send edit blocks from full document to saved draft
Tool-->>Controller: Return applied edits result
Controller->>Controller: Update full document baseline, keep newer draft dirty if it changed during save
Controller-->>UI: Store updated payload override and refresh save status
Generated by CodeAnt AI |
Comment on lines
+958
to
+960
| onOpenLink: (href) => { | ||
| void navigateLink(payload, href); | ||
| }, |
Contributor
There was a problem hiding this comment.
🟠 Architect Review — HIGH
Anchor navigation for markdown links still relies on DOM heading IDs via scrollHeadingIntoView/pendingAnchor, but the new CodeMirror-based editor no longer renders headings with IDs. As a result, #anchor links and [[note#heading]] wiki links now fail with "Heading not found" even when the heading exists in the markdown source.
Suggestion: Resolve anchors against the parsed markdown outline (slug → line) and use markdownEditorHandle.revealLine (for both same-file and cross-file navigation) instead of querying the DOM for heading IDs, updating pendingAnchor handling accordingly.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** src/ui/file-preview/src/markdown/controller.ts
**Line:** 958:960
**Comment:**
*HIGH: Anchor navigation for markdown links still relies on DOM heading IDs via scrollHeadingIntoView/pendingAnchor, but the new CodeMirror-based editor no longer renders headings with IDs. As a result, `#anchor` links and `[[note#heading]]` wiki links now fail with "Heading not found" even when the heading exists in the markdown source.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
| const selectedText = existingLink?.label ?? getSelectedText().trim(); | ||
| linkModal.removeAttribute('hidden'); | ||
| updateLinkMode('url'); | ||
| updateLinkMode(existingLink?.kind === 'wiki' ? 'file' : 'url'); |
Contributor
There was a problem hiding this comment.
Suggestion: Editing an existing wikilink is wired into file-link mode, but the modal immediately clears the selected target and does not preselect the current link target. As a result, pressing Insert after opening "Edit link" on an existing wikilink can silently do nothing. Prepopulate the file selection (and heading/alias) from the existing wikilink before clearing modal state, or block closing when no target is selected. [logic error]
Severity Level: Major ⚠️
- ❌ Editing existing wikilinks from popover often produces no changes.
- ⚠️ Markdown preview editor link editing feels unreliable, confusing users.
- ⚠️ Note-link maintenance workflows in markdown editor become error-prone.Steps of Reproduction ✅
1. Open any markdown note in the desktop UI so that the markdown editor is mounted in
preview mode (`view: 'markdown'`). This flows through `attachHandlers()` in
`src/ui/file-preview/src/markdown/controller.ts:11-41`, which calls
`mountMarkdownEditor()` at `controller.ts:21-28` with `view: state.editorView`.
2. In the markdown editor created by `mountMarkdownEditor()`
(`src/ui/file-preview/src/markdown/editor.ts:370-381`), create or locate an existing
wikilink of the form `[[some/path#heading|Alias]]` so that `findMarkdownLinkAtPosition()`
/ `findEnclosingMarkdownLink()` treat it as `{ kind: 'wiki', href: '[[...]]', label:
'Alias' }` (`editor.ts:849-887`, `901-908`).
3. Move the mouse over the wikilink text so that `handleLinkMouseMove()`
(`editor.ts:1166-1177`) finds the link and calls `showLinkPopover()`
(`editor.ts:1119-1164`), then click the "edit link" button in the popover. This triggers
`handleLinkPopoverClick()` (`editor.ts:1184-1202`), which calls
`openLinkModalForSelection(link)` with the existing `MarkdownLinkRange` for the wikilink.
4. Inside `openLinkModalForSelection()` (`editor.ts:910-942`), the modal is opened in
file-link mode (`updateLinkMode(existingLink?.kind === 'wiki' ? 'file' : 'url');` at
`editor.ts:919`), but `linkSearchResults` and `selectedLinkItem` are immediately reset to
empty/null (`editor.ts:937-938`) and no initial file is selected. If the user edits only
the alias field and presses "Insert", `handleLinkApply()` (`editor.ts:944-985`) executes
with `linkMode === 'file'` and `selectedLinkItem === null`, so the `else if
(selectedLinkItem)` branch is skipped and nothing is inserted before `closeLinkModal()`
runs. The dialog closes with no change to the underlying wikilink, silently discarding the
user's edit.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/markdown/editor.ts
**Line:** 919:938
**Comment:**
*Logic Error: Editing an existing wikilink is wired into file-link mode, but the modal immediately clears the selected target and does not preselect the current link target. As a result, pressing Insert after opening "Edit link" on an existing wikilink can silently do nothing. Prepopulate the file selection (and heading/alias) from the existing wikilink before clearing modal state, or block closing when no target is selected.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
Contributor
Sequence DiagramThis PR replaces the WYSIWYG markdown stack with a CodeMirror-based source editor, wiring draft changes through a debounced autosave flow and enabling clickable markdown and wiki links that navigate directly from the editor. sequenceDiagram
participant User
participant FilePreviewUI
participant MarkdownController
participant MarkdownEditor
participant MCPTools
User->>FilePreviewUI: Open markdown file in preview
FilePreviewUI->>MarkdownController: Build markdown workspace and mount source editor
MarkdownController->>MarkdownEditor: Initialize with raw markdown text
User->>MarkdownEditor: Type and format markdown
MarkdownEditor-->>MarkdownController: Report draft content change
MarkdownController->>MarkdownController: Debounce autosave and outline refresh
MarkdownController->>MCPTools: Save changed blocks via edit tool
MCPTools-->>MarkdownController: Confirm save and update baseline outline
User->>MarkdownEditor: Click markdown or wiki link
MarkdownEditor-->>MarkdownController: onOpenLink with resolved href
MarkdownController->>FilePreviewUI: Navigate to target note or heading
Generated by CodeAnt AI |
Contributor
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Comment on lines
+309
to
+311
| if (headingMatch) { | ||
| className = `cm-md-heading cm-md-heading-${headingMatch[1].length}`; | ||
| addMark(markerRanges, line.from, 0, headingMatch[0].length, 'cm-md-hidden-marker'); |
Contributor
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The new CodeMirror-based markdown editor only styles heading lines and no longer emits heading elements with stable id attributes, while the markdown controller still resolves #anchor and wiki-heading links by querying document.getElementById(...) in scrollHeadingIntoView. In edit/preview mode there are no such heading ids in the DOM, so in-document #anchor links and wiki links to headings fail to scroll to the target heading and instead surface "Heading not found" errors.
Suggestion: Remove the DOM id–based heading lookup and drive anchor navigation from the parsed outline (slug → line), using the editor handle's revealLine(line, headingId) as the single navigation mechanism for both the table of contents and #anchor/wiki-heading links.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** src/ui/file-preview/src/markdown/editor.ts
**Line:** 309:311
**Comment:**
*HIGH: The new CodeMirror-based markdown editor only styles heading lines and no longer emits heading elements with stable id attributes, while the markdown controller still resolves `#anchor` and wiki-heading links by querying `document.getElementById(...)` in `scrollHeadingIntoView`. In edit/preview mode there are no such heading ids in the DOM, so in-document `#anchor` links and wiki links to headings fail to scroll to the target heading and instead surface "Heading not found" errors.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
codeant-ai
Bot
added
size:XXL
Contributor
Sequence DiagramThis PR replaces the previous WYSIWYG markdown pipeline with a CodeMirror-based editor that edits raw markdown as the canonical state, updates the outline from that source, and autosaves via edit_block without round-tripping through HTML. sequenceDiagram
participant User
participant MarkdownEditor
participant MarkdownController
participant Outline
participant EditBlockTool
User->>MarkdownEditor: Type markdown text
MarkdownEditor-->>MarkdownController: onChange(raw markdown)
MarkdownController->>MarkdownController: Update draftContent and mark dirty
MarkdownController->>MarkdownController: Schedule autosave and outline refresh
MarkdownController->>Outline: Debounced extract outline from draftContent
Outline-->>MarkdownController: Updated heading list
MarkdownController-->>Outline: Refresh document outline view
MarkdownController->>EditBlockTool: After debounce compute and send edit blocks
EditBlockTool-->>MarkdownController: Applied edits result
MarkdownController->>MarkdownController: Update fullDocumentContent baseline and dirty flag
Generated by CodeAnt AI |
Contributor
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Comment on lines
+958
to
+960
| onOpenLink: (href) => { | ||
| void navigateLink(payload, href); | ||
| }, |
Contributor
There was a problem hiding this comment.
🟠 Architect Review — HIGH
Anchor link clicks in the new CodeMirror-based markdown editor still call navigateLink, which uses scrollHeadingIntoView to locate DOM headings by id; because the editor no longer renders heading elements with ids, #anchor and [[note#heading]] links now fail with "Heading not found" instead of jumping to the heading.
Suggestion: Resolve anchors against the markdown outline (heading id → source line) and use the editor handle's revealLine to scroll to the target heading, keeping DOM-based scrolling only for views that render real heading elements with ids.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** src/ui/file-preview/src/markdown/controller.ts
**Line:** 958:960
**Comment:**
*HIGH: Anchor link clicks in the new CodeMirror-based markdown editor still call navigateLink, which uses scrollHeadingIntoView to locate DOM headings by id; because the editor no longer renders heading elements with ids, #anchor and [[note#heading]] links now fail with "Heading not found" instead of jumping to the heading.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
Contributor
Sequence DiagramThis PR replaces the WYSIWYG markdown stack with a source-backed CodeMirror editor inside the file preview, keeping raw markdown as the canonical state while autosaving diffs and updating the document outline directly from the source. sequenceDiagram
participant User
participant FilePreview
participant Controller
participant Editor
participant Host
User->>FilePreview: Open markdown file
FilePreview->>Controller: Request markdown workspace for payload
Controller->>FilePreview: Return editor shell with outline from raw markdown
FilePreview->>Editor: Mount source editor with draft content and callbacks
User->>Editor: Edit text and use formatting or link tools
Editor->>Controller: Send updated markdown draft
Controller->>Controller: Mark dirty and schedule autosave and outline refresh
Controller->>Host: On autosave apply edit blocks from baseline to draft snapshot
Host-->>Controller: Confirm edits applied on disk
Controller->>FilePreview: Update saved baseline while keeping newer unsaved draft and outline in state
Generated by CodeAnt AI |
Comment on lines
+1284
to
1292
| revealLine: (lineNumber: number) => { | ||
| const targetLine = Math.max(1, Math.min(view.state.doc.lines, Math.floor(lineNumber))); | ||
| const line = view.state.doc.line(targetLine); | ||
| view.dispatch({ | ||
| selection: EditorSelection.cursor(line.from), | ||
| effects: EditorView.scrollIntoView(line.from, { y: 'start', yMargin: 48 }), | ||
| }); | ||
| view.focus(); | ||
| }, |
Contributor
There was a problem hiding this comment.
🟠 Architect Review — HIGH
Anchor link navigation still relies on DOM heading IDs (via scrollHeadingIntoView in the controller), but the new CodeMirror-based editor no longer renders headings with id attributes, so in-editor anchor links (e.g. #section or wiki links with #anchor) and cross-note heading links no longer scroll the editor to the target heading.
Suggestion: Replace the DOM-id-based scrolling for anchors with a source-backed flow: use the outline's slug IDs and line numbers to resolve an anchor to a line, and then call the editor's revealLine API for both same-note anchors and cross-note wiki anchors instead of using scrollHeadingIntoView.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** src/ui/file-preview/src/markdown/editor.ts
**Line:** 1284:1292
**Comment:**
*HIGH: Anchor link navigation still relies on DOM heading IDs (via scrollHeadingIntoView in the controller), but the new CodeMirror-based editor no longer renders headings with id attributes, so in-editor anchor links (e.g. `#section` or wiki links with `#anchor`) and cross-note heading links no longer scroll the editor to the target heading.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+849
to
+859
| const findMarkdownLinkInLine = (line: { from: number; text: string }, relativeFrom: number, relativeTo: number = relativeFrom): MarkdownLinkRange | null => { | ||
| for (const match of line.text.matchAll(/\[([^\]\n]+)\]\(([^)\n]+)\)/g)) { | ||
| const start = match.index ?? 0; | ||
| const label = match[1] ?? ''; | ||
| if (line.text[start - 1] === '!' || label.startsWith('![')) { | ||
| continue; | ||
| } | ||
| const end = start + match[0].length; | ||
| if (start <= relativeFrom && relativeTo <= end) { | ||
| const href = (match[2] ?? '').trim(); | ||
| return { |
Contributor
There was a problem hiding this comment.
🟠 Architect Review — HIGH
Link detection in the editor uses a simple [label](href) regex that stops at the first ) and treats everything inside the parentheses as the href, so valid Markdown links with titles (e.g. [x](a.md "title")) or URLs containing parentheses (e.g. [x](https://example.com/foo(bar))) are misparsed and cause the "open/edit link" behavior to use an incorrect href.
Suggestion: Derive link ranges and hrefs from the markdown syntax tree (Lezer) instead of ad-hoc regex, so that the editor can correctly extract the bare href (without the title) and support URLs with nested parentheses for all link open/edit operations.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** src/ui/file-preview/src/markdown/editor.ts
**Line:** 849:859
**Comment:**
*HIGH: Link detection in the editor uses a simple `[label](href)` regex that stops at the first `)` and treats everything inside the parentheses as the href, so valid Markdown links with titles (e.g. `[x](a.md "title")`) or URLs containing parentheses (e.g. `[x](https://example.com/foo(bar))`) are misparsed and cause the "open/edit link" behavior to use an incorrect href.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Owner
|
I've put together a 14-case regression suite for the silent corruption symptoms from #437 and #440. Same test file produces a clean before/after:
Cases cover everything reported in #437 (pipe tables, tilde escape, adjacent-heading spacing, wikilinks, YAML frontmatter, bracket escape) and #440 (tilde paths, underscore identifiers, loose lists), plus a live in-the-wild corruption I captured separately where a 200+ line README got truncated to ~22 lines after a single Stacked PR against this branch: #444. Plus a sibling
LGTM on the architectural direction either way — raw markdown as canonical state is the right fix and removes a whole class of corruption that no amount of serializer-tweaking would have. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/ui/file-preview/src/markdown/editor.ts (1)
1266-1285:⚠️ Potential issue | 🔴 Critical
onOpenLinkstill forwards arbitraryhreffrom markdown without scheme validation.
link.hrefcomes from untrusted markdown source and is passed straight through tooptions.onOpenLink(→controller.navigateLink→dependencies.openExternalLinkand, on fallback,window.open(resolvedLink.url, '_blank', 'noopener')).noopenerdoes not blockjavascript:,data:, orvbscript:schemes, so a note containing[Click](javascript:fetch('/api/...'))or[Click](data:text/html,<script>…)can execute arbitrary script when the user clicks the popover’s "Open" button.This was raised on a previous commit and is still unmitigated here. Since
isExternalHrefinlinking.tsonly checks for^[A-Za-z][A-Za-z0-9+.-]*:with no allowlist, the easiest fix is to enforce the allowlist on the resolved URL before invokingonOpenLink.🛡️ Validate scheme before calling `onOpenLink`
if (button.id === 'link-popover-open') { - void options.onOpenLink?.(link.href); + const safeSchemes = /^(?:https?:|mailto:|#|\/|\.\.?\/|[^a-z][^:]*$)/i; + // Allow http(s)/mailto, in-doc anchors, and relative paths; block javascript:/data:/vbscript: etc. + if (safeSchemes.test(link.href)) { + void options.onOpenLink?.(link.href); + } return; }A more durable fix is to add the allowlist inside
resolveMarkdownLink/isExternalHrefso every caller (popover, future programmatic openers) gets the same guarantee.#!/bin/bash # Confirm there is still no scheme allowlist between the popover and window.open. rg -nP --type=ts -C3 'isExternalHref|resolveMarkdownLink' src/ui/file-preview/src/markdown rg -nP --type=ts -C3 'window\.open\(' src/ui/file-preview/src/markdown🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/file-preview/src/markdown/editor.ts` around lines 1266 - 1285, handleLinkPopoverClick currently forwards unvalidated link.href to options.onOpenLink, allowing dangerous schemes (javascript:, data:, vbscript:) to be opened; before calling options.onOpenLink (or controller.navigateLink), validate the resolved URL scheme against a safe allowlist (e.g., http, https, mailto, ftp as appropriate) or reuse/extend resolveMarkdownLink/isExternalHref to perform an allowlist check and reject or sanitize disallowed schemes; update handleLinkPopoverClick to call the validator for activePopoverLink.href and only invoke options.onOpenLink when the scheme is allowed (otherwise show an error/ignore), and consider moving the allowlist into resolveMarkdownLink/isExternalHref so all callers get consistent protection.src/ui/file-preview/src/markdown/controller.ts (1)
239-247:⚠️ Potential issue | 🟡 MinorOutline refresh can still apply to a stale workspace state.
outlineRefreshTimeris closure-scoped, butgetStatereplacesworkspaceState(differentfilePathorfullDocumentContent) without cancelling a pending refresh. A timer scheduled against the previous state can then runapplyOutlineUpdate(state, …)andmarkdownTocHandle?.refresh(state.outline, …)on a state object that is no longer the active one — corrupting the active document’s outline/active heading and the TOC.
disposeHandles()covers theclear()path but not the in-place workspace swap insidegetState.♻️ Pin the timer to the active state
function scheduleOutlineRefresh(state: MarkdownWorkspaceState): void { if (outlineRefreshTimer !== null) { clearTimeout(outlineRefreshTimer); } outlineRefreshTimer = setTimeout(() => { outlineRefreshTimer = null; + if (state !== workspaceState) { + return; + } applyOutlineUpdate(state, extractMarkdownOutline(state.draftContent)); }, OUTLINE_REFRESH_DEBOUNCE_MS); }Alternatively, call
cancelOutlineRefresh()whenevergetStatereassignsworkspaceState.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/file-preview/src/markdown/controller.ts` around lines 239 - 247, scheduleOutlineRefresh currently captures a state object that can become stale if getState replaces workspaceState; modify scheduleOutlineRefresh (or getState) so pending timers are invalidated when the active workspace changes: either (A) in getState/wherever workspaceState is reassigned call cancelOutlineRefresh() to clear outlineRefreshTimer, or (B) when scheduling in scheduleOutlineRefresh capture a lightweight identifier from the active state (e.g. state.filePath or state.id) and, inside the setTimeout callback, compare that identifier to the current workspaceState identifier and return early if they differ before calling applyOutlineUpdate and markdownTocHandle?.refresh; reference symbols: scheduleOutlineRefresh, outlineRefreshTimer, getState, workspaceState, applyOutlineUpdate, cancelOutlineRefresh, and markdownTocHandle.refresh.
🧹 Nitpick comments (1)
src/ui/file-preview/src/markdown/editor.ts (1)
442-456: Note: full-doc rescan on every viewport change.
collectSpanningWrapperRangesreadsview.state.doc.toString()and does a linear scan over the whole document on everydocChanged || viewportChangedupdate — i.e., also on plain scroll. For typical notes this is fine, but on very large markdown files it can show up as scroll jank.If you ever see it, two cheap mitigations:
- Cache the spanning-wrapper ranges and only recompute on
docChanged(scroll-only updates would reuse the cache).- Limit the inline regex sweep to
view.visibleRanges(already done) and only spanning wrappers need the full pass.Not blocking — just flagging while the editor is fresh.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/file-preview/src/markdown/editor.ts` around lines 442 - 456, The plugin markdownLinePreviewPlugin rebuilds decorations on every docChanged or viewportChanged by calling buildMarkdownLineDecorations, which internally calls collectSpanningWrapperRanges (which scans view.state.doc.toString()) causing full-doc rescans on scroll; fix by caching the spanning-wrapper ranges inside the plugin instance (e.g., a cachedSpanningRanges field on the class) and only recomputing that cache when update sees update.docChanged is true, while on viewport-only updates reuse the cached ranges and still limit any inline regex sweeps inside buildMarkdownLineDecorations to view.visibleRanges; update the constructor, update method, and buildMarkdownLineDecorations/collectSpanningWrapperRanges call sites accordingly so scrolls no longer trigger a full document toString() pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/ui/file-preview/src/markdown/controller.ts`:
- Around line 239-247: scheduleOutlineRefresh currently captures a state object
that can become stale if getState replaces workspaceState; modify
scheduleOutlineRefresh (or getState) so pending timers are invalidated when the
active workspace changes: either (A) in getState/wherever workspaceState is
reassigned call cancelOutlineRefresh() to clear outlineRefreshTimer, or (B) when
scheduling in scheduleOutlineRefresh capture a lightweight identifier from the
active state (e.g. state.filePath or state.id) and, inside the setTimeout
callback, compare that identifier to the current workspaceState identifier and
return early if they differ before calling applyOutlineUpdate and
markdownTocHandle?.refresh; reference symbols: scheduleOutlineRefresh,
outlineRefreshTimer, getState, workspaceState, applyOutlineUpdate,
cancelOutlineRefresh, and markdownTocHandle.refresh.
In `@src/ui/file-preview/src/markdown/editor.ts`:
- Around line 1266-1285: handleLinkPopoverClick currently forwards unvalidated
link.href to options.onOpenLink, allowing dangerous schemes (javascript:, data:,
vbscript:) to be opened; before calling options.onOpenLink (or
controller.navigateLink), validate the resolved URL scheme against a safe
allowlist (e.g., http, https, mailto, ftp as appropriate) or reuse/extend
resolveMarkdownLink/isExternalHref to perform an allowlist check and reject or
sanitize disallowed schemes; update handleLinkPopoverClick to call the validator
for activePopoverLink.href and only invoke options.onOpenLink when the scheme is
allowed (otherwise show an error/ignore), and consider moving the allowlist into
resolveMarkdownLink/isExternalHref so all callers get consistent protection.
---
Nitpick comments:
In `@src/ui/file-preview/src/markdown/editor.ts`:
- Around line 442-456: The plugin markdownLinePreviewPlugin rebuilds decorations
on every docChanged or viewportChanged by calling buildMarkdownLineDecorations,
which internally calls collectSpanningWrapperRanges (which scans
view.state.doc.toString()) causing full-doc rescans on scroll; fix by caching
the spanning-wrapper ranges inside the plugin instance (e.g., a
cachedSpanningRanges field on the class) and only recomputing that cache when
update sees update.docChanged is true, while on viewport-only updates reuse the
cached ranges and still limit any inline regex sweeps inside
buildMarkdownLineDecorations to view.visibleRanges; update the constructor,
update method, and buildMarkdownLineDecorations/collectSpanningWrapperRanges
call sites accordingly so scrolls no longer trigger a full document toString()
pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8cfb0317-7b70-47b5-bb39-008f2e6f8368
📒 Files selected for processing (3)
src/ui/file-preview/src/markdown/controller.tssrc/ui/file-preview/src/markdown/editor.tssrc/ui/file-preview/src/panel-actions.ts
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
2 participants
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.
User description
Summary
Fixes #437 #440
CodeAnt-AI Description
Edit markdown directly with clickable links and exact saves
What Changed
Impact
✅ Exact markdown saves✅ Fewer lost edits during autosave✅ Open links without leaving edit mode🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style