Feat/md document editor#427
Merged
Merged
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>
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors the file-preview UI into a composition root and adds a complete Markdown workspace (editor, outline, slugging, linking, save/undo), directory tree controller, file-type handlers, selection/context wiring, sessionStorage-backed widget persistence, host open-in/editor commands, CSS for workspace/fullscreen, and many new runtime/unit tests. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/UI
participant App as App (renderApp)
participant FileHandler as FileTypeHandlers
participant Markdown as MarkdownController
participant Host as Host / callTool
participant Storage as sessionStorage
User->>App: open payload / interact
App->>Storage: syncFromPersistedWidgetState / persistPayload
App->>FileHandler: renderPayloadBody(payload, htmlMode, startLine)
FileHandler-->>App: RenderBodyResult (html, notice)
App->>Markdown: createMarkdownController / attachHandlers (if markdown)
Markdown->>Host: callTool (read_file / edit_block / list_directory / start_process)
Host-->>Markdown: tool responses
User->>App: panel action (copy, toggle, load-more, open)
App->>Host: start_process (open in folder/editor) or callTool(read_file)
App->>Markdown: editor actions (edit, save, fullscreen)
Markdown->>App: rerender / persist local override
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| body.dc-ready { | ||
| max-height: none; | ||
| overflow: visible; | ||
| overflow: clip; |
Contributor
There was a problem hiding this comment.
Suggestion: overflow: clip is not supported in some engines, and when it is ignored the body keeps inheriting overflow: hidden from the base body rule, which leaves the UI clipped after readiness. Add a fallback overflow value before clip so unsupported browsers still restore a scrollable/visible layout. [logic error]
Severity Level: Critical 🚨
- ❌ Config editor UI becomes non-scrollable on unsupported browsers.
- ❌ File preview and markdown workspace remain clipped, no scrolling.
- ⚠️ Affected users cannot fully view tool-shell content.
- ⚠️ Accessibility degraded due to unrecoverable root-level clipping.
Suggested change
| overflow: clip; | |
| overflow: auto; |
Steps of Reproduction ✅
1. Open the Config Editor UI in a browser whose engine does not support `overflow: clip`
(e.g., older Safari/Firefox); the app is bootstrapped via
`src/ui/config-editor/src/main.ts:1-3`, which imports and calls
`bootstrapConfigEditorApp()` from `src/ui/config-editor/src/app.ts:805-807`.
2. On initial load, the global stylesheet `src/ui/styles/base.css:42-49` applies `body {
max-height: 32px; overflow: hidden; }`, intentionally clipping the page and disabling
scrolling while the widget initializes.
3. After the first render completes, `scheduleRender()` in
`src/ui/config-editor/src/app.ts:879-901` calls `render(...)` and then `markReady()`;
`markReady()` at `app.ts:801-803` executes `document.body.classList.add('dc-ready')`, and
similarly the File Preview UI does `document.body.classList.add('dc-ready')` in
`renderStatusState` and `renderLoadingState` (`src/ui/file-preview/src/app.ts:1762-1769,
1771-1778`) and again at the end of `renderApp` (`app.ts:1934`).
4. The `.dc-ready` CSS rule in `src/ui/styles/base.css:51-53` sets `body.dc-ready {
max-height: none; overflow: clip; }`; in engines that do not support `overflow: clip`,
this declaration is ignored as invalid, so `overflow` continues to inherit the previous
`hidden` value from `body` (line 48). The result is that, after readiness, the config
editor and file preview UIs render full-height content inside `body` but remain
non-scrollable and visually clipped to the viewport with no way for the user to scroll.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/styles/base.css
**Line:** 53:53
**Comment:**
*Logic Error: `overflow: clip` is not supported in some engines, and when it is ignored the body keeps inheriting `overflow: hidden` from the base `body` rule, which leaves the UI clipped after readiness. Add a fallback overflow value before `clip` so unsupported browsers still restore a scrollable/visible layout.
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 (import.meta.url === `file://${process.argv[1]}`) { |
Contributor
There was a problem hiding this comment.
Suggestion: The direct-execution guard compares import.meta.url against a raw file://${process.argv[1]} string, which breaks when the script path contains spaces or other characters that must be URL-encoded. Build the URL with new URL(...) so the comparison is normalized and the test file still runs when executed directly from such paths. [logic error]
Severity Level: Major ⚠️
- ⚠️ Direct CLI execution skips markdown workspace tests entirely.
- ⚠️ Test script may exit success without running assertions.
Suggested change
| if (import.meta.url === `file://${process.argv[1]}`) { | |
| if (process.argv[1] && import.meta.url === new URL(process.argv[1], `file://${process.cwd()}/`).href) { |
Steps of Reproduction ✅
1. From the project root `/workspace/DesktopCommanderMCP`, run the test file directly with
a relative path: `node test/test-markdown-workspace-links.js`. This executes
`test/test-markdown-workspace-links.js`, which defines `runTests` at lines 228–248 and the
direct-execution guard at line 250.
2. During this run, in Node's ESM mode, `import.meta.url` for this module becomes a
normalized file URL such as
`file:///workspace/DesktopCommanderMCP/test/test-markdown-workspace-links.js`, while
`process.argv[1]` is the relative string `test/test-markdown-workspace-links.js` (verified
in `test/test-markdown-workspace-links.js:250`).
3. The guard at `test/test-markdown-workspace-links.js:250` evaluates `if (import.meta.url
=== \`file://${process.argv[1]}\`) { ... }`. With a relative path,
`\`file://${process.argv[1]}\`` becomes `file://test/test-markdown-workspace-links.js`,
which never equals the absolute, normalized `import.meta.url`, so the condition is false
and the `runTests()` invocation at lines 251–256 is skipped.
4. Place the repo under a path containing spaces (for example `/Users/tester/My
Workspace/DesktopCommanderMCP`) and run `node /Users/tester/My
Workspace/DesktopCommanderMCP/test/test-markdown-workspace-links.js`. In this case
`import.meta.url` is
`file:///Users/tester/My%20Workspace/DesktopCommanderMCP/test/test-markdown-workspace-links.js`,
while `\`file://${process.argv[1]}\`` is `file:///Users/tester/My
Workspace/DesktopCommanderMCP/test/test-markdown-workspace-links.js` (spaces unencoded),
so the equality at line 250 still fails and `runTests()` is never called, meaning the
markdown workspace tests do not execute even though the script exits successfully.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** test/test-markdown-workspace-links.js
**Line:** 250:250
**Comment:**
*Logic Error: The direct-execution guard compares `import.meta.url` against a raw `file://${process.argv[1]}` string, which breaks when the script path contains spaces or other characters that must be URL-encoded. Build the URL with `new URL(...)` so the comparison is normalized and the test file still runs when executed directly from such paths.
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.
| write(state: T): void; | ||
| } | ||
|
|
||
| const FALLBACK_WIDGET_STATE_KEY = 'desktop-commander:file-preview:widget-state'; |
Contributor
There was a problem hiding this comment.
Suggestion: Using a single hardcoded fallback key for all widgets causes different apps in the same origin/session to overwrite each other's cached state, so persistence becomes nondeterministic across tool switches. Scope the key by pathname (or another app-unique identifier) so each widget stores and reads its own state. [logic error]
Severity Level: Major ⚠️
- ⚠️ File preview fallback restore fails after using config editor.
- ⚠️ Config editor can lose its cached state after preview usage.
- ⚠️ Cross-widget interference in same-origin sandboxed hosts.
Suggested change
| const FALLBACK_WIDGET_STATE_KEY = 'desktop-commander:file-preview:widget-state'; | |
| const FALLBACK_WIDGET_STATE_KEY = typeof window !== 'undefined' | |
| ? `desktop-commander:${window.location.pathname}:widget-state` | |
| : 'desktop-commander:widget-state'; |
Steps of Reproduction ✅
1. Host `dc-app` or another MCP host that uses a same-origin sandbox (explicitly mentioned
in `src/ui/shared/widget-state.ts:38-41`) embeds both the file preview UI
(`src/ui/file-preview/src/app.ts`) and the config editor UI
(`src/ui/config-editor/src/app.ts`) in iframes that share `sessionStorage`.
2. In the file preview iframe, the app initializes `widgetState` via
`createWidgetStateStorage<RenderPayload>(...)` at `src/ui/file-preview/src/app.ts:2015`
and later calls `widgetState.write(payload)` (e.g., in `renderAndSync` at lines 2019–23)
which writes `{ payload: RenderPayload }` under the shared key `FALLBACK_WIDGET_STATE_KEY`
in `sessionStorage` at `src/ui/shared/widget-state.ts:66`.
3. In the same browser session, the user opens the config editor, which initializes its
own `widgetState` via
`createWidgetStateStorage<ConfigEditorPayload>(isConfigEditorPayload)` at
`src/ui/config-editor/src/app.ts:823` and eventually calls
`widgetState.write(controller.state.payload)` inside the render pipeline at
`src/ui/config-editor/src/app.ts:78–80`, overwriting the same `FALLBACK_WIDGET_STATE_KEY`
with `{ payload: ConfigEditorPayload }`.
4. After a page refresh or host reconnect, when the file preview app runs again and its
`onConnected` handler reads from `widgetState.read()` (used at
`src/ui/file-preview/src/app.ts:2026` and `2197`), the shared key now contains a
config-editor payload; the file preview validator rejects it, so `read()` returns
`undefined`, and the preview cannot restore its previous cached state, degrading the
intended "survive refresh" behavior for any session where multiple widgets run on the same
origin.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/shared/widget-state.ts
**Line:** 20:20
**Comment:**
*Logic Error: Using a single hardcoded fallback key for all widgets causes different apps in the same origin/session to overwrite each other's cached state, so persistence becomes nondeterministic across tool switches. Scope the key by pathname (or another app-unique identifier) so each widget stores and reads its own state.
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 (!isChatGPT()) { | ||
| // Other hosts don't have widget state persistence - return no-op | ||
| const storage = typeof window !== 'undefined' ? window.sessionStorage : undefined; |
Contributor
There was a problem hiding this comment.
Suggestion: Accessing window.sessionStorage can throw a SecurityError in sandboxed/opaque-origin hosts, and this happens outside the current try/catch, so creating the storage adapter can fail and break app initialization. Resolve this by safely acquiring storage inside a guarded block and falling back to undefined when access is blocked. [possible bug]
Severity Level: Critical 🚨
- ❌ File preview app fails to initialize in storage-blocked hosts.
- ❌ Config editor app crashes on startup in restricted iframes.
- ⚠️ Any future non-ChatGPT host with blocked storage breaks widgets.
Suggested change
| const storage = typeof window !== 'undefined' ? window.sessionStorage : undefined; | |
| const storage = (() => { | |
| if (typeof window === 'undefined') return undefined; | |
| try { | |
| return window.sessionStorage; | |
| } catch { | |
| return undefined; | |
| } | |
| })(); |
Steps of Reproduction ✅
1. Run a non-ChatGPT MCP host that embeds the UI apps (file preview or config editor) in a
sandboxed iframe where `window.sessionStorage` access throws a `SecurityError` (e.g.,
opaque-origin or severely restricted sandbox).
2. The host loads the file preview app entrypoint `src/ui/file-preview/src/app.ts`, which
during initialization calls `createWidgetStateStorage<RenderPayload>(...)` at line 2015
(from Grep output), or loads the config editor app `src/ui/config-editor/src/app.ts`,
which calls `createWidgetStateStorage<ConfigEditorPayload>(...)` at line 823.
3. Inside `createWidgetStateStorage` in `src/ui/shared/widget-state.ts:46-48`,
`isChatGPT()` returns `false` (no `window.openai`), so execution enters the non-ChatGPT
branch and evaluates `const storage = typeof window !== 'undefined' ?
window.sessionStorage : undefined;` at line 47.
4. In the restricted iframe, accessing `window.sessionStorage` at `widget-state.ts:47`
throws a `SecurityError` DOMException outside any `try/catch`, causing
`createWidgetStateStorage` to throw and aborting the surrounding app initialization in
`app.ts` so the widget UI never finishes setting up its event handlers or rendering.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/shared/widget-state.ts
**Line:** 47:47
**Comment:**
*Possible Bug: Accessing `window.sessionStorage` can throw a `SecurityError` in sandboxed/opaque-origin hosts, and this happens outside the current `try/catch`, so creating the storage adapter can fail and break app initialization. Resolve this by safely acquiring storage inside a guarded block and falling back to `undefined` when access is blocked.
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.
Comment on lines
+26
to
+33
| const nextCount = (counts.get(baseSlug) ?? 0) + 1; | ||
| counts.set(baseSlug, nextCount); | ||
|
|
||
| if (nextCount === 1) { | ||
| return baseSlug; | ||
| } | ||
|
|
||
| return `${baseSlug}-${nextCount}`; |
Contributor
There was a problem hiding this comment.
Suggestion: The current slug deduplication can generate duplicate IDs when a heading's natural slug matches a generated suffix from another heading (for example: foo, foo-2, foo produces foo-2 twice). This breaks DOM ID uniqueness and causes anchor navigation/TOC selection to target the wrong heading. Track used slugs and advance the suffix until an unused ID is found. [logic error]
Severity Level: Major ⚠️
- ❌ Markdown workspace headings can share duplicate DOM id attributes.
- ❌ TOC clicks may scroll to wrong markdown heading.
- ⚠️ Wiki anchor links can resolve to unintended heading targets.
Suggested change
| const nextCount = (counts.get(baseSlug) ?? 0) + 1; | |
| counts.set(baseSlug, nextCount); | |
| if (nextCount === 1) { | |
| return baseSlug; | |
| } | |
| return `${baseSlug}-${nextCount}`; | |
| let nextCount = (counts.get(baseSlug) ?? 0) + 1; | |
| let candidate = nextCount === 1 ? baseSlug : `${baseSlug}-${nextCount}`; | |
| while (counts.has(candidate)) { | |
| nextCount += 1; | |
| candidate = `${baseSlug}-${nextCount}`; | |
| } | |
| counts.set(baseSlug, nextCount); | |
| counts.set(candidate, 1); | |
| return candidate; |
Steps of Reproduction ✅
1. Open a markdown file in the File Preview app (entrypoint `renderBody` in
`src/ui/file-preview/src/app.ts`, lines 107–105) where `payload.fileType === 'markdown'`,
and include headings:
`# foo`, `# foo-2`, `# foo` in the document content.
2. During rendering, `buildMarkdownWorkspaceBody` in `src/ui/file-preview/src/app.ts`
(lines 60–105) calls `extractMarkdownOutline` in
`src/ui/file-preview/src/markdown-workspace/outline.ts` (lines 49–80), which creates
`nextSlug = createSlugTracker()` (line 51) and then calls it for each heading text. With
the sample headings, the current implementation of `createSlugTracker` in
`src/ui/file-preview/src/markdown-workspace/slugify.ts` (lines 21–33) produces IDs `foo`,
`foo-2`, `foo-2` — note the duplicate `foo-2`.
3. The HTML article is rendered via `renderMarkdownWorkspacePreview` in
`src/ui/file-preview/src/markdown-workspace/preview.ts` (lines 26–41), which calls
`renderMarkdown`. Inside `renderMarkdown`, `markdown-renderer.ts` overrides
`markdown.renderer.rules.heading_open` (see
`src/ui/file-preview/src/components/markdown-renderer.ts`, lines 56–73 from grep) and
again uses `createSlugTracker()` to assign `id` attributes on `<h*>` elements; the same
duplicate sequence of IDs (`foo`, `foo-2`, `foo-2`) is applied to actual DOM headings.
4. In the running UI, click the table-of-contents entry for the third heading or follow a
wiki link that resolves to `#foo-2`:
- TOC clicks: `attachMarkdownToc` in
`src/ui/file-preview/src/markdown-workspace/toc.ts` (lines 43–99) wires
`onSelect(headingId)`; in preview mode this calls
`scrollMarkdownHeadingIntoView(headingId)` (`app.ts`, lines 18–27 in the 1030–1109
block), which uses `findMarkdownHeading` (`app.ts` line 1044 from grep) and
`document.getElementById('foo-2')`. Because there are two headings with `id="foo-2"`,
the browser returns the first one, so the TOC item for the third `# foo` scrolls to the
second `# foo-2` instead.
- Wiki links: `navigateMarkdownLink` in `src/ui/file-preview/src/app.ts` (lines 11–66
in the 1188–1267 block) resolves `[[#foo-2]]` through `resolveMarkdownLink` in
`src/ui/file-preview/src/markdown-workspace/linking.ts` (lines 94–113), obtains
`anchor: 'foo-2'`, and again calls `scrollMarkdownHeadingIntoView('foo-2')`, which
targets the first DOM element with id `foo-2`, not necessarily the intended heading.
This demonstrates that the current slug deduplication allows duplicate IDs and can
route navigation/TOC selection to the wrong heading.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/markdown-workspace/slugify.ts
**Line:** 26:33
**Comment:**
*Logic Error: The current slug deduplication can generate duplicate IDs when a heading's natural slug matches a generated suffix from another heading (for example: `foo`, `foo-2`, `foo` produces `foo-2` twice). This breaks DOM ID uniqueness and causes anchor navigation/TOC selection to target the wrong heading. Track used slugs and advance the suffix until an unused ID is found.
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.
| return undefined; | ||
| } | ||
|
|
||
| return decodeURIComponent(fragment); |
Contributor
There was a problem hiding this comment.
Suggestion: decodeURIComponent throws a URIError for malformed percent-escapes (for example #100%), and this exception bubbles out of link navigation. Guard the decode call so malformed anchors fall back to the raw fragment instead of crashing navigation. [possible bug]
Severity Level: Major ⚠️
- ❌ Malformed `%` anchors crash markdown link navigation.
- ⚠️ Internal anchors containing `%` cannot be safely followed.
Suggested change
| return decodeURIComponent(fragment); | |
| try { | |
| return decodeURIComponent(fragment); | |
| } catch { | |
| return fragment; | |
| } |
Steps of Reproduction ✅
1. Open a markdown file in the file preview so that a `RenderPayload` is built from
`FilePreviewStructuredContent` (types at `src/types.ts:75-81`, construction via
`buildRenderPayload` in `src/ui/file-preview/src/app.ts:95-99`).
2. Ensure the markdown content includes an internal link with a malformed percent escape
in its fragment, for example `[broken](#100%)`; this content becomes `payload.content`
used throughout `app.ts`.
3. The markdown preview is rendered with `renderMarkdown(content)` in
`src/ui/file-preview/src/components/markdown-renderer.ts:106-107`, which produces an `<a>`
element inside `.markdown-doc` with `href="#100%"`.
4. Click this link in the preview: the wrapper click handler in
`src/ui/file-preview/src/app.ts:1560-1619` retrieves `href="#100%"` and calls
`navigateMarkdownLink(payload, href)` (around line 35). `navigateMarkdownLink` calls
`resolveMarkdownLink(payload.filePath, href)` at `app.ts:49-58`; `resolveMarkdownLink`
treats `href` starting with `#` as an anchor and calls
`decodeAnchorFragment(href.slice(1))` at
`src/ui/file-preview/src/markdown-workspace/linking.ts:242-247`, which then executes
`decodeURIComponent('100%')` in `decodeAnchorFragment` (`linking.ts:119-125`), throwing
`URIError: URI malformed` and aborting navigation for that click.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/markdown-workspace/linking.ts
**Line:** 124:124
**Comment:**
*Possible Bug: `decodeURIComponent` throws a `URIError` for malformed percent-escapes (for example `#100%`), and this exception bubbles out of link navigation. Guard the decode call so malformed anchors fall back to the raw fragment instead of crashing navigation.
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.| const normalized = normalizeFilePath(filePath); | ||
| const lastSlashIndex = normalized.lastIndexOf('/'); | ||
| if (lastSlashIndex < 0) { | ||
| return normalized; |
Contributor
There was a problem hiding this comment.
Suggestion: When currentPath has no slash (like README.md), getDirectoryPath returns the filename instead of a directory, causing relative link resolution to produce paths like /README.md/other.md. Return . for slashless paths so relative links resolve from the current directory. [logic error]
Severity Level: Major ⚠️
- ❌ Relative links from slashless file paths resolve to nonexistent targets.
- ⚠️ Markdown link navigation fails for some top-level documents.
Suggested change
| return normalized; | |
| return '.'; | |
| } | |
| if (lastSlashIndex === 0) { | |
| return '/'; |
Steps of Reproduction ✅
1. Have the host provide `FilePreviewStructuredContent` for a top-level markdown file
whose `filePath` contains no slash, for example `'README.md'` (structure defined in
`src/types.ts:75-81` and validated in `src/ui/file-preview/src/app.ts:83-93`). The
markdown content includes a relative link `[See other](other.md)`.
2. Render this document in the markdown preview: the wrapper click handler in
`src/ui/file-preview/src/app.ts:1560-1619` attaches to `.markdown-doc`, and the markdown
is rendered via `renderMarkdown(content)` in
`src/ui/file-preview/src/components/markdown-renderer.ts:106-107`. The generated `<a>`
element has `href="other.md"` because `rewriteWikiLinks` does not alter standard
`[text](other.md)` links.
3. Click the `[See other](other.md)` link in the preview. The wrapper handler
(`app.ts:23-37` in the snippet starting at line 1560) finds the `<a>` inside
`.markdown-doc`, reads `href="other.md"`, calls `event.preventDefault()`, and then invokes
`navigateMarkdownLink(payload, href)` (`app.ts:35-36`).
4. `navigateMarkdownLink` calls `resolveMarkdownLink(payload.filePath, href)` at
`src/ui/file-preview/src/app.ts:49-58`. For `rawHref="other.md"`, `resolveMarkdownLink`
falls through to the last branch (`linking.ts:250-255`), calling
`resolveFileTargetPath(currentPath, pathPart)` with `currentPath='README.md'` and
`pathPart='other.md'` (`linking.ts:177-185`). `resolveFileTargetPath` computes
`baseDirectory = getDirectoryPath(currentPath)` (`linking.ts:183`), and because
`getDirectoryPath` returns the filename `'README.md'` when there is no slash
(`linking.ts:139-146), `toDirectoryFileUrl('README.md')` constructs the base URL
`file:///README.md/` (`linking.ts:149-162`). Building a URL for `'other.md'` against this
base yields `file:///README.md/other.md`, which `fromFileUrl`/`normalizeFilePath` turn
into `'/README.md/other.md'` (`linking.ts:164-171, 185`). `navigateMarkdownLink` then uses
this incorrect `resolvedLink.targetPath` when calling `openExternalLink` or
`readMarkdownPayload` (`src/ui/file-preview/src/app.ts:79-99`), so navigation attempts to
open `/README.md/other.md`, which typically does not exist, and the relative link from the
top-level file fails.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/markdown-workspace/linking.ts
**Line:** 143:143
**Comment:**
*Logic Error: When `currentPath` has no slash (like `README.md`), `getDirectoryPath` returns the filename instead of a directory, causing relative link resolution to produce paths like `/README.md/other.md`. Return `.` for slashless paths so relative links resolve from the current directory.
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 (fenceMatch) { | ||
| const marker = fenceMatch[1]; | ||
| if (!activeFence) { | ||
| activeFence = marker[0].repeat(marker.length); |
Contributor
There was a problem hiding this comment.
Suggestion: Fence closing logic currently closes any active fence when it sees at least three of the same marker, even if the opening fence used more characters (e.g. opening with four backticks). This can prematurely exit code blocks and incorrectly rewrite wiki links inside code content; close only when the closing fence marker type matches and its length is at least the opening length. [logic error]
Severity Level: Major ⚠️
- ⚠️ Nested fenced code blocks misrender wiki links as hyperlinks.
- ⚠️ Headings inside code may pollute markdown outline.
Suggested change
| activeFence = marker[0].repeat(marker.length); | |
| activeFence = marker; | |
| } else if (marker[0] === activeFence[0] && marker.length >= activeFence.length) { |
Steps of Reproduction ✅
1. Create a markdown document that contains a fenced code block opened with more than
three fence characters, for example:
````markdown
````markdown
```sh
echo '[[Wiki#Section]]'
This content is delivered as payload.content in RenderPayload (constructed in
src/ui/file-preview/src/app.ts:95-99).
-
Render this document in the markdown preview:
renderMarkdown(content)in
src/ui/file-preview/src/components/markdown-renderer.ts:106-107calls
rewriteWikiLinks(source)from
src/ui/file-preview/src/markdown-workspace/linking.ts:188-211before passing text to
markdown-it. -
Inside
rewriteWikiLinks, the outer opening fence line starting with four backticks
matchesFENCE_PATTERN(linking.ts:17-18, 193-198), soactiveFenceis set based on
that marker in theif (fenceMatch)block atlinking.ts:195-199. -
When the inner line starting with three backticks (e.g. ```sh) is encountered,
FENCE_PATTERNagain matches, `fenceMatch` is truthy, and because the current closing
logic only checks `trimmedStart.startsWith(activeFence[0].repeat(3))`
(`linking.ts:199-201`), it incorrectly treats this shorter triple fence as a closing
fence, clears `activeFence`, and returns the line (`linking.ts:202`). Subsequent
lines—which are still visually inside the original four-backtick code block—are processed
with `replaceWikiLinksOutsideInlineCode(line)` (`linking.ts:205-209`), causing
`[[Wiki#Section]]` and similar wiki syntax to be rewritten into real links. As a result,
markdown preview (`renderMarkdown`) and outline extraction (`extractMarkdownOutline`
calling `rewriteWikiLinks` at
`src/ui/file-preview/src/markdown-workspace/outline.ts:49-51`) misinterpret part of the
fenced code block as normal text with clickable links.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/markdown-workspace/linking.ts
**Line:** 198:198
**Comment:**
*Logic Error: Fence closing logic currently closes any active fence when it sees at least three of the same marker, even if the opening fence used more characters (e.g. opening with four backticks). This can prematurely exit code blocks and incorrectly rewrite wiki links inside code content; close only when the closing fence marker type matches and its length is at least the opening length.
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.
|
|
||
| async function resolveMarkdownLinkSearchRoot(filePath: string): Promise<string> { | ||
| const ancestors = getAncestorDirectories(filePath); | ||
| const markers = ['.git/', '.obsidian/', 'package.json', 'pnpm-workspace.yaml', 'turbo.json']; |
Contributor
There was a problem hiding this comment.
Suggestion: Workspace-root detection checks for .git/ and .obsidian/, but list_directory entries are emitted without trailing slashes (e.g., [DIR] .git). This prevents expected roots from being detected in repositories/vaults that rely on those markers. [logic error]
Severity Level: Major ⚠️
- ⚠️ Markdown link search misses files outside current subdirectory.
- ⚠️ Workspace-root detection unreliable in non-JavaScript repositories.
- ⚠️ Obsidian vaults without package.json mis-detected as non-roots.
- ⚠️ Users must manually navigate instead of using link search.
Suggested change
| const markers = ['.git/', '.obsidian/', 'package.json', 'pnpm-workspace.yaml', 'turbo.json']; | |
| const markers = ['.git', '.obsidian', 'package.json', 'pnpm-workspace.yaml', 'turbo.json']; |
Steps of Reproduction ✅
1. Open a Markdown file that lives inside a Git-based project or Obsidian vault where the
project root contains `.git` or `.obsidian` but does not contain `package.json`,
`pnpm-workspace.yaml`, or `turbo.json` (root detection logic in
`resolveMarkdownLinkSearchRoot()` at `src/ui/file-preview/src/app.ts:193-210`).
2. In the markdown editor, trigger link target search (e.g., via whatever UI in
`mountMarkdownEditor` uses the `searchLinks` callback), which calls
`searchMarkdownLinkTargets(payload.filePath, query)` (`app.ts:213-246`); this in turn
calls `resolveMarkdownLinkSearchRoot(filePath)` to choose a search root path.
3. `resolveMarkdownLinkSearchRoot()` iterates ancestor directories and for each one calls
the MCP `list_directory` tool via `rpcCallTool('list_directory', { path: ancestor, depth:
1 })`, then parses its text listing into `entries` (`app.ts:197-203`). The server-side
implementation `listDirectory()` in `src/tools/filesystem.ts:620-654` produces entries
like `"[DIR] .git"` without any trailing slash, and `handleListDirectory()` returns them
as a newline-separated string (`src/handlers/filesystem-handlers.ts:22-39`).
4. Because the current `markers` array includes `'.git/'` and `'.obsidian/'` with trailing
slashes (`app.ts:195`), the check `entries.some(entry => entry.includes(marker))` at
`app.ts:202` never matches these directory markers, so repositories or vaults that only
have `.git`/`.obsidian` and no package manifest are not recognized as roots.
`resolveMarkdownLinkSearchRoot()` then falls back to `getParentDirectory(filePath)`
(`app.ts:210`), causing `searchMarkdownLinkTargets()` to search only from the immediate
parent directory instead of the intended project root, so link search suggestions will
omit markdown files located elsewhere in the repo/vault tree.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/app.ts
**Line:** 195:195
**Comment:**
*Logic Error: Workspace-root detection checks for `.git/` and `.obsidian/`, but `list_directory` entries are emitted without trailing slashes (e.g., `[DIR] .git`). This prevents expected roots from being detected in repositories/vaults that rely on those markers.
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.
Comment on lines
+1476
to
+1481
| await rpcCallTool?.('edit_block', { | ||
| file_path: ws.filePath, | ||
| old_string: block.old_string, | ||
| new_string: block.new_string, | ||
| expected_replacements: 1, | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggestion: The save flow assumes every edit_block call succeeded as long as no exception was thrown, but this tool can return failure messages in normal responses. That can mark a failed save as successful and overwrite local state incorrectly. Validate each tool response and throw when the edit did not actually apply. [logic error]
Severity Level: Critical 🚨
- ❌ Markdown autosave may silently fail without surfacing tool errors.
- ❌ UI shows "Saved" while edit_block made no changes.
- ⚠️ Widget state can persist unsaved content inconsistent with disk.
- ⚠️ Subsequent sessions may drop user edits on refresh.
Suggested change
| await rpcCallTool?.('edit_block', { | |
| file_path: ws.filePath, | |
| old_string: block.old_string, | |
| new_string: block.new_string, | |
| expected_replacements: 1, | |
| }); | |
| const editResult = await rpcCallTool?.('edit_block', { | |
| file_path: ws.filePath, | |
| old_string: block.old_string, | |
| new_string: block.new_string, | |
| expected_replacements: 1, | |
| }); | |
| const resultText = extractToolText(editResult) ?? ''; | |
| if ((isObjectRecord(editResult) && editResult.isError === true) || !/^Successfully applied\b/i.test(resultText)) { | |
| throw new Error(resultText || 'Failed to save markdown changes.'); | |
| } |
Steps of Reproduction ✅
1. Open any Markdown file via the Desktop Commander file preview so that `bootstrapApp()`
mounts the app and `renderApp()` renders a markdown workspace, invoking
`attachMarkdownWorkspaceHandlers()` which mounts the editor with autosave wiring
(`src/ui/file-preview/src/app.ts:1522-1580`).
2. Edit the markdown so that the workspace becomes dirty: the editor `onChange` callback
at `app.ts:1543-1553` updates `markdownWorkspaceState.draftContent` and sets
`markdownWorkspaceState.dirty = value !== markdownWorkspaceState.fullDocumentContent`.
3. Trigger autosave by blurring the editor (e.g., clicking outside it); the `onBlur`
handler passed to `mountMarkdownEditor` at `app.ts:1554-1556` calls
`maybeAutosaveMarkdownDocument()`, which in turn calls `saveMarkdownDocument()` when dirty
(`app.ts:1515-1520`).
4. In a situation where the on-disk file content no longer matches
`ws.fullDocumentContent` (for example, an AI tool or another editor has changed the file
between the last read and this save), `saveMarkdownDocument()` (`app.ts:1456-1513`)
computes edit blocks and calls `rpcCallTool('edit_block', ...)` for each block
(`app.ts:1475-1481`). The server-side `handleEditBlock()` in `src/tools/edit.ts:112-201`
can return non-throwing responses that indicate failure or no-op (e.g., empty search
string, unexpected match count, fuzzy "not found" guidance) without setting `isError` to
true (`edit.ts:114-223, 260-55`). Because the current client code ignores the `editResult`
and only checks that no exception was thrown, it proceeds to mark the workspace as saved
(`ws.dirty = false; ws.saveIndicator = 'saved';` at `app.ts:1484-1489`) even though the
edits were not actually applied, leading to a silently failed save and potential data loss
once the user closes or reloads the preview.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/app.ts
**Line:** 1476:1481
**Comment:**
*Logic Error: The save flow assumes every `edit_block` call succeeded as long as no exception was thrown, but this tool can return failure messages in normal responses. That can mark a failed save as successful and overwrite local state incorrectly. Validate each tool response and throw when the edit did not actually apply.
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.
| pointer-events: none; | ||
| padding: 4px 8px; | ||
| gap: 6px; | ||
| background: var(--color-bg-primary, #fff); |
Contributor
There was a problem hiding this comment.
Suggestion: The host-framed topbar uses --color-bg-primary, but the project defines --color-background-primary (and --panel) as the theme surface token. Because of this typo, the fallback #fff is used, which can render a white topbar in dark themes. [logic error]
Severity Level: Major ⚠️
- ⚠️ Host-framed topbar background ignores host theme variable.
- ⚠️ Dark themed hosts display jarring white strip around actions.
Suggested change
| background: var(--color-bg-primary, #fff); | |
| background: var(--color-background-primary, var(--panel)); |
Steps of Reproduction ✅
1. Host supplies theme variables via `applySharedHostContext()` in
`src/ui/shared/host-context.ts:20-36`, which calls `applyHostStyleVariables(variables)` so
host-provided `--color-*` tokens (including `--color-background-primary`) become CSS
custom properties.
2. Base tokens in `src/ui/styles/base.css:11` define `--panel:
var(--color-background-primary, light-dark(#ffffff, #1e1e2e));`, and `file-preview.css`
comments at `src/ui/styles/apps/file-preview.css:37-38` state that the panel background
should come directly from host `--color-background-primary`.
3. When the host requests `hideSummaryRow: true` in its context,
`applySharedHostContext()` sets `chrome.hideSummaryRow` at
`src/ui/shared/host-context.ts:49-55`, which is synced into the `hideSummaryRow` flag in
`src/ui/file-preview/src/app.ts:16-25` via `connectWithSharedHostContext()` at
`2167-2173`, causing `<main id="tool-shell">` to render with `class="... tool-shell ...
host-framed ..."` at `src/ui/file-preview/src/app.ts:1907`.
4. In host-framed mode, the topbar uses the rule at
`src/ui/styles/apps/file-preview.css:93-107`, where line `104` reads `background:
var(--color-bg-primary, #fff);`; because the project never defines `--color-bg-primary`
(only `--color-background-primary` in `base.css`), this resolves to the hardcoded `#fff`
fallback so the host-framed topbar renders as a white strip instead of using the active
themed surface color.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/styles/apps/file-preview.css
**Line:** 104:104
**Comment:**
*Logic Error: The host-framed topbar uses `--color-bg-primary`, but the project defines `--color-background-primary` (and `--panel`) as the theme surface token. Because of this typo, the fallback `#fff` is used, which can render a white topbar in dark themes.
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.
Comment on lines
+329
to
+352
| linkResults.innerHTML = linkSearchResults.map((item) => ` | ||
| <button type="button" class="markdown-link-result${selectedLinkItem?.path === item.path ? ' is-active' : ''}" data-link-path="${item.path}"> | ||
| <span class="markdown-link-result-title">${item.title}</span> | ||
| <span class="markdown-link-result-path">${item.relativePath}</span> | ||
| </button> | ||
| `).join(''); | ||
|
|
||
| const buttons = Array.from(linkResults.querySelectorAll<HTMLButtonElement>('[data-link-path]')); | ||
| for (const button of buttons) { | ||
| button.addEventListener('click', async () => { | ||
| const nextItem = linkSearchResults.find((item) => item.path === button.dataset.linkPath); | ||
| if (!nextItem) { | ||
| return; | ||
| } | ||
|
|
||
| selectedLinkItem = nextItem; | ||
| renderLinkResults(); | ||
| if (!linkHeadingSelect) { | ||
| return; | ||
| } | ||
|
|
||
| linkHeadingSelect.innerHTML = '<option value="">Loading…</option>'; | ||
| const headings = await options.loadHeadings?.(nextItem.path) ?? []; | ||
| linkHeadingSelect.innerHTML = `<option value="">None</option>${headings.map((heading) => `<option value="${heading.text}">${heading.text}</option>`).join('')}`; |
Contributor
There was a problem hiding this comment.
Suggestion: Rendering search results with innerHTML directly injects file metadata into the DOM without escaping. A markdown file path/title containing HTML can execute script in the editor surface. Build result rows with DOM APIs (createElement/textContent) instead of string HTML. [security]
Severity Level: Critical 🚨
- ❌ Markdown editor link search can inject arbitrary HTML/JS.
- ⚠️ Malicious repository file names compromise file-preview workspace.
Suggested change
| linkResults.innerHTML = linkSearchResults.map((item) => ` | |
| <button type="button" class="markdown-link-result${selectedLinkItem?.path === item.path ? ' is-active' : ''}" data-link-path="${item.path}"> | |
| <span class="markdown-link-result-title">${item.title}</span> | |
| <span class="markdown-link-result-path">${item.relativePath}</span> | |
| </button> | |
| `).join(''); | |
| const buttons = Array.from(linkResults.querySelectorAll<HTMLButtonElement>('[data-link-path]')); | |
| for (const button of buttons) { | |
| button.addEventListener('click', async () => { | |
| const nextItem = linkSearchResults.find((item) => item.path === button.dataset.linkPath); | |
| if (!nextItem) { | |
| return; | |
| } | |
| selectedLinkItem = nextItem; | |
| renderLinkResults(); | |
| if (!linkHeadingSelect) { | |
| return; | |
| } | |
| linkHeadingSelect.innerHTML = '<option value="">Loading…</option>'; | |
| const headings = await options.loadHeadings?.(nextItem.path) ?? []; | |
| linkHeadingSelect.innerHTML = `<option value="">None</option>${headings.map((heading) => `<option value="${heading.text}">${heading.text}</option>`).join('')}`; | |
| linkResults.replaceChildren(); | |
| const fragment = document.createDocumentFragment(); | |
| for (const item of linkSearchResults) { | |
| const button = document.createElement('button'); | |
| button.type = 'button'; | |
| button.className = `markdown-link-result${selectedLinkItem?.path === item.path ? ' is-active' : ''}`; | |
| button.dataset.linkPath = item.path; | |
| const title = document.createElement('span'); | |
| title.className = 'markdown-link-result-title'; | |
| title.textContent = item.title; | |
| const path = document.createElement('span'); | |
| path.className = 'markdown-link-result-path'; | |
| path.textContent = item.relativePath; | |
| button.append(title, path); | |
| button.addEventListener('click', async () => { | |
| const nextItem = linkSearchResults.find((candidate) => candidate.path === button.dataset.linkPath); | |
| if (!nextItem) { | |
| return; | |
| } | |
| selectedLinkItem = nextItem; | |
| renderLinkResults(); | |
| if (!linkHeadingSelect) { | |
| return; | |
| } | |
| linkHeadingSelect.replaceChildren(new Option('Loading…', '')); | |
| const headings = await options.loadHeadings?.(nextItem.path) ?? []; | |
| linkHeadingSelect.replaceChildren(new Option('None', '')); | |
| headings.forEach((heading) => { | |
| linkHeadingSelect?.add(new Option(heading.text, heading.text)); | |
| }); | |
| }); | |
| fragment.appendChild(button); | |
| } | |
| linkResults.appendChild(fragment); |
Steps of Reproduction ✅
1. Open a markdown file in the File Preview app so `attachMarkdownWorkspaceHandlers()` in
`src/ui/file-preview/src/app.ts:1522-1529` runs and, when `workspaceState.mode ===
'edit'`, calls `mountMarkdownEditor()` at `app.ts:1535-1543` to initialize the rich
markdown editor.
2. Ensure the editor is in `'markdown'` view so `mountMarkdownEditor()` in
`src/ui/file-preview/src/markdown-workspace/editor.ts:249-282` creates the contenteditable
surface and the link toolbar button handled by `handleFormatClick()` at
`editor.ts:422-455` is available.
3. In the editor, click the link toolbar button (data-format `"link"`) to open the link
modal, then type a query in the "Find note" input `#markdown-link-search` (defined at
`editor.ts:269`) that matches a `.md` file whose path or filename contains HTML/JS
characters (e.g. a file returned by `searchMarkdownLinkTargets()` at
`src/ui/file-preview/src/app.ts:213-247` with name `"><img src=x onerror=alert(1)>.md`).
4. The async `runLinkSearch()` at `editor.ts:369-392` calls `options.searchLinks(query)`
and, after it resolves, `renderLinkResults()` at `editor.ts:319-355` executes
`linkResults.innerHTML = …` on lines `329-334`, interpolating `item.path`, `item.title`
and `item.relativePath` directly into HTML; the crafted filename/path breaks out of
attribute/text context and injects attacker-controlled markup or event handlers into the
`.markdown-link-results` DOM.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/markdown-workspace/editor.ts
**Line:** 329:352
**Comment:**
*Security: Rendering search results with `innerHTML` directly injects file metadata into the DOM without escaping. A markdown file path/title containing HTML can execute script in the editor surface. Build result rows with DOM APIs (`createElement`/`textContent`) instead of string HTML.
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.
Comment on lines
+385
to
+389
| linkSearchResults = await options.searchLinks(query); | ||
| selectedLinkItem = linkSearchResults[0] ?? null; | ||
| renderLinkResults(); | ||
| if (selectedLinkItem && linkHeadingSelect) { | ||
| const headings = await options.loadHeadings?.(selectedLinkItem.path) ?? []; |
Contributor
There was a problem hiding this comment.
Suggestion: Link search requests are async and can resolve out of order; an older slow response can overwrite newer results and headings, showing stale entries. Guard updates by verifying the query is still current before mutating UI state. [race condition]
Severity Level: Major ⚠️
- ⚠️ Link search shows stale results after rapid user typing.
- ⚠️ Heading dropdown can mismatch selected file and headings.
Suggested change
| linkSearchResults = await options.searchLinks(query); | |
| selectedLinkItem = linkSearchResults[0] ?? null; | |
| renderLinkResults(); | |
| if (selectedLinkItem && linkHeadingSelect) { | |
| const headings = await options.loadHeadings?.(selectedLinkItem.path) ?? []; | |
| const results = await options.searchLinks(query); | |
| if (linkSearchInput.value.trim() !== query) { | |
| return; | |
| } | |
| linkSearchResults = results; | |
| selectedLinkItem = linkSearchResults[0] ?? null; | |
| renderLinkResults(); | |
| if (selectedLinkItem && linkHeadingSelect) { | |
| const headings = await options.loadHeadings?.(selectedLinkItem.path) ?? []; | |
| if (linkSearchInput.value.trim() !== query) { | |
| return; | |
| } |
Steps of Reproduction ✅
1. Open a markdown file so `attachMarkdownWorkspaceHandlers()` in
`src/ui/file-preview/src/app.ts:1522-1543` sets up edit mode and `mountMarkdownEditor()`
at `app.ts:1535-1543` initializes the rich editor with `searchLinks: (query) =>
searchMarkdownLinkTargets(...)` and `loadHeadings: (path) =>
loadMarkdownLinkHeadings(...)`.
2. In the editor, click the link toolbar button to open the "Find note" modal and start
typing quickly into `#markdown-link-search` (defined at
`src/ui/file-preview/src/markdown-workspace/editor.ts:269`), which triggers its input
handler `handleSearchInput()` at `editor.ts:523-524` and repeatedly calls the async
`runLinkSearch()` at `editor.ts:369-392` for queries `"f"`, `"fo"`, `"foo"`, etc.
3. Each `runLinkSearch()` invocation issues `options.searchLinks(query)` (wired to
`searchMarkdownLinkTargets()` in `app.ts:213-247`, which itself calls
`rpcCallTool('start_search', …)` at `app.ts:220-227`) and then, if a result is selected,
calls `options.loadHeadings(selectedLinkItem.path)` (wired to `loadMarkdownLinkHeadings()`
in `app.ts:248-260`).
4. If an earlier slower request (e.g. for `"f"`) finishes after a later faster request
(e.g. for `"foo"`), the later-resolving `runLinkSearch()` call executes lines `385-391` in
`editor.ts`, overwriting `linkSearchResults` and the heading `<select>` with results for
the stale query because there is no check that `linkSearchInput.value` still matches
`query`, so the UI shows outdated file and heading results inconsistent with the user's
current search text.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/markdown-workspace/editor.ts
**Line:** 385:389
**Comment:**
*Race Condition: Link search requests are async and can resolve out of order; an older slow response can overwrite newer results and headings, showing stale entries. Guard updates by verifying the query is still current before mutating UI state.
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.
Comment on lines
+490
to
+506
| editor.focus(); | ||
| restoreSelection(); | ||
| if (linkMode === 'url') { | ||
| const href = linkInput?.value?.trim(); | ||
| const label = linkLabelInput?.value?.trim() || window.getSelection()?.toString().trim() || href || 'link'; | ||
| if (href) { | ||
| document.execCommand('insertHTML', false, `<a href="${href}">${label}</a>`); | ||
| syncFromEditor(); | ||
| } | ||
| } else if (selectedLinkItem) { | ||
| const selectedHeading = linkHeadingSelect?.value?.trim(); | ||
| const alias = linkAliasInput?.value?.trim(); | ||
| const pathPart = selectedLinkItem.path === options.currentFilePath ? '' : selectedLinkItem.wikiPath; | ||
| const wikiLink = `[[${pathPart}${selectedHeading ? `#${selectedHeading}` : ''}${alias ? `|${alias}` : ''}]]`; | ||
| const href = `${selectedLinkItem.relativePath}${selectedHeading ? `#${selectedHeading}` : ''}`; | ||
| const label = alias || selectedHeading || selectedLinkItem.title; | ||
| document.execCommand('insertHTML', false, `<a href="${href}" data-wiki-link="${wikiLink}">${label}</a>`); |
Contributor
There was a problem hiding this comment.
Suggestion: Creating links with insertHTML and raw href/label values allows HTML injection and broken markup when users enter quotes or tags. Insert an anchor node through DOM APIs so text is treated as text and attributes are safely set. [security]
Severity Level: Critical 🚨
- ❌ Link insertion modal allows arbitrary HTML in editor surface.
- ⚠️ Malicious labels or URLs can execute script while editing.
Suggested change
| editor.focus(); | |
| restoreSelection(); | |
| if (linkMode === 'url') { | |
| const href = linkInput?.value?.trim(); | |
| const label = linkLabelInput?.value?.trim() || window.getSelection()?.toString().trim() || href || 'link'; | |
| if (href) { | |
| document.execCommand('insertHTML', false, `<a href="${href}">${label}</a>`); | |
| syncFromEditor(); | |
| } | |
| } else if (selectedLinkItem) { | |
| const selectedHeading = linkHeadingSelect?.value?.trim(); | |
| const alias = linkAliasInput?.value?.trim(); | |
| const pathPart = selectedLinkItem.path === options.currentFilePath ? '' : selectedLinkItem.wikiPath; | |
| const wikiLink = `[[${pathPart}${selectedHeading ? `#${selectedHeading}` : ''}${alias ? `|${alias}` : ''}]]`; | |
| const href = `${selectedLinkItem.relativePath}${selectedHeading ? `#${selectedHeading}` : ''}`; | |
| const label = alias || selectedHeading || selectedLinkItem.title; | |
| document.execCommand('insertHTML', false, `<a href="${href}" data-wiki-link="${wikiLink}">${label}</a>`); | |
| const insertSafeLink = (href: string, label: string, wikiLink?: string): void => { | |
| const selection = window.getSelection(); | |
| if (!selection || selection.rangeCount === 0) { | |
| return; | |
| } | |
| const range = selection.getRangeAt(0); | |
| range.deleteContents(); | |
| const anchor = document.createElement('a'); | |
| anchor.setAttribute('href', href); | |
| if (wikiLink) { | |
| anchor.setAttribute('data-wiki-link', wikiLink); | |
| } | |
| anchor.textContent = label; | |
| range.insertNode(anchor); | |
| range.setStartAfter(anchor); | |
| range.collapse(true); | |
| selection.removeAllRanges(); | |
| selection.addRange(range); | |
| }; | |
| editor.focus(); | |
| restoreSelection(); | |
| if (linkMode === 'url') { | |
| const href = linkInput?.value?.trim(); | |
| const label = linkLabelInput?.value?.trim() || window.getSelection()?.toString().trim() || href || 'link'; | |
| if (href) { | |
| insertSafeLink(href, label); | |
| syncFromEditor(); | |
| } | |
| } else if (selectedLinkItem) { | |
| const selectedHeading = linkHeadingSelect?.value?.trim(); | |
| const alias = linkAliasInput?.value?.trim(); | |
| const pathPart = selectedLinkItem.path === options.currentFilePath ? '' : selectedLinkItem.wikiPath; | |
| const wikiLink = `[[${pathPart}${selectedHeading ? `#${selectedHeading}` : ''}${alias ? `|${alias}` : ''}]]`; | |
| const href = `${selectedLinkItem.relativePath}${selectedHeading ? `#${selectedHeading}` : ''}`; | |
| const label = alias || selectedHeading || selectedLinkItem.title; | |
| insertSafeLink(href, label, wikiLink); |
Steps of Reproduction ✅
1. Open a markdown document so `attachMarkdownWorkspaceHandlers()` in
`src/ui/file-preview/src/app.ts:1522-1543` runs and, in edit mode, calls
`mountMarkdownEditor()` at `app.ts:1535-1543` with `view: 'markdown'`, creating the rich
editor in `editor.ts:249-282`.
2. In the editor UI, click the link toolbar button (handled by `handleFormatClick()` at
`src/ui/file-preview/src/markdown-workspace/editor.ts:422-455`) to open the link modal,
then click the "URL" tab (`#markdown-link-mode-url`) which triggers the handler at
`editor.ts:518-522` to switch `linkMode` to `'url'` and focus `#markdown-link-input`.
3. In the URL mode, enter a URL and label containing HTML, for example set
`#markdown-link-input` to `https://example.com/" onmouseover="alert(1)` and
`#markdown-link-label` to `<img src=x onerror="alert(2)">`.
4. Click the "Insert" button wired to `handleLinkApply()` at `editor.ts:489-510`; when
`linkMode === 'url'`, lines `493-497` build a raw string ``<a
href="${href}">${label}</a>`` and pass it to `document.execCommand('insertHTML', …)`,
causing the browser to parse and insert an `<a>` containing attacker-controlled
attributes/markup so that the injected HTML (e.g. `<img onerror>`) executes in the context
of the markdown editor surface.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/markdown-workspace/editor.ts
**Line:** 490:506
**Comment:**
*Security: Creating links with `insertHTML` and raw `href`/label values allows HTML injection and broken markup when users enter quotes or tags. Insert an anchor node through DOM APIs so text is treated as text and attributes are safely set.
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.| updateLinkMode('url'); | ||
| linkInput?.focus(); | ||
| }); | ||
| const handleSearchInput = (): void => { void runLinkSearch(); }; |
Contributor
There was a problem hiding this comment.
Suggestion: The input handler fires runLinkSearch() without handling promise rejection, so failures from searchLinks/loadHeadings become unhandled rejections and can break UI flow. Attach a catch and reset state on failure. [possible bug]
Severity Level: Major ⚠️
- ⚠️ Search RPC failures cause unhandled rejections in browser console.
- ⚠️ Link search UI keeps outdated results after failed calls.
Suggested change
| const handleSearchInput = (): void => { void runLinkSearch(); }; | |
| const handleSearchInput = (): void => { | |
| void runLinkSearch().catch(() => { | |
| linkSearchResults = []; | |
| selectedLinkItem = null; | |
| if (linkHeadingSelect) { | |
| linkHeadingSelect.innerHTML = '<option value="">None</option>'; | |
| } | |
| renderLinkResults(); | |
| }); | |
| }; |
Steps of Reproduction ✅
1. Ensure the markdown editor is initialized in edit mode as in
`attachMarkdownWorkspaceHandlers()` at `src/ui/file-preview/src/app.ts:1522-1543` and
`mountMarkdownEditor()` at `app.ts:1535-1543`, so that `searchLinks` and `loadHeadings`
are wired to `searchMarkdownLinkTargets()` and `loadMarkdownLinkHeadings()` respectively.
2. Induce a failure in the underlying tools used by these functions—for example, by having
the host's `rpcCallTool` implementation reject calls to `'start_search'` (used in
`searchMarkdownLinkTargets()` at `app.ts:220-227`) or `'read_file'` (used in
`loadMarkdownLinkHeadings()` at `app.ts:253-257`) due to a crashed tool process or invalid
path.
3. In the link modal's "Find note" tab, type into `#markdown-link-search`
(`editor.ts:269`), which triggers `handleSearchInput()` at
`src/ui/file-preview/src/markdown-workspace/editor.ts:523-524`; this calls `void
runLinkSearch();`, and `runLinkSearch()` at `editor.ts:369-392` awaits the failing
`options.searchLinks`/`options.loadHeadings` promises.
4. Because `handleSearchInput()` discards the returned promise and `runLinkSearch()` lacks
its own try/catch, the rejection becomes an unhandled promise rejection in the browser,
leaving `linkSearchResults` and the heading `<select>` in their previous state with no
error feedback, and in host environments that treat unhandled rejections as fatal this can
interrupt subsequent script execution in the File Preview UI.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/markdown-workspace/editor.ts
**Line:** 523:523
**Comment:**
*Possible Bug: The input handler fires `runLinkSearch()` without handling promise rejection, so failures from `searchLinks`/`loadHeadings` become unhandled rejections and can break UI flow. Attach a catch and reset state on failure.
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.
Contributor
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
src/ui/file-preview/src/markdown-workspace/preview.ts (1)
5-24: Entity decoding could miss numeric/hex entities.The hardcoded entity replacements cover common cases but would leave numeric entities like
<or<as literals. For a copy-text feature this is likely acceptable, but consider using a DOM-based approach for comprehensive decoding if edge cases arise.💡 Alternative using DOM for comprehensive entity decoding
export function getRenderedMarkdownCopyText(content: string): string { const html = renderMarkdown(content); const normalizedHtml = html .replace(/<\s*br\s*\/?>/gi, '\n') .replace(/<\/p>/gi, '\n\n') .replace(/<\/h[1-6]>/gi, '\n\n') .replace(/<\/li>/gi, '\n') .replace(/<li>/gi, '- ') .replace(/<[^>]+>/g, ''); // Use DOM for comprehensive entity decoding const textarea = document.createElement('textarea'); textarea.innerHTML = normalizedHtml; const decoded = textarea.value; return decoded .replace(/\n{3,}/g, '\n\n') .trim(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/file-preview/src/markdown-workspace/preview.ts` around lines 5 - 24, getRenderedMarkdownCopyText currently does manual entity replacements that miss numeric/hex entities; replace the hardcoded .replace(...) entity decoding with a DOM-based decoder: after you compute normalizedHtml in getRenderedMarkdownCopyText, create a temporary DOM element (e.g., document.createElement('textarea') or 'div'), set its innerHTML to normalizedHtml, read the decoded text from element.value (or textContent) to get comprehensive entity decoding (including { and {), then continue with the existing newline-collapse (/\n{3,}/) and .trim() steps; keep the initial tag-to-newline and list substitutions intact and remove the manual /&/... replacements.src/ui/file-preview/src/components/markdown-renderer.ts (1)
56-80: Heading ID generation must stay synchronized with outline.ts.Both
outline.tsandmarkdown-renderer.tsindependently create aslugTrackerand iterate headings. The generated IDs will match only if:
- Both use identical
markdown-itconfigurations- Both preprocess with
rewriteWikiLinks- Both extract inline text identically
Currently this holds, but this coupling is fragile. If either module's parsing diverges (e.g., different token filtering), TOC navigation will break since
document.getElementById(item.id)relies on exact ID matches.Consider extracting a shared function that returns both the outline and a map of heading→ID, or document this synchronization requirement prominently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/file-preview/src/components/markdown-renderer.ts` around lines 56 - 80, The heading ID generation in markdown-renderer.ts (inside the override of markdown.renderer.rules.heading_open using nextSlug/createSlugTracker and extractInlineText) must be centralized so it stays synchronized with outline.ts; refactor by extracting a shared utility (e.g., generateHeadingIdMap or getHeadingIdForText) that accepts the markdown-it tokens (or heading text), applies rewriteWikiLinks and the same slug-tracking logic (createSlugTracker behavior), and returns the canonical id; export and import that utility from both markdown-renderer.ts and outline.ts, replace the local nextSlug/token text extraction with calls to the shared function, and ensure the shared utility uses the same markdown-it config and inline-text extraction logic so both modules derive identical IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/file-preview/src/app.ts`:
- Around line 1104-1133: refreshMarkdownFromDisk currently clobbers the user's
in-memory draft by unconditionally setting localPayloadOverride and clearing
markdownWorkspaceState when a newer file is read from disk; change the function
to first check the workspace dirty/saving state (e.g.,
markdownWorkspaceState?.fileDirty, markdownWorkspaceState?.isSaving or similar
flags) and if true do not overwrite localPayloadOverride or reset
markdownWorkspaceState — instead defer replacing content and trigger a
user-visible reload/merge prompt or set a pendingRemoteUpdate flag so the UI can
surface a choice; keep the existing syncPayload and rerenderCurrent behavior
only when the workspace is clean (not dirty/saving).
- Around line 1475-1512: The save loop applies edit_block RPCs one-by-one which
can leave the on-disk file partially updated if a later RPC fails; to fix, make
the save atomic or, simpler, on any catch re-sync the workspace from disk: in
the catch block call the remote read (e.g. via rpcCallTool('read_file', {
file_path: ws.filePath })) to obtain the true file contents, set
ws.fullDocumentContent, ws.sourceContent and ws.draftContent to that value, set
ws.dirty/ws.saving/saveIndicator appropriately, update currentPayload and
localPayloadOverride and call persistPayload(savedPayload) if applicable, and
update the DOM via updateSaveStatusDOM and revert button state so subsequent
retries generate diffs against the real file state instead of the stale pre-save
state; alternatively implement a server-side batched/atomic edit RPC to apply
all blocks together.
- Around line 1294-1305: revertMarkdownEditing currently resets the workspace to
ws.initialContent (which never gets updated by saveMarkdownDocument) causing
restores of stale text; change revertMarkdownEditing to use
ws.fullDocumentContent as the revert baseline: set ws.draftContent and
ws.sourceContent to ws.fullDocumentContent, compute ws.dirty by comparing
ws.fullDocumentContent to ws.fullDocumentContent (or the correct comparison used
elsewhere), clear ws.error/ws.notice as before, and update currentPayload to use
content: ws.fullDocumentContent; also ensure any disabled-state checks that
currently reference initialContent are switched to reference fullDocumentContent
so saved/auto-saved state is consistent with saveMarkdownDocument and autosaves
cannot reintroduce stale content.
In `@src/ui/file-preview/src/markdown-workspace/editor.ts`:
- Around line 319-355: The renderLinkResults function currently injects
unescaped item.title, item.relativePath and heading.text directly into
linkResults.innerHTML and linkHeadingSelect.innerHTML; instead construct DOM
nodes programmatically: clear linkResults, create a button element per
linkSearchResults item (set className, dataset.linkPath and use textContent for
title and relativePath span children), attach the click handler that sets
selectedLinkItem and then populates linkHeadingSelect by creating option
elements with option.value and option.textContent from headings (loaded via
options.loadHeadings), rather than interpolating raw HTML strings; update
references in renderLinkResults, linkResults, linkHeadingSelect,
linkSearchResults, selectedLinkItem, options.loadHeadings and headings
accordingly.
- Around line 213-243: The code uses document.execCommand('insertHTML', ...)
with interpolated strings (in applyMarkdownFormat's 'code' case and other
insertHTML usages around the wiki/link insertion paths), creating DOM-injection
sinks; replace these insertHTML code paths by constructing DOM nodes via
document.createElement and safely assigning text via textContent and attributes
via href/dataset instead of concatenating HTML. Specifically, in
applyMarkdownFormat (the 'code' case) use window.getSelection() to get the
Range, create a <code> element, set its textContent to the selected text (or
fallback), and insert the element via Range.insertNode; for the link/wiki
insertions (the other insertHTML occurrences referenced around lines 489–506),
create an <a> element, set a.textContent, a.href, and a.dataset.alias (or
dataset values) as needed, then insert or replace the selection with that
element rather than using insertHTML. Ensure all user-provided strings (labels,
hrefs, aliases, headings) are set via DOM properties (textContent, href,
dataset) and never concatenated into HTML strings.
- Around line 291-308: syncFromEditor currently calls
options.onChange(htmlToMarkdown(editor.innerHTML)) on mount and selection/focus
events which triggers a lossy round-trip even when nothing was edited; change it
so htmlToMarkdown + options.onChange are only invoked when the document content
actually changed (e.g. compute markdown = htmlToMarkdown(editor.innerHTML),
compare to a stored previousMarkdown and call options.onChange only if
different) or move the options.onChange call to the actual edit/input handler
instead of selection/focus handlers; keep the context-menu positioning logic in
syncFromEditor but prevent it from serializing the whole document unless a real
edit is detected (refer to syncFromEditor, htmlToMarkdown, options.onChange, and
any stored previousMarkdown state you add).
In `@src/ui/file-preview/src/markdown-workspace/linking.ts`:
- Around line 99-116: The current replaceWikiLinksOutsideInlineCode function
only protects single-backtick inline code and can mis-handle multi-backtick code
spans or fenced blocks; update the segmentation/parsing so code spans are
recognized by arbitrary-length backtick runs and matched with the same number of
backticks (i.e., use a regex or small parser that captures `+...same-backticks`
via a backreference such as /(`+[\s\S]*?\1)/g or implement a stateful scanner
that treats any run of N backticks as a delimiter and ignores content until the
matching N backticks), then only run WIKI_LINK_PATTERN replacement (and calls to
parseWikiLink/buildWikiDisplayText/buildWikiHref) on non-code segments; also
ensure any logic that closes fences uses the exact same backtick length instead
of hardcoding ``` so fenced blocks are preserved.
In `@src/ui/file-preview/src/markdown-workspace/outline.ts`:
- Around line 31-47: The function extractInlineText is duplicated verbatim in
markdown-workspace/outline.ts and markdown-renderer.ts; move the implementation
into a new shared helper (e.g., export from
src/ui/file-preview/src/markdown-workspace/utils.ts) and import it from both
modules. Concretely: create a named export extractInlineText in utils.ts with
the current logic, replace the local extractInlineText definitions in outline.ts
and markdown-renderer.ts with imports from that utils module, and update any
references to use the imported function to avoid duplication.
- Around line 67-69: outline.ts currently skips headings when variable "text" is
empty; to match markdown-renderer.ts you should treat empty heading text as a
fallback "section" entry instead of continuing. In the heading-processing loop
in outline.ts replace the early "if (!text) { continue; }" behavior by assigning
a fallback label/id (e.g., text = 'section' or using the same id-generation
logic used in markdown-renderer.ts) and ensure you create the outline item with
that id so TOC links (id="section") exist and navigation works for headings like
"# ". Use the same id generation strategy/format as markdown-renderer.ts to keep
IDs consistent.
In `@src/ui/styles/apps/file-preview.css`:
- Around line 1039-1040: Update the CSS rule for the selector .markdown-doc
a:hover to use the lowercase keyword "currentcolor" instead of "currentColor" so
it satisfies the stylelint value-keyword-case rule; locate the rule with the
property text-decoration-color in the .markdown-doc a:hover block and replace
the value with currentcolor.
In `@test/test-file-handlers.js`:
- Around line 367-368: The test assertion for edit_block success uses a
hardcoded string without the optional "(s)"; update the test in
test-file-handlers.js to match the actual message produced by handleEditBlock
(src/tools/edit.ts: function handling edits) by either asserting against the
dynamic fragment "Successfully applied ${result.editsApplied} edit(s)" or by
checking for the prefix "Successfully applied" plus the editsApplied value,
e.g., compare result.content[0].text to include the computed string using
result.editsApplied so singular/plural formatting matches the implementation.
---
Nitpick comments:
In `@src/ui/file-preview/src/components/markdown-renderer.ts`:
- Around line 56-80: The heading ID generation in markdown-renderer.ts (inside
the override of markdown.renderer.rules.heading_open using
nextSlug/createSlugTracker and extractInlineText) must be centralized so it
stays synchronized with outline.ts; refactor by extracting a shared utility
(e.g., generateHeadingIdMap or getHeadingIdForText) that accepts the markdown-it
tokens (or heading text), applies rewriteWikiLinks and the same slug-tracking
logic (createSlugTracker behavior), and returns the canonical id; export and
import that utility from both markdown-renderer.ts and outline.ts, replace the
local nextSlug/token text extraction with calls to the shared function, and
ensure the shared utility uses the same markdown-it config and inline-text
extraction logic so both modules derive identical IDs.
In `@src/ui/file-preview/src/markdown-workspace/preview.ts`:
- Around line 5-24: getRenderedMarkdownCopyText currently does manual entity
replacements that miss numeric/hex entities; replace the hardcoded .replace(...)
entity decoding with a DOM-based decoder: after you compute normalizedHtml in
getRenderedMarkdownCopyText, create a temporary DOM element (e.g.,
document.createElement('textarea') or 'div'), set its innerHTML to
normalizedHtml, read the decoded text from element.value (or textContent) to get
comprehensive entity decoding (including { and {), then continue with
the existing newline-collapse (/\n{3,}/) and .trim() steps; keep the initial
tag-to-newline and list substitutions intact and remove the manual
/&/... replacements.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3a7a822-fc77-4ba8-9af9-f92873bce0f8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
src/ui/file-preview/src/app.tssrc/ui/file-preview/src/components/markdown-renderer.tssrc/ui/file-preview/src/markdown-workspace/editor.tssrc/ui/file-preview/src/markdown-workspace/linking.tssrc/ui/file-preview/src/markdown-workspace/outline.tssrc/ui/file-preview/src/markdown-workspace/preview.tssrc/ui/file-preview/src/markdown-workspace/slugify.tssrc/ui/file-preview/src/markdown-workspace/toc.tssrc/ui/file-preview/src/markdown-workspace/workspace-controller.tssrc/ui/shared/widget-state.tssrc/ui/styles/apps/file-preview.csssrc/ui/styles/base.csstest/test-file-handlers.jstest/test-markdown-workspace-links.js
Comment on lines
+99
to
+116
| function replaceWikiLinksOutsideInlineCode(line: string): string { | ||
| const segments = line.split(/(`[^`]*`)/g); | ||
| return segments.map((segment) => { | ||
| if (segment.startsWith('`') && segment.endsWith('`')) { | ||
| return segment; | ||
| } | ||
|
|
||
| return segment.replace(WIKI_LINK_PATTERN, (match) => { | ||
| const parsed = parseWikiLink(match); | ||
| if (!parsed) { | ||
| return match; | ||
| } | ||
|
|
||
| const displayText = buildWikiDisplayText(parsed); | ||
| const href = buildWikiHref(parsed); | ||
| return `[${displayText}](${href} "mcp-wiki:${encodeURIComponent(match)}")`; | ||
| }); | ||
| }).join(''); |
Contributor
There was a problem hiding this comment.
The wiki-link rewriter still touches valid code samples.
Lines 100-116 only protect single-backtick inline code, and Lines 197-200 close any backtick fence with ``` even when the opener was longer. That means wiki links inside valid multi-backtick code spans or longer fenced blocks can still be rewritten unexpectedly.
Also applies to: 188-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/file-preview/src/markdown-workspace/linking.ts` around lines 99 - 116,
The current replaceWikiLinksOutsideInlineCode function only protects
single-backtick inline code and can mis-handle multi-backtick code spans or
fenced blocks; update the segmentation/parsing so code spans are recognized by
arbitrary-length backtick runs and matched with the same number of backticks
(i.e., use a regex or small parser that captures `+...same-backticks` via a
backreference such as /(`+[\s\S]*?\1)/g or implement a stateful scanner that
treats any run of N backticks as a delimiter and ignores content until the
matching N backticks), then only run WIKI_LINK_PATTERN replacement (and calls to
parseWikiLink/buildWikiDisplayText/buildWikiHref) on non-code segments; also
ensure any logic that closes fences uses the exact same backtick length instead
of hardcoding ``` so fenced blocks are preserved.
Comment on lines
+67
to
+69
| if (!text) { | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
Empty-heading handling differs from markdown-renderer.ts, risking ID mismatch.
When heading text is empty:
- outline.ts: skips the heading entirely (
continue) - markdown-renderer.ts: generates an ID using
'section'fallback
For edge cases like # (hash + whitespace only), the renderer produces a heading with id="section" but no corresponding outline entry exists. This breaks TOC → heading navigation for such headings.
Align the behavior—either both skip empty headings, or both generate a fallback entry.
🔧 Option A: Include empty headings in outline with fallback text
const text = extractInlineText(inlineToken).trim();
- if (!text) {
- continue;
- }
+ const displayText = text || 'section';
const lineMap = Array.isArray(token.map) ? token.map : undefined;
outline.push({
- id: nextSlug(text),
- text,
+ id: nextSlug(displayText),
+ text: displayText,
level,
line: Array.isArray(lineMap) && typeof lineMap[0] === 'number' ? lineMap[0] + 1 : outline.length + 1,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/file-preview/src/markdown-workspace/outline.ts` around lines 67 - 69,
outline.ts currently skips headings when variable "text" is empty; to match
markdown-renderer.ts you should treat empty heading text as a fallback "section"
entry instead of continuing. In the heading-processing loop in outline.ts
replace the early "if (!text) { continue; }" behavior by assigning a fallback
label/id (e.g., text = 'section' or using the same id-generation logic used in
markdown-renderer.ts) and ensure you create the outline item with that id so TOC
links (id="section") exist and navigation works for headings like "# ". Use the
same id generation strategy/format as markdown-renderer.ts to keep IDs
consistent.
…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>
|
Refactor file preview into smaller modules (split ~2000 line app.ts monolith into document-layout, panel-actions, |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (4)
src/ui/styles/apps/file-preview.css (1)
195-220: Show icon-only action tooltips on keyboard focus too.The custom tooltip only appears on
:hover, so sighted keyboard users lose the label while tabbing through the toolbar. Mirror this on:focus-visibleas well.Proposed change
-.panel-action[title]:hover::after { +.panel-action[title]:hover::after, +.panel-action[title]:focus-visible::after { opacity: 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/styles/apps/file-preview.css` around lines 195 - 220, The tooltip currently only becomes visible on hover; update the visibility rule so the pseudo-element for .panel-action[title] (selector .panel-action[title]::after) is also shown when the action receives keyboard focus by adding the :focus-visible state to the rule that sets opacity to 1 (i.e., change the .panel-action[title]:hover::after rule to include .panel-action[title]:focus-visible::after or combine selectors so both :hover and :focus-visible set opacity: 1); also verify the interactive elements using the .panel-action class are keyboard-focusable (have appropriate tabindex or are native focusable elements) so :focus-visible can apply.src/ui/file-preview/src/host/external-actions.ts (1)
95-100: Error detection heuristic may produce false negatives.Checking for 'error' or 'execution' in the output (line 95) could miss other error conditions and might false-positive on files with those words in their paths. Consider checking the tool result's
isErrorproperty instead if available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/file-preview/src/host/external-actions.ts` around lines 95 - 100, The current output-filtering heuristic in external-actions.ts (the block that checks the variable text and uses text.toLowerCase().includes('error')/includes('execution')) can produce false negatives/positives; change this to rely on the tool result's explicit error flag (e.g., result.isError or toolResult.isError) before early-returning, and remove or at least de-emphasize the substring checks on text. Update the logic around the variables text, lines, appName and appPath so you first check the result's isError property and only fall back to a safer text-based check (e.g., full-line match or structured parsing) if no isError is available, ensuring appName/appPath extraction still uses the trimmed lines array.src/ui/file-preview/src/directory-controller.ts (1)
100-100: Module-level counter may cause ID collisions across concurrent renders.
dirEntryIdCounteris module-scoped and only reset inrenderDirectoryBody. If multiple directory trees are rendered concurrently or ifrenderDirTreeis called directly elsewhere, IDs could collide. Consider passing the counter through the call stack or generating unique prefixes per render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/file-preview/src/directory-controller.ts` at line 100, The module-level dirEntryIdCounter can collide across concurrent renders; make the counter instance-local per render by generating a unique render prefix or an instance-scoped counter inside renderDirectoryBody (e.g., uniqueRenderId or localCounter) and pass that value or a nextId function through the call stack to renderDirTree and any helper that builds IDs (update signatures where needed), or replace the global counter with a per-render UUID-based prefix (crypto.randomUUID()) combined with a local increment to preserve stable but unique IDs per render.src/ui/file-preview/src/markdown/controller.ts (1)
69-116: LCS diff algorithm has O(n²) memory complexity.The
computeDiffHunksfunction allocates a fulldpmatrix of size(oldLength+1) × (newLength+1). While there's a guard at line 73-74 for very large inputs (> 1M cells), files with ~1000 lines each would still allocate ~4MB. Consider using a space-optimized LCS that only keeps two rows, or the Myers diff algorithm.🤖 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 69 - 116, computeDiffHunks currently builds a full (oldLength+1)x(newLength+1) dp matrix (dp) causing O(n²) memory; replace this with a linear-space diff (e.g., Myers' algorithm or Hirschberg's LCS) so memory scales O(n) and still produces the same matches -> hunks output. Concretely, remove the large dp allocation and the backtracking that relies on it, implement or call a linear-space diff routine (name it e.g., myersDiff or hirschbergLCS) that returns the sequence of matched index pairs (matches: Array<[number, number]>), then keep the existing hunks construction logic that consumes matches (previousOld/previousNew → hunks) so computeDiffHunks continues to return DiffHunk[] without changing its callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/file-preview/src/app.ts`:
- Around line 376-383: The code calls
markdownController.requestEditMode(payload) and
markdownController.refreshFromDisk(payload) concurrently, risking a race when a
fullscreen markdown file is initialized; change the sequence so requestEditMode
is awaited before calling refreshFromDisk when getCurrentDisplayMode() ===
'fullscreen' (use the existing renderAndSync/hostPayload/payload context),
otherwise still call refreshFromDisk for non-fullscreen markdown—i.e., ensure
markdownController.requestEditMode(...) completes first for fullscreen cases to
avoid inconsistent state.
In `@src/ui/file-preview/src/directory-controller.ts`:
- Around line 180-181: Replace the unsafe non-null assertions on
querySelector('.dir-warning-text') in the load-more button handling with a safe
existence check: for the occurrences around the loadMoreBtn usage (refer to the
loadMoreBtn variable and the calls setting .textContent and disabling the
button) first query the element into a const (e.g., warningEl =
loadMoreBtn.querySelector('.dir-warning-text')) and if it is null either
early-return or skip setting textContent; then continue to disable the button.
Apply the same pattern to both places (the lines around the current .textContent
assignment and the other occurrence at 197-198) so no code uses the `!` operator
on querySelector results.
- Around line 255-270: The click handler adds 'dir-loading' to the clicked row
but only removes it in the catch path; ensure the class is removed on all code
paths by calling target.classList.remove('dir-loading') after successful
processing and before any early returns (e.g., after validating result and after
invoking options.onOpenPayload(buildRenderPayload(...))), or move cleanup into a
finally block so the 'dir-loading' class is always removed whether
options.callTool('read_file'), extractToolText, or options.onOpenPayload succeed
or throw.
In `@src/ui/file-preview/src/document-layout.ts`:
- Around line 24-35: renderMarkdownSaveStatus currently returns an empty string
when workspace.saveIndicator === 'saving', so no status pill is shown during an
in-flight save; update renderMarkdownSaveStatus to render a status pill for both
'saving' and 'saved' states (and still show "File deleted" when
workspace.fileDeleted), deriving the displayed label and CSS modifier from
workspace.saveIndicator (or falling back to workspace.saving for class if
needed) so the pill shows "Saving…" (or similar) while saving and "Saved" when
complete; reference renderMarkdownSaveStatus, workspace.saveIndicator,
workspace.saving, and workspace.fileDeleted to locate and change the conditional
that currently returns '' for non-'saved' indicators.
- Around line 92-98: The trailing "Load lines" banner is suppressed when exactly
one line is missing due to the (range.totalLines - range.toLine) > 1 check;
update the hasMissingAfter logic in document-layout.ts to remove the > 1 guard
so a missing single line still shows the banner (i.e., change the
hasMissingAfter expression used by loadAfterBanner to use range?.isPartial &&
range.toLine < range.totalLines), then keep the existing loadAfterBanner
template which already computes the correct line range.
In `@src/ui/file-preview/src/document-workspace.ts`:
- Around line 8-13: The stripReadStatusLine function is too permissive and can
remove legitimate content; change it to only strip when the leading line exactly
matches the specific system status pattern used by parseReadRange (or reuse
parseReadRange to detect a valid status line) — i.e., require the full exact
format (matching the same regex capturing groups used in parseReadRange,
including the closing parenthesis/total and bracket) at the very start before
removing the first line so ordinary markdown like "[Reading ...]" is not
stripped accidentally; update stripReadStatusLine to validate against that exact
pattern (or call parseReadRange and only strip when it returns a value).
In `@src/ui/file-preview/src/host/external-actions.ts`:
- Around line 88-97: The AppleScript string interpolation into detectCommand
using trimmedPath is vulnerable because only double quotes are escaped; update
the construction so trimmedPath is robustly escaped (handle backslashes,
newlines, carriage returns and other control chars) or avoid direct
interpolation by passing the path as a separate, safely quoted argument to
osascript (e.g., use multiple -e args or a safe-quoting helper). Specifically,
modify the code around detectCommand and the use of shellQuote and trimmedPath
so the script receives a fully escaped/encoded POSIX path (or use a safe-quoting
utility) before calling options.callTool('start_process'), ensuring no special
characters can break or inject into the AppleScript.
In `@src/ui/file-preview/src/host/selection-context.ts`:
- Around line 104-113: The selection check currently allows mixed selections
that start inside the preview and extend outside because it only returns when
both anchor and focus are outside; change the condition in the selection
handling block so that it requires both endpoints to be inside the wrapper
(i.e., use || to detect when either anchorNode or focusNode is outside) and bail
out when either endpoint is outside; update the same branch that resets
lastSelectedText, calls options.updateContext('') and hideHint() to run when the
selection is not fully contained (refer to the variables wrapper, selection,
lastSelectedText, options.updateContext, and hideHint in selection-context.ts).
- Around line 18-25: When contentWrapper is missing the code currently returns
without aborting the prior controller; change the early-return branch in
selection-context.ts to first call options.previousAbortController?.abort() (and
clear any related state) before returning so the previous selectionchange
listener is removed; update any references around contentWrapper,
options.previousAbortController, and abortController to ensure the old
controller is aborted and not reused when wrapper is temporarily absent.
In `@src/ui/file-preview/src/markdown/controller.ts`:
- Around line 474-488: The code in refreshFromDisk currently calls readPayload
and immediately treats its return as freshPayload, then checks
isMissingFileErrorResult(freshPayload) which is wrong because freshPayload can
be null; instead capture the raw tool result (e.g., const rawResult = await
readPayload(...)) before extracting the payload, call
isMissingFileErrorResult(rawResult) to detect the missing-file case and update
workspaceState.fileDeleted and dependencies.updateSaveStatus('File
deleted','saved') there, and only then assign freshPayload = rawResult.payload
(or the extracted payload) for further use in refreshFromDisk.
In `@src/ui/file-preview/src/markdown/editor.ts`:
- Around line 625-651: The click handlers attached to linkModeFile and
linkModeUrl (which call updateLinkMode and focus linkInput) are not removed in
the destroy() cleanup causing potential leaks; update the destroy() return
object to also remove those listeners by calling
linkModeFile?.removeEventListener('click', /* same callback */) and
linkModeUrl?.removeEventListener('click', /* same callback */) (i.e., remove the
exact handlers that were registered for linkModeFile and linkModeUrl so the
registered functions are detached when destroy() runs).
In `@src/ui/file-preview/src/panel-actions.ts`:
- Around line 174-201: The "load after" branch passes offset: range.toLine which
is 0-index/0-based but user/status is 1-index, causing an off-by-one; update the
read_args in the options.callTool('read_file', ...) for the after case to use
offset: range.toLine + 1 (i.e. change the offset expression in the readArgs
ternary for direction === 'after' to range.toLine + 1) so the data requested
matches the status line parsing (alternatively, if you prefer adjusting display,
ensure the status generation that uses offset adds +1 like excel.ts does); keep
the rest of the merging and status logic (newFrom/newTo/isStillPartial)
unchanged.
In `@src/ui/file-preview/src/path-utils.ts`:
- Around line 23-27: getPathSegments currently treats "." as a real segment
which makes relative paths wrong; modify getPathSegments (which calls
normalizeFilePath and getPathRoot) to exclude "." segments (not just falsy
values) when splitting the normalized path so that getPathSegments('.') returns
an empty array; update the filter from filter(Boolean) to a predicate that
filters out both empty strings and "." (e.g., filter(s => s && s !== '.')) so
toPosixRelativePath and other consumers produce correct same-directory links.
In `@src/ui/file-preview/src/payload-utils.ts`:
- Around line 58-70: The current fallback calling
extractToolText(value.structuredContent) is ineffective because
structuredContent is a FilePreviewStructuredContent and doesn't have the
expected content array; update extractRenderPayload so it extracts text from the
resolved meta instead of value.structuredContent — i.e., after computing meta
via isPreviewStructuredContent, call extractToolText(value) ??
extractToolText(meta) ?? '' (or if meta holds a nested content array use
extractToolText(meta.content) accordingly) before passing to buildRenderPayload;
refer to extractRenderPayload, isPreviewStructuredContent, extractToolText, and
buildRenderPayload when making the change.
---
Nitpick comments:
In `@src/ui/file-preview/src/directory-controller.ts`:
- Line 100: The module-level dirEntryIdCounter can collide across concurrent
renders; make the counter instance-local per render by generating a unique
render prefix or an instance-scoped counter inside renderDirectoryBody (e.g.,
uniqueRenderId or localCounter) and pass that value or a nextId function through
the call stack to renderDirTree and any helper that builds IDs (update
signatures where needed), or replace the global counter with a per-render
UUID-based prefix (crypto.randomUUID()) combined with a local increment to
preserve stable but unique IDs per render.
In `@src/ui/file-preview/src/host/external-actions.ts`:
- Around line 95-100: The current output-filtering heuristic in
external-actions.ts (the block that checks the variable text and uses
text.toLowerCase().includes('error')/includes('execution')) can produce false
negatives/positives; change this to rely on the tool result's explicit error
flag (e.g., result.isError or toolResult.isError) before early-returning, and
remove or at least de-emphasize the substring checks on text. Update the logic
around the variables text, lines, appName and appPath so you first check the
result's isError property and only fall back to a safer text-based check (e.g.,
full-line match or structured parsing) if no isError is available, ensuring
appName/appPath extraction still uses the trimmed lines array.
In `@src/ui/file-preview/src/markdown/controller.ts`:
- Around line 69-116: computeDiffHunks currently builds a full
(oldLength+1)x(newLength+1) dp matrix (dp) causing O(n²) memory; replace this
with a linear-space diff (e.g., Myers' algorithm or Hirschberg's LCS) so memory
scales O(n) and still produces the same matches -> hunks output. Concretely,
remove the large dp allocation and the backtracking that relies on it, implement
or call a linear-space diff routine (name it e.g., myersDiff or hirschbergLCS)
that returns the sequence of matched index pairs (matches: Array<[number,
number]>), then keep the existing hunks construction logic that consumes matches
(previousOld/previousNew → hunks) so computeDiffHunks continues to return
DiffHunk[] without changing its callers.
In `@src/ui/styles/apps/file-preview.css`:
- Around line 195-220: The tooltip currently only becomes visible on hover;
update the visibility rule so the pseudo-element for .panel-action[title]
(selector .panel-action[title]::after) is also shown when the action receives
keyboard focus by adding the :focus-visible state to the rule that sets opacity
to 1 (i.e., change the .panel-action[title]:hover::after rule to include
.panel-action[title]:focus-visible::after or combine selectors so both :hover
and :focus-visible set opacity: 1); also verify the interactive elements using
the .panel-action class are keyboard-focusable (have appropriate tabindex or are
native focusable elements) so :focus-visible can apply.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3333952f-dab0-4acc-a2b5-0eac39fc6999
📒 Files selected for processing (32)
scripts/build-ui-runtime.cjssrc/ui/file-preview/src/app.tssrc/ui/file-preview/src/components/markdown-renderer.tssrc/ui/file-preview/src/components/toolbar.tssrc/ui/file-preview/src/directory-controller.tssrc/ui/file-preview/src/document-layout.tssrc/ui/file-preview/src/document-outline.tssrc/ui/file-preview/src/document-workspace.tssrc/ui/file-preview/src/file-type-handlers.tssrc/ui/file-preview/src/host/external-actions.tssrc/ui/file-preview/src/host/selection-context.tssrc/ui/file-preview/src/markdown/controller.tssrc/ui/file-preview/src/markdown/editor.tssrc/ui/file-preview/src/markdown/linking.tssrc/ui/file-preview/src/markdown/outline.tssrc/ui/file-preview/src/markdown/parser.tssrc/ui/file-preview/src/markdown/preview.tssrc/ui/file-preview/src/markdown/slugify.tssrc/ui/file-preview/src/markdown/utils.tssrc/ui/file-preview/src/model.tssrc/ui/file-preview/src/panel-actions.tssrc/ui/file-preview/src/path-utils.tssrc/ui/file-preview/src/payload-utils.tssrc/ui/shared/tool-header.tssrc/ui/shared/widget-state.tssrc/ui/styles/apps/file-preview.csssrc/ui/styles/base.csssrc/ui/styles/components/tool-header.csstest/test-file-handlers.jstest/test-file-preview-directory-runtime.jstest/test-markdown-preview.jstest/test-widget-state-runtime.js
💤 Files with no reviewable changes (4)
- scripts/build-ui-runtime.cjs
- src/ui/styles/components/tool-header.css
- src/ui/shared/tool-header.ts
- src/ui/file-preview/src/components/toolbar.ts
✅ Files skipped from review due to trivial changes (1)
- test/test-file-handlers.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ui/styles/base.css
- src/ui/file-preview/src/components/markdown-renderer.ts
Comment on lines
+180
to
+181
| loadMoreBtn.querySelector('.dir-warning-text')!.textContent = 'Loading…'; | ||
| (loadMoreBtn as HTMLButtonElement).disabled = true; |
Contributor
There was a problem hiding this comment.
Non-null assertions on querySelector results can throw.
Lines 180 and 197 use ! on .querySelector('.dir-warning-text') which will throw if the element structure changes. Consider using optional chaining or an early return.
🛡️ Proposed fix
- loadMoreBtn.querySelector('.dir-warning-text')!.textContent = 'Loading…';
+ const warningText = loadMoreBtn.querySelector('.dir-warning-text');
+ if (warningText) warningText.textContent = 'Loading…';- loadMoreBtn.querySelector('.dir-warning-text')!.textContent = 'Failed to load';
+ const warningText = loadMoreBtn.querySelector('.dir-warning-text');
+ if (warningText) warningText.textContent = 'Failed to load';Also applies to: 197-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/file-preview/src/directory-controller.ts` around lines 180 - 181,
Replace the unsafe non-null assertions on querySelector('.dir-warning-text') in
the load-more button handling with a safe existence check: for the occurrences
around the loadMoreBtn usage (refer to the loadMoreBtn variable and the calls
setting .textContent and disabling the button) first query the element into a
const (e.g., warningEl = loadMoreBtn.querySelector('.dir-warning-text')) and if
it is null either early-return or skip setting textContent; then continue to
disable the button. Apply the same pattern to both places (the lines around the
current .textContent assignment and the other occurrence at 197-198) so no code
uses the `!` operator on querySelector results.
Comment on lines
+255
to
+270
| if (target.classList.contains('dir-row-file')) { | ||
| target.classList.add('dir-loading'); | ||
| try { | ||
| const result = await options.callTool?.('read_file', { path: fullPath }); | ||
| if (!result || typeof result !== 'object' || result === null) { | ||
| return; | ||
| } | ||
| const structuredContent = (result as { structuredContent?: unknown }).structuredContent; | ||
| if (structuredContent && typeof structuredContent === 'object') { | ||
| const text = extractToolText(result) ?? ''; | ||
| options.onOpenPayload(buildRenderPayload(structuredContent as any, text)); | ||
| } | ||
| } catch { | ||
| target.classList.remove('dir-loading'); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
File click handler doesn't remove loading class on success.
When a file is clicked, dir-loading class is added (line 256) but only removed in the catch block (line 268). On success, the class remains, which could cause visual artifacts if the same element is reused.
🐛 Proposed fix
if (target.classList.contains('dir-row-file')) {
target.classList.add('dir-loading');
try {
const result = await options.callTool?.('read_file', { path: fullPath });
if (!result || typeof result !== 'object' || result === null) {
+ target.classList.remove('dir-loading');
return;
}
const structuredContent = (result as { structuredContent?: unknown }).structuredContent;
if (structuredContent && typeof structuredContent === 'object') {
const text = extractToolText(result) ?? '';
options.onOpenPayload(buildRenderPayload(structuredContent as any, text));
}
+ target.classList.remove('dir-loading');
} catch {
target.classList.remove('dir-loading');
}
}📝 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.
Suggested change
| if (target.classList.contains('dir-row-file')) { | |
| target.classList.add('dir-loading'); | |
| try { | |
| const result = await options.callTool?.('read_file', { path: fullPath }); | |
| if (!result || typeof result !== 'object' || result === null) { | |
| return; | |
| } | |
| const structuredContent = (result as { structuredContent?: unknown }).structuredContent; | |
| if (structuredContent && typeof structuredContent === 'object') { | |
| const text = extractToolText(result) ?? ''; | |
| options.onOpenPayload(buildRenderPayload(structuredContent as any, text)); | |
| } | |
| } catch { | |
| target.classList.remove('dir-loading'); | |
| } | |
| } | |
| if (target.classList.contains('dir-row-file')) { | |
| target.classList.add('dir-loading'); | |
| try { | |
| const result = await options.callTool?.('read_file', { path: fullPath }); | |
| if (!result || typeof result !== 'object' || result === null) { | |
| target.classList.remove('dir-loading'); | |
| return; | |
| } | |
| const structuredContent = (result as { structuredContent?: unknown }).structuredContent; | |
| if (structuredContent && typeof structuredContent === 'object') { | |
| const text = extractToolText(result) ?? ''; | |
| options.onOpenPayload(buildRenderPayload(structuredContent as any, text)); | |
| } | |
| target.classList.remove('dir-loading'); | |
| } catch { | |
| target.classList.remove('dir-loading'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/file-preview/src/directory-controller.ts` around lines 255 - 270, The
click handler adds 'dir-loading' to the clicked row but only removes it in the
catch path; ensure the class is removed on all code paths by calling
target.classList.remove('dir-loading') after successful processing and before
any early returns (e.g., after validating result and after invoking
options.onOpenPayload(buildRenderPayload(...))), or move cleanup into a finally
block so the 'dir-loading' class is always removed whether
options.callTool('read_file'), extractToolText, or options.onOpenPayload succeed
or throw.
Comment on lines
+24
to
+35
| function renderMarkdownSaveStatus(workspace: MarkdownWorkspaceState): string { | ||
| if (workspace.fileDeleted) { | ||
| return '<span class="panel-save-status panel-save-status--saved">File deleted</span>'; | ||
| } | ||
|
|
||
| if (workspace.saveIndicator !== 'saved') { | ||
| return ''; | ||
| } | ||
|
|
||
| const variant = workspace.saving ? 'saving' : 'saved'; | ||
| return `<span class="panel-save-status panel-save-status--${variant}">Saved</span>`; | ||
| } |
Contributor
There was a problem hiding this comment.
Render the actual in-flight save state.
saveIndicator === 'saving' currently renders nothing, so the new status pill never appears while a save is running. The workspace.saving boolean only changes the CSS class on a hardcoded "Saved" label, which can also drift out of sync.
Proposed change
function renderMarkdownSaveStatus(workspace: MarkdownWorkspaceState): string {
if (workspace.fileDeleted) {
return '<span class="panel-save-status panel-save-status--saved">File deleted</span>';
}
- if (workspace.saveIndicator !== 'saved') {
- return '';
- }
-
- const variant = workspace.saving ? 'saving' : 'saved';
- return `<span class="panel-save-status panel-save-status--${variant}">Saved</span>`;
+ if (workspace.saveIndicator === 'saving') {
+ return '<span class="panel-save-status panel-save-status--saving">Saving…</span>';
+ }
+
+ if (workspace.saveIndicator === 'saved') {
+ return '<span class="panel-save-status panel-save-status--saved">Saved</span>';
+ }
+
+ return '';
}📝 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.
Suggested change
| function renderMarkdownSaveStatus(workspace: MarkdownWorkspaceState): string { | |
| if (workspace.fileDeleted) { | |
| return '<span class="panel-save-status panel-save-status--saved">File deleted</span>'; | |
| } | |
| if (workspace.saveIndicator !== 'saved') { | |
| return ''; | |
| } | |
| const variant = workspace.saving ? 'saving' : 'saved'; | |
| return `<span class="panel-save-status panel-save-status--${variant}">Saved</span>`; | |
| } | |
| function renderMarkdownSaveStatus(workspace: MarkdownWorkspaceState): string { | |
| if (workspace.fileDeleted) { | |
| return '<span class="panel-save-status panel-save-status--saved">File deleted</span>'; | |
| } | |
| if (workspace.saveIndicator === 'saving') { | |
| return '<span class="panel-save-status panel-save-status--saving">Saving…</span>'; | |
| } | |
| if (workspace.saveIndicator === 'saved') { | |
| return '<span class="panel-save-status panel-save-status--saved">Saved</span>'; | |
| } | |
| return ''; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/file-preview/src/document-layout.ts` around lines 24 - 35,
renderMarkdownSaveStatus currently returns an empty string when
workspace.saveIndicator === 'saving', so no status pill is shown during an
in-flight save; update renderMarkdownSaveStatus to render a status pill for both
'saving' and 'saved' states (and still show "File deleted" when
workspace.fileDeleted), deriving the displayed label and CSS modifier from
workspace.saveIndicator (or falling back to workspace.saving for class if
needed) so the pill shows "Saving…" (or similar) while saving and "Saved" when
complete; reference renderMarkdownSaveStatus, workspace.saveIndicator,
workspace.saving, and workspace.fileDeleted to locate and change the conditional
that currently returns '' for non-'saved' indicators.
Comment on lines
+92
to
+98
| const hasMissingBefore = range?.isPartial && range.fromLine > 1; | ||
| const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines && (range.totalLines - range.toLine) > 1; | ||
| const loadBeforeBanner = hasMissingBefore | ||
| ? `<button class="load-lines-banner" id="load-before">↑ Load lines 1–${range!.fromLine - 1}</button>` | ||
| : ''; | ||
| const loadAfterBanner = hasMissingAfter | ||
| ? `<button class="load-lines-banner" id="load-after">↓ Load lines ${range!.toLine + 1}–${range!.totalLines}</button>` |
Contributor
There was a problem hiding this comment.
Keep the trailing load banner when only one line is missing.
The > 1 guard hides "Load lines …" when the partial read stops one line before EOF, so that last omitted line can’t be loaded from this UI path.
Proposed change
- const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines && (range.totalLines - range.toLine) > 1;
+ const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines;📝 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.
Suggested change
| const hasMissingBefore = range?.isPartial && range.fromLine > 1; | |
| const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines && (range.totalLines - range.toLine) > 1; | |
| const loadBeforeBanner = hasMissingBefore | |
| ? `<button class="load-lines-banner" id="load-before">↑ Load lines 1–${range!.fromLine - 1}</button>` | |
| : ''; | |
| const loadAfterBanner = hasMissingAfter | |
| ? `<button class="load-lines-banner" id="load-after">↓ Load lines ${range!.toLine + 1}–${range!.totalLines}</button>` | |
| const hasMissingBefore = range?.isPartial && range.fromLine > 1; | |
| const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines; | |
| const loadBeforeBanner = hasMissingBefore | |
| ? `<button class="load-lines-banner" id="load-before">↑ Load lines 1–${range!.fromLine - 1}</button>` | |
| : ''; | |
| const loadAfterBanner = hasMissingAfter | |
| ? `<button class="load-lines-banner" id="load-after">↓ Load lines ${range!.toLine + 1}–${range!.totalLines}</button>` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/file-preview/src/document-layout.ts` around lines 92 - 98, The
trailing "Load lines" banner is suppressed when exactly one line is missing due
to the (range.totalLines - range.toLine) > 1 check; update the hasMissingAfter
logic in document-layout.ts to remove the > 1 guard so a missing single line
still shows the banner (i.e., change the hasMissingAfter expression used by
loadAfterBanner to use range?.isPartial && range.toLine < range.totalLines),
then keep the existing loadAfterBanner template which already computes the
correct line range.
Comment on lines
+174
to
+201
| try { | ||
| const readArgs = direction === 'before' | ||
| ? { path: options.payload.filePath, offset: 0, length: range.fromLine - 1 } | ||
| : { path: options.payload.filePath, offset: range.toLine }; | ||
|
|
||
| const result = await options.callTool?.('read_file', readArgs); | ||
| const newText = extractToolText(result); | ||
|
|
||
| if (newText && typeof newText === 'string') { | ||
| const cleanNew = stripReadStatusLine(newText); | ||
| const merged = direction === 'before' | ||
| ? `${cleanNew}${cleanNew.endsWith('\n') ? '' : '\n'}${currentContent}` | ||
| : `${currentContent}${currentContent.endsWith('\n') ? '' : '\n'}${cleanNew}`; | ||
|
|
||
| const newFrom = direction === 'before' ? 1 : range.fromLine; | ||
| const newTo = direction === 'after' ? range.totalLines : range.toLine; | ||
| const lineCount = newTo - newFrom + 1; | ||
| const remaining = range.totalLines - newTo; | ||
| const isStillPartial = newFrom > 1 || newTo < range.totalLines; | ||
| const statusLine = isStillPartial | ||
| ? `[Reading ${lineCount} lines from ${newFrom === 1 ? 'start' : `line ${newFrom}`} (total: ${range.totalLines} lines, ${remaining} remaining)]\n` | ||
| : ''; | ||
|
|
||
| options.render({ | ||
| ...options.payload, | ||
| content: statusLine + merged, | ||
| }, options.htmlMode, options.getIsExpanded()); | ||
| return; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how read_file offset/length parameters are interpreted
rg -n -A10 'offset.*length|offset.*offset' --type=ts -g '!node_modules' -g '!dist'Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 39819
🏁 Script executed:
# Check the readFileWithSmartPositioning implementation
rg -n -A20 'readFileWithSmartPositioning' src/utils/files/text.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1914
🏁 Script executed:
# Check range.fromLine and range.toLine semantics in panel-actions.ts
rg -n -B5 'range.fromLine\|range.toLine' src/ui/file-preview/src/panel-actions.ts | head -50Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 58
🏁 Script executed:
# Check the parseReadRange function to understand how range is constructed
rg -n -A15 'function parseReadRange\|export.*parseReadRange' src/Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 58
🏁 Script executed:
# Find where range is defined - likely from parseReadRange
rg -n 'parseReadRange\|interface.*Range\|type.*Range' src/ui/file-preview/src/ -A5Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 58
🏁 Script executed:
# Check panel-actions.ts more comprehensively for range-related code
cat -n src/ui/file-preview/src/panel-actions.ts | head -250 | tail -100Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 4886
🏁 Script executed:
# Look at the complete context around lines 174-201
sed -n '150,220p' src/ui/file-preview/src/panel-actions.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 2874
🏁 Script executed:
# Find parseReadRange function
rg -n -A20 'parseReadRange' src/ui/file-preview/src/Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 29586
🏁 Script executed:
# Also check the RenderPayload type to understand the structure
rg -n 'interface.*RenderPayload\|type.*RenderPayload' src/ -A10Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 58
🏁 Script executed:
# Look for where the status line is generated to understand line numbering
rg -n '\[Reading.*lines from' src/ -B5 -A2Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 4917
🏁 Script executed:
# Check if there's offset adjustment when passing to read_file or if offset is actually meant to be 1-indexed
rg -n 'offset.*\+.*1|offset.*-.*1' src/ -A2 -B2 | head -100Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 4783
🏁 Script executed:
# Look at where offset is computed in panel-actions.ts and verify the after case logic
sed -n '174,205p' src/ui/file-preview/src/panel-actions.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1704
🏁 Script executed:
# Check how the "after" case actually uses offset and what the readFileInternal/readFromStartWithReadline contract is
rg -n -B5 -A10 'readFromStartWithReadline' src/utils/files/text.ts | grep -A15 'lineNumber'Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 58
🏁 Script executed:
# Get the complete generateEnhancedStatusMessage function to see how offset is reported
sed -n '140,180p' src/utils/files/text.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1665
🏁 Script executed:
# Check server.ts documentation more carefully about offset semantics
sed -n '295,310p' src/server.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1124
🏁 Script executed:
# Check readFileInternal to see how it interprets offset
sed -n '499,535p' src/tools/filesystem.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1589
🏁 Script executed:
# Verify the exact interpretation: when offset=75 is used for read_file, what does the status message say?
# Check how generateEnhancedStatusMessage is called in context
rg -n -B10 'generateEnhancedStatusMessage' src/utils/files/text.ts | head -50Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1881
🏁 Script executed:
# Let me verify the contract one more time by looking at actual test cases or usage patterns
rg -n 'offset.*range\|range.*offset' src/ui/file-preview/ -B2 -A2Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 58
🏁 Script executed:
# Check what the offset parameter documentation says more precisely
sed -n '32,33p' src/tools/schemas.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 275
🏁 Script executed:
# Verify how offset is actually used in the actual read implementations
sed -n '350,365p' src/utils/files/text.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 450
🏁 Script executed:
# Final check: trace through a concrete example by looking at the actual status format
# Let's see what parseReadRange expects
sed -n '12,27p' src/ui/file-preview/src/document-workspace.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 625
🏁 Script executed:
# Let me verify the actual behavior by checking if offset should be adjusted by looking at how it's used in readFileInternal
# Check the contract more carefully by looking at concrete usage
rg -n 'offset.*0\|offset.*1' src/utils/files/text.ts -A2 -B2 | head -60Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 58
🏁 Script executed:
# Check if there's any 1-index adjustment anywhere in the pipeline
rg -n 'offset.*[+-].*1' src/utils/files/ -A1 -B1Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 651
🏁 Script executed:
# Perfect! This confirms offset is 0-indexed. Let me verify this applies to text files too
# Check if text.ts also should add 1 when reporting offset in status message
sed -n '154,165p' src/utils/files/text.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 682
🏁 Script executed:
# One final confirmation: let's see how the "before" case works to understand the intended contract
# In "before" case: offset: 0, length: range.fromLine - 1
# If range.fromLine = 50, then offset: 0, length: 49
# This reads lines[0:49] = indices 0-48 = lines 1-49 (1-indexed)
# Status: "from start" ✓ parseReadRange extracts fromLine=1 ✓
#
# In "after" case: offset: range.toLine
# If range.toLine = 100, then offset: 100
# This reads lines[100:] starting at index 100 = line 101+ (1-indexed)
# Status: "from line 100" ✗ parseReadRange extracts fromLine=100 but should be 101
# Let's check if there's any comment in the code acknowledging this
rg -n 'fromLine.*toLine\|toLine.*offset' src/ui/file-preview/src/panel-actions.ts -B3 -A3Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 58
Partial load "after" case has an off-by-one error in line range status.
For the "load after" case (line 177), the code passes offset: range.toLine. Since offset is 0-indexed internally but the status message reports it as a 1-indexed line number, this creates a mismatch:
- If
range.toLine = 100, the code requestsoffset: 100 readFromStartWithReadlinereads starting fromlineNumber >= 100, which is indices 100+ = lines 101+ (1-indexed)- But the status message reports:
[Reading ... from line ${offset}]= "from line 100" parseReadRangeextractsfromLine = 100from this status- Result: the range reports lines 100–N, but actual content starts at line 101
This is consistent with text.ts:157 which reports offset as-is without adjustment, differing from excel.ts:57 which correctly reports (offset + 1) for user-facing display. The "before" case avoids this because offset: 0 is special-cased to "from start" in the status message.
Fix: Either change line 177 to offset: range.toLine + 1, or ensure the status message generation adds 1 to offset when reporting (like Excel does).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/file-preview/src/panel-actions.ts` around lines 174 - 201, The "load
after" branch passes offset: range.toLine which is 0-index/0-based but
user/status is 1-index, causing an off-by-one; update the read_args in the
options.callTool('read_file', ...) for the after case to use offset:
range.toLine + 1 (i.e. change the offset expression in the readArgs ternary for
direction === 'after' to range.toLine + 1) so the data requested matches the
status line parsing (alternatively, if you prefer adjusting display, ensure the
status generation that uses offset adds +1 like excel.ts does); keep the rest of
the merging and status logic (newFrom/newTo/isStillPartial) unchanged.
Comment on lines
+23
to
+27
| function getPathSegments(value: string): string[] { | ||
| const normalized = normalizeFilePath(value); | ||
| const root = getPathRoot(normalized); | ||
| return normalized.slice(root.length).split('/').filter(Boolean); | ||
| } |
Contributor
There was a problem hiding this comment.
"." is treated as a real path segment, producing incorrect relative paths.
getPathSegments('.') returns ['.'], so toPosixRelativePath('.', 'docs/a.md') resolves to ../docs/a.md instead of docs/a.md. This breaks same-directory link generation.
🔧 Proposed fix
function getPathSegments(value: string): string[] {
const normalized = normalizeFilePath(value);
const root = getPathRoot(normalized);
- return normalized.slice(root.length).split('/').filter(Boolean);
+ return normalized
+ .slice(root.length)
+ .split('/')
+ .filter((segment) => segment.length > 0 && segment !== '.');
}📝 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.
Suggested change
| function getPathSegments(value: string): string[] { | |
| const normalized = normalizeFilePath(value); | |
| const root = getPathRoot(normalized); | |
| return normalized.slice(root.length).split('/').filter(Boolean); | |
| } | |
| function getPathSegments(value: string): string[] { | |
| const normalized = normalizeFilePath(value); | |
| const root = getPathRoot(normalized); | |
| return normalized | |
| .slice(root.length) | |
| .split('/') | |
| .filter((segment) => segment.length > 0 && segment !== '.'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/file-preview/src/path-utils.ts` around lines 23 - 27, getPathSegments
currently treats "." as a real segment which makes relative paths wrong; modify
getPathSegments (which calls normalizeFilePath and getPathRoot) to exclude "."
segments (not just falsy values) when splitting the normalized path so that
getPathSegments('.') returns an empty array; update the filter from
filter(Boolean) to a predicate that filters out both empty strings and "."
(e.g., filter(s => s && s !== '.')) so toPosixRelativePath and other consumers
produce correct same-directory links.
Comment on lines
+58
to
+70
| export function extractRenderPayload(value: unknown): RenderPayload | undefined { | ||
| if (!isObjectRecord(value)) { | ||
| return undefined; | ||
| } | ||
| const meta = isPreviewStructuredContent(value.structuredContent) | ||
| ? value.structuredContent | ||
| : isPreviewStructuredContent(value) | ||
| ? value | ||
| : null; | ||
| if (!meta) return undefined; | ||
| const text = extractToolText(value) ?? extractToolText(value.structuredContent) ?? ''; | ||
| return buildRenderPayload(meta, text); | ||
| } |
Contributor
There was a problem hiding this comment.
extractToolText(value.structuredContent) is likely ineffective.
Line 68 calls extractToolText(value.structuredContent), but structuredContent is typically a FilePreviewStructuredContent object that doesn't contain a content array. This fallback will always return undefined. If intended to extract text from a nested structure, the logic needs adjustment.
🔍 Suggested simplification
export function extractRenderPayload(value: unknown): RenderPayload | undefined {
if (!isObjectRecord(value)) {
return undefined;
}
const meta = isPreviewStructuredContent(value.structuredContent)
? value.structuredContent
: isPreviewStructuredContent(value)
? value
: null;
if (!meta) return undefined;
- const text = extractToolText(value) ?? extractToolText(value.structuredContent) ?? '';
+ const text = extractToolText(value) ?? '';
return buildRenderPayload(meta, text);
}📝 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.
Suggested change
| export function extractRenderPayload(value: unknown): RenderPayload | undefined { | |
| if (!isObjectRecord(value)) { | |
| return undefined; | |
| } | |
| const meta = isPreviewStructuredContent(value.structuredContent) | |
| ? value.structuredContent | |
| : isPreviewStructuredContent(value) | |
| ? value | |
| : null; | |
| if (!meta) return undefined; | |
| const text = extractToolText(value) ?? extractToolText(value.structuredContent) ?? ''; | |
| return buildRenderPayload(meta, text); | |
| } | |
| export function extractRenderPayload(value: unknown): RenderPayload | undefined { | |
| if (!isObjectRecord(value)) { | |
| return undefined; | |
| } | |
| const meta = isPreviewStructuredContent(value.structuredContent) | |
| ? value.structuredContent | |
| : isPreviewStructuredContent(value) | |
| ? value | |
| : null; | |
| if (!meta) return undefined; | |
| const text = extractToolText(value) ?? ''; | |
| return buildRenderPayload(meta, text); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/file-preview/src/payload-utils.ts` around lines 58 - 70, The current
fallback calling extractToolText(value.structuredContent) is ineffective because
structuredContent is a FilePreviewStructuredContent and doesn't have the
expected content array; update extractRenderPayload so it extracts text from the
resolved meta instead of value.structuredContent — i.e., after computing meta
via isPreviewStructuredContent, call extractToolText(value) ??
extractToolText(meta) ?? '' (or if meta holds a nested content array use
extractToolText(meta.content) accordingly) before passing to buildRenderPayload;
refer to extractRenderPayload, isPreviewStructuredContent, extractToolText, and
buildRenderPayload when making the change.
Owner
|
Does this PR include any analytics/telemetry captures for the markdown editor interactions (e.g. opening edit mode, saving, etc.)? I don't see any |
Owner
|
I am not sure I understand this |
Contributor
|
CodeAnt AI is running Incremental 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 refactors the file preview widget to add a built-in markdown editor that saves via edit_block, reloads content when needed, and resolves disk conflicts through a user-facing dialog. sequenceDiagram
participant User
participant FilePreview
participant Backend
participant Filesystem
User->>Backend: read_file for markdown file
Backend-->>FilePreview: Structured markdown payload (possibly partial)
User->>FilePreview: Enter fullscreen edit and modify text
FilePreview->>Backend: edit_block with computed edit hunks
Backend->>Filesystem: Apply markdown edits
Filesystem-->>Backend: Write result
alt No disk conflicts
Backend-->>FilePreview: Updated snippet and structuredContent
FilePreview->>FilePreview: Refresh baseline, mark Saved, cache widget state
else Disk changed externally
Backend-->>FilePreview: Edit failure with partial or no hunks applied
FilePreview->>Backend: read_file to refresh from disk
FilePreview-->>User: Show conflict dialog keep disk vs save mine
User->>FilePreview: Choose save my changes
FilePreview->>Backend: Retry edit_block against refreshed baseline
end
Generated by CodeAnt AI |
Comment on lines
+526
to
+530
| if (needsContentRead(payload)) { | ||
| renderLoadingState(container); | ||
| void readAndResolvePayload(payload, (p) => renderAndSync(getEffectiveIncomingPayload(p))); | ||
| } else { | ||
| renderAndSync(getEffectiveIncomingPayload(payload)); |
Contributor
There was a problem hiding this comment.
Suggestion: This async refresh path can render stale data out of order: if two tool results arrive quickly, an older readAndResolvePayload promise can finish after a newer one and overwrite the current preview with outdated content. Add a request/version token (or cancellation) and ignore late completions that are no longer current. [race condition]
Severity Level: Critical 🚨
- ❌ File preview can revert to older tool result.
- ❌ Markdown editor may show stale document content.
- ⚠️ Users might edit outdated version after rapid operations.Steps of Reproduction ✅
1. Load the File Preview UI so `bootstrapApp()` runs from
`src/ui/file-preview/src/main.ts:6`, which initializes the MCP `App` instance and wires
`app.ontoolresult` in `src/ui/file-preview/src/app.ts:506-536`.
2. Trigger two tool operations that both produce structured payloads requiring a follow-up
`read_file` (so `needsContentRead(payload)` is true), e.g., back‑to‑back
`edit_block`/`write_file` calls on different files; each result is delivered through
`app.ontoolresult` at `app.ts:506-536`.
3. When the first such result arrives, the branch at `app.ts:525-528` executes:
`needsContentRead(payload)` is true, `renderLoadingState(container)` is called, and
`readAndResolvePayload(payload, cb1)` starts an async `read_file` via
`markdownController.readPayload` (`markdown/controller.ts:249-258`), which eventually
calls `cb1(freshPayload1)` and then
`renderAndSync(getEffectiveIncomingPayload(freshPayload1))`.
4. Before `cb1` runs, a second result for a newer operation arrives; `app.ontoolresult`
starts a second `readAndResolvePayload(payload2, cb2)` at `app.ts:525-528`, and `cb2`
completes first, calling `renderAndSync(getEffectiveIncomingPayload(payload2))` so the UI
shows `payload2`. When `cb1` later completes, it still calls
`renderAndSync(getEffectiveIncomingPayload(payload1))` (no request/version guard is
checked in `readAndResolvePayload` at `app.ts:164-181` or in `renderAndSync` at
`app.ts:406-411`), causing the older `payload1` to overwrite the newer `payload2` in the
preview and widget 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/app.ts
**Line:** 526:530
**Comment:**
*Race Condition: This async refresh path can render stale data out of order: if two tool results arrive quickly, an older `readAndResolvePayload` promise can finish after a newer one and overwrite the current preview with outdated content. Add a request/version token (or cancellation) and ignore late completions that are no longer current.
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
+547
to
+553
| const handleFocusSync = (): void => { | ||
| // Only sync cross-tab state if the page was hidden (tab switch). | ||
| // Simple focus changes within the same page should not trigger a re-render | ||
| // as it destroys the active editor. | ||
| if (document.visibilityState !== 'visible') { | ||
| syncFromPersistedWidgetState(); | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: This focus handler condition is inverted for its intent: focus events normally fire when the document is visible, so this branch almost never runs and cross-tab state sync on focus is effectively disabled. Track prior hidden state or check for visible in the right phase before syncing. [logic error]
Severity Level: Major ⚠️
- ⚠️ Focus-based preview recovery never runs, contrary to comment.
- ⚠️ Stale preview possible when host reuses iframe on focus.Steps of Reproduction ✅
1. When `bootstrapApp()` runs from `src/ui/file-preview/src/main.ts:6`, it sets up
persistence via `createWidgetStateStorage` and a sync helper
`syncFromPersistedWidgetState` at `app.ts:412-427`, then registers `handleVisibilitySync`
and `handleFocusSync` as listeners on `document` and `window` respectively at
`app.ts:566-567`.
2. `handleVisibilitySync` at `app.ts:542-546` correctly rehydrates preview state when
`document.visibilityState === 'visible'` after a visibility change, calling
`syncFromPersistedWidgetState()` to load a cached `RenderPayload` and then `renderAndSync`
(`app.ts:406-427`).
3. `handleFocusSync` at `app.ts:547-553` is documented to "only sync cross-tab state if
the page was hidden (tab switch)" and to avoid re-renders on simple focus changes, but its
implementation checks `if (document.visibilityState !== 'visible') {
syncFromPersistedWidgetState(); }`. In normal browsers, focus events fire while
`document.visibilityState` is `'visible'`, so this condition is false and the intended
sync never runs.
4. In scenarios where the host relies on focus-driven recovery without a
`visibilitychange` (for example, an already-visible preview iframe regains focus or the
host reuses a frame and expects the widget to pull from `widgetState` defined in
`src/ui/shared/widget-state.ts:114-169`), the stale preview will not be updated on focus
because `handleFocusSync`'s guard prevents `syncFromPersistedWidgetState()` from
executing.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/app.ts
**Line:** 547:553
**Comment:**
*Logic Error: This focus handler condition is inverted for its intent: focus events normally fire when the document is visible, so this branch almost never runs and cross-tab state sync on focus is effectively disabled. Track prior hidden state or check for `visible` in the right phase before syncing.
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
+51
to
+57
| if (title?.startsWith('mcp-wiki:')) { | ||
| const rawWikiLink = decodeURIComponent(title.slice('mcp-wiki:'.length)); | ||
| token.attrSet?.('data-wiki-link', rawWikiLink); | ||
| if (Array.isArray(token.attrs)) { | ||
| token.attrs = token.attrs.filter(([name]) => name !== 'title'); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: decodeURIComponent can throw on malformed percent-encoding in link titles, which will break markdown rendering for that payload. Use a safe decode helper or wrap decoding in try/catch and fall back to the raw value. [possible bug]
Severity Level: Major ⚠️
- ❌ Copy-active-markdown fails on malformed mcp-wiki title encodings.
- ⚠️ Any future renderMarkdown consumers can crash on such links.Steps of Reproduction ✅
1. Open a markdown file in the DesktopCommander file preview whose content includes a link
with a manually crafted wiki title using malformed percent-encoding, e.g. `[alias](href
"mcp-wiki:%E0%A4%A")` (invalid UTF-8 sequence) in the file text loaded into the markdown
workspace (`state.draftContent` in
`src/ui/file-preview/src/markdown/controller.ts:1019-1022`).
2. In the UI, switch to markdown editor/preview so that the markdown controller is active
for this file, then click the "Copy active markdown" button wired in
`src/ui/file-preview/src/panel-actions.ts:19-31` (button `copy-active-markdown` calls
`options.markdownController.getCopyText(options.payload)`).
3. `getCopyText` in `src/ui/file-preview/src/markdown/controller.ts:1018-1024` calls
`getRenderedMarkdownCopyText(source)` for non-raw view, which in turn calls
`renderMarkdown(content)` in `src/ui/file-preview/src/markdown/preview.ts:3-4`.
4. `renderMarkdown` in `src/ui/file-preview/src/components/markdown-renderer.ts:66-67`
invokes the markdown-it pipeline; when the link is processed, the `link_open` rule
override at lines 42-53 executes, `title` is `"mcp-wiki:%E0%A4%A"`, and
`decodeURIComponent(title.slice('mcp-wiki:'.length))` at line 52 throws a `URIError`,
causing `renderMarkdown` and thus `getRenderedMarkdownCopyText` / `getCopyText` to throw,
aborting the click handler so no text is copied and the UI logs an error. This contrasts
with `restoreWikiLinks` in `src/ui/file-preview/src/markdown/linking.ts:119-125`, where
`decodeURIComponent` is wrapped in try/catch, indicating the authors already treat
malformed encodings as realistic.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/components/markdown-renderer.ts
**Line:** 51:57
**Comment:**
*Possible Bug: `decodeURIComponent` can throw on malformed percent-encoding in link titles, which will break markdown rendering for that payload. Use a safe decode helper or wrap decoding in try/catch and fall back to the raw value.
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
+180
to
+199
| loadMoreBtn.querySelector('.dir-warning-text')!.textContent = 'Loading…'; | ||
| (loadMoreBtn as HTMLButtonElement).disabled = true; | ||
| try { | ||
| const result = await options.callTool?.('list_directory', { path: loadPath, depth: 1 }); | ||
| const text = extractToolText(result) ?? ''; | ||
| if (text) { | ||
| const parsed = parseDirectoryEntries(text); | ||
| const html = renderDirTree(parsed.entries, loadPath); | ||
| const parentChildren = loadMoreBtn.closest('.dir-children'); | ||
| if (parentChildren) { | ||
| const temp = document.createElement('div'); | ||
| temp.innerHTML = html; | ||
| const inner = temp.querySelector('.dir-tree'); | ||
| parentChildren.innerHTML = inner ? inner.innerHTML : ''; | ||
| } | ||
| } | ||
| } catch { | ||
| loadMoreBtn.querySelector('.dir-warning-text')!.textContent = 'Failed to load'; | ||
| (loadMoreBtn as HTMLButtonElement).disabled = false; | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: The load-more handler disables the button and sets "Loading…" before the request, but if list_directory returns no text (or undefined) the code never restores the button state. This leaves the warning row permanently disabled without feedback; add a reset path when no usable text is returned. [logic error]
Severity Level: Major ⚠️
- ⚠️ Directory warning rows can become permanently disabled.
- ⚠️ Users cannot retry loading directories after empty responses.
- ⚠️ Confusing "Loading…" state persists despite operation completion.Steps of Reproduction ✅
1. In the file preview UI, trigger a directory listing that produces a warning entry with
a "click to load all" control rendered via `renderDirectoryBody()` in
`src/ui/file-preview/src/directory-controller.ts:134-141` (this constructs
`.dir-load-more` buttons for warning entries in `renderDirTree()` at lines 100-132).
2. Click the "load all" warning row; this is handled by the click listener in
`attachDirectoryHandlers()` at `src/ui/file-preview/src/directory-controller.ts:173-201`.
The handler sets the button label to `Loading…` and disables the button at lines 180-181,
then calls `options.callTool('list_directory', { path: loadPath, depth: 1 })` at line 183.
3. Have the `list_directory` tool (wired in via `options.callTool`) resolve successfully
but return a result that leads `extractToolText(result)` to produce `undefined` or an
empty string, so `text` becomes `''` by the nullish coalescing operator at line 184.
4. Because `if (text)` at line 185 fails, no DOM update occurs and the `catch` block is
not entered; execution falls out of the handler without restoring the warning text or
re-enabling the button, leaving the warning row permanently disabled and stuck showing
"Loading…" with no further user action possible on that row.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/directory-controller.ts
**Line:** 180:199
**Comment:**
*Logic Error: The load-more handler disables the button and sets "Loading…" before the request, but if `list_directory` returns no text (or undefined) the code never restores the button state. This leaves the warning row permanently disabled without feedback; add a reset path when no usable text is returned.
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
+255
to
+271
| if (target.classList.contains('dir-row-file')) { | ||
| target.classList.add('dir-loading'); | ||
| try { | ||
| const result = await options.callTool?.('read_file', { path: fullPath }); | ||
| if (!result || typeof result !== 'object' || result === null) { | ||
| return; | ||
| } | ||
| const structuredContent = (result as { structuredContent?: unknown }).structuredContent; | ||
| if (structuredContent && typeof structuredContent === 'object') { | ||
| const text = extractToolText(result) ?? ''; | ||
| options.onOpenPayload(buildRenderPayload(structuredContent as any, text)); | ||
| } | ||
| } catch { | ||
| target.classList.remove('dir-loading'); | ||
| } | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggestion: The file row is marked with dir-loading, but early returns on unexpected read_file payload shapes exit without clearing that class. This leaves the row visually stuck as loading; ensure dir-loading is removed in all non-navigation paths (e.g., finally block). [state inconsistency]
Severity Level: Major ⚠️
- ⚠️ File entries can appear permanently stuck in loading.
- ⚠️ Users receive no feedback when `read_file` payload malformed.
- ⚠️ Subsequent clicks may be ignored due to lingering loading state.Steps of Reproduction ✅
1. In the file preview UI, click on a file row rendered by `renderDirTree()` in
`src/ui/file-preview/src/directory-controller.ts:100-132`, which gives the row the
`dir-row-file` class and a `data-path` attribute; clicks are handled in
`attachDirectoryHandlers()` at lines 203-271.
2. The click handler detects `target.classList.contains('dir-row-file')` at line 255, adds
the `dir-loading` CSS class at line 256, and calls `options.callTool('read_file', { path:
fullPath })` at line 258.
3. Have the `read_file` tool resolve to a value that is either non-object (e.g., a string
or number) or `null`, or an object without a usable `structuredContent` shape (e.g.,
missing or not an object). In this scenario the guard at lines 259-262 trips and executes
`return;` before any `target.classList.remove('dir-loading')` call.
4. Because the `return` occurs inside the `try` block and the `catch` only runs on thrown
errors, no code path removes `dir-loading` for these "non-error but unusable payload"
cases, leaving the clicked file row visually stuck in a loading state while no navigation
or error indication occurs.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/directory-controller.ts
**Line:** 255:271
**Comment:**
*State Inconsistency: The file row is marked with `dir-loading`, but early returns on unexpected `read_file` payload shapes exit without clearing that class. This leaves the row visually stuck as loading; ensure `dir-loading` is removed in all non-navigation paths (e.g., finally block).
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
+670
to
+671
| if (!workspaceState || workspaceState.saving || !workspaceState.dirty || workspaceState.fileDeleted) { | ||
| return; |
Contributor
There was a problem hiding this comment.
Suggestion: Autosave can silently miss edits when a timer fires during an in-flight save: saveDocument returns immediately while saving is true and does not queue another attempt. If the user types during a slow save, those later edits can remain dirty but unsaved until another blur/manual trigger. Track a pending-save flag (or reschedule autosave in finally) when saving is true and dirty is still true. [race condition]
Severity Level: Critical 🚨
- ❌ Markdown autosave can drop edits during slow saves.
- ⚠️ Risk of lost work when closing preview post-edit.
- ⚠️ Conflict dialog misattributes autosave race as external change.Steps of Reproduction ✅
1. Open a markdown file in the File Preview app so that `renderApp()` in
`src/ui/file-preview/src/app.ts:201-241` is called with a `RenderPayload` whose `fileType
=== 'markdown'`.
2. `renderApp()` detects the markdown file and calls
`markdownController.buildBody(payload)` and then
`markdownController.attachHandlers(payload)` at
`src/ui/file-preview/src/app.ts:233-241,300-302`, which mounts the editor via
`mountMarkdownEditor()` in `attachHandlers()` at
`src/ui/file-preview/src/markdown/controller.ts:911-956`.
3. Type in the markdown editor and pause briefly so that `onChange` in `attachHandlers()`
(`controller.ts:922-950`) marks `state.dirty = true` and calls `scheduleAutosave()`
(`controller.ts:198-206`), which arms a 1s timer that later invokes `saveDocument()`.
4. When the autosave fires, `saveDocument()` starts (`controller.ts:669-877`), sets
`state.saving = true` and performs one or more `await dependencies.callTool('edit_block',
...)` calls (`controller.ts:697-705`); while those async calls are in flight, continue
typing so `onChange` schedules another autosave. When that second timer fires while
`state.saving` is still true, the guard at `controller.ts:670-671` (`if (!workspaceState
|| workspaceState.saving || !workspaceState.dirty || workspaceState.fileDeleted) { return;
}`) causes `saveDocument()` to exit immediately without queuing another save, leaving the
later edits unsaved until a future blur/manual trigger (and `teardown()` in
`src/ui/file-preview/src/app.ts:556-565` does not attempt a final save).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:** 670:671
**Comment:**
*Race Condition: Autosave can silently miss edits when a timer fires during an in-flight save: `saveDocument` returns immediately while `saving` is true and does not queue another attempt. If the user types during a slow save, those later edits can remain dirty but unsaved until another blur/manual trigger. Track a pending-save flag (or reschedule autosave in `finally`) when `saving` is true and `dirty` is still true.
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
+454
to
+455
| const pathPart = selectedLinkItem.path === options.currentFilePath ? '' : selectedLinkItem.wikiPath; | ||
| const wikiLink = `[[${pathPart}${selectedHeadingId ? `#${selectedHeadingId}` : ''}${alias ? `|${alias}` : ''}]]`; |
Contributor
There was a problem hiding this comment.
Suggestion: Creating a file link to the current file with no heading builds an empty wiki target ([[]]) because the path is forced to empty for same-file links. That produces invalid/restored markdown when round-tripping via mcp-wiki titles. Keep the file path for same-file links unless an anchor-only link is explicitly intended. [logic error]
Severity Level: Major ⚠️
- ❌ Same-file links without headings save as empty wiki links.
- ⚠️ Round-tripped markdown renders as broken anchor-only links.
- ⚠️ Users lose intent when linking current note root.Steps of Reproduction ✅
1. Open a markdown file in the desktop app so that it is rendered and then switched into
edit mode via the markdown controller. This flows through `createMarkdownController()` in
`src/ui/file-preview/src/markdown/controller.ts:190` and `attachHandlers()` at
`controller.ts:904-923`, which mounts the editor by calling `mountMarkdownEditor({
currentFilePath: payload.filePath, searchLinks: (query) =>
searchLinkTargets(payload.filePath, query), ... })` at `controller.ts:914-921`.
2. In the markdown editor UI (Tiptap-based surface mounted by `mountMarkdownEditor()` in
`src/ui/file-preview/src/markdown/editor.ts:143-233`), select some text and click the
"Link" formatting button in the context menu. This invokes `handleFormatClick()` (editor
`handleFormatClick` switch with `case 'link': openLinkModalForSelection();` at
approximately `editor.ts:478-506`), which opens the `#markdown-link-modal` link dialog in
"URL" mode.
3. Switch the link dialog to "File" mode (`updateLinkMode('file')` in `editor.ts:338-347`)
and search for a term that returns the current file in results. The search uses
`searchLinkTargets(filePath, query)` in
`src/ui/file-preview/src/markdown/controller.ts:359-392`, which maps each result into a
`MarkdownLinkSearchItem` with `path` set to the full file path and `wikiPath` derived from
the relative path. Because there is no filter to exclude the current file, the current
markdown file will appear with `selectedLinkItem.path === payload.filePath` when chosen.
4. With the current file selected in the results, leave the "Heading" select at its
default "None" and the "Alias" input empty, then click "Insert". In `handleLinkApply()`'s
file-branch in `src/ui/file-preview/src/markdown/editor.ts:430-46`, `selectedHeadingId`
and `alias` are both empty and `pathPart` is computed as `''` because
`selectedLinkItem.path === options.currentFilePath`. This builds `wikiLink = '[[]]'` at
`editor.ts:454-455`. The link mark is stored as a markdown link with `title: "mcp-wiki:" +
encodeURIComponent(wikiLink)` and a non-empty `href`. When the document is later
serialized via `getTiptapMarkdown()` (which calls `restoreWikiLinks()` from
`src/ui/file-preview/src/markdown/linking.ts:208-215`), the `mcp-wiki` title is decoded
back to the raw wiki text, leaving a literal `[[]]` in the saved markdown. On the next
render, `rewriteWikiLinks()` (`linking.ts:95-105, 218-241`) parses `[[]]` into a link with
empty path and anchor, producing an effectively broken, empty markdown link instead of a
same-file wiki link like `[[current-file]]`.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:** 454:455
**Comment:**
*Logic Error: Creating a file link to the current file with no heading builds an empty wiki target (`[[]]`) because the path is forced to empty for same-file links. That produces invalid/restored markdown when round-tripping via `mcp-wiki` titles. Keep the file path for same-file links unless an anchor-only link is explicitly intended.
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 · |
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 diagram shows how markdown edits flow from tools into the new file preview workspace, how saves are applied via edit_block/write_file, and how the conflict dialog resolves cases where the file changed on disk while the user was editing. sequenceDiagram
participant ToolServer
participant FileSystem
participant FilePreview
participant MarkdownEditor
participant User
ToolServer->>FileSystem: Apply write_file or edit_block on markdown file
FileSystem-->>ToolServer: Structured result with preview snippet
ToolServer-->>FilePreview: Tool result with structuredContent
FilePreview->>MarkdownEditor: Open markdown workspace and load content
User->>MarkdownEditor: Edit markdown and trigger save
MarkdownEditor->>FileSystem: Save changes via edit_block with computed blocks
alt No disk conflict
FileSystem-->>MarkdownEditor: Updated content preview
MarkdownEditor-->>FilePreview: Refresh preview and show saved status
else File changed on disk
FileSystem-->>MarkdownEditor: Conflict error and fresh file content
MarkdownEditor->>User: Show conflict dialog with disk vs my changes options
User->>MarkdownEditor: Choose use disk version or save my changes
end
Generated by CodeAnt AI |
Comment on lines
+87
to
+93
| const incomingContent = stripReadStatusLine(payload.content); | ||
| const overriddenContent = stripReadStatusLine(localPayloadOverride.content); | ||
| if (incomingContent === overriddenContent) { | ||
| return payload; | ||
| } | ||
|
|
||
| return { hint: hintLines.join('\n').trim(), entries: root }; | ||
| return localPayloadOverride; |
Contributor
There was a problem hiding this comment.
Suggestion: The local payload override is never cleared when an incoming payload matches it, so a stale override can later keep winning over newer host updates for the same file path. Clear localPayloadOverride once host and override content are equal, otherwise future legitimate disk/host updates can be masked by outdated cached content. [logic error]
Severity Level: Major ⚠️
- ⚠️ Markdown preview may ignore later disk updates.
- ⚠️ Users can unknowingly overwrite newer external file changes.
- ⚠️ Cross-tab/widget-state recovery can restore outdated markdown content.Steps of Reproduction ✅
1. Open the File Preview app so `bootstrapApp()` runs
(`src/ui/file-preview/src/app.ts:367-647`) and load a markdown file via the host, which
delivers an initial `RenderPayload` through `app.ontoolresult` (`app.ts:506-536`).
`getEffectiveIncomingPayload()` (`app.ts:77-94`) sees no `localPayloadOverride` initially
and returns the host `payload`.
2. Edit and save the markdown file inside the built-in editor so `saveDocument()` runs in
the markdown controller (`src/ui/file-preview/src/markdown/controller.ts:669-877`). On
successful save, it builds a new `RenderPayload` with updated `content` and calls
`dependencies.storePayloadOverride(...)` (`controller.ts:741-747`), which is wired to
`storePayloadOverride()` in `app.ts:71-75`, setting `localPayloadOverride` to the saved
payload and persisting it.
3. Modify the same file *outside* this preview (e.g., external editor or other tool) and
then re-run the host's `read_file` for the same path so the File Preview app receives a
new tool result for that file. The host delivers a fresh `RenderPayload` via
`app.ontoolresult` (`app.ts:506-536`), which passes it into
`getEffectiveIncomingPayload()` (`app.ts:77-94`).
4. In `getEffectiveIncomingPayload()`, the file path matches (`app.ts:82-85`) and
`localPayloadOverride` is still set from step 2. If the external change made the file
content differ from the last saved override, `incomingContent !== overriddenContent`
(`app.ts:87-93`), so the function returns `localPayloadOverride` instead of the new host
payload. Because the equality branch (`incomingContent === overriddenContent`) never
clears `localPayloadOverride`, the stale override continues to win over all future host
updates for that file path, so legitimate disk/host changes are masked and the UI keeps
showing the old content.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/app.ts
**Line:** 87:93
**Comment:**
*Logic Error: The local payload override is never cleared when an incoming payload matches it, so a stale override can later keep winning over newer host updates for the same file path. Clear `localPayloadOverride` once host and override content are equal, otherwise future legitimate disk/host updates can be masked by outdated cached content.
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
+21
to
+23
| function encodeLinkPath(pathValue: string): string { | ||
| return encodeURI(normalizePathSeparators(pathValue)); | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: Using encodeURI here leaves characters like ) unescaped, and those characters can prematurely terminate markdown link destinations when wiki links are rewritten, producing malformed links for filenames such as notes).md. Encode markdown-destination-sensitive characters or wrap destinations in angle brackets when constructing markdown links. [possible bug]
Severity Level: Critical 🚨
- ❌ Wiki links with ')' corrupt on editor roundtrip.
- ⚠️ Markdown editor loses original wiki link targets.Steps of Reproduction ✅
1. Author a markdown file containing a wiki-style link with a closing parenthesis in the
target, e.g. `[[notes).md]]`, and open it in the markdown preview/editor. The preview
pipeline calls `prepareMarkdownSource(source)` in
`src/ui/file-preview/src/markdown/parser.ts:54-57`, which delegates to
`rewriteWikiLinks(source)` in `linking.ts:218-241`.
2. `rewriteWikiLinks()` scans each line and, for non-code-fence lines, calls
`replaceWikiLinksOutsideInlineCode()` (lines 108-138), which in turn uses
`rewriteWikiLinksInPlainText()` (lines 95-106). That function parses the `[[notes).md]]`
token via `parseWikiLink()` (lines 33-44) and then constructs a markdown link using
`buildWikiHref()` (lines 70-93) and `encodeLinkPath()` (lines 21-23).
3. Because `encodeLinkPath()` uses `encodeURI(normalizePathSeparators(pathValue))`, it
does *not* escape the `)` character in the path, so the href becomes something like
`./notes).md`. The final rewritten markdown therefore contains `[notes).md](./notes).md
"mcp-wiki:%5B%5Bnotes).md%5D%5D")`, with an unescaped `)` inside the link destination.
4. When the edited content is later serialized back to markdown, `restoreWikiLinks()` in
`linking.ts:208-215` is used (as documented in its comment) to invert this transformation.
Its regex `/\[([^\]]*)\]\(([^)\s]*)(?:\s+"mcp-wiki:([^"]+)")\)/g` stops the `href` group
at the internal `)` in `./notes).md`, leaving the `encoded` group undefined. The
replacement callback then calls `decodeURIComponent(encoded)` which throws, and the
`catch` block returns `[[${encoded}]]`, i.e. `[[undefined]]`. As a result, wiki links
whose paths contain `)` are corrupted on round-trip through the editor; tightening
`encodeLinkPath` to escape `)` (or wrapping destinations in angle brackets) would prevent
this by ensuring the destination string never contains a raw closing parenthesis.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/linking.ts
**Line:** 21:23
**Comment:**
*Possible Bug: Using `encodeURI` here leaves characters like `)` unescaped, and those characters can prematurely terminate markdown link destinations when wiki links are rewritten, producing malformed links for filenames such as `notes).md`. Encode markdown-destination-sensitive characters or wrap destinations in angle brackets when constructing markdown links.
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
+184
to
+186
| function isExternalHref(rawHref: string): boolean { | ||
| return /^[A-Za-z][A-Za-z0-9+.-]*:/.test(rawHref) && !isWindowsAbsolutePath(rawHref); | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: Protocol-relative URLs (like //example.com/docs) are not recognized as external links and are treated as local file paths, causing the navigator to attempt opening /example.com/docs on disk. Extend external-link detection to handle protocol-relative URLs explicitly. [logic error]
Severity Level: Major ⚠️
- ❌ Protocol-relative links open as local filesystem paths.
- ⚠️ Markdown preview misclassifies certain web URLs as files.Steps of Reproduction ✅
1. Open a local markdown file via the `read_file` tool; the handler `handleReadFile()` in
`src/handlers/filesystem-handlers.ts:92-131` returns a `ServerResult` whose
`structuredContent.filePath` is an absolute filesystem path (e.g.
`/home/user/docs/start.md`).
2. The file-preview app constructs a `RenderPayload` (see
`src/ui/file-preview/src/model.ts:5-12`) and passes it into the markdown controller. When
the user clicks a link like `[Docs](//example.com/docs)` in the rendered preview,
`navigateLink()` in `src/ui/file-preview/src/markdown/controller.ts:11-21` is invoked with
`payload.filePath` as `currentPath` and the clicked `href` as `"//example.com/docs"`.
3. `navigateLink()` calls `resolveMarkdownLink(payload.filePath, href)` at
`controller.ts:20`. Inside `resolveMarkdownLink()`
(`src/ui/file-preview/src/markdown/linking.ts:243-287`), the `isExternalHref(rawHref)`
check at lines 184-186 returns `false` for `"//example.com/docs"` because it does not
match the `^[A-Za-z][A-Za-z0-9+.-]*:` scheme regex.
4. Since the URL is not treated as external, execution falls through to the file-link
branch (lines 280-286): `splitHref(rawHref)` yields `pathPart = "//example.com/docs"`,
`resolveFileTargetPath(currentPath, pathPart)` normalizes this to the local path
`"/example.com/docs"`, and `navigateLink()` then attempts
`dependencies.openExternalLink?.(resolvedLink.targetPath)` or
`readPayload(resolvedLink.targetPath)` at `controller.ts:40-47`. This causes the app to
try opening `/example.com/docs` as a local file, so protocol-relative links fail to
navigate to the intended web resource.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/linking.ts
**Line:** 184:186
**Comment:**
*Logic Error: Protocol-relative URLs (like `//example.com/docs`) are not recognized as external links and are treated as local file paths, causing the navigator to attempt opening `/example.com/docs` on disk. Extend external-link detection to handle protocol-relative URLs explicitly.
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 · |
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 refactors the file preview widget into a markdown workspace where users can edit notes in-place, autosave via edit_block, and resolve disk-change conflicts through a modal that either reapplies their edits or restores the on-disk version. sequenceDiagram
participant User
participant MarkdownEditor
participant Server
participant ConflictDialog
User->>MarkdownEditor: Edit markdown in preview panel
MarkdownEditor->>Server: Save changes via edit_block
alt Save succeeds
Server-->>MarkdownEditor: Success + updated preview metadata
MarkdownEditor->>MarkdownEditor: Update baseline and autosave status
else File changed on disk
Server-->>MarkdownEditor: Save failed (text changed on disk)
MarkdownEditor->>ConflictDialog: Load latest file and show disk vs my changes
User->>ConflictDialog: Choose keep edits or use disk version
ConflictDialog->>MarkdownEditor: Retry save or reset draft
end
Generated by CodeAnt AI |
Comment on lines
+11
to
+15
| if (options.isMarkdownEditing) { | ||
| if (options.previousAbortController) { | ||
| options.previousAbortController.abort(); | ||
| } | ||
| return null; |
Contributor
There was a problem hiding this comment.
Suggestion: When markdown editing mode is active, the function aborts listeners and returns without clearing any previously published selection context, leaving stale selected text in host model context. Clear context before returning so entering markdown edit mode does not keep outdated selection data. [logic error]
Severity Level: Major ⚠️
- ⚠️ Markdown previews keep context from previously selected different file.
- ⚠️ AI suggestions during markdown editing can reference wrong file.
- ⚠️ Analytics about selection use stale, cross-file data.Steps of Reproduction ✅
1. In the file preview UI, the host renders a non-markdown file by calling
`renderApp(container, payloadA, 'rendered', false)` in
`src/ui/file-preview/src/app.ts:201-207` where `payloadA.fileType !== 'markdown'`;
`attachSelectionContext` is called at `app.ts:105-112` with `isMarkdownEditing: false`, so
the selection listener in `selection-context.ts:88-128` is attached and
`options.updateContext` is `rpcUpdateContext` (`app.ts:464-471`).
2. The user selects some text inside the non-markdown file body within
`.panel-content-wrapper` (`document-layout.ts:18-23`), triggering the `selectionchange`
handler in `selection-context.ts:88-121`, which sets `lastSelectedText`, builds the
`context` string for `payloadA.filePath` at `selection-context.ts:117-120`, and calls
`options.updateContext(context)` at `selection-context.ts:121`, causing `rpcUpdateContext`
(`app.ts:464-471`) to push that selection into the host model context.
3. Without clearing the selection context in the host, the user switches to a markdown
file (e.g., opens `README.md`), so the host calls `renderApp(container, payloadB,
'rendered', false)` with `payloadB.fileType === 'markdown'`; `renderApp` creates a
markdown workspace via `markdownController.getState(payloadB)` at `app.ts:41-42`, passes
it into `buildDocumentLayout` at `app.ts:64-79`, and then calls `attachSelectionContext`
at `app.ts:105-112` with `isMarkdownEditing: payloadB.fileType === 'markdown' &&
!!markdownWorkspace` (true for markdown).
4. Inside `attachSelectionContext` at `selection-context.ts:11-15`, the
`isMarkdownEditing` branch executes: it aborts the previous `selectionAbortController` if
present (removing the old `selectionchange` listener) but immediately returns `null`
without calling `options.updateContext?.('')`, so no new listener is installed for the
markdown view and the host model context keeps the stale selection text from `payloadA`
while the user is now editing or viewing the markdown file `payloadB`, with no mechanism
in this path to clear or update that selection-based context.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/host/selection-context.ts
**Line:** 11:15
**Comment:**
*Logic Error: When markdown editing mode is active, the function aborts listeners and returns without clearing any previously published selection context, leaving stale selected text in host model context. Clear context before returning so entering markdown edit mode does not keep outdated selection data.
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
+164
to
+171
| const oldBlock = oldLines.slice(contextBefore, contextAfter).join('\n'); | ||
| const newBlock = [ | ||
| ...oldLines.slice(contextBefore, hunk.oldStart), | ||
| ...newLines.slice(hunk.newStart, hunk.newEnd), | ||
| ...oldLines.slice(hunk.oldEnd, contextAfter), | ||
| ].join('\n'); | ||
|
|
||
| return { old_string: oldBlock, new_string: newBlock }; |
Contributor
There was a problem hiding this comment.
Suggestion: computeEditBlocks can generate edit blocks with an empty old_string (for example when inserting into an empty file or pure insert hunks), but the backend edit_block tool explicitly rejects empty search strings. This makes valid saves fail with "Empty search strings are not allowed." Ensure insertion-only cases use a non-empty anchor/context or fall back to a full-file write path. [logic error]
Severity Level: Major ⚠️
- ❌ Empty markdown file edits cannot be saved.
- ⚠️ New markdown documents fail on first save.
- ⚠️ Users see confusing "Empty search strings" error.Steps of Reproduction ✅
1. Open an empty Markdown file in the File Preview app so that `app.ts` initializes
`markdownController` at `src/ui/file-preview/src/app.ts:116-144` and
`readAndResolvePayload()` at `app.ts:164-180` delivers a `RenderPayload` whose `content`
(after `stripReadStatusLine`) is an empty string.
2. `markdownController.getState()` at
`src/ui/file-preview/src/markdown/controller.ts:278-305` is called, setting
`state.sourceContent`, `state.fullDocumentContent`, and `state.draftContent` to `''` for
that file, and building an initial outline from the empty content.
3. In the inline editor, type some Markdown text so that `attachHandlers()` at
`controller.ts:903-955` wires `mountMarkdownEditor`'s `onChange` callback, which sets
`state.draftContent` to the new non-empty text and `state.dirty = value !==
state.fullDocumentContent`. On blur, `onBlur` calls `saveDocument()` (see
`controller.ts:949-955` and `saveDocument` starting at `controller.ts:670`).
4. Inside `saveDocument()` at `controller.ts:681`, the code invokes
`computeEditBlocks(state.fullDocumentContent, state.draftContent)`. For this scenario
`oldText === ''` and `newText` is non-empty. `computeEditBlocks` (defined at
`controller.ts:141-173`) computes `oldLines = ['']`, detects that `totalChanged >
oldLines.length * 0.7`, and returns a single full-file block `{ old_string: oldText,
new_string: newText }`, i.e. `{ old_string: '', new_string: <your text> }`, via the branch
at lines `153-158`.
5. Still in `saveDocument()`, the loop at `controller.ts:698-713` sends each block to the
backend via `dependencies.callTool?.('edit_block', { file_path: state.filePath,
old_string: block.old_string, new_string: block.new_string, expected_replacements: 1 })`.
On the server side, `performSearchReplace()` in `src/tools/edit.ts:116-140` maps this
`old_string` to `block.search` and hits the guard at lines `129-139`, which returns a
`ServerResult` whose `content[0].text` is `"Empty search strings are not allowed. Please
provide a non-empty string to search for."` because `block.search === ""`.
6. Back in the frontend, `assertSuccessfulEditBlockResult()` at
`src/ui/file-preview/src/payload-utils.ts:13-35` inspects the result: `result.isError` is
not `true`, but `result.structuredContent` is missing, so it throws an `Error` using the
backend message. This is caught by the `catch` block in `saveDocument()` at
`controller.ts:768-876`. For an empty file, `readCompletePayload(state.filePath)` returns
the same empty content, so `reloadedFromDisk` stays `false` and none of the partial/total
conflict branches apply. The function falls through to the generic error path at
`controller.ts:859-867`, sets `state.error` to the "Empty search strings..." message,
flashes "Save failed", and leaves the file on disk unchanged — meaning valid
insertion-only edits into an empty Markdown file cannot be saved through this path.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:** 164:171
**Comment:**
*Logic Error: `computeEditBlocks` can generate edit blocks with an empty `old_string` (for example when inserting into an empty file or pure insert hunks), but the backend `edit_block` tool explicitly rejects empty search strings. This makes valid saves fail with "Empty search strings are not allowed." Ensure insertion-only cases use a non-empty anchor/context or fall back to a full-file write path.
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
+95
to
+105
| function rewriteWikiLinksInPlainText(segment: string): string { | ||
| return segment.replace(WIKI_LINK_PATTERN, (match) => { | ||
| const parsed = parseWikiLink(match); | ||
| if (!parsed) { | ||
| return match; | ||
| } | ||
|
|
||
| const displayText = buildWikiDisplayText(parsed); | ||
| const href = buildWikiHref(parsed); | ||
| return `[${displayText}](${href} "mcp-wiki:${encodeURIComponent(match)}")`; | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggestion: Wiki-link rewriting is done with a plain regex on non-code text, so [[...]] inside existing markdown link destinations/titles can be rewritten too, producing invalid nested markdown links and corrupting content. Restrict rewriting to actual text nodes (or parse markdown tokens first) instead of raw line-level regex replacement. [logic error]
Severity Level: Major ⚠️
- ⚠️ Markdown preview mis-renders links with inline wiki syntax.
- ⚠️ WYSIWYG editor round-trip corrupts such mixed link content.Steps of Reproduction ✅
1. Open a markdown file in the preview/editor whose content includes a markdown link whose
*label* contains a wiki link, e.g. the line `See [RFC
[[123]]](https://example.com/rfc-123) for details.` in a file with `fileType: 'markdown'`
(as in the payload shape from `src/types.ts:16–22`).
2. For preview rendering, the markdown source is passed through `prepareMarkdownSource()`
in `src/ui/file-preview/src/markdown/parser.ts:14–16`, which unconditionally calls
`rewriteWikiLinks(source)`.
3. `rewriteWikiLinks()` in `src/ui/file-preview/src/markdown/linking.ts:218–241` splits
the text into lines, skips fenced code blocks, and for non-fenced lines calls
`replaceWikiLinksOutsideInlineCode()` at lines 108–137, which in turn calls
`rewriteWikiLinksInPlainText()` (lines 95–105) on all non–inline-code spans.
4. On the example line, the substring `[[123]]` inside the markdown link label is *not*
inside backticks and is therefore processed by `rewriteWikiLinksInPlainText()`, which
matches `WIKI_LINK_PATTERN` (line 18) and replaces `[[123]]` with another markdown link of
the form `[123](./123.md "mcp-wiki:...")`. The overall line becomes syntactically nested
links (`[RFC [123](./123.md ...)](https://...)`), which is invalid/ambiguous markdown and
can corrupt how the link text and href render in both the preview and the WYSIWYG editor.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/linking.ts
**Line:** 95:105
**Comment:**
*Logic Error: Wiki-link rewriting is done with a plain regex on non-code text, so `[[...]]` inside existing markdown link destinations/titles can be rewritten too, producing invalid nested markdown links and corrupting content. Restrict rewriting to actual text nodes (or parse markdown tokens first) instead of raw line-level regex replacement.
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
| ) { | ||
| isExpanded = true; | ||
| chrome.expanded = true; | ||
| const restorePayload = inlinePayloadBeforeFullscreen ?? hostPayload; |
Contributor
There was a problem hiding this comment.
Suggestion: The fullscreen-exit restore path falls back to hostPayload, but hostPayload is only initialized during initial state resolution and is not kept in sync with later payload changes. If fullscreen is entered before inlinePayloadBeforeFullscreen is set (or it is cleared), exiting fullscreen can restore an outdated file/content snapshot and overwrite the current view state. Keep hostPayload updated whenever new tool payloads are rendered, or avoid this stale fallback. [logic error]
Severity Level: Major ⚠️
- ❌ Exiting fullscreen can reopen a different markdown file.
- ⚠️ Persisted widget state may point to outdated payload.Steps of Reproduction ✅
1. Start the File Preview UI by loading `src/ui/file-preview/src/main.ts:1-6`, which calls
`bootstrapApp()` in `src/ui/file-preview/src/app.ts:367-373`, creating the `App` instance
and wiring `app.ontoolresult`, `connectWithSharedHostContext`, and the markdown
controller.
2. Let the host send the first `read_file` result for markdown file A: `app.ontoolresult`
at `src/ui/file-preview/src/app.ts:506-536` calls `resolveInitialState` at `439-457`,
which sets `hostPayload = payload` (file A) at line 445 and renders it via
`renderAndSync`, establishing `currentPayload` for A.
3. While the host display mode remains `'fullscreen'` (so there has been no `inline` →
`fullscreen` transition for this session), open a different markdown file B via directory
navigation: `attachDirectoryHandlers` in `renderApp` at
`src/ui/file-preview/src/app.ts:313-323` calls tools that eventually produce another tool
result; `app.ontoolresult` (lines 506-531) now updates `currentPayload` to file B via
`renderAndSync`, but `hostPayload` remains pointing to the original file A because it is
only assigned inside `resolveInitialState`.
4. From this state, have the host switch display mode from `'fullscreen'` to `'inline'`
(e.g., via host UI or `requestDisplayMode`): `connectWithSharedHostContext` in
`src/ui/shared/host-context.ts:60-73` invokes `onContextApplied` in
`src/ui/file-preview/src/app.ts:577-621`. The exit branch at `586-611` sees
`previousDisplayMode === 'fullscreen'` and `nextDisplayMode === 'inline'` and
`currentPayload` for file B, but `inlinePayloadBeforeFullscreen` is still `undefined`, so
at line 593 `const restorePayload = inlinePayloadBeforeFullscreen ?? hostPayload;`
resolves to the stale `hostPayload` for file A. For partial reads, lines 594-603 set
`currentPayload`, `localPayloadOverride`, and `widgetState` back to file A and pass it to
`markdownController.handleInlineExitFromFullscreen`, causing the inline view to reopen the
wrong file/content snapshot instead of the current fullscreen document B.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/app.ts
**Line:** 593:593
**Comment:**
*Logic Error: The fullscreen-exit restore path falls back to `hostPayload`, but `hostPayload` is only initialized during initial state resolution and is not kept in sync with later payload changes. If fullscreen is entered before `inlinePayloadBeforeFullscreen` is set (or it is cleared), exiting fullscreen can restore an outdated file/content snapshot and overwrite the current view state. Keep `hostPayload` updated whenever new tool payloads are rendered, or avoid this stale fallback.
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 · |
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 diagram shows how edit_block and write_file results now drive the file preview UI: tool responses include preview metadata, the file preview app reloads the actual file content, and the built-in markdown editor opens a persistent workspace for further edits and saves. sequenceDiagram
participant User
participant Host
participant FileTools
participant FilePreviewApp
participant MarkdownEditor
User->>Host: Request markdown edit (edit_block or write_file)
Host->>FileTools: Run edit_block / write_file on target file
FileTools-->>Host: Return success text + structuredContent for file
Host-->>FilePreviewApp: Deliver tool result to preview widget
FilePreviewApp->>FileTools: Read file content if tool result lacks read header
FileTools-->>FilePreviewApp: Return markdown file content
FilePreviewApp->>MarkdownEditor: Open markdown workspace with outline and full document
User->>MarkdownEditor: Edit markdown and trigger save
MarkdownEditor->>FileTools: Apply incremental edits via edit_block
FileTools-->>MarkdownEditor: Confirm save and update preview-backed content
Generated by CodeAnt AI |
Comment on lines
+13
to
+21
| return normalizedHtml | ||
| .replace(/ /g, ' ') | ||
| .replace(/&/g, '&') | ||
| .replace(/</g, '<') | ||
| .replace(/>/g, '>') | ||
| .replace(/'/g, "'") | ||
| .replace(/"/g, '"') | ||
| .replace(/\n{3,}/g, '\n\n') | ||
| .trim(); |
Contributor
There was a problem hiding this comment.
Suggestion: Entity decoding order causes double-decoding: replacing & before </> turns literal text like &lt; into < instead of <. This changes copied content semantics and can corrupt markdown/code examples. Decode entities in a way that preserves single-pass behavior. [logic error]
Severity Level: Major ⚠️
- ⚠️ Copied rendered markdown corrupts literal HTML entity examples.
- ⚠️ Code snippets containing `&lt;` paste as real `<` tags.
- ⚠️ Documentation copied from preview may differ from source.Steps of Reproduction ✅
1. Open a markdown document through the markdown workspace (using
`createMarkdownController()` in `src/ui/file-preview/src/markdown/controller.ts:190-237`)
and switch the editor view to rendered markdown (`state.editorView === 'markdown'` is the
non-raw mode used in `getCopyText()` at `controller.ts:1018-1021`).
2. Ensure the markdown content includes literal HTML entity text such as
`&lt;div&gt;` that you want to preserve as `<div>` when copying (for
example, a code snippet describing how to write `<div>` in HTML).
3. Click the "copy active markdown" button, whose handler in
`src/ui/file-preview/src/panel-actions.ts:9-21` calls
`options.markdownController.getCopyText(options.payload)`. Because the view is not `raw`,
`getCopyText()` returns `getRenderedMarkdownCopyText(source)` at
`controller.ts:1018-1023`.
4. Inside `getRenderedMarkdownCopyText()` in
`src/ui/file-preview/src/markdown/preview.ts:3-21`, the markdown is first rendered to HTML
via `renderMarkdown(content)` (`preview.ts:3-5`), then `normalizedHtml` is processed:
`.replace(/&/g, '&')` runs before `.replace(/</g, '<')` and `.replace(/>/g,
'>')`. This causes the sequence `&lt;` to become `<` after the `&` replacement,
and then `<` after the `<` replacement, so the copied text becomes `<div>` instead of
the intended literal `<div>`, altering the semantics of code/examples in the
clipboard.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/preview.ts
**Line:** 13:21
**Comment:**
*Logic Error: Entity decoding order causes double-decoding: replacing `&` before `<`/`>` turns literal text like `&lt;` into `<` instead of `<`. This changes copied content semantics and can corrupt markdown/code examples. Decode entities in a way that preserves single-pass behavior.
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
+77
to
94
| function getEffectiveIncomingPayload(payload: RenderPayload): RenderPayload { | ||
| if (!localPayloadOverride) { | ||
| return payload; | ||
| } | ||
|
|
||
| // Build flat list | ||
| const flat: { name: string; fullPath: string; isDir: boolean; isDenied: boolean; isWarning: boolean; warningText: string; depth: number }[] = []; | ||
| for (const line of entryLines) { | ||
| if (line.startsWith('[WARNING]')) { | ||
| // Format: [WARNING] dirName: N items hidden (showing first M of T total) | ||
| const warnBody = line.replace(/^\[WARNING\]\s*/, ''); | ||
| const colonIdx = warnBody.indexOf(':'); | ||
| const dirName = colonIdx >= 0 ? warnBody.slice(0, colonIdx).trim() : ''; | ||
| const msg = colonIdx >= 0 ? warnBody.slice(colonIdx + 1).trim() : warnBody; | ||
| // Depth matches the directory it belongs to — infer from dirName path segments | ||
| const parts = dirName.replace(/\\/g, '/').split('/').filter(Boolean); | ||
| const depth = parts.length; // warning sits inside the dir, so same depth as children | ||
| flat.push({ name: dirName, fullPath: dirName, isDir: false, isDenied: false, isWarning: true, warningText: msg, depth }); | ||
| continue; | ||
| } | ||
| const isDir = line.startsWith('[DIR]'); | ||
| const isDenied = line.startsWith('[DENIED]'); | ||
| const name = line.replace(/^\[(DIR|FILE|DENIED)\]\s*/, ''); | ||
| const parts = name.replace(/\\/g, '/').split('/'); | ||
| flat.push({ name, fullPath: name, isDir, isDenied, isWarning: false, warningText: '', depth: parts.length - 1 }); | ||
| if (localPayloadOverride.filePath !== payload.filePath) { | ||
| localPayloadOverride = undefined; | ||
| return payload; | ||
| } | ||
|
|
||
| // Build tree from flat list | ||
| const root: DirEntry[] = []; | ||
| const stack: DirEntry[][] = [root]; | ||
|
|
||
| for (const item of flat) { | ||
| const baseName = item.fullPath.replace(/\\/g, '/').split('/').pop() ?? item.fullPath; | ||
| const entry: DirEntry = { name: baseName, isDir: item.isDir, isDenied: item.isDenied, isWarning: item.isWarning, warningText: item.warningText, depth: item.depth, children: [], relativePath: item.fullPath }; | ||
|
|
||
| // Adjust stack to match depth | ||
| while (stack.length > item.depth + 1) stack.pop(); | ||
|
|
||
| const parent = stack[stack.length - 1]; | ||
| parent.push(entry); | ||
|
|
||
| if (item.isDir) { | ||
| stack.push(entry.children); | ||
| } | ||
| const incomingContent = stripReadStatusLine(payload.content); | ||
| const overriddenContent = stripReadStatusLine(localPayloadOverride.content); | ||
| if (incomingContent === overriddenContent) { | ||
| return payload; | ||
| } | ||
|
|
||
| return { hint: hintLines.join('\n').trim(), entries: root }; | ||
| return localPayloadOverride; | ||
| } |
Contributor
There was a problem hiding this comment.
🟠 Architect Review — HIGH
Incoming payloads for a given file are always overridden by localPayloadOverride whenever content differs, regardless of whether there is an active dirty draft. After localPayloadOverride is set (e.g., after a successful markdown save or fullscreen/inline restore), later tool-driven updates for the same file (including edit_block/write_file flows that re-read from disk) can be discarded and the preview remains on the older override snapshot instead of the new disk content.
Suggestion: Restrict localPayloadOverride to situations where there is an actual draft to protect (or track a version/timestamp), and clear or replace it when a newer disk/host payload is known to be authoritative. When there is no dirty draft, accept newer host/disk payloads instead of forcing the override.
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/app.ts
**Line:** 77:94
**Comment:**
*HIGH: Incoming payloads for a given file are always overridden by localPayloadOverride whenever content differs, regardless of whether there is an active dirty draft. After localPayloadOverride is set (e.g., after a successful markdown save or fullscreen/inline restore), later tool-driven updates for the same file (including edit_block/write_file flows that re-read from disk) can be discarded and the preview remains on the older override snapshot instead of the new disk content.
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
| if (/^https?:\/\//i.test(payload.filePath)) { | ||
| return false; | ||
| } | ||
| return !parseReadRange(payload.content); |
Contributor
There was a problem hiding this comment.
Suggestion: This logic assumes parseReadRange succeeds for all real file payloads, but read_file also emits valid status lines without (total: ... lines) (for unknown totals). Those payloads are misclassified as "needs read", causing an unnecessary second disk read for many files and avoidable latency. [performance]
Severity Level: Major ⚠️
- ⚠️ Text file previews incur redundant disk reads for some.
- ⚠️ Large files experience slower initial preview rendering latency.Steps of Reproduction ✅
1. A client calls the `read_file` tool for a large text file. On the server,
`handleReadFile()` in `src/handlers/filesystem-handlers.ts:72-108` invokes
`readFile(parsed.path, options)` which routes to `readFileFromDisk()` in
`src/tools/filesystem.ts:133-224`.
2. `readFileFromDisk()` (`src/tools/filesystem.ts:207-219`) calls the text handler's
`read()` method (`src/utils/files/text.ts:4-11`), which delegates to
`readFileWithSmartPositioning()` (`src/utils/files/text.ts:157-191`) with
`includeStatusMessage: true`. The status line is built by
`generateEnhancedStatusMessage()` at `src/utils/files/text.ts:88-117`; when `totalLines`
is `undefined` (e.g., very large files where `getFileLineCount()` at lines 72-82 declines
to count), it emits headers like `[Reading 100 lines from start]` without a `(total: ...
lines)` segment.
3. The File Preview app receives this result as a `RenderPayload` via
`extractRenderPayload()` (`src/ui/file-preview/src/payload-utils.ts:58-69`) in
`app.ontoolresult` (`src/ui/file-preview/src/app.ts:506-517`). The helper
`needsContentRead()` at `src/ui/file-preview/src/app.ts:154-161` calls
`parseReadRange(payload.content)` implemented in
`src/ui/file-preview/src/document-workspace.ts:14-31`, whose regex on line 15 requires a
`(total: (\d+) lines` fragment and thus returns `undefined` for headers without `total:`.
4. Because `parseReadRange()` returns `undefined`, `needsContentRead()` returns `true` at
`src/ui/file-preview/src/app.ts:161`, causing `app.ontoolresult` (lines 510-517) to invoke
`readAndResolvePayload()` (`src/ui/file-preview/src/app.ts:164-181`), which calls
`markdownController.readPayload(payload.filePath)`
(`src/ui/file-preview/src/markdown/controller.ts:257-258`). This triggers a second
`read_file` disk read for the same path even though the original payload already contained
the requested content, adding avoidable latency especially for large files whose total
line count is unknown.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/app.ts
**Line:** 161:161
**Comment:**
*Performance: This logic assumes `parseReadRange` succeeds for all real file payloads, but `read_file` also emits valid status lines without `(total: ... lines)` (for unknown totals). Those payloads are misclassified as "needs read", causing an unnecessary second disk read for many files and avoidable latency.
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
+31
to
+35
| function shouldIgnoreBlur(shell: Element | null | undefined, event: FocusEvent): boolean { | ||
| const nextTarget = event.relatedTarget as Node | null; | ||
| const widgetShell = shell?.closest('.tool-shell'); | ||
| return Boolean(nextTarget && (shell?.contains(nextTarget) || widgetShell?.contains(nextTarget))); | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: Blur is ignored for any focus move within the entire tool shell, not just editor-owned UI, so clicking navigation/topbar controls can skip the immediate save path and leave changes only in the debounce timer. If a rerender/navigation happens before the timer fires, pending edits are dropped; limit blur suppression to true editor-internal controls (context menu/modal) so actionable navigation triggers save. [incorrect condition logic]
Severity Level: Critical 🚨
- ❌ Unsaved markdown edits can be lost on navigation.
- ⚠️ Save-on-blur contract violated for display-mode buttons.
- ⚠️ Users may distrust markdown editor persistence guarantees.Steps of Reproduction ✅
1. Open a markdown file so `renderApp()` in `src/ui/file-preview/src/app.ts:201-365`
renders with `payload.fileType === 'markdown'`; this constructs a markdown workspace and
calls `markdownController.attachHandlers(payload)` at `app.ts:301`, which in turn mounts
the editor at `src/ui/file-preview/src/markdown/controller.ts:911-955` using
`mountMarkdownEditor()` with an `onBlur` callback that cancels autosave and calls
`saveDocument()` (controller.ts:951-954).
2. In `mountMarkdownEditor()`'s markdown-branch
(`src/ui/file-preview/src/markdown/editor.ts:178-677`), the TipTap `Editor` is created
with an `onBlur` handler at lines 224-232 that first calls `shouldIgnoreBlur(shell, event
as FocusEvent)` (line 225) and returns early if it is true, otherwise hiding the context
menu and invoking `options.onBlur?.()` so the controller can save on blur.
3. Type changes into the editor so `onChange` in `controller.ts:922-950` marks
`state.draftContent` dirty and calls `scheduleAutosave()` (controller.ts:196-205),
starting a 1-second debounce timer; before that timer fires, click a control inside the
`.tool-shell` but outside the editor pane—e.g., the fullscreen button wired at
`controller.ts:975-978` (`#expand-fullscreen`) or other topbar controls created in the
layout.
4. That click shifts focus from the editor to a button inside `.tool-shell`, so in
`shouldIgnoreBlur()` (editor.ts:31-35) `widgetShell` is the enclosing `.tool-shell` and
`nextTarget` is contained within it; the function returns true, causing the TipTap
`onBlur` handler to skip `options.onBlur`, so no immediate `saveDocument()` runs before
navigation or re-render. If the user then quickly triggers a new tool result that
re-renders to a non-markdown payload (calling `markdownController.clear()` at
`app.ts:212-216`) or another file, the workspace state is cleared while autosave has not
yet fired, and the unsaved editor changes never reach disk despite the app's comment at
`app.ts:585-587` assuming blur would persist dirty drafts.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:** 31:35
**Comment:**
*Incorrect Condition Logic: Blur is ignored for any focus move within the entire tool shell, not just editor-owned UI, so clicking navigation/topbar controls can skip the immediate save path and leave changes only in the debounce timer. If a rerender/navigation happens before the timer fires, pending edits are dropped; limit blur suppression to true editor-internal controls (context menu/modal) so actionable navigation triggers save.
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 · |
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
3 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 by CodeRabbit
New Features
Style
Documentation
Tests
CodeAnt-AI Description
Add a built-in Markdown editor and richer file preview actions
What Changed
Impact
✅ Edit markdown without leaving the preview✅ Fewer lost changes during file conflicts✅ Faster navigation in large folders✅ Reliable preview restore after refresh🔄 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.