Open
Conversation
…ssion validation - Create /view/[linkId]/f/[folderId] page with getServerSideProps - Create /view/domains/[domain]/[slug]/f/[folderId] page for custom domains - Both routes validate session cookies server-side via Redis - Pre-validated sessions skip loading spinner and render content immediately - Falls back to access form when no valid session exists Co-authored-by: Marc Seitz <mfts@users.noreply.github.com>
- Add initialFolderId prop to set initial folder on direct link access - Add preValidatedSession prop for server-side validated sessions - When pre-validated, render content immediately without loading spinner - Background API call records the view without blocking the UI - handleSubmission supports background mode to avoid UI state changes Co-authored-by: Marc Seitz <mfts@users.noreply.github.com>
- Folder navigation updates URL via history.pushState (no page reload)
- URLs follow pattern /view/[linkId]/f/[folderId] or /{slug}/f/[folderId]
- Back/forward browser buttons work via popstate event listener
- Initial folder state is stored in history.replaceState for consistency
- Folder tree sidebar, breadcrumbs, and folder cards all use URL-aware navigation
- Relax setFolderId type in view-tree to accept simple callback
Co-authored-by: Marc Seitz <mfts@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughUpdated setFolderId prop type from React.Dispatch to a plain function callback across folder components. Added PreValidatedSession type and related props to DataroomView. Implemented client-side navigation synchronization with URL history tracking in dataroom-viewer. Created two new Next.js pages for folder-specific dataroom views with server-side session validation and data fetching. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pages/view/[linkId]/f/[folderId].tsx (1)
149-157:⚠️ Potential issue | 🟡 MinorSame issue:
isTeamMemberfield not populated.Same as the domain/slug version - the field is declared in the type but not set during session construction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pages/view/`[linkId]/f/[folderId].tsx around lines 149 - 157, preValidatedSession is missing the isTeamMember property when constructed; update the object created in the session-building code (preValidatedSession) to set isTeamMember from the viewer or session data (e.g., viewer?.isTeamMember or dataroomSession.isTeamMember as appropriate) so the declared type is fully populated; ensure you use the same nullish/default pattern used for other fields (viewer?.isTeamMember ?? undefined or a boolean fallback) and keep references to preValidatedSession, dataroomSession, viewer, and linkConfig when making the change.
🧹 Nitpick comments (4)
components/view/viewer/dataroom-viewer.tsx (1)
233-242: MissingfolderIdin dependency array; add an ESLint suppression or include it.The effect intentionally runs once on mount to set initial history state. However,
folderIdis accessed inside but not listed in dependencies, which will triggerreact-hooks/exhaustive-depslint warnings.If the intent is truly "run once on mount," add an ESLint disable comment. Alternatively, consider whether this should track
folderIdchanges (though that would cause unwanted replaceState calls on navigation).🔧 Proposed fix: Add ESLint suppression for clarity
// Replace the initial history entry so back-button state is consistent useEffect(() => { if (folderId) { window.history.replaceState( { folderId }, "", buildFolderUrl(folderId), ); } + // eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally runs once on mount with initial folderId }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/view/viewer/dataroom-viewer.tsx` around lines 233 - 242, The useEffect that calls window.history.replaceState currently reads folderId but omits it from the dependency array which triggers react-hooks/exhaustive-deps; decide to run it only on mount and add an explicit ESLint suppression above the effect (e.g., // eslint-disable-next-line react-hooks/exhaustive-deps) or, if you prefer to respond to folderId changes, include folderId in the dependency array and guard the call, referencing the useEffect that calls window.history.replaceState({ folderId }, "", buildFolderUrl(folderId)) so the intent is clear and lint warnings are resolved.pages/view/[linkId]/f/[folderId].tsx (1)
58-207: Consider extracting shared logic between the two folder page files.This file and
pages/view/domains/[domain]/[slug]/f/[folderId].tsxshare ~90% identical code. Key differences are only:
- Route parameter extraction (linkId vs domain/slug)
- Link fetching function (
fetchLinkDataByIdvsfetchLinkDataByDomainSlug)- Cookie key naming
Consider extracting common utilities:
- Shared
buildPreValidatedSession()function- Shared
FolderViewPageContentcomponent- Shared
buildFolderPageProps()for the props constructionThis would reduce duplication and ensure both routes stay in sync.
Also applies to: 209-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pages/view/`[linkId]/f/[folderId].tsx around lines 58 - 207, The two folder page variants duplicate nearly identical server-side logic; extract the shared parts into reusable utilities to avoid drift: factor out a shared function (e.g., buildFolderPageProps) that accepts the differing inputs (route params and a link fetcher like fetchLinkDataById or fetchLinkDataByDomainSlug) to perform validation, document transformation, feature-flag fetch, lastUpdatedAt calculation and props assembly, extract the session handling into buildPreValidatedSession which wraps verifyDataroomSessionInPagesRouter and the prisma lookups, and move the UI into a shared FolderViewPageContent component; then update getServerSideProps in this file to call the new buildFolderPageProps with linkId and fetchLinkDataById and adjust cookie/key naming at the call-site only.pages/view/domains/[domain]/[slug]/f/[folderId].tsx (2)
196-206: Hardcoded team IDs should be extracted to configuration.Magic strings for
useCustomAccessFormandlogoOnAccessFormare duplicated across multiple page files and will require coordinated updates if team IDs change. Consider:
- A shared config file
- Team-level database flags
- Feature flag system (already used for
dataroomIndex,textSelection)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pages/view/domains/`[domain]/[slug]/f/[folderId].tsx around lines 196 - 206, Extract the hardcoded team ID arrays used for useCustomAccessForm and logoOnAccessForm into a shared configuration (e.g., export constants or a map) so multiple pages can import them; update the occurrences in this file to reference the shared constants instead of inline arrays and consider replacing the boolean checks that use includes(teamId) with a feature-check helper (e.g., isTeamEnabledForFeature(teamId, "customAccessForm") or isTeamEnabledForFeature(teamId, "logoOnAccessForm")) so you can later swap the source to a DB or feature-flag system without touching the page code (update references to useCustomAccessForm, logoOnAccessForm, and the teamId checks).
28-37: ImportPreValidatedSessionfromdataroom-view.tsxinstead of duplicating.This type is already exported from
components/view/dataroom/dataroom-view.tsx(lines 51-60) with identical structure. Duplicating types creates maintenance burden and risks divergence.♻️ Proposed fix
+import DataroomView, { PreValidatedSession } from "@/components/view/dataroom/dataroom-view"; -import DataroomView from "@/components/view/dataroom/dataroom-view"; -type PreValidatedSession = { - viewId: string; - viewerEmail?: string; - viewerId?: string; - conversationsEnabled?: boolean; - enableVisitorUpload?: boolean; - isTeamMember?: boolean; - agentsEnabled?: boolean; - dataroomName?: string; -};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pages/view/domains/`[domain]/[slug]/f/[folderId].tsx around lines 28 - 37, Remove the duplicated PreValidatedSession type declaration and instead import the exported PreValidatedSession type from the existing dataroom-view module; update the current file to delete the local type definition (the one with viewId, viewerEmail, viewerId, conversationsEnabled, enableVisitorUpload, isTeamMember, agentsEnabled, dataroomName) and add a named import for PreValidatedSession so all references in this file use the shared type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/view/dataroom/dataroom-view.tsx`:
- Around line 254-261: The background effect that calls handleSubmission(true)
when hasPreValidatedSession is true can cause duplicate API calls, duplicate
analytics events, and overwrite existing preValidatedSession state; update the
useEffect (and surrounding logic using hasPreValidatedSession,
preValidatedSession, and viewData) to short-circuit for pre-validated sessions:
either (A) skip calling handleSubmission and instead call
analytics.capture("Link Viewed", ...) only, preserving viewData, or (B) make
handleSubmission detect a preValidatedSession and return early (no POST to
/api/views-dataroom, no state overwrite) while still optionally emitting
analytics; ensure the change references useEffect, handleSubmission,
preValidatedSession, analytics.capture, and viewData so the API call and state
update are avoided for already-validated sessions.
---
Duplicate comments:
In `@pages/view/`[linkId]/f/[folderId].tsx:
- Around line 149-157: preValidatedSession is missing the isTeamMember property
when constructed; update the object created in the session-building code
(preValidatedSession) to set isTeamMember from the viewer or session data (e.g.,
viewer?.isTeamMember or dataroomSession.isTeamMember as appropriate) so the
declared type is fully populated; ensure you use the same nullish/default
pattern used for other fields (viewer?.isTeamMember ?? undefined or a boolean
fallback) and keep references to preValidatedSession, dataroomSession, viewer,
and linkConfig when making the change.
---
Nitpick comments:
In `@components/view/viewer/dataroom-viewer.tsx`:
- Around line 233-242: The useEffect that calls window.history.replaceState
currently reads folderId but omits it from the dependency array which triggers
react-hooks/exhaustive-deps; decide to run it only on mount and add an explicit
ESLint suppression above the effect (e.g., // eslint-disable-next-line
react-hooks/exhaustive-deps) or, if you prefer to respond to folderId changes,
include folderId in the dependency array and guard the call, referencing the
useEffect that calls window.history.replaceState({ folderId }, "",
buildFolderUrl(folderId)) so the intent is clear and lint warnings are resolved.
In `@pages/view/`[linkId]/f/[folderId].tsx:
- Around line 58-207: The two folder page variants duplicate nearly identical
server-side logic; extract the shared parts into reusable utilities to avoid
drift: factor out a shared function (e.g., buildFolderPageProps) that accepts
the differing inputs (route params and a link fetcher like fetchLinkDataById or
fetchLinkDataByDomainSlug) to perform validation, document transformation,
feature-flag fetch, lastUpdatedAt calculation and props assembly, extract the
session handling into buildPreValidatedSession which wraps
verifyDataroomSessionInPagesRouter and the prisma lookups, and move the UI into
a shared FolderViewPageContent component; then update getServerSideProps in this
file to call the new buildFolderPageProps with linkId and fetchLinkDataById and
adjust cookie/key naming at the call-site only.
In `@pages/view/domains/`[domain]/[slug]/f/[folderId].tsx:
- Around line 196-206: Extract the hardcoded team ID arrays used for
useCustomAccessForm and logoOnAccessForm into a shared configuration (e.g.,
export constants or a map) so multiple pages can import them; update the
occurrences in this file to reference the shared constants instead of inline
arrays and consider replacing the boolean checks that use includes(teamId) with
a feature-check helper (e.g., isTeamEnabledForFeature(teamId,
"customAccessForm") or isTeamEnabledForFeature(teamId, "logoOnAccessForm")) so
you can later swap the source to a DB or feature-flag system without touching
the page code (update references to useCustomAccessForm, logoOnAccessForm, and
the teamId checks).
- Around line 28-37: Remove the duplicated PreValidatedSession type declaration
and instead import the exported PreValidatedSession type from the existing
dataroom-view module; update the current file to delete the local type
definition (the one with viewId, viewerEmail, viewerId, conversationsEnabled,
enableVisitorUpload, isTeamMember, agentsEnabled, dataroomName) and add a named
import for PreValidatedSession so all references in this file use the shared
type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff283de9-4469-43d3-bc32-2cc88bc4c70c
📒 Files selected for processing (5)
components/datarooms/folders/view-tree.tsxcomponents/view/dataroom/dataroom-view.tsxcomponents/view/viewer/dataroom-viewer.tsxpages/view/[linkId]/f/[folderId].tsxpages/view/domains/[domain]/[slug]/f/[folderId].tsx
Comment on lines
+254
to
+261
| // For pre-validated sessions: fire background API call to record the view | ||
| // without blocking the UI (content is already visible) | ||
| useEffect(() => { | ||
| if (hasPreValidatedSession && !didMount.current) { | ||
| didMount.current = true; | ||
| handleSubmission(true); | ||
| } | ||
| }, [hasPreValidatedSession]); |
Contributor
There was a problem hiding this comment.
Background submission may cause redundant API calls and analytics events.
When preValidatedSession is provided, this effect fires handleSubmission(true) which:
- Makes a POST to
/api/views-dataroomthat should be idempotent (session exists → no duplicate view created) - Calls
analytics.capture("Link Viewed", ...)(lines 197-206) which may duplicate the view event if already tracked server-side - Overwrites
viewDatastate (lines 212-222) with API response, potentially replacing the already-correctpreValidatedSessionvalues
Consider either:
- Skipping the full submission flow for pre-validated sessions (just call analytics)
- Or ensuring the API returns early for existing sessions without side effects
🔍 Verification: Check if API handles existing sessions correctly
#!/bin/bash
# Search for how the API handles dataroomSession presence
rg -n -A 10 'dataroomSession' --type=ts -g 'app/api/views-dataroom/*'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/view/dataroom/dataroom-view.tsx` around lines 254 - 261, The
background effect that calls handleSubmission(true) when hasPreValidatedSession
is true can cause duplicate API calls, duplicate analytics events, and overwrite
existing preValidatedSession state; update the useEffect (and surrounding logic
using hasPreValidatedSession, preValidatedSession, and viewData) to
short-circuit for pre-validated sessions: either (A) skip calling
handleSubmission and instead call analytics.capture("Link Viewed", ...) only,
preserving viewData, or (B) make handleSubmission detect a preValidatedSession
and return early (no POST to /api/views-dataroom, no state overwrite) while
still optionally emitting analytics; ensure the change references useEffect,
handleSubmission, preValidatedSession, analytics.capture, and viewData so the
API call and state update are avoided for already-validated sessions.
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.
Enable direct links to dataroom folders, enhance navigation with browser history, and improve performance by validating direct link sessions server-side.
Previously, direct links to datarooms (documents or folders) relied on client-side session validation, which often resulted in a slow render screen before access was granted or denied. This PR moves session validation to the server-side for direct links, allowing for immediate rendering of either the content or the access form, significantly improving perceived performance.
Summary by CodeRabbit