Make the Tauri frontend handle workspace file links and views properly#1434
Closed
senamakel wants to merge 5 commits into
Closed
Make the Tauri frontend handle workspace file links and views properly#1434senamakel wants to merge 5 commits into
senamakel wants to merge 5 commits into
Conversation
This commit introduces a unified workspace-link and workspace-file interaction layer for the desktop app. It adds three distinct actions for local workspace paths: open, reveal, and preview. - It adds new Tauri commands `open_workspace_path`, `reveal_workspace_path`, and `read_workspace_file_string` in a new module `workspace.rs`. - Extends the conversations renderer so it can recognize workspace-local link patterns instead of only `http/https/mailto`. - Implements those capabilities into the Memory Workspace graph so summary nodes open natively without bouncing through custom links.
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Contributor
📝 WalkthroughWalkthroughThis PR adds desktop-app workspace-file integration by implementing three Tauri commands (open, reveal, read), frontend TypeScript utilities for workspace-link detection, and integrating workspace-link handling into MemoryGraph, MemoryWorkspace, and AgentMessageBubble components with full test coverage. ChangesWorkspace Link Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 docstrings
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Acknowledged. |
This commit introduces a unified workspace-link and workspace-file interaction layer for the desktop app. It adds three distinct actions for local workspace paths: open, reveal, and preview. - It adds new Tauri commands `open_workspace_path`, `reveal_workspace_path`, and `read_workspace_file_string` in a new module `workspace.rs`. - Extends the conversations renderer so it can recognize workspace-local link patterns instead of only `http/https/mailto`. - Implements those capabilities into the Memory Workspace graph so summary nodes open natively without bouncing through custom links.
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/pages/conversations/components/AgentMessageBubble.tsx (1)
67-83: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPlease add tests for both workspace-link and external-link branches (both renderers).
These click handlers now contain new routing logic in two places. Please cover at least: (1) file/workspace link →
openWorkspacePath, (2) allowed external link →openUrl, (3) disallowed link → no launch.As per coding guidelines: “Co-locate unit tests as
*.test.tsxunderapp/src/**” and “add tests for new/changed lines (happy path + at least one failure/edge case).”Also applies to: 103-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/pages/conversations/components/AgentMessageBubble.tsx` around lines 67 - 83, Add a co-located test file AgentMessageBubble.test.tsx under app/src/pages/conversations/components that renders the AgentMessageBubble (both renderer variants referenced in the diff) and asserts the three behaviors: clicking a workspace link (mock isWorkspaceLink/getWorkspacePathFromHref to return true/path) calls openWorkspacePath with the expected path; clicking an allowed external link (mock isAllowedExternalHref to true) calls openUrl with the href; and clicking a disallowed link (mock isAllowedExternalHref to false and isWorkspaceLink false) does not call openUrl or openWorkspacePath. Mock/stub openWorkspacePath and openUrl to return rejected/resolved promises as needed and use user-event or fireEvent to click the link node, then assert calls and that no launcher errors are thrown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src-tauri/src/workspace.rs`:
- Around line 9-33: Add unit tests for resolve_and_validate that exercise
traversal and boundary cases: create tests that (1) pass a relative path
containing ".." and assert Err("Path traversal denied"), (2) pass an absolute
path outside the workspace root and assert Err("Path is outside the workspace"),
(3) create a symlink inside the workspace that points outside and assert
canonicalization rejects it (Err), and (4) verify valid relative and absolute
paths inside the workspace return Ok canonical target. Use temporary directories
and filesystem operations (creating files, dirs, and symlinks) and call
resolve_and_validate and get_workspace_root to set up the workspace root stubs
as needed so both the path-normalization branch (Path::is_absolute) and the
canonicalize unwrap_or branches are covered.
- Around line 99-104: The function that reads text previews (e.g.,
read_workspace_file_string) currently calls std::fs::read_to_string(&resolved)
which can load arbitrarily large files; instead, before reading, check the file
size via std::fs::metadata(&resolved).len() and reject or truncate files above a
defined limit (e.g., 1MB) or open the file and read only the first N bytes using
a reader with take() (File::open + BufReader::new(...).take(MAX_PREVIEW_BYTES))
into a String/Vec<u8>, returning a clear error or a truncated preview; update
the match arm (extensions "md"|"txt"|"json"|"csv"|"log") to perform this bounded
read using resolved and propagate errors as strings consistent with the existing
map_err(|e| e.to_string()) behavior.
In `@app/src/components/intelligence/__tests__/MemoryWorkspace.test.tsx`:
- Around line 138-147: Add a test in MemoryWorkspace.test.tsx that renders
<MemoryWorkspace />, finds and clicks the "memory-reveal-workspace" button
(query by test id 'memory-reveal-workspace' or the rendered button), and asserts
that openWorkspacePath was called with the graph.content_root_abs value; locate
the test near the existing 'clicking a summary node opens that file in
workspace' case and reuse the mocked graph data and openWorkspacePath spy so the
new test verifies openWorkspacePath(graph.content_root_abs) is invoked when the
button is clicked.
In `@app/src/utils/workspaceLinks.ts`:
- Around line 26-34: isWorkspaceLink currently treats any string without '://'
as a workspace link, which incorrectly classifies mailto:, tel:, fragment links
(#...), and other protocols; update isWorkspaceLink to only treat absolute
file:// URLs or true filesystem/relative paths as workspace links by: keeping
the URL(file:) branch, and changing the fallback to return true only for paths
that start with '/' or './' or '../' (or otherwise look like relative filesystem
paths) while explicitly excluding strings that start with a scheme like
/^[a-zA-Z][\w+.-]*:/ (e.g., mailto:, tel:, javascript:) and fragments starting
with '#'; reference the isWorkspaceLink function and the href parameter when
making this change.
---
Outside diff comments:
In `@app/src/pages/conversations/components/AgentMessageBubble.tsx`:
- Around line 67-83: Add a co-located test file AgentMessageBubble.test.tsx
under app/src/pages/conversations/components that renders the AgentMessageBubble
(both renderer variants referenced in the diff) and asserts the three behaviors:
clicking a workspace link (mock isWorkspaceLink/getWorkspacePathFromHref to
return true/path) calls openWorkspacePath with the expected path; clicking an
allowed external link (mock isAllowedExternalHref to true) calls openUrl with
the href; and clicking a disallowed link (mock isAllowedExternalHref to false
and isWorkspaceLink false) does not call openUrl or openWorkspacePath. Mock/stub
openWorkspacePath and openUrl to return rejected/resolved promises as needed and
use user-event or fireEvent to click the link node, then assert calls and that
no launcher errors are thrown.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48c4cc16-7111-4619-b7c9-6b0c6ab075ea
⛔ Files ignored due to path filters (1)
app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
app/src-tauri/src/lib.rsapp/src-tauri/src/workspace.rsapp/src/components/intelligence/MemoryGraph.tsxapp/src/components/intelligence/MemoryWorkspace.tsxapp/src/components/intelligence/__tests__/MemoryWorkspace.test.tsxapp/src/pages/conversations/components/AgentMessageBubble.tsxapp/src/utils/workspaceLinks.tscommit_message.txt
Comment on lines
+9
to
+33
| fn resolve_and_validate(path: &str) -> Result<PathBuf, String> { | ||
| let root = get_workspace_root(); | ||
|
|
||
| // Support both relative paths and absolute paths that are inside the root | ||
| let target = if Path::new(path).is_absolute() { | ||
| PathBuf::from(path) | ||
| } else { | ||
| root.join(path) | ||
| }; | ||
|
|
||
| // Prevent traversing outside workspace via .. | ||
| if path.contains("..") { | ||
| return Err("Path traversal denied".into()); | ||
| } | ||
|
|
||
| // Attempt to canonicalize | ||
| let canonical_root = root.canonicalize().unwrap_or(root.clone()); | ||
| let canonical_target = target.canonicalize().unwrap_or(target.clone()); | ||
|
|
||
| if !canonical_target.starts_with(&canonical_root) { | ||
| return Err("Path is outside the workspace".into()); | ||
| } | ||
|
|
||
| Ok(canonical_target) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add unit tests for resolve_and_validate boundary/security cases.
This resolver is the trust boundary for all three workspace commands, but there’s no test coverage here for traversal attempts, absolute outside-root paths, and symlink escapes.
As per coding guidelines: “PRs must meet ≥ 80% coverage on changed lines … add tests for new/changed lines” and “Ship enough unit tests and coverage for the behavior you are adding or changing before building additional features on top of it.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src-tauri/src/workspace.rs` around lines 9 - 33, Add unit tests for
resolve_and_validate that exercise traversal and boundary cases: create tests
that (1) pass a relative path containing ".." and assert Err("Path traversal
denied"), (2) pass an absolute path outside the workspace root and assert
Err("Path is outside the workspace"), (3) create a symlink inside the workspace
that points outside and assert canonicalization rejects it (Err), and (4) verify
valid relative and absolute paths inside the workspace return Ok canonical
target. Use temporary directories and filesystem operations (creating files,
dirs, and symlinks) and call resolve_and_validate and get_workspace_root to set
up the workspace root stubs as needed so both the path-normalization branch
(Path::is_absolute) and the canonicalize unwrap_or branches are covered.
Comment on lines
+99
to
+104
| // Optionally check if file is text/markdown | ||
| let ext = resolved.extension().and_then(|s| s.to_str()).unwrap_or(""); | ||
| match ext.to_lowercase().as_str() { | ||
| "md" | "txt" | "json" | "csv" | "log" => { | ||
| std::fs::read_to_string(&resolved).map_err(|e| e.to_string()) | ||
| } |
Contributor
There was a problem hiding this comment.
Bound the preview file size before read_to_string.
read_workspace_file_string currently reads the entire file into memory. A large .log/.csv can trigger avoidable memory pressure in the shell process.
💡 Suggested patch
pub fn read_workspace_file_string(path: String) -> Result<String, String> {
let resolved = resolve_and_validate(&path)?;
@@
if !resolved.is_file() {
return Err("Path is not a file".into());
}
+
+ const MAX_PREVIEW_BYTES: u64 = 1024 * 1024; // 1 MiB
+ let meta = std::fs::metadata(&resolved).map_err(|e| e.to_string())?;
+ if meta.len() > MAX_PREVIEW_BYTES {
+ return Err(format!(
+ "File is too large for preview ({} bytes > {} bytes)",
+ meta.len(),
+ MAX_PREVIEW_BYTES
+ ));
+ }
@@
"md" | "txt" | "json" | "csv" | "log" => {
std::fs::read_to_string(&resolved).map_err(|e| e.to_string())
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src-tauri/src/workspace.rs` around lines 99 - 104, The function that
reads text previews (e.g., read_workspace_file_string) currently calls
std::fs::read_to_string(&resolved) which can load arbitrarily large files;
instead, before reading, check the file size via
std::fs::metadata(&resolved).len() and reject or truncate files above a defined
limit (e.g., 1MB) or open the file and read only the first N bytes using a
reader with take() (File::open + BufReader::new(...).take(MAX_PREVIEW_BYTES))
into a String/Vec<u8>, returning a clear error or a truncated preview; update
the match arm (extensions "md"|"txt"|"json"|"csv"|"log") to perform this bounded
read using resolved and propagate errors as strings consistent with the existing
map_err(|e| e.to_string()) behavior.
Comment on lines
+26
to
+34
| export function isWorkspaceLink(href: string): boolean { | ||
| try { | ||
| const url = new URL(href); | ||
| return url.protocol === 'file:'; | ||
| } catch { | ||
| // If it's not a valid URL (e.g., relative path like /wiki/summaries or wiki/summaries), | ||
| // it's a potential workspace link | ||
| return href.startsWith('/') || !href.includes('://'); | ||
| } |
Contributor
There was a problem hiding this comment.
isWorkspaceLink is over-broad and hijacks non-file links.
Line 33 (!href.includes('://')) classifies mailto:, tel:, and #fragment as workspace links. That breaks expected link behavior in chat/table markdown.
💡 Suggested patch
export function isWorkspaceLink(href: string): boolean {
+ const trimmed = href.trim();
+ if (!trimmed) return false;
+
try {
- const url = new URL(href);
+ const url = new URL(trimmed);
return url.protocol === 'file:';
} catch {
- // If it's not a valid URL (e.g., relative path like /wiki/summaries or wiki/summaries),
- // it's a potential workspace link
- return href.startsWith('/') || !href.includes('://');
+ // Reject non-file URI schemes like mailto:, tel:, data:, etc.
+ if (/^[a-zA-Z][a-zA-Z\d+.-]*:/.test(trimmed)) return false;
+ // Accept likely local paths.
+ return (
+ trimmed.startsWith('/') ||
+ trimmed.startsWith('./') ||
+ trimmed.startsWith('../')
+ );
}
}📝 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 isWorkspaceLink(href: string): boolean { | |
| try { | |
| const url = new URL(href); | |
| return url.protocol === 'file:'; | |
| } catch { | |
| // If it's not a valid URL (e.g., relative path like /wiki/summaries or wiki/summaries), | |
| // it's a potential workspace link | |
| return href.startsWith('/') || !href.includes('://'); | |
| } | |
| export function isWorkspaceLink(href: string): boolean { | |
| const trimmed = href.trim(); | |
| if (!trimmed) return false; | |
| try { | |
| const url = new URL(trimmed); | |
| return url.protocol === 'file:'; | |
| } catch { | |
| // Reject non-file URI schemes like mailto:, tel:, data:, etc. | |
| if (/^[a-zA-Z][a-zA-Z\d+.-]*:/.test(trimmed)) return false; | |
| // Accept likely local paths. | |
| return ( | |
| trimmed.startsWith('/') || | |
| trimmed.startsWith('./') || | |
| trimmed.startsWith('../') | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/utils/workspaceLinks.ts` around lines 26 - 34, isWorkspaceLink
currently treats any string without '://' as a workspace link, which incorrectly
classifies mailto:, tel:, fragment links (#...), and other protocols; update
isWorkspaceLink to only treat absolute file:// URLs or true filesystem/relative
paths as workspace links by: keeping the URL(file:) branch, and changing the
fallback to return true only for paths that start with '/' or './' or '../' (or
otherwise look like relative filesystem paths) while explicitly excluding
strings that start with a scheme like /^[a-zA-Z][\w+.-]*:/ (e.g., mailto:, tel:,
javascript:) and fragments starting with '#'; reference the isWorkspaceLink
function and the href parameter when making this change.
This commit introduces a unified workspace-link and workspace-file interaction layer for the desktop app. It adds three distinct actions for local workspace paths: open, reveal, and preview. - It adds new Tauri commands `open_workspace_path`, `reveal_workspace_path`, and `read_workspace_file_string` in a new module `workspace.rs`. - Extends the conversations renderer so it can recognize workspace-local link patterns instead of only `http/https/mailto`. - Implements those capabilities into the Memory Workspace graph so summary nodes open natively without bouncing through custom links.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/utils/__tests__/workspaceLinks.test.ts (1)
1-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrettier check is failing for this test file.
CI already reports style-check failure here; please run Prettier on this file before merge to unblock the TypeScript check job.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/utils/__tests__/workspaceLinks.test.ts` around lines 1 - 72, Prettier formatting is failing for this test file; run Prettier to reformat the file containing tests for isWorkspaceLink, getWorkspacePathFromHref, openWorkspacePath, revealWorkspacePath and readWorkspaceFileString and then commit the changes (for example, run your project's prettier command or npx prettier --write <this test file>), then re-run CI to ensure the TypeScript/style check passes.app/src/components/intelligence/__tests__/MemoryGraph.test.tsx (1)
1-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrettier check is failing for this test file.
This file is also reported by CI style checks. Please run Prettier before merge so TypeScript check passes cleanly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/intelligence/__tests__/MemoryGraph.test.tsx` around lines 1 - 41, Prettier formatting is failing for the test file that imports MemoryGraph and openWorkspacePath; run Prettier to fix the style issues (e.g. run prettier --write on app/src/components/intelligence/__tests__/MemoryGraph.test.tsx or your repo-wide prettier command), then re-run the test/CI; ensure imports, spacing, and trailing commas match project config so the MemoryGraph test and TypeScript checks pass and openWorkspacePath mock lines remain intact.
🧹 Nitpick comments (2)
app/src/utils/__tests__/workspaceLinks.test.ts (1)
45-70: ⚡ Quick winAdd non-Tauri edge-case tests for
openWorkspacePathandrevealWorkspacePath.Only
readWorkspaceFileStringcurrently asserts the non-Tauri path. Please also verifyopenWorkspacePath/revealWorkspacePathno-op behavior (isTauri=false) and ensureinvokeis not called.Suggested test additions
describe('Tauri wrappers', () => { @@ it('throws error for readWorkspaceFileString if isTauri is false', async () => { vi.mocked(isTauri).mockReturnValue(false); await expect(readWorkspaceFileString('/test/path')).rejects.toThrow('Workspace file preview is only supported in Tauri.'); }); + + it('openWorkspacePath is a no-op when isTauri is false', async () => { + vi.mocked(isTauri).mockReturnValue(false); + await openWorkspacePath('/test/path'); + expect(invoke).not.toHaveBeenCalled(); + }); + + it('revealWorkspacePath is a no-op when isTauri is false', async () => { + vi.mocked(isTauri).mockReturnValue(false); + await revealWorkspacePath('/test/path'); + expect(invoke).not.toHaveBeenCalled(); + }); });As per coding guidelines, “PRs must meet ≥ 80% coverage on changed lines ... add tests for new/changed lines (happy path + at least one failure/edge case).”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/utils/__tests__/workspaceLinks.test.ts` around lines 45 - 70, Add tests that cover the non-Tauri edge cases for openWorkspacePath and revealWorkspacePath by mocking isTauri to return false, calling each function (openWorkspacePath('/test/path') and revealWorkspacePath('/test/path')), and asserting that invoke was not called (expect(invoke).not.toHaveBeenCalled()). Ensure you use vi.mocked(isTauri).mockReturnValue(false) in those tests and reset/clear the invoke mock between tests (or in a beforeEach) so previous Tauri tests don't affect assertions.app/src/components/intelligence/__tests__/MemoryGraph.test.tsx (1)
12-21: ⚡ Quick winAvoid
as anyin the node fixture to preserve type-safety.
as anyhere can hide contract drift between the test fixture andMemoryGraphprops. A typed fixture (e.g., viasatisfies) keeps this test behavior-focused while still catching schema breaks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/intelligence/__tests__/MemoryGraph.test.tsx` around lines 12 - 21, Replace the untyped test fixture that uses "as any" with a properly typed fixture to preserve type-safety: declare the nodes array so it satisfies the expected MemoryGraph props type (e.g., use TypeScript's "satisfies" with the MemoryGraphProps node type or use MemoryGraphProps['nodes']) instead of casting to any, ensuring the shape (id, kind, tree_kind, level, file_basename, label) matches the MemoryGraph component's prop type (reference the nodes fixture in MemoryGraph.test.tsx and the MemoryGraph prop/interface name) so future schema drift is caught by the compiler.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/src/components/intelligence/__tests__/MemoryGraph.test.tsx`:
- Around line 1-41: Prettier formatting is failing for the test file that
imports MemoryGraph and openWorkspacePath; run Prettier to fix the style issues
(e.g. run prettier --write on
app/src/components/intelligence/__tests__/MemoryGraph.test.tsx or your repo-wide
prettier command), then re-run the test/CI; ensure imports, spacing, and
trailing commas match project config so the MemoryGraph test and TypeScript
checks pass and openWorkspacePath mock lines remain intact.
In `@app/src/utils/__tests__/workspaceLinks.test.ts`:
- Around line 1-72: Prettier formatting is failing for this test file; run
Prettier to reformat the file containing tests for isWorkspaceLink,
getWorkspacePathFromHref, openWorkspacePath, revealWorkspacePath and
readWorkspaceFileString and then commit the changes (for example, run your
project's prettier command or npx prettier --write <this test file>), then
re-run CI to ensure the TypeScript/style check passes.
---
Nitpick comments:
In `@app/src/components/intelligence/__tests__/MemoryGraph.test.tsx`:
- Around line 12-21: Replace the untyped test fixture that uses "as any" with a
properly typed fixture to preserve type-safety: declare the nodes array so it
satisfies the expected MemoryGraph props type (e.g., use TypeScript's
"satisfies" with the MemoryGraphProps node type or use
MemoryGraphProps['nodes']) instead of casting to any, ensuring the shape (id,
kind, tree_kind, level, file_basename, label) matches the MemoryGraph
component's prop type (reference the nodes fixture in MemoryGraph.test.tsx and
the MemoryGraph prop/interface name) so future schema drift is caught by the
compiler.
In `@app/src/utils/__tests__/workspaceLinks.test.ts`:
- Around line 45-70: Add tests that cover the non-Tauri edge cases for
openWorkspacePath and revealWorkspacePath by mocking isTauri to return false,
calling each function (openWorkspacePath('/test/path') and
revealWorkspacePath('/test/path')), and asserting that invoke was not called
(expect(invoke).not.toHaveBeenCalled()). Ensure you use
vi.mocked(isTauri).mockReturnValue(false) in those tests and reset/clear the
invoke mock between tests (or in a beforeEach) so previous Tauri tests don't
affect assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89045fb3-4a93-4b16-8953-4f2f2d181dbe
📒 Files selected for processing (6)
app/src-tauri/src/lib.rsapp/src-tauri/src/workspace.rsapp/src-tauri/src/workspace_tests.rsapp/src/components/intelligence/__tests__/MemoryGraph.test.tsxapp/src/pages/conversations/components/__tests__/AgentMessageBubble.test.tsxapp/src/utils/__tests__/workspaceLinks.test.ts
✅ Files skipped from review due to trivial changes (2)
- app/src-tauri/src/workspace_tests.rs
- app/src/pages/conversations/components/tests/AgentMessageBubble.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src-tauri/src/lib.rs
- app/src-tauri/src/workspace.rs
- Add tests for `MemoryGraph.tsx`, `MemoryWorkspace.tsx`, `workspaceLinks.ts` and `AgentMessageBubble.tsx`. - Cover missing `!isTauri()` checks. - Cover promise `.catch()` handlers for missing edge cases. - Bypass markdown anchor rewrites via mocked protocols for bubbling clicks correctly in tests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/components/intelligence/MemoryGraph.tsx (1)
166-182: 💤 Low valueConsider extracting shared path-building logic to reduce duplication.
openSummaryInWorkspaceandopenSummaryInObsidianshare identical guard clauses (lines 143-145 vs 167-169) and relative path construction (lines 152-156 vs 171-175). Extracting a helper likegetSummaryRelPath(node)would reduce duplication and ensure consistency.♻️ Optional: Extract shared path helper
+function getSummaryRelPath(node: GraphNode): string | null { + if (node.kind !== 'summary' || !node.tree_kind || node.level == null || !node.file_basename) { + return null; + } + const slug = slugify(node.tree_scope ?? ''); + return node.tree_kind === 'global' + ? 'wiki/summaries' + : `wiki/summaries/${node.tree_kind}-${slug}/L${node.level}/${node.file_basename}.md`; +} + async function openSummaryInWorkspace(node: GraphNode, contentRootAbs: string): Promise<void> { - if (node.kind !== 'summary' || !node.tree_kind || node.level == null || !node.file_basename) { - return; - } - const slug = slugify(node.tree_scope ?? ''); - const rel = - node.tree_kind === 'global' - ? `wiki/summaries` - : `wiki/summaries/${node.tree_kind}-${slug}/L${node.level}/${node.file_basename}.md`; + const rel = getSummaryRelPath(node); + if (!rel) return; const abs = joinPath(contentRootAbs, rel); // ... rest unchanged🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/intelligence/MemoryGraph.tsx` around lines 166 - 182, Both openSummaryInWorkspace and openSummaryInObsidian repeat the same guard checks and relative-path construction; extract a single helper (e.g., getSummaryRelPath(node): string | null) that performs the guard (returns null if node is not a valid summary) and returns the relative path string like `wiki/summaries` or `wiki/summaries/${node.tree_kind}-${slug}/L${node.level}/${node.file_basename}.md`; replace the duplicated guard + rel construction in openSummaryInWorkspace and openSummaryInObsidian to call getSummaryRelPath(node) and bail out if it returns null, then compute abs/joinPath and call openWorkspacePath/openObsidianPath as before, ensuring you reuse slugify and maintain existing logging and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src-tauri/src/workspace.rs`:
- Around line 20-22: The current path traversal check uses path.contains("..")
which can be bypassed by encoded or alternate sequences; update the validation
to either (A) percent-decode and normalize the incoming path before checking
(i.e. decode percent-encodings, collapse components, then check for any ".."
segments and reject), or (B) remove the naive contains check and rely solely on
std::fs::canonicalize + starts_with(root) (the existing canonicalize and
starts_with calls) to determine if the resolved path is inside the workspace;
implement one of these approaches and adjust the code around the
path.contains("..") check accordingly (use a decode/normalize helper or remove
the check and keep the canonicalize/starts_with logic).
---
Nitpick comments:
In `@app/src/components/intelligence/MemoryGraph.tsx`:
- Around line 166-182: Both openSummaryInWorkspace and openSummaryInObsidian
repeat the same guard checks and relative-path construction; extract a single
helper (e.g., getSummaryRelPath(node): string | null) that performs the guard
(returns null if node is not a valid summary) and returns the relative path
string like `wiki/summaries` or
`wiki/summaries/${node.tree_kind}-${slug}/L${node.level}/${node.file_basename}.md`;
replace the duplicated guard + rel construction in openSummaryInWorkspace and
openSummaryInObsidian to call getSummaryRelPath(node) and bail out if it returns
null, then compute abs/joinPath and call openWorkspacePath/openObsidianPath as
before, ensuring you reuse slugify and maintain existing logging and error
handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b86f327-e2d2-486b-9c20-40a1c4302298
📒 Files selected for processing (8)
app/src-tauri/src/lib.rsapp/src-tauri/src/workspace.rsapp/src/components/intelligence/MemoryGraph.tsxapp/src/components/intelligence/__tests__/MemoryGraph.test.tsxapp/src/components/intelligence/__tests__/MemoryWorkspace.test.tsxapp/src/pages/conversations/components/__tests__/AgentMessageBubble.test.tsxapp/src/utils/__tests__/workspaceLinks.test.tscommit_message.txt
✅ Files skipped from review due to trivial changes (1)
- commit_message.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src-tauri/src/lib.rs
Comment on lines
+20
to
+22
| if path.contains("..") { | ||
| return Err("Path traversal denied".into()); | ||
| } |
Contributor
There was a problem hiding this comment.
Path traversal check may be bypassable with encoded or alternate sequences.
The string-based path.contains("..") check doesn't catch URL-encoded sequences like %2e%2e or alternate traversal patterns. While the canonicalize + starts_with check on lines 28-30 provides a secondary defense, consider normalizing the path first or rejecting any non-canonical-looking inputs.
🛡️ Suggested hardening
+ // Reject obviously suspicious patterns before path normalization
+ let decoded = urlencoding::decode(path).unwrap_or(std::borrow::Cow::Borrowed(path));
+ if decoded.contains("..") || path.contains("..") {
return Err("Path traversal denied".into());
}Or rely solely on the canonicalize check and remove the string check to avoid false confidence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src-tauri/src/workspace.rs` around lines 20 - 22, The current path
traversal check uses path.contains("..") which can be bypassed by encoded or
alternate sequences; update the validation to either (A) percent-decode and
normalize the incoming path before checking (i.e. decode percent-encodings,
collapse components, then check for any ".." segments and reject), or (B) remove
the naive contains check and rely solely on std::fs::canonicalize +
starts_with(root) (the existing canonicalize and starts_with calls) to determine
if the resolved path is inside the workspace; implement one of these approaches
and adjust the code around the path.contains("..") check accordingly (use a
decode/normalize helper or remove the check and keep the
canonicalize/starts_with logic).
Member
Author
|
@jules fix the typecheck issue. |
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.
This PR makes the Tauri frontend properly handle workspace file links and views. It achieves this by:
open_workspace_path,reveal_workspace_path, andread_workspace_file_stringin a new moduleworkspace.rs.workspaceLinks.tsutility.AgentMessageBubble.tsx) to allow intentional local workspace links rather than rejecting them.PR created automatically by Jules for task 7537295491042602194 started by @senamakel
Summary by CodeRabbit