fix(markdown): follow-up workspace inline path link handling#264
Open
DerrickBarra wants to merge 5 commits intodaggerhashimoto:masterfrom
Open
fix(markdown): follow-up workspace inline path link handling#264DerrickBarra wants to merge 5 commits intodaggerhashimoto:masterfrom
DerrickBarra wants to merge 5 commits intodaggerhashimoto:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds workspace-aware path normalization to server file resolution and tightens markdown inline path linkification. Tests expanded for host-absolute workspace paths, wrapped/ punctuated inline tokens, inline-code linkification, and workspace-root guardrails returning 404. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as MarkdownRenderer
participant API as /api/files/resolve
participant FS as FileSystem
Renderer->>API: Request resolve(normalizedTarget, relativeTo?)
API->>API: normalizeWorkspaceLookupPath(path, workspaceRoot)
API->>FS: Lookup resolved path
FS-->>API: file metadata / 404
API-->>Renderer: {ok:true, path:..., type:..., binary:...} / 404
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/features/markdown/MarkdownRenderer.test.tsx (1)
86-96: Assert “no links rendered” in the overmatch regressions.These cases still pass if the renderer links the whole token (
path=/workspace/...) instead of the inner slice. Since the failure mode here is overreaching tokenization, I’d make the assertion stricter and require zero links.Suggested test tightening
- expect(screen.queryByRole('link', { name: '/workspace/src/App.tsx' })).toBeNull(); + expect(screen.queryAllByRole('link')).toHaveLength(0); ... - expect(screen.queryByRole('link', { name: '/home/derrick/.openclaw/workspace/src/App.tsx' })).toBeNull(); - expect(screen.queryByRole('link', { name: '/workspace/src/App.tsx' })).toBeNull(); + expect(screen.queryAllByRole('link')).toHaveLength(0);Also applies to: 186-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/markdown/MarkdownRenderer.test.tsx` around lines 86 - 96, Tighten the test assertion in MarkdownRenderer.test.tsx so it requires that no links are rendered (not just that the exact slice isn't linked): update the test using the MarkdownRenderer case with content "Open path=/workspace/src/App.tsx, now" (and the similar test around lines 186-197) to assert that queryAllByRole('link') returns zero (i.e., no anchor elements are present) instead of only asserting the specific link name is absent; keep the same props (onOpenWorkspacePath, pathLinkPrefixes) and behavior but replace the specific queryByRole(name...) check with a global no-links assertion to catch overmatch regressions.
🤖 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/features/markdown/inlineReferences.tsx`:
- Around line 22-42: normalizeWorkspaceCandidate currently forwards
percent-escaped strings verbatim; decode percent-escapes before returning to
ensure paths like "file:///workspace/My%20Doc.md" map correctly. In
normalizeWorkspaceCandidate (and when handling the file:// branch and the
prefix-matching branch where you build `${CANONICAL_WORKSPACE_PREFIX}${...}`),
call decodeURIComponent on the candidate or the computed suffix (wrap in
try/catch and fall back to the original string on error) so you return decoded
workspace paths; keep the length checks and leading-slash trimming logic but
apply them to the decoded values.
In `@src/features/markdown/MarkdownRenderer.tsx`:
- Around line 340-347: The inline branch currently calls
renderInlinePathReferences(...) unconditionally and can create nested <a> tags;
modify the inlineContent assignment to first detect whether the current code
span is rendered inside an existing markdown link (reuse the existing
processChildren safeguard or a new isInsideLink check) and only call
renderInlinePathReferences when not inside a link; otherwise keep the original
children (i.e., let processChildren leave <a> descendants alone). Update
references: change the inlineContent logic around renderInlinePathReferences,
using onOpenWorkspacePath and currentDocumentPath as before, and add a
regression test that renders a code span inside a markdown link (e.g.,
[`/workspace/src/App.tsx`](docs/todo.md)) asserting no nested anchor is produced
and only the outer link handler fires.
---
Nitpick comments:
In `@src/features/markdown/MarkdownRenderer.test.tsx`:
- Around line 86-96: Tighten the test assertion in MarkdownRenderer.test.tsx so
it requires that no links are rendered (not just that the exact slice isn't
linked): update the test using the MarkdownRenderer case with content "Open
path=/workspace/src/App.tsx, now" (and the similar test around lines 186-197) to
assert that queryAllByRole('link') returns zero (i.e., no anchor elements are
present) instead of only asserting the specific link name is absent; keep the
same props (onOpenWorkspacePath, pathLinkPrefixes) and behavior but replace the
specific queryByRole(name...) check with a global no-links assertion to catch
overmatch regressions.
🪄 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: 43d7a09b-c4aa-4052-bba9-bade704c4ab3
📒 Files selected for processing (5)
server/routes/file-browser.test.tsserver/routes/file-browser.tssrc/features/markdown/MarkdownRenderer.test.tsxsrc/features/markdown/MarkdownRenderer.tsxsrc/features/markdown/inlineReferences.tsx
Contributor
Author
|
Addressed the actionable review feedback on this branch in d0d4b40. What changed:
Validation:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.plans/2026-04-11-workhorse-packaging-and-chat-links-hardening.md (1)
187-196: Consider making the final summary explicitly partial instead of fully pending.Since Tasks 1–3 are complete, a brief “Partial results” summary here would improve scanability for reviewers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.plans/2026-04-11-workhorse-packaging-and-chat-links-hardening.md around lines 187 - 196, Update the "Final Results" section to reflect a partial summary instead of marking everything as Pending: rename or add a "Partial Results" subsection under the "Final Results" header and set Status to something like "Partial — Tasks 1–3 complete"; briefly list completed items (e.g., "Tasks 1–3 completed") and note remaining items as pending (e.g., "What We Built: partial; Commits: pending; Lessons Learned: pending") so reviewers can quickly see progress without losing the existing pending placeholders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.plans/2026-04-11-workhorse-packaging-and-chat-links-hardening.md:
- Line 150: Update the review follow-up note to capitalize "Markdown" where it
currently reads "markdown tests" so the sentence reads "...Focused Markdown
tests and targeted eslint passed after the fix."; locate the sentence
referencing "inline workspace candidates...", "inline code stops linkifying..."
and change the lowercase "markdown" token to "Markdown" to maintain proper noun
consistency.
---
Nitpick comments:
In @.plans/2026-04-11-workhorse-packaging-and-chat-links-hardening.md:
- Around line 187-196: Update the "Final Results" section to reflect a partial
summary instead of marking everything as Pending: rename or add a "Partial
Results" subsection under the "Final Results" header and set Status to something
like "Partial — Tasks 1–3 complete"; briefly list completed items (e.g., "Tasks
1–3 completed") and note remaining items as pending (e.g., "What We Built:
partial; Commits: pending; Lessons Learned: pending") so reviewers can quickly
see progress without losing the existing pending placeholders.
🪄 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: 47d3782a-84c3-45a7-8c87-a1987e7912d2
📒 Files selected for processing (4)
.plans/2026-04-11-workhorse-packaging-and-chat-links-hardening.mdsrc/features/markdown/MarkdownRenderer.test.tsxsrc/features/markdown/MarkdownRenderer.tsxsrc/features/markdown/inlineReferences.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/features/markdown/MarkdownRenderer.tsx
- src/features/markdown/inlineReferences.tsx
- src/features/markdown/MarkdownRenderer.test.tsx
| - `719db38` <- `a04876e` | ||
| - `5435706` <- `6cf2616` | ||
| - Packaging note: the new issue explicitly references prior art `#237` / merged PR `#239` and scopes this lane as a post-merge bugfix follow-up rather than reopening the original feature request. | ||
| - Review follow-up update (2026-04-11): addressed PR `#264` review feedback on the owning branch — inline workspace candidates now percent-decode before dispatch, inline code stops linkifying when already inside a markdown link to avoid nested anchors, and overmatch regressions now assert zero rendered links. Focused markdown tests and targeted eslint passed after the fix. |
There was a problem hiding this comment.
Capitalize “Markdown” in the review follow-up note.
Use “Markdown” (proper noun) for consistency in technical docs at Line 150.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~150-~150: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ... stops linkifying when already inside a markdown link to avoid nested anchors, and overm...
(MARKDOWN_NNP)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.plans/2026-04-11-workhorse-packaging-and-chat-links-hardening.md at line
150, Update the review follow-up note to capitalize "Markdown" where it
currently reads "markdown tests" so the sentence reads "...Focused Markdown
tests and targeted eslint passed after the fix."; locate the sentence
referencing "inline workspace candidates...", "inline code stops linkifying..."
and change the lowercase "markdown" token to "Markdown" to maintain proper noun
consistency.
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/workspace/...path slices inside surrounding textCloses #262.
In Plain English
This PR makes workspace path links feel less brittle and more human.
Before this follow-up, the feature worked for the basic case, but there were still several real-world chat/message patterns where links either failed to render or could match too aggressively. That meant people could paste something that clearly looked like a workspace path and not get a clickable link — or, worse, nearby text could get treated like part of the path when it should not.
These changes make valid workspace paths render more reliably in natural writing patterns, while tightening the matcher so only intended workspace paths become actionable. In short: better link detection, fewer weird misses, and safer rendering behavior.
Provenance
Clean branch commit stack:
36cca92—fix(markdown): linkify embedded workspace path slicesd3d3a64—fix(markdown): narrow inline path match typinga04876e—Fix wrapped workspace inline path links6cf2616—Tighten workspace-rooted inline path matchingValidated downstream via local
workhorsecherry-picks:4e3c8fb<-36cca922b064f0<-d3d3a64719db38<-a04876e5435706<-6cf2616Context
This is the post-merge follow-up lane to the original workspace path links feature from #237 / #239.
Summary by CodeRabbit
New Features
Improvements
file://URIs, and wrapped path formats.