feat: add secure folder share links - #54
Open
bmdavis419 wants to merge 4 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds durable folder sharing with expiring grants, scoped folder/video browsing, signed playback, dashboard sharing controls, public routing, deletion invalidation, and extensive Convex tests. ChangesFolder Sharing Feature
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ProjectDashboard
participant FolderShareDialog
participant folderShares
participant FolderSharePage
participant getFolderSharedPlaybackSession
participant VideoPlayer
ProjectDashboard->>FolderShareDialog: open folder sharing
FolderShareDialog->>folderShares: create or revoke durable link
FolderSharePage->>folderShares: issue access grant
FolderSharePage->>folderShares: load folders, videos, and comments
FolderSharePage->>getFolderSharedPlaybackSession: claim and sign video playback
getFolderSharedPlaybackSession-->>FolderSharePage: signed URLs and expiry
FolderSharePage->>VideoPlayer: attach signed playback
VideoPlayer-->>FolderSharePage: report readiness
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
convex/folderShareAccess.ts (1)
48-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the grant-token length as one constant to avoid generation/validation drift.
The generated token length is hardcoded as
40here, whileresolveSharedFolder/resolveSharedVideoinconvex/folderShares.tsreject tokens whose length!== FOLDER_SHARE_GRANT_TOKEN_LENGTH(also40, but defined independently). If either value is changed without the other, every issued grant would fail the length check and all shared reads would silently resolve tonull— breaking access with no error. Define the length once and reuse it in both modules.♻️ Proposed fix
export const FOLDER_SHARE_ACCESS_GRANT_TTL_MS = 60 * 60 * 1000; export const FOLDER_SHARE_ANCESTRY_WALK_LIMIT = 12; +export const FOLDER_SHARE_GRANT_TOKEN_LENGTH = 40;const token = await generateUniqueToken( - 40, + FOLDER_SHARE_GRANT_TOKEN_LENGTH, async (candidate) =>Then in
convex/folderShares.ts, import and drop the local duplicate:import { FOLDER_SHARE_ANCESTRY_WALK_LIMIT, + FOLDER_SHARE_GRANT_TOKEN_LENGTH, findFolderShareLinkByToken, ... } from "./folderShareAccess"; -const FOLDER_SHARE_GRANT_TOKEN_LENGTH = 40;🤖 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 `@convex/folderShareAccess.ts` around lines 48 - 72, The token length is duplicated between issueFolderShareAccessGrant and the length check in resolveSharedFolder/resolveSharedVideo, so unify it into a single shared constant to prevent drift. Define or reuse FOLDER_SHARE_GRANT_TOKEN_LENGTH as the source of truth in convex/folderShareAccess.ts for generateUniqueToken and import that same constant into convex/folderShares.ts, removing the local duplicate so both issuance and validation stay aligned.app/routes/-folder-share.data.ts (1)
87-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate auto-load-more effect logic.
The folders (Lines 87-98) and videos (Lines 100-111) auto-load effects are structurally identical modulo the collection name and the load/status/ref identifiers. Extracting a small shared helper (e.g.
useAutoLoadMore({ status, folderKey, collection, pageSize, memory, countRef, loadMore })) would remove the duplication and reduce the risk of the two copies drifting apart.🤖 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/routes/-folder-share.data.ts` around lines 87 - 111, The folders and videos auto-load-more effects are duplicated in the same component and should be consolidated. Extract the shared logic from the two useEffect blocks into a small helper or reusable hook in the same module, parameterized by folderKey, status, pageSize, the pagination memory ref, the restored count ref, and the loadMore callback. Keep the behavior identical for both folders and videos while removing the near-copy/paste logic.src/lib/convexRouteData.test.ts (1)
33-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the actually risky path.
Both new tests only exercise the custom-
dedupeKeypath. Per the concern raised inconvexRouteData.ts(makeRouteQuerySpec/prewarmSpecs), the unprotected path is the default key (nodedupeKey) with sensitive args — that's the one that currently leaks intoconsole.warn. A test asserting that scenario is not yet safe (or asserting it is fixed, once addressed) would give real regression coverage.🤖 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 `@src/lib/convexRouteData.test.ts` around lines 33 - 68, The current tests only cover the custom dedupeKey path, but the risky case is the default key path in makeRouteQuerySpec/prewarmSpecs when no dedupeKey is provided and sensitive args are used. Add a test that exercises this default path with a bearer-like argument and verifies whether the warning from prewarmSpecs/console.warn does not leak the sensitive value (or documents the current leak if that path is still unfixed). Use the existing symbols makeRouteQuerySpec, prewarmSpecs, and resetPrewarmDedupeForTests to keep the test aligned with the implementation.
🤖 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/routes/-folder-share.tsx`:
- Around line 297-314: The shared acquireGrant flow currently treats every
failure as fatal by setting grantError, which causes ShareUnavailable to replace
the entire page even when a background renewal fails. Update acquireGrant and
its callers so only the initial grant fetch can hard-fail the view, while
periodic renewals surface a non-blocking error state or retry without clearing
already loaded folder/video content. Use the existing grantRequestPendingRef,
setGrantError, and grantToken handling to distinguish initial load from renewal,
and ensure the render path that shows ShareUnavailable is only triggered for the
true bootstrap failure case.
- Around line 512-521: The breadcrumb SharedFolderLink usages currently rely on
hover:underline only, which leaves links underlined only on hover instead of by
default. Update both breadcrumb link instances in the folder share route,
including the folder-view breadcrumb, so SharedFolderLink always has an
underline while keeping the existing hover styling. Use the SharedFolderLink
component in the breadcrumb render blocks as the place to apply the class
change.
- Around line 321-330: The expiry timer in the folder-share flow is clearing the
grant state, which forces the UI to reset and re-bootstrap playback when the
token expires. Update the effect around the grant expiry handling in the
folder-share route so the timeout does not call setGrantToken(null) or otherwise
trigger the full-screen loading path; instead, keep the existing session active
and only refresh or renew the grant state in place. Use the
grantExpiresAt/grantToken effect and the loadPlayback path as the main places to
adjust.
In `@src/lib/convexRouteData.ts`:
- Around line 21-32: The default key generation in makeRouteQuerySpec still
serializes raw args via buildQueryKey(getFunctionName(query), args), so those
values can leak through console.warn even when redactErrorDetails is enabled.
Change the RouteQuerySpec construction so redaction is explicit for callers
without a dedupeKey, or replace the fallback key with a non-sensitive identifier
that does not embed args. Update makeRouteQuerySpec and any callers that rely on
the default key behavior to ensure prewarm failures never log sensitive request
data.
---
Nitpick comments:
In `@app/routes/-folder-share.data.ts`:
- Around line 87-111: The folders and videos auto-load-more effects are
duplicated in the same component and should be consolidated. Extract the shared
logic from the two useEffect blocks into a small helper or reusable hook in the
same module, parameterized by folderKey, status, pageSize, the pagination memory
ref, the restored count ref, and the loadMore callback. Keep the behavior
identical for both folders and videos while removing the near-copy/paste logic.
In `@convex/folderShareAccess.ts`:
- Around line 48-72: The token length is duplicated between
issueFolderShareAccessGrant and the length check in
resolveSharedFolder/resolveSharedVideo, so unify it into a single shared
constant to prevent drift. Define or reuse FOLDER_SHARE_GRANT_TOKEN_LENGTH as
the source of truth in convex/folderShareAccess.ts for generateUniqueToken and
import that same constant into convex/folderShares.ts, removing the local
duplicate so both issuance and validation stay aligned.
In `@src/lib/convexRouteData.test.ts`:
- Around line 33-68: The current tests only cover the custom dedupeKey path, but
the risky case is the default key path in makeRouteQuerySpec/prewarmSpecs when
no dedupeKey is provided and sensitive args are used. Add a test that exercises
this default path with a bearer-like argument and verifies whether the warning
from prewarmSpecs/console.warn does not leak the sensitive value (or documents
the current leak if that path is still unfixed). Use the existing symbols
makeRouteQuerySpec, prewarmSpecs, and resetPrewarmDedupeForTests to keep the
test aligned with the implementation.
🪄 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: 9916d080-1da4-4e84-b3bb-c0b5d9241c38
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (22)
app/routeTree.gen.tsapp/routes/-folder-share.data.tsapp/routes/-folder-share.tsxapp/routes/dashboard/-project.tsxapp/routes/folder-share.$token.tsxconvex/crons.tsconvex/folderShareAccess.tsconvex/folderShares.tsconvex/folderShares.vitest.tsconvex/mux.tsconvex/projects.tsconvex/schema.tsconvex/teams.tsconvex/videoActions.tssrc/components/projects/FolderShareDialog.tsxsrc/components/video-player/VideoPlayer.tsxsrc/lib/convexRouteData.test.tssrc/lib/convexRouteData.tssrc/lib/folderShareNavigation.test.tssrc/lib/folderShareNavigation.tssrc/lib/routes.test.tssrc/lib/routes.ts
Align the folder share system with the existing single-video share patterns instead of introducing parallel machinery: - Use the same 24h access-grant TTL as video shares and drop the in-place renewal mutation, its three rate-limit buckets, and the client renewal/retention loop. Revocation stays instant because every public read re-loads the durable link. - Remove the bespoke focus-restore and pagination-memory navigation layer; no other page in the app restores focus or page depth. - Remove the shadow first-page queries that duplicated each usePaginatedQuery subscription during initial load. - Drop the unused RouteQuerySpec.redactErrorDetails field and document why buildQueryKey fingerprints serialized args. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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/routes/-folder-share.tsx (2)
249-273: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDiscard grant responses from obsolete route tokens.
Resetting
grantRequestPendingRefallows the new token request to overlap the old one, but either response can subsequently setgrantToken. If the old request resolves last, the page queries and displays the previous share under the new URL.Proposed request-sequence guard
const grantRequestPendingRef = useRef(false); +const grantRequestSequenceRef = useRef(0); useEffect(() => { + grantRequestSequenceRef.current += 1; setGrantToken(null); setHasAttemptedGrant(false); setGrantError(false); grantRequestPendingRef.current = false; }, [token]); const acquireGrant = useCallback(async () => { if (grantRequestPendingRef.current) return; + const requestSequence = grantRequestSequenceRef.current + 1; + grantRequestSequenceRef.current = requestSequence; grantRequestPendingRef.current = true; setHasAttemptedGrant(true); setGrantError(false); try { const result = await issueAccessGrant({ token }); + if (requestSequence !== grantRequestSequenceRef.current) return; if (result.ok && result.grantToken) { setGrantToken(result.grantToken); } else { setGrantError(true); } } catch { - setGrantError(true); + if (requestSequence === grantRequestSequenceRef.current) { + setGrantError(true); + } } finally { - grantRequestPendingRef.current = false; + if (requestSequence === grantRequestSequenceRef.current) { + grantRequestPendingRef.current = false; + } } }, [issueAccessGrant, token]);🤖 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/routes/-folder-share.tsx` around lines 249 - 273, Guard grant responses in acquireGrant against the route token that initiated the request, so results from obsolete tokens cannot update grantToken or grantError after token changes. Capture the current token/request sequence before issueAccessGrant and apply state updates only when it still matches the active token, while preserving the existing pending reset and error behavior.
447-449: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUnderline the remaining plain navigation links by default.
The two
lawn.links and footer link are only visually emphasized through hover color. Addunderlineto their base classes.As per coding guidelines, “Links should use underlines, not color-only differentiation” and “Do not hide information behind hover states.”
Also applies to: 633-635, 768-772
🤖 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/routes/-folder-share.tsx` around lines 447 - 449, Update the Link elements containing the “lawn.” branding and the footer link to include the underline utility in their base className, covering the instances near the shown Link and the additional locations noted in the comment. Preserve their existing hover styling and navigation targets.Source: Coding guidelines
🧹 Nitpick comments (2)
src/lib/convexRouteData.ts (1)
27-29: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCanonicalize arguments before fingerprinting.
JSON.stringifyis insertion-order-sensitive, so equivalent argument objects with different key order produce different fingerprints and bypassprewarmSpecsdeduplication. Use a canonical serializer that recursively sorts object keys, and add a reversed-key-order test.🤖 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 `@src/lib/convexRouteData.ts` around lines 27 - 29, Update buildQueryKey to canonicalize convexToJson arguments before fingerprinting by recursively sorting object keys, ensuring equivalent objects with different insertion order produce the same serialized representation. Add a test covering reversed key order and confirming prewarmSpecs deduplication.app/routes/-folder-share.tsx (1)
88-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove decorative shadows and the video-card gradient.
The folder card, video card, and player frame are primary UI elements but use several
shadow-*classes; Line 144 also adds a radial gradient. Increase the folder card’sp-4to the prescribed generous padding while adjusting these styles.As per coding guidelines, “Do not use gradients or shadows in primary UI elements” and “Use generous padding (p-6 to p-8) for spacing.”
Also applies to: 129-168, 503-540
🤖 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/routes/-folder-share.tsx` around lines 88 - 111, Update SharedFolderCard and the related video card and player frame sections to remove all shadow-* classes and the radial gradient near the video card. Increase the folder card padding from p-4 to the prescribed generous spacing, using p-6 or p-8, while preserving the existing layout and hover/focus behavior otherwise.Source: Coding guidelines
🤖 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/routes/-folder-share.tsx`:
- Around line 249-273: Guard grant responses in acquireGrant against the route
token that initiated the request, so results from obsolete tokens cannot update
grantToken or grantError after token changes. Capture the current token/request
sequence before issueAccessGrant and apply state updates only when it still
matches the active token, while preserving the existing pending reset and error
behavior.
- Around line 447-449: Update the Link elements containing the “lawn.” branding
and the footer link to include the underline utility in their base className,
covering the instances near the shown Link and the additional locations noted in
the comment. Preserve their existing hover styling and navigation targets.
---
Nitpick comments:
In `@app/routes/-folder-share.tsx`:
- Around line 88-111: Update SharedFolderCard and the related video card and
player frame sections to remove all shadow-* classes and the radial gradient
near the video card. Increase the folder card padding from p-4 to the prescribed
generous spacing, using p-6 or p-8, while preserving the existing layout and
hover/focus behavior otherwise.
In `@src/lib/convexRouteData.ts`:
- Around line 27-29: Update buildQueryKey to canonicalize convexToJson arguments
before fingerprinting by recursively sorting object keys, ensuring equivalent
objects with different insertion order produce the same serialized
representation. Add a test covering reversed key order and confirming
prewarmSpecs deduplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f06d0b78-c039-41d8-ae35-988ce83ef59b
📒 Files selected for processing (7)
app/routes/-folder-share.data.tsapp/routes/-folder-share.tsxconvex/folderShareAccess.tsconvex/folderShares.tsconvex/folderShares.vitest.tssrc/lib/convexRouteData.test.tssrc/lib/convexRouteData.ts
💤 Files with no reviewable changes (3)
- src/lib/convexRouteData.test.ts
- convex/folderShares.ts
- convex/folderShares.vitest.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- convex/folderShareAccess.ts
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
Implementation
The reimplementation follows this rollout plan:
Security and lifecycle
Data model
Yes, this touches the data model:
folderShareLinksfor the stable one-link-per-folder relationship.folderShareAccessGrantsfor short-lived viewer sessions.projects.deletionStartedAtto make asynchronous deletion immediately visible to authorization checks.All additions are backward-compatible and require no backfill.
Validation
bun run checkgit diff --checkOrigin and credit
This is a fresh reimplementation inspired by PR #21, originally proposed and implemented by @Engm4nn. Thank you to @Engm4nn for the project-sharing concept and the first pass.
No code was cherry-picked from #21; this was rewritten from current
mainaround folder-level sharing, nested navigation, current lifecycle behavior, and stronger public-boundary security.Note
Add public folder share links with time-limited access grants and video playback
/folder-share/$tokenroute where recipients can browse shared project folders and watch videos without an account.FolderShareDialogto create, copy, or revoke a durable share link.folderShareAccessGrants) issued viafolderShares.issueAccessGrant, with layered rate limiting and an hourly sweep cron for expired grants.videoActions.getFolderSharedPlaybackSession; the player auto-refreshes before expiry.deletionStartedAt, removes its share link, and invalidates active grants; team deletion also cleans up share links.folderShareLinks,folderShareAccessGrants,deletionStartedAton projects, new indexes) require a Convex migration deployment before the UI changes take effect.Macroscope summarized c622d37.
Summary by CodeRabbit
New Features
Bug Fixes