feat: Enhance Markdown link handling and navigation#1440
Merged
Conversation
- Added LinkNode component for rendering markdown links with context. - Updated MarkdownRenderer to support custom link rendering and context propagation. - Implemented useMarkdownLinkNavigation for handling link navigation logic. - Enhanced MarkdownArtifact to pass link context for artifacts. - Updated MessageBlockContent and WorkspacePreviewPane to include link context. - Added tests for LinkNode and updated tests for MarkdownRenderer and MessageBlockContent to cover new link navigation features. - Introduced linkTypes for better type management of markdown links. - Improved workspacePresenter to resolve markdown linked files with context.
Contributor
📝 WalkthroughWalkthroughThis PR introduces a comprehensive markdown link navigation system enabling links in markdown content to be handled contextually (opening fragments, web URLs, external protocols, or local files appropriately). It adds file-level preview authorization, dangerous protocol blocking in window.open requests, and infrastructure for propagating link context through the renderer component tree. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LinkNode as LinkNode.vue
participant Navigator as useMarkdownLinkNavigation
participant Classifier as classifyMarkdownLink
participant Handlers as Navigation Handlers
participant External as External APIs<br/>(Browser, Shell, etc.)
participant Presenter as Presenters<br/>(YoBrowser, Workspace)
User->>LinkNode: Click link (href)
LinkNode->>Navigator: navigateLink(href, event)
Navigator->>Classifier: classifyMarkdownLink(href)
Classifier-->>Navigator: MarkdownLinkTarget (kind + payload)
alt Fragment Link
Navigator->>Handlers: scrollToFragment(fragment)
Handlers-->>Navigator: success/failure
else Web Link (Alt held)
Navigator->>External: window.api.openExternal(url)
External-->>Navigator: success/failure
else Web Link (Default)
Navigator->>Presenter: YoBrowser.loadUrl(sessionId, url)
Presenter-->>Navigator: success/failure
else System/External Link
Navigator->>External: window.api.openExternal(url)
External-->>Navigator: success/failure
else Local Markdown File
Navigator->>Presenter: resolveMarkdownLinkedFile({href, sourceFilePath, workspacePath})
Presenter->>Presenter: resolve & authorize file
Presenter-->>Navigator: WorkspaceLinkedFileResolution | null
alt Resolution succeeded
Navigator->>Presenter: openFile or selectFile (session-dependent)
Presenter-->>Navigator: success/failure
else Resolution failed
Navigator-->>Navigator: return false
end
end
Navigator-->>LinkNode: Promise<boolean>
LinkNode-->>User: Navigation complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 7
🤖 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/main/presenter/browser/YoBrowserPresenter.ts`:
- Line 57: Rename the instance constant embeddedHostReadyTimeoutMs to
SCREAMING_SNAKE_CASE (EMBEDDED_HOST_READY_TIMEOUT_MS) and update all usages to
the new name (e.g., references inside methods that use
this.embeddedHostReadyTimeoutMs such as the error message and setTimeout call);
also consider renaming embeddedHostReadyStableMs to
EMBEDDED_HOST_READY_STABLE_MS for consistency and update all its references
similarly. Ensure the property declaration and every access uses the new
identifier(s) (this.EMBEDDED_HOST_READY_TIMEOUT_MS and
this.EMBEDDED_HOST_READY_STABLE_MS) so TypeScript references remain valid.
In `@src/renderer/src/components/artifacts/MarkdownArtifact.vue`:
- Around line 6-10: MarkdownArtifact.vue currently passes only { source:
'artifact' } to the MarkdownRenderer link-context, dropping session/thread info;
update the component props (e.g., via defineProps in MarkdownArtifact.vue) to
accept sessionId and/or threadId from the caller and include them in the
link-context passed to <MarkdownRenderer> (so link-context becomes { source:
'artifact', sessionId, threadId } or similar), and ensure callers that
instantiate this component propagate their sessionId/threadId through to this
prop so artifact-relative links resolve correctly; keep existing event handler
handleCopyClick unchanged.
In `@src/renderer/src/components/markdown/LinkNode.vue`:
- Around line 31-43: The code classifies links via
classifyMarkdownLink(href.value) and routes external links through navigateLink/
openExternal without validating schemes, allowing dangerous protocols like
javascript:, data:, blob:, about: to be opened; fix by adding a protocol
whitelist (e.g., http, https, mailto, tel) or explicit blocklist for dangerous
schemes inside the external-link handling path (either inside openExternal or
immediately before calling it from navigateLink/handleClick in LinkNode.vue),
ensure classifyMarkdownLink continues classifying but that
navigateLink/openExternal rejects or sanitizes any href whose URL.protocol is
not allowed, and add regression tests asserting links with javascript:, data:,
blob:, about: are blocked and safe ones still open.
In `@src/renderer/src/components/markdown/useMarkdownLinkNavigation.ts`:
- Around line 95-117: The openLocalFile function can throw if
workspacePresenter.resolveMarkdownLinkedFile or workspacePresenter.openFile
rejects, causing navigateLink to reject; wrap the async calls in a try/catch
inside openLocalFile (around resolveMarkdownLinkedFile and the subsequent
openFile/selectFile logic) and on any caught error log a warning including the
href and return false; preserve existing behavior of returning true on success
and returning false when resolution is falsy or an exception occurs.
- Around line 124-127: In the case 'fragment' branch of
useMarkdownLinkNavigation.ts, don't unconditionally call event?.preventDefault()
and return true; instead call scrollToFragment(target.fragment), store its
boolean result, and only call event?.preventDefault() and return true when that
result indicates the fragment was handled; otherwise allow native fallback by
returning false (or not consuming the event). This ensures the branch uses the
actual scrollToFragment handling result to decide whether to preventDefault and
consume the event.
- Around line 56-70: The scrollToFragment function can throw at
decodeURIComponent and document.querySelector; wrap the
decodeURIComponent(fragment) call in a try/catch to handle URIError and return
false on decode failure, and wrap the querySelector call (after using
buildSafeAttributeSelector) in try/catch to handle DOMException/SyntaxError and
treat it as "not found". Also improve or ensure buildSafeAttributeSelector fully
escapes CSS special characters (at least bracket/newline/backslash/quote) or
catch selector errors instead of relying on it. Finally, in navigateLink update
the fragment branch to use the boolean return value from scrollToFragment (call
scrollToFragment(fragment) and if it returns true prevent default/stop
navigation, otherwise return false to allow browser fallback) so unresolved
fragments fall back to default behavior.
In `@src/renderer/src/components/sidepanel/BrowserPanel.vue`:
- Around line 373-383: The request URL assignment to urlInput is being
overwritten by loadState and the extra nextTick is redundant; fix by calling
await loadState(currentSessionId.value) before setting urlInput.value (or alter
loadState to preserve an already-set urlInput), then remove the standalone await
nextTick() since ensureVisibleAttachment() already awaits nextTick() internally,
and keep the conditional call to await ensureVisibleAttachment() when
isBrowserPanelVisible.value is true.
🪄 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: f28c060d-53f5-4220-9e3a-4841e5fe25b7
📒 Files selected for processing (19)
src/main/presenter/browser/YoBrowserPresenter.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/workspacePresenter/index.tssrc/main/presenter/workspacePresenter/workspacePreviewProtocol.tssrc/renderer/src/components/artifacts/MarkdownArtifact.vuesrc/renderer/src/components/markdown/LinkNode.vuesrc/renderer/src/components/markdown/MarkdownRenderer.vuesrc/renderer/src/components/markdown/linkTypes.tssrc/renderer/src/components/markdown/useMarkdownLinkNavigation.tssrc/renderer/src/components/message/MessageBlockContent.vuesrc/renderer/src/components/sidepanel/BrowserPanel.vuesrc/renderer/src/components/sidepanel/viewer/WorkspacePreviewPane.vuesrc/shared/types/presenters/index.d.tssrc/shared/types/presenters/workspace.d.tstest/main/presenter/workspacePresenter.test.tstest/renderer/components/LinkNode.test.tstest/renderer/components/MarkdownRenderer.test.tstest/renderer/components/WorkspacePreviewPane.test.tstest/renderer/components/message/MessageBlockContent.test.ts
| private readonly downloadManager = new DownloadManager() | ||
| private readonly windowPresenter: IWindowPresenter | ||
| private readonly embeddedHostReadyTimeoutMs = 2000 | ||
| private readonly embeddedHostReadyTimeoutMs = 5000 |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename constant to follow SCREAMING_SNAKE_CASE convention.
The embeddedHostReadyTimeoutMs constant uses camelCase, but should use SCREAMING_SNAKE_CASE per the coding guidelines. As per coding guidelines, constants must use SCREAMING_SNAKE_CASE naming for files in src/**/*.{ts,tsx,js,jsx,vue}.
♻️ Proposed fix to rename the constant
- private readonly embeddedHostReadyTimeoutMs = 5000
+ private readonly EMBEDDED_HOST_READY_TIMEOUT_MS = 5000Also update all references to use the new name (line 632 and 635):
// Line 632-633
const error = new Error(
`Session browser host ${hostWindowId} did not become ready within ${this.EMBEDDED_HOST_READY_TIMEOUT_MS}ms`
)
// Line 635
}, this.EMBEDDED_HOST_READY_TIMEOUT_MS)Note: Consider also updating embeddedHostReadyStableMs on line 58 for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/browser/YoBrowserPresenter.ts` at line 57, Rename the
instance constant embeddedHostReadyTimeoutMs to SCREAMING_SNAKE_CASE
(EMBEDDED_HOST_READY_TIMEOUT_MS) and update all usages to the new name (e.g.,
references inside methods that use this.embeddedHostReadyTimeoutMs such as the
error message and setTimeout call); also consider renaming
embeddedHostReadyStableMs to EMBEDDED_HOST_READY_STABLE_MS for consistency and
update all its references similarly. Ensure the property declaration and every
access uses the new identifier(s) (this.EMBEDDED_HOST_READY_TIMEOUT_MS and
this.EMBEDDED_HOST_READY_STABLE_MS) so TypeScript references remain valid.
Comment on lines
+6
to
+10
| <MarkdownRenderer | ||
| :content="props.block.content || ''" | ||
| :link-context="{ source: 'artifact' }" | ||
| @copy="handleCopyClick" | ||
| /> |
Contributor
There was a problem hiding this comment.
Pass the session through for artifact markdown links.
This only forwards { source: 'artifact' }, so artifact markdown loses the session before it reaches the new navigation layer. Chat artifacts already have threadId upstream, and without threading that through here, relative links inside artifacts cannot be resolved via the workspace-aware markdown link flow.
Suggested fix
<MarkdownRenderer
:content="props.block.content || ''"
- :link-context="{ source: 'artifact' }"
+ :link-context="{
+ source: 'artifact',
+ sessionId: props.sessionId
+ }"
`@copy`="handleCopyClick"
/>const props = defineProps<{
block: {
artifact: {
type: string
title: string
}
content: string
}
sessionId?: string
}>()Update the caller that already has threadId/sessionId to pass it through as well.
📝 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
| <MarkdownRenderer | |
| :content="props.block.content || ''" | |
| :link-context="{ source: 'artifact' }" | |
| @copy="handleCopyClick" | |
| /> | |
| <MarkdownRenderer | |
| :content="props.block.content || ''" | |
| :link-context="{ | |
| source: 'artifact', | |
| sessionId: props.sessionId | |
| }" | |
| `@copy`="handleCopyClick" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/artifacts/MarkdownArtifact.vue` around lines 6 -
10, MarkdownArtifact.vue currently passes only { source: 'artifact' } to the
MarkdownRenderer link-context, dropping session/thread info; update the
component props (e.g., via defineProps in MarkdownArtifact.vue) to accept
sessionId and/or threadId from the caller and include them in the link-context
passed to <MarkdownRenderer> (so link-context becomes { source: 'artifact',
sessionId, threadId } or similar), and ensure callers that instantiate this
component propagate their sessionId/threadId through to this prop so
artifact-relative links resolve correctly; keep existing event handler
handleCopyClick unchanged.
Comment on lines
+31
to
+43
| const linkClass = computed(() => { | ||
| const baseClass = 'cursor-pointer underline decoration-from-font hover:opacity-80' | ||
| const target = classifyMarkdownLink(href.value) | ||
|
|
||
| if (target.kind === 'local-file' || target.kind === 'fragment') { | ||
| return `${baseClass} text-purple-600 dark:text-purple-400` | ||
| } | ||
|
|
||
| return `${baseClass} text-blue-600 dark:text-blue-400` | ||
| }) | ||
|
|
||
| const handleClick = async (event: MouseEvent) => { | ||
| await navigateLink(href.value, event) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nav_file="$(fd -p 'useMarkdownLinkNavigation.ts' src test | head -n1)"
test_file="$(fd -p 'LinkNode.test.ts' test | head -n1)"
echo '== useMarkdownLinkNavigation.ts =='
sed -n '1,260p' "$nav_file"
echo
echo '== dangerous protocol references =='
rg -n "javascript:|data:|blob:|about:|openExternal|window\\.open|classifyMarkdownLink" \
"$nav_file" \
"$test_file" \
src/renderer/src/components/markdown/linkTypes.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 6133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== linkTypes.ts implementation ==="
cat -n src/renderer/src/components/markdown/linkTypes.ts
echo ""
echo "=== LinkNode.test.ts for dangerous scheme tests ==="
rg -n "javascript:|data:|blob:|about:" test/renderer/components/LinkNode.test.ts -A 3 -B 1Repository: ThinkInAIXYZ/deepchat
Length of output: 2374
Add protocol validation to block dangerous schemes before external navigation.
classifyMarkdownLink() does not filter dangerous protocols—javascript:, data:, blob:, and about: all pass the protocol regex, create valid URL objects, and fall into the 'external' case. These are then forwarded directly to openExternal() without any validation, creating an XSS vulnerability.
Add a protocol whitelist or blocklist in openExternal() or at the point where 'external' links are handled, and add regression tests for dangerous schemes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/markdown/LinkNode.vue` around lines 31 - 43, The
code classifies links via classifyMarkdownLink(href.value) and routes external
links through navigateLink/ openExternal without validating schemes, allowing
dangerous protocols like javascript:, data:, blob:, about: to be opened; fix by
adding a protocol whitelist (e.g., http, https, mailto, tel) or explicit
blocklist for dangerous schemes inside the external-link handling path (either
inside openExternal or immediately before calling it from
navigateLink/handleClick in LinkNode.vue), ensure classifyMarkdownLink continues
classifying but that navigateLink/openExternal rejects or sanitizes any href
whose URL.protocol is not allowed, and add regression tests asserting links with
javascript:, data:, blob:, about: are blocked and safe ones still open.
Comment on lines
+56
to
+70
| const scrollToFragment = (fragment: string): boolean => { | ||
| const decodedFragment = decodeURIComponent(fragment) | ||
| if (!decodedFragment) { | ||
| return true | ||
| } | ||
|
|
||
| const byId = document.getElementById(decodedFragment) | ||
| if (byId) { | ||
| byId.scrollIntoView({ block: 'start' }) | ||
| return true | ||
| } | ||
|
|
||
| const byName = document.querySelector( | ||
| `[name="${buildSafeAttributeSelector(decodedFragment)}"]` | ||
| ) as HTMLElement | null |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/renderer/src/components/markdown/useMarkdownLinkNavigation.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 5371
🌐 Web query:
decodeURIComponent URIError invalid percent sequences JavaScript
💡 Result:
decodeURIComponent() throws URIError: URI malformed (often described as “invalid percent sequences”) when the input is not valid percent-encoding (or decodes to invalid UTF‑8).
Common causes
- A raw
%or incomplete/invalid escape, e.g."100%","%2","%ZZ"(a%must be followed by two hex digits). (developer.mozilla.org) - You’re decoding a string that was never encoded with
encodeURIComponent()(or was corrupted/truncated in transit). (developer.mozilla.org) - Double-decoding (decoding something that’s already been decoded) can expose stray
%and break. (rfc-editor.org)
Fixes / safe patterns
- Encode before you decode (only decode data that’s actually URL-encoded):
const encoded = encodeURIComponent(value);
const decoded = decodeURIComponent(encoded);- If you must decode “maybe-encoded” input, guard it:
function safeDecodeURIComponent(s) {
try { return decodeURIComponent(s); }
catch { return s; } // or return null / throw your own error
}- If the problem is stray
%, escape them before decoding:
const fixed = s.replace(/%(?![0-9A-Fa-f]{2})/g, '%25');
const decoded = decodeURIComponent(fixed);(Transforms invalid % into a literal % via %25.) (utmguard.com)
If you paste the exact string that’s failing (and where it came from: path, querystring, form data, etc.), I can point to the specific invalid sequence and the best fix.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Malformed_URI?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Malformed_URI?utm_source=openai
- 3: https://www.rfc-editor.org/rfc/pdfrfc/rfc3986.txt.pdf?utm_source=openai
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Malformed_URI?utm_source=openai
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Malformed_URI?utm_source=openai
- 6: https://www.utmguard.com/docs/syntax/malformed-percent-encoding?utm_source=openai
🌐 Web query:
document.querySelector DOMException SyntaxError invalid CSS selector
💡 Result:
document.querySelector(...) throws DOMException: SyntaxError when the argument isn’t a valid CSS selector string (the API literally parses it as CSS). (developer.mozilla.org)
Common causes + fixes:
-
Empty / undefined / non-string selector
- Example:
document.querySelector()ordocument.querySelector(sel)whereselis"",undefined, an array, or an object. - Fix: log/validate first:
console.log(sel, typeof sel); ensure you pass a string. (developer.mozilla.org)
- Example:
-
ID/class contains characters that must be escaped in CSS (or isn’t a valid CSS identifier)
- Examples that often break: IDs starting with a digit (
#22), containing/(#/my-id), etc. (stackoverflow.com) - Fix (recommended): escape the raw id/class value:
(developer.mozilla.org)
const id = el.id; // e.g. "22" or "/my-id" const node = document.querySelector(`#${CSS.escape(id)}`);
- Examples that often break: IDs starting with a digit (
-
Attribute selector syntax errors
- Example: missing quotes/brackets, or stray characters/newlines in the value.
- Fix: ensure proper CSS:
audio[data-key="65"](quotes are safest). (forum.freecodecamp.org)
If you paste the exact selector string you’re passing (and the element’s actual id/class), I can point out exactly which character is making it invalid and the correct escaped selector.
Citations:
- 1: https://developer.mozilla.org/docs/Web/API/Document/querySelector?utm_source=openai
- 2: https://developer.mozilla.org/docs/Web/API/Document/querySelector?utm_source=openai
- 3: https://stackoverflow.com/questions/37270787/uncaught-syntaxerror-failed-to-execute-queryselector-on-document?utm_source=openai
- 4: https://developer.mozilla.org/docs/Web/API/Document/querySelector?utm_source=openai
- 5: https://forum.freecodecamp.org/t/keyboard-html-16-uncaught-domexception-failed-to-execute-queryselector-on-document-audio-data-key-65-is-not-a-valid-selector/252067?utm_source=openai
Guard fragment decoding against URIError and selector lookup against SyntaxError.
Line 57: decodeURIComponent(fragment) throws URIError for malformed percent-encoded sequences (e.g., "%", "%ZZ"). Line 68-70: document.querySelector() throws DOMException (SyntaxError) for invalid CSS selectors. The current buildSafeAttributeSelector escapes only quotes and backslashes but not CSS special characters like ], [, or newlines, which can still produce invalid selectors. Both operations should be guarded with try-catch to prevent click-path runtime failures.
Additionally, line 126 ignores the boolean return value from scrollToFragment, and line 127 always returns true, preventing the browser's default fragment navigation fallback behavior when a scroll target is unresolved.
Suggested fix
const scrollToFragment = (fragment: string): boolean => {
- const decodedFragment = decodeURIComponent(fragment)
+ let decodedFragment: string
+ try {
+ decodedFragment = decodeURIComponent(fragment)
+ } catch (error) {
+ console.warn('[markdown-links] Invalid fragment encoding:', fragment, error)
+ return false
+ }
if (!decodedFragment) {
return true
}
const byId = document.getElementById(decodedFragment)
if (byId) {
byId.scrollIntoView({ block: 'start' })
return true
}
- const byName = document.querySelector(
- `[name="${buildSafeAttributeSelector(decodedFragment)}"]`
- ) as HTMLElement | null
+ let byName: HTMLElement | null = null
+ try {
+ byName = document.querySelector(
+ `[name="${CSS.escape(decodedFragment)}"]`
+ ) as HTMLElement | null
+ } catch (error) {
+ console.warn('[markdown-links] Invalid fragment selector:', decodedFragment, error)
+ return false
+ }
if (byName) {
byName.scrollIntoView({ block: 'start' })
return true
}
return false
}Additionally, update the fragment case in navigateLink to check the return value:
case 'fragment':
event?.preventDefault()
- scrollToFragment(target.fragment)
+ return scrollToFragment(target.fragment)
- return true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/markdown/useMarkdownLinkNavigation.ts` around
lines 56 - 70, The scrollToFragment function can throw at decodeURIComponent and
document.querySelector; wrap the decodeURIComponent(fragment) call in a
try/catch to handle URIError and return false on decode failure, and wrap the
querySelector call (after using buildSafeAttributeSelector) in try/catch to
handle DOMException/SyntaxError and treat it as "not found". Also improve or
ensure buildSafeAttributeSelector fully escapes CSS special characters (at least
bracket/newline/backslash/quote) or catch selector errors instead of relying on
it. Finally, in navigateLink update the fragment branch to use the boolean
return value from scrollToFragment (call scrollToFragment(fragment) and if it
returns true prevent default/stop navigation, otherwise return false to allow
browser fallback) so unresolved fragments fall back to default behavior.
Comment on lines
+95
to
+117
| const openLocalFile = async (href: string): Promise<boolean> => { | ||
| const { sessionId, workspacePath, sourceFilePath } = getSessionContext() | ||
| const resolution = await workspacePresenter.resolveMarkdownLinkedFile({ | ||
| workspacePath, | ||
| href, | ||
| sourceFilePath | ||
| }) | ||
|
|
||
| if (!resolution) { | ||
| console.warn('[markdown-links] Failed to resolve local markdown link:', href) | ||
| return false | ||
| } | ||
|
|
||
| if (sessionId) { | ||
| sidepanelStore.selectFile(sessionId, resolution.path, { | ||
| open: true, | ||
| viewMode: 'preview' | ||
| }) | ||
| return true | ||
| } | ||
|
|
||
| await workspacePresenter.openFile(resolution.path) | ||
| return true |
Contributor
There was a problem hiding this comment.
Catch presenter failures in local-file navigation.
If resolveMarkdownLinkedFile or openFile throws, navigateLink rejects instead of returning false, which can break link click flow.
Suggested fix
const openLocalFile = async (href: string): Promise<boolean> => {
const { sessionId, workspacePath, sourceFilePath } = getSessionContext()
- const resolution = await workspacePresenter.resolveMarkdownLinkedFile({
- workspacePath,
- href,
- sourceFilePath
- })
-
- if (!resolution) {
- console.warn('[markdown-links] Failed to resolve local markdown link:', href)
- return false
- }
-
- if (sessionId) {
- sidepanelStore.selectFile(sessionId, resolution.path, {
- open: true,
- viewMode: 'preview'
+ try {
+ const resolution = await workspacePresenter.resolveMarkdownLinkedFile({
+ workspacePath,
+ href,
+ sourceFilePath
})
- return true
- }
- await workspacePresenter.openFile(resolution.path)
- return true
+ if (!resolution) {
+ console.warn('[markdown-links] Failed to resolve local markdown link:', href)
+ return false
+ }
+
+ if (sessionId) {
+ sidepanelStore.selectFile(sessionId, resolution.path, {
+ open: true,
+ viewMode: 'preview'
+ })
+ return true
+ }
+
+ await workspacePresenter.openFile(resolution.path)
+ return true
+ } catch (error) {
+ console.warn('[markdown-links] Failed to open local markdown link:', href, error)
+ return false
+ }
}📝 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 openLocalFile = async (href: string): Promise<boolean> => { | |
| const { sessionId, workspacePath, sourceFilePath } = getSessionContext() | |
| const resolution = await workspacePresenter.resolveMarkdownLinkedFile({ | |
| workspacePath, | |
| href, | |
| sourceFilePath | |
| }) | |
| if (!resolution) { | |
| console.warn('[markdown-links] Failed to resolve local markdown link:', href) | |
| return false | |
| } | |
| if (sessionId) { | |
| sidepanelStore.selectFile(sessionId, resolution.path, { | |
| open: true, | |
| viewMode: 'preview' | |
| }) | |
| return true | |
| } | |
| await workspacePresenter.openFile(resolution.path) | |
| return true | |
| const openLocalFile = async (href: string): Promise<boolean> => { | |
| const { sessionId, workspacePath, sourceFilePath } = getSessionContext() | |
| try { | |
| const resolution = await workspacePresenter.resolveMarkdownLinkedFile({ | |
| workspacePath, | |
| href, | |
| sourceFilePath | |
| }) | |
| if (!resolution) { | |
| console.warn('[markdown-links] Failed to resolve local markdown link:', href) | |
| return false | |
| } | |
| if (sessionId) { | |
| sidepanelStore.selectFile(sessionId, resolution.path, { | |
| open: true, | |
| viewMode: 'preview' | |
| }) | |
| return true | |
| } | |
| await workspacePresenter.openFile(resolution.path) | |
| return true | |
| } catch (error) { | |
| console.warn('[markdown-links] Failed to open local markdown link:', href, error) | |
| return false | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/markdown/useMarkdownLinkNavigation.ts` around
lines 95 - 117, The openLocalFile function can throw if
workspacePresenter.resolveMarkdownLinkedFile or workspacePresenter.openFile
rejects, causing navigateLink to reject; wrap the async calls in a try/catch
inside openLocalFile (around resolveMarkdownLinkedFile and the subsequent
openFile/selectFile logic) and on any caught error log a warning including the
href and return false; preserve existing behavior of returning true on success
and returning false when resolution is falsy or an exception occurs.
Comment on lines
+124
to
+127
| case 'fragment': | ||
| event?.preventDefault() | ||
| scrollToFragment(target.fragment) | ||
| return true |
Contributor
There was a problem hiding this comment.
Return actual fragment handling result before consuming the event.
Line 125 always prevents default and Line 127 always returns true, even when no fragment target is found. That removes native fallback behavior.
Suggested fix
- case 'fragment':
- event?.preventDefault()
- scrollToFragment(target.fragment)
- return true
+ case 'fragment': {
+ const handled = scrollToFragment(target.fragment)
+ if (handled) {
+ event?.preventDefault()
+ }
+ return handled
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/markdown/useMarkdownLinkNavigation.ts` around
lines 124 - 127, In the case 'fragment' branch of useMarkdownLinkNavigation.ts,
don't unconditionally call event?.preventDefault() and return true; instead call
scrollToFragment(target.fragment), store its boolean result, and only call
event?.preventDefault() and return true when that result indicates the fragment
was handled; otherwise allow native fallback by returning false (or not
consuming the event). This ensures the branch uses the actual scrollToFragment
handling result to decide whether to preventDefault and consume the event.
Comment on lines
+373
to
383
| if (url) { | ||
| urlInput.value = url | ||
| } | ||
|
|
||
| await loadState(currentSessionId.value) | ||
|
|
||
| // Wait for panel to be visible and DOM ready before attaching | ||
| await nextTick() | ||
| if (isBrowserPanelVisible.value) { | ||
| await ensureVisibleAttachment() | ||
| } |
Contributor
There was a problem hiding this comment.
urlInput gets overwritten by loadState, and the extra nextTick is redundant.
On Line 373-375, urlInput.value is set from the request, but Line 377 immediately calls loadState, which resets urlInput from presenter state (Line 244). If status lags, the requested URL is lost. Also, ensureVisibleAttachment() already awaits nextTick() internally, so Line 380 adds avoidable latency.
Proposed patch
- // Update the URL input to reflect the requested URL
- if (url) {
- urlInput.value = url
- }
-
await loadState(currentSessionId.value)
-
- // Wait for panel to be visible and DOM ready before attaching
- await nextTick()
+ // Reflect requested URL even if presenter status hasn't updated yet.
+ if (url) {
+ urlInput.value = url
+ }
if (isBrowserPanelVisible.value) {
await ensureVisibleAttachment()
}📝 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 (url) { | |
| urlInput.value = url | |
| } | |
| await loadState(currentSessionId.value) | |
| // Wait for panel to be visible and DOM ready before attaching | |
| await nextTick() | |
| if (isBrowserPanelVisible.value) { | |
| await ensureVisibleAttachment() | |
| } | |
| await loadState(currentSessionId.value) | |
| // Reflect requested URL even if presenter status hasn't updated yet. | |
| if (url) { | |
| urlInput.value = url | |
| } | |
| if (isBrowserPanelVisible.value) { | |
| await ensureVisibleAttachment() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/sidepanel/BrowserPanel.vue` around lines 373 -
383, The request URL assignment to urlInput is being overwritten by loadState
and the extra nextTick is redundant; fix by calling await
loadState(currentSessionId.value) before setting urlInput.value (or alter
loadState to preserve an already-set urlInput), then remove the standalone await
nextTick() since ensureVisibleAttachment() already awaits nextTick() internally,
and keep the conditional call to await ensureVisibleAttachment() when
isBrowserPanelVisible.value is true.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Security