Implement shareable links#16
Merged
warunalakshitha merged 4 commits intoballerina-platform:mainfrom Mar 30, 2026
Merged
Conversation
snelusha
force-pushed
the
feat/shareable-links
branch
from
4d5d04a to
b157030
Compare
|
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:
📝 WalkthroughWalkthroughMounts shared example trees under Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User
participant Sidebar as "App Sidebar"
participant HookCopy as "useCopyShareLink"
participant ShareLib as "share.ts"
participant FS as "LayeredFS / EphemeralFS"
participant Clipboard as "Navigator.clipboard"
participant Router as "Router / useShare"
participant Store as "FileTreeStore"
Sidebar->>HookCopy: copyShareLink(nodePath)
HookCopy->>ShareLib: generateShareUrl(fs, nodePath, activeFile)
ShareLib->>FS: toFileNode(fs, nodePath)
FS-->>ShareLib: FileNode root
ShareLib->>ShareLib: encodeSharePayload(root, openRelativePath)
ShareLib-->>HookCopy: share URL
HookCopy->>Clipboard: writeText(url)
Clipboard-->>HookCopy: success
HookCopy-->>Sidebar: show success toast
RemoteUser->>Router: Open URL with ?share=...
Router->>Router: validate & normalize `share`
Router->>Router: useShare decodes payload
Router->>FS: graftSharedTree(root, openRelativePath)
FS-->>Store: graft mounted under /tmp/shared
Router->>Store: loadSharedFiles(...) -> open file
Router-->>Sidebar: show ShareNoticeDialog
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 11
🧹 Nitpick comments (2)
web/src/styles.css (1)
154-159: Prefer not to applyfont-sanson the universal selector.
font-sanson*(Line 155) is broader than needed and duplicates Line 158. Keeping font assignment onbodyavoids global cascade side effects.Suggested diff
`@layer` base { * { - `@apply` border-border outline-ring/50 font-sans; + `@apply` border-border outline-ring/50; } body { `@apply` font-sans bg-background text-foreground; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/styles.css` around lines 154 - 159, Remove the font-sans application from the universal selector to avoid broad cascade side effects: in the rule for "*" remove "font-sans" from the `@apply` list and keep the "body" selector's `@apply` font-sans bg-background text-foreground as the single source for the base font; ensure only border-border and outline-ring/50 remain on "*" so typography is controlled solely by the body rule.web/src/lib/fs/core/file-node-utils.ts (1)
33-35: Consider usingjoin()for path construction consistency.Line 34 uses string interpolation
${path}/${entry.name}while thejoinfunction is already imported and used elsewhere in this file (lines 47, 54, 55). Usingjoinwould ensure consistent path normalization.Suggested change
const children = (fs.readDir(path) ?? []) - .map((entry) => toFileNode(fs, `${path}/${entry.name}`)) + .map((entry) => toFileNode(fs, join(path, entry.name))) .filter((n): n is FileNode => n !== null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/fs/core/file-node-utils.ts` around lines 33 - 35, The code builds child paths with string interpolation in the children creation block; replace `${path}/${entry.name}` with the existing join function to ensure consistent path normalization—update the map that calls toFileNode(fs, `${path}/${entry.name}`) to use join(path, entry.name) (keeping the surrounding fs.readDir and toFileNode usage intact) so path handling matches other uses in this module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/app-sidebar.tsx`:
- Around line 145-168: The "Fork" action currently calls renameFile which
performs a cross-layer move (via LayeredFS._moveToTarget) and deletes the shared
source; change the UI handlers that currently call renameFile (the
DropdownMenuItem fork handlers that call saveFile(),
sharedToLocalDestination(path), exists(dest), renameFile(path, dest),
expandDir(dirname(dest)), and the similar block elsewhere) to instead call a
dedicated copy/fork function (e.g., forkFile or copyFileAcrossLayers) that
copies the file from the shared layer to the local layer without removing the
source; implement or call a copy operation in LayeredFS that performs copy
semantics (not _moveToTarget) and ensure the UI uses basename(path) for
defaultName and still calls expandDir(dirname(dest)) after a successful copy,
and replace renameFile(path, dest) invocations with the new copy API to preserve
the shared source.
In `@web/src/components/file-tree-dialog.tsx`:
- Around line 219-225: The dialog is being closed unconditionally after the fork
branch, which discards the user's input when renameFile(...) returns false;
change the control flow in the "fork-file"/"fork-folder" handling so that
close() is only called when renameFile(path, targetPath) returns true (i.e.,
success); keep the dialog open on failure (and still call
expandDir(dirname(targetPath)) only on success), by moving the close() call
inside the success branch and not invoking it when renameFile returns false.
- Around line 219-221: The fork handlers currently call renameFile(path,
targetPath) which uses the FS move implementation (and LayeredFS._moveToTarget)
that deletes the source when moving across layers; change the fork logic to
perform a copy instead of a move: add or use an FS copy method (e.g.,
copyFile/copyEntry) on your filesystem interface and call that from the
"fork-file" and "fork-folder" branches (replace renameFile with the new copy
API, then call expandDir(dirname(targetPath))); alternatively, if adding a copy
API is not feasible, special-case LayeredFS to expose a non-destructive copy
operation and invoke that from the fork handlers so shared nodes are duplicated
rather than removed.
In `@web/src/components/ui/sonner.tsx`:
- Around line 66-70: The toastOptions.classNames.toast references a CSS class
named "cn-toast" that doesn't exist; either add a .cn-toast rule to your global
stylesheet (styles.css) with the desired toast styles (positioning, padding,
background, border-radius, etc.) or remove the toastOptions.classNames.toast
entry from the toastOptions object in web/src/components/ui/sonner.tsx if no
custom styling is required; locate the toastOptions object in the Sonner
component and update it accordingly.
In `@web/src/hooks/use-copy-share-link.ts`:
- Around line 14-24: The current copyShareLink callback silently returns when
generateShareUrl(fs, nodePath, activeFilePath) yields a falsy value; instead
detect that failure and surface it to the user via toast (e.g.,
toast.error("Could not generate share link") plus any available error details),
so replace the early `if (!url) return;` with logic that shows an error toast
and returns; keep the existing clipboard success/error flow unchanged and
reference the same symbols (copyShareLink, generateShareUrl, fs, activeFilePath,
toast).
In `@web/src/lib/fs/core/path-utils.ts`:
- Around line 70-83: getForkTargetPath currently trims and rejects slashes from
newBasename but allows "." or "..", which can produce unsafe paths; update
getForkTargetPath to validate newBasename against path traversal by either
calling isSafeRelativePath(newBasename) (or explicitly rejecting newBasename ===
"." || newBasename === "..") before proceeding, and return null if the check
fails so the later logic that uses getRelativePath(SHARED_ROOT,
sourceSharedPath), pathSegments(rel), and join(LOCAL_ROOT, ...) never constructs
a path containing "." or "..".
In `@web/src/lib/fs/ephemeral-fs.ts`:
- Around line 11-12: insertSubtree currently calls this._seed([root],
parentPrefix) without ensuring the destination prefix exists, which causes
writeFile to fail for file roots; modify insertSubtree to first ensure/create
the parentPrefix directory before seeding the root (e.g., create the directory
nodes for parentPrefix or call the class's mkdir/ensure-directory helper to
create the prefix), then call this._seed([root], parentPrefix) so files under
root are written into an existing directory.
In `@web/src/lib/share.ts`:
- Around line 35-49: The isFileNode type guard currently accepts any string for
FileNode.name; update isFileNode to reject unsafe names by ensuring o.name is a
non-empty string that is not "." or ".." and does not contain path separators
(e.g., '/' or '\\') or NUL, before accepting file or dir nodes. Keep the
existing children validation (Array.isArray(o.children) &&
o.children.every(isFileNode)) and apply the same name check for both the "file"
and "dir" branches so that only single safe path segments are accepted when
grafting URL data.
- Around line 15-23: The compressAndEncode function currently spreads the entire
compressed Uint8Array into String.fromCharCode, which will throw RangeError for
large payloads; update compressAndEncode to convert the Uint8Array to a binary
string in safe-sized chunks (e.g., 32k–64k bytes per chunk), iterating over the
Uint8Array returned from new Uint8Array(compressed), converting each slice to a
string (using String.fromCharCode on each chunk or another safe conversion) and
concatenating the chunk strings, then call btoa on the combined binary string;
keep the function name compressAndEncode and the existing steps (TextEncoder,
CompressionStream, Response(stream.readable).arrayBuffer()) but replace the
single spread conversion with chunked conversion to avoid engine argument
limits.
In `@web/src/main.tsx`:
- Around line 30-34: The app root rendering currently mounts <RouterProvider
router={router} /> and <Toaster /> directly under React StrictMode; because
Toaster calls useTheme() from next-themes you must wrap the tree with
ThemeProvider from next-themes so theme state is provided. Import ThemeProvider
and wrap the components (e.g., put <ThemeProvider> as the parent of
<RouterProvider ... /> and <Toaster />), optionally passing common props like
attribute="class" or defaultTheme if your app uses class-based theming, so theme
detection and switching work for the Toaster.
In `@web/src/stores/file-tree-store.ts`:
- Around line 296-310: The function loadSharedFiles currently treats a
successful graft (mounting) as a failure if graftSharedTree(root,
openRelativePath) returns no path; change its return type and behavior to return
a richer result object (e.g. { loaded: boolean, openPath: string | null }) so
callers can distinguish "shared subtree mounted but nothing to open" from an
actual failure. Specifically, keep the graft call to _fs().graftSharedTree and
the state updates to s.tempTree = _fs().tempTree() and s.localTree =
_fs().localTree(), then return { loaded: true, openPath: openPath } when graft
succeeded (even if openPath is null) and { loaded: false, openPath: null } when
graft failed; update callers of loadSharedFiles (that expect a boolean) to
handle the new object and call get().openFile(openPath) only when openPath is
not null.
---
Nitpick comments:
In `@web/src/lib/fs/core/file-node-utils.ts`:
- Around line 33-35: The code builds child paths with string interpolation in
the children creation block; replace `${path}/${entry.name}` with the existing
join function to ensure consistent path normalization—update the map that calls
toFileNode(fs, `${path}/${entry.name}`) to use join(path, entry.name) (keeping
the surrounding fs.readDir and toFileNode usage intact) so path handling matches
other uses in this module.
In `@web/src/styles.css`:
- Around line 154-159: Remove the font-sans application from the universal
selector to avoid broad cascade side effects: in the rule for "*" remove
"font-sans" from the `@apply` list and keep the "body" selector's `@apply` font-sans
bg-background text-foreground as the single source for the base font; ensure
only border-border and outline-ring/50 remain on "*" so typography is controlled
solely by the body rule.
🪄 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: e297ac71-2fab-4c53-a44b-bb6c3c10b4d3
⛔ Files ignored due to path filters (1)
web/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
scripts/example_gen/main.goweb/package.jsonweb/src/assets/examples.jsonweb/src/components/app-sidebar.tsxweb/src/components/file-route-sync.tsxweb/src/components/file-tree-dialog.tsxweb/src/components/share-notice-dialog.tsxweb/src/components/ui/checkbox.tsxweb/src/components/ui/sonner.tsxweb/src/hooks/use-copy-share-link.tsweb/src/hooks/use-share-notice.tsweb/src/hooks/use-share.tsweb/src/lib/file-tree-utils.tsweb/src/lib/fs/core/file-node-utils.tsweb/src/lib/fs/core/path-utils.tsweb/src/lib/fs/ephemeral-fs.tsweb/src/lib/fs/fs-roots.tsweb/src/lib/fs/layered-fs.tsweb/src/lib/share.tsweb/src/main.tsxweb/src/routes/$.tsxweb/src/stores/file-tree-store.tsweb/src/styles.css
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
web/src/lib/share.ts (2)
19-20:⚠️ Potential issue | 🟠 MajorAwait stream writes/closes to avoid silent share payload corruption.
Line 19/Line 20 and Line 37/Line 38 drop
write/closepromises; stream failures can be swallowed and produce invalid payloads.Suggested fix
async function compressAndEncode(text: string): Promise<string> { const bytes = new TextEncoder().encode(text); const stream = new CompressionStream("gzip"); const writer = stream.writable.getWriter(); - void writer.write(bytes); - void writer.close(); + await writer.write(bytes); + await writer.close(); const compressed = new Uint8Array( await new Response(stream.readable).arrayBuffer(), ); @@ async function decodeAndDecompress(b64: string): Promise<string> { const bytes = Uint8Array.from(atob(b64), (c) => c.charCodeAt(0)); const stream = new DecompressionStream("gzip"); const writer = stream.writable.getWriter(); - void writer.write(bytes); - void writer.close(); + await writer.write(bytes); + await writer.close(); const decompressed = await new Response(stream.readable).arrayBuffer(); return new TextDecoder().decode(decompressed); }For WritableStreamDefaultWriter, is it safe to call writer.write(...) and writer.close() without awaiting either promise when consuming the paired readable via Response(...).arrayBuffer(), and what failure modes can be missed?Also applies to: 37-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/share.ts` around lines 19 - 20, Await the WritableStream writer promises instead of dropping them: replace the calls to writer.write(bytes) and writer.close() (and the other occurrence at the second writer usage) with awaited calls (e.g., await writer.write(bytes); await writer.close();) and wrap them in a try/catch to propagate or log stream errors so write/close failures aren’t swallowed; refer to the writer variable and its write/close calls in this file to locate both spots.
43-50:⚠️ Potential issue | 🟠 MajorHarden node-name validation against
\and NUL characters.
isSafeNodeNamestill accepts names containing backslash and NUL, which weakens the single-segment safety boundary for URL-sourced trees.Suggested fix
function isSafeNodeName(name: unknown): name is string { return ( typeof name === "string" && name.length > 0 && name !== "." && name !== ".." && - !name.includes("/") + !name.includes("/") && + !name.includes("\\") && + !name.includes("\0") ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/share.ts` around lines 43 - 50, The isSafeNodeName type guard currently allows backslash and NUL characters; update isSafeNodeName to reject names containing "\\" and the NUL character (e.g. "\0") in addition to existing checks so that any string with backslash or NUL returns false; locate the function isSafeNodeName and add checks like !name.includes("\\") and !name.includes("\0") (or equivalent indexOf checks) to harden single-segment validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/routes/__root.tsx`:
- Line 1: Replace direct import of Toaster from "sonner" with the app-level
wrapped Toaster export so theme context from your wrapper is applied; locate the
import statement that brings in Toaster (and any other occurrences of Toaster
usage in this file) and change its module source to the project's UI sonner
wrapper export (the component that wraps Sonner and uses useTheme()), ensuring
you only import the wrapped Toaster symbol used in __root.tsx (update both the
initial import and the duplicate occurrence near the later usage).
---
Duplicate comments:
In `@web/src/lib/share.ts`:
- Around line 19-20: Await the WritableStream writer promises instead of
dropping them: replace the calls to writer.write(bytes) and writer.close() (and
the other occurrence at the second writer usage) with awaited calls (e.g., await
writer.write(bytes); await writer.close();) and wrap them in a try/catch to
propagate or log stream errors so write/close failures aren’t swallowed; refer
to the writer variable and its write/close calls in this file to locate both
spots.
- Around line 43-50: The isSafeNodeName type guard currently allows backslash
and NUL characters; update isSafeNodeName to reject names containing "\\" and
the NUL character (e.g. "\0") in addition to existing checks so that any string
with backslash or NUL returns false; locate the function isSafeNodeName and add
checks like !name.includes("\\") and !name.includes("\0") (or equivalent indexOf
checks) to harden single-segment validation.
🪄 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: d8af2ede-63c6-4c7d-a9a8-0a259c371e9d
📒 Files selected for processing (6)
web/src/components/file-tree-dialog.tsxweb/src/hooks/use-copy-share-link.tsweb/src/lib/fs/core/path-utils.tsweb/src/lib/share.tsweb/src/main.tsxweb/src/routes/__root.tsx
✅ Files skipped from review due to trivial changes (2)
- web/src/main.tsx
- web/src/lib/fs/core/path-utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/file-tree-dialog.tsx
snelusha
force-pushed
the
feat/shareable-links
branch
from
a44c2dc to
d4ec5f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7b81942-cc19-48bb-9b77-c4bac7a768be
📒 Files selected for processing (6)
web/src/components/file-tree-dialog.tsxweb/src/hooks/use-copy-share-link.tsweb/src/lib/fs/core/path-utils.tsweb/src/lib/share.tsweb/src/main.tsxweb/src/routes/__root.tsx
✅ Files skipped from review due to trivial changes (1)
- web/src/main.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/routes/__root.tsx
- web/src/hooks/use-copy-share-link.ts
- web/src/components/file-tree-dialog.tsx
- web/src/lib/share.ts
snelusha
force-pushed
the
feat/shareable-links
branch
from
d4ec5f6 to
0644367
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/src/lib/share.ts (1)
43-51: Consider rejecting backslash in node names.
isSafeNodeNamevalidates against/but not\. While the in-memory FS likely normalizes paths, a node name containing backslash could cause issues if the code ever interacts with Windows-style paths or is serialized/deserialized across platforms.Suggested fix
function isSafeNodeName(name: unknown): name is string { return ( typeof name === "string" && name.length > 0 && name !== "." && name !== ".." && - !name.includes("/") + !name.includes("/") && + !name.includes("\\") ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/share.ts` around lines 43 - 51, The isSafeNodeName type guard currently blocks "/" but allows "\" which can introduce Windows-style path separators; update isSafeNodeName to also reject backslash characters by adding a check that the name does not include "\\" (i.e., ensure !name.includes("\\")). Keep the other guards (non-empty string, not "." or "..") intact so the function (isSafeNodeName) consistently forbids both forward and backward slashes across platforms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/hooks/use-share.ts`:
- Around line 33-64: The async decodeSharePayload flow can apply stale results
and make isProcessingShare inconsistent; wrap the effect's async work with a
cancellation guard (e.g. let cancelled = false or an AbortController) inside the
useEffect and return a cleanup that sets cancelled=true so the promise callback
returns early if cancelled or if share changed, and only call loadSharedFiles,
openFile, dropShareParam, and assign processed.current when not cancelled and
the local captured share matches the current share; also replace the synchronous
isProcessingShare check with a guarded state/ref (e.g. processingRef or
processing state) updated at effect start and cleared only after the guarded
completion to avoid transient false negatives.
In `@web/src/lib/share.ts`:
- Around line 118-132: generateShareUrl can produce very large query payloads
because toFileNode serializes whole subtrees; after calling
encodeSharePayload(root, openRelativePath) validate the encoded payload length
against a configured max (e.g. safeLimitBytes = 2048 and hardLimitBytes = 8192)
and handle oversized payloads (return null or throw an explicit error) before
calling appendShareParam. Update generateShareUrl to use the encoded result from
encodeSharePayload, check its byte length (or base64 length) and fail fast with
a clear outcome (or alternative flow) when limits are exceeded so
appendShareParam is never given an oversized value. Ensure references to
generateShareUrl, toFileNode, encodeSharePayload, and appendShareParam are used
so the change is easy to locate.
---
Nitpick comments:
In `@web/src/lib/share.ts`:
- Around line 43-51: The isSafeNodeName type guard currently blocks "/" but
allows "\" which can introduce Windows-style path separators; update
isSafeNodeName to also reject backslash characters by adding a check that the
name does not include "\\" (i.e., ensure !name.includes("\\")). Keep the other
guards (non-empty string, not "." or "..") intact so the function
(isSafeNodeName) consistently forbids both forward and backward slashes across
platforms.
🪄 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: e399ae6c-10da-4ee3-8bb8-f3ee9b516866
📒 Files selected for processing (8)
web/src/components/file-tree-dialog.tsxweb/src/hooks/use-copy-share-link.tsweb/src/hooks/use-share.tsweb/src/lib/fs/core/path-utils.tsweb/src/lib/share.tsweb/src/main.tsxweb/src/routes/__root.tsxweb/src/stores/file-tree-store.ts
✅ Files skipped from review due to trivial changes (1)
- web/src/main.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/routes/__root.tsx
- web/src/stores/file-tree-store.ts
- web/src/hooks/use-copy-share-link.ts
snelusha
force-pushed
the
feat/shareable-links
branch
from
0644367 to
94917a0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/src/stores/file-tree-store.ts (1)
299-313: The try/catch is unreachable sincegraftSharedTreedoesn't throw.Per
web/src/lib/fs/layered-fs.ts(lines 76-90),graftSharedTreereturnsstring | nulland doesn't throw exceptions. The catch block will never execute. Consider removing the try/catch or checking fornullreturn explicitly:Proposed simplification
loadSharedFiles( root: FileNode, openRelativePath?: string | null, ): { loaded: boolean; openPath: string | null } { - try { - const openPath = _fs().graftSharedTree(root, openRelativePath); - set((s) => { - s.tempTree = _fs().tempTree(); - s.localTree = _fs().localTree(); - }); - return { loaded: true, openPath }; - } catch { - return { loaded: false, openPath: null }; - } + const openPath = _fs().graftSharedTree(root, openRelativePath); + set((s) => { + s.tempTree = _fs().tempTree(); + s.localTree = _fs().localTree(); + }); + return { loaded: true, openPath }; },Note: If you want to handle failures (e.g., if
graftSharedTreeis later modified to throw), keep the try/catch. Otherwise, the current code always returnsloaded: true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/file-tree-store.ts` around lines 299 - 313, The try/catch in loadSharedFiles is unreachable because graftSharedTree returns string|null and doesn't throw; change loadSharedFiles to call const openPath = _fs().graftSharedTree(root, openRelativePath) and then check if openPath === null to treat it as a failure (return { loaded: false, openPath: null }), otherwise update state via set(...) using _fs().tempTree() and _fs().localTree() and return { loaded: true, openPath }; remove the try/catch block so the behavior matches graftSharedTree's contract (or keep it only if you intentionally want to guard against future throws).web/src/lib/share.ts (1)
43-51: Consider blocking backslash and NUL characters in node names.
isSafeNodeNamecorrectly blocks/, but untrusted payloads could contain\(Windows-style separator) or NUL bytes that may cause issues when the name is later used in path operations or displayed. Adding these checks hardens the validation.Proposed validation tightening
function isSafeNodeName(name: unknown): name is string { return ( typeof name === "string" && name.length > 0 && name !== "." && name !== ".." && - !name.includes("/") + !name.includes("/") && + !name.includes("\\") && + !name.includes("\0") ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/share.ts` around lines 43 - 51, The isSafeNodeName type guard currently rejects "/" and path dots but still allows backslash and NUL characters; update the isSafeNodeName function to also reject names containing "\\" and the NUL character ("\0") by adding checks like !name.includes("\\") and !name.includes("\0") so node names cannot include Windows path separators or embedded NULs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/src/lib/share.ts`:
- Around line 43-51: The isSafeNodeName type guard currently rejects "/" and
path dots but still allows backslash and NUL characters; update the
isSafeNodeName function to also reject names containing "\\" and the NUL
character ("\0") by adding checks like !name.includes("\\") and
!name.includes("\0") so node names cannot include Windows path separators or
embedded NULs.
In `@web/src/stores/file-tree-store.ts`:
- Around line 299-313: The try/catch in loadSharedFiles is unreachable because
graftSharedTree returns string|null and doesn't throw; change loadSharedFiles to
call const openPath = _fs().graftSharedTree(root, openRelativePath) and then
check if openPath === null to treat it as a failure (return { loaded: false,
openPath: null }), otherwise update state via set(...) using _fs().tempTree()
and _fs().localTree() and return { loaded: true, openPath }; remove the
try/catch block so the behavior matches graftSharedTree's contract (or keep it
only if you intentionally want to guard against future throws).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d424ebf8-8ae2-47fb-a673-c003e0c8d2c8
📒 Files selected for processing (8)
web/src/components/file-tree-dialog.tsxweb/src/hooks/use-copy-share-link.tsweb/src/hooks/use-share.tsweb/src/lib/fs/core/path-utils.tsweb/src/lib/share.tsweb/src/main.tsxweb/src/routes/__root.tsxweb/src/stores/file-tree-store.ts
✅ Files skipped from review due to trivial changes (1)
- web/src/main.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/routes/__root.tsx
- web/src/hooks/use-copy-share-link.ts
- web/src/lib/fs/core/path-utils.ts
snelusha
force-pushed
the
feat/shareable-links
branch
from
94917a0 to
df54033
Compare
| break; | ||
| case "fork-file": | ||
| case "fork-folder": | ||
| if (!renameFile(path, targetPath)) return; |
Contributor
There was a problem hiding this comment.
this is unclear. if we are forking (means copy) why rename?
Contributor
Author
There was a problem hiding this comment.
Yes, shared files are loaded into temp, and fork currently moves that node to local space. Because it’s a move into local, name conflicts can happen if that name already exists.
snelusha
force-pushed
the
feat/shareable-links
branch
from
1b74150 to
d719e65
Compare
snelusha
force-pushed
the
feat/shareable-links
branch
from
530346a to
4147385
Compare
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.
Purpose
Resolves #8
Summary
Implements shareable playground links that serialize a playground subtree (files and directories) and an optional open path into a compressed, base64 query parameter so users can share and open temporary playground states via URL.
Key changes
Share payload encoding/decoding
sharequery parameter and validate/normalize incomingsharevalues.Filesystem and tree integration
UI and UX
Routing, store, and interactions
sharesearch parameter.fork-file/fork-folderdialog operation kinds.Examples and assets
/tmp/examples(examples JSON restructured and example_gen tool adjusted accordingly).Dependencies
Notable added/updated files
Intent / outcome
Enable users to create and open temporary, shareable playground states via URL while integrating the shared content into the in-memory filesystem and providing UI affordances (share/fork, notice, toasts) for a smoother sharing and collaboration experience.