-
-
Notifications
You must be signed in to change notification settings - Fork 907
[Feat] Documents | Add Jotai & Tanstack #529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Removed unnecessary comments in createDocument function.
Refactor error handling for document upload requests.
Refactor error handling for searchDocuments method.
…nt in SourceDetailSheet
…n SourceDetailSheet
…SourceDetailSheet - Remove useDocumentByChunk hook dependency - Use useQuery with centralized cache keys and descriptive variable names - Replace all document, loading, and error references with new implementation - Remove manual fetch/clear effects as useQuery handles lifecycle automatically
…atom - Replace imperative fetch API with uploadDocumentMutationAtom in DocumentUploadTab - Maintain backward compatibility with existing UI behavior - Remove unused document query atoms from document-query.atoms.ts - Use mutation state (isPending) for loading state management - Preserve progress animation and error handling
…ache keys - Replace useDocumentByChunk hook with useQuery implementation - Use descriptive variable names (isDocumentByChunkFetching, documentByChunkFetchingError) - Integrate with centralized cache key management - Update all loading and error state references - Add 5-minute stale time for document queries
- Replace imperative authenticatedFetch with createDocumentMutationAtom - Use mutation's isPending state instead of local isSubmitting state - Maintain backward compatibility with existing UI behavior - Keep all validation, error handling, and toast notifications - Remove dependency on auth-utils for API calls
- Delete use-document-by-chunk.ts file - Remove export from hooks index - Hook has been replaced with useQuery implementation in SourceDetailSheet
- Replace useDocumentTypes hook with documentTypeCountsAtom in ChatInputGroup - Replace useDocumentTypes hook with documentTypeCountsAtom in researcher page - Delete obsolete use-document-types.ts hook - Transform atom response to maintain backward compatibility
|
@CREDO23 is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by RecurseML
🔍 Review performed on 9bbea0b..aff7ca0
| Severity | Location | Issue | Delete |
|---|---|---|---|
| surfsense_web/atoms/documents/document-mutation.atoms.ts:84 | SSR crash from localStorage access |
✅ Files analyzed, no issues (19)
• .gitignore
• surfsense_backend/alembic/versions/1_add_github_connector_enum.py
• surfsense_backend/alembic/versions/40_move_llm_preferences_to_searchspace.py
• surfsense_backend/alembic/versions/43_add_blocknote_fields_to_documents.py
• surfsense_web/app/dashboard/[search_space_id]/researcher/[[...chat_id]]/page.tsx
• surfsense_web/atoms/documents/document-query.atoms.ts
• surfsense_web/atoms/documents/ui.atoms.ts
• surfsense_web/components/chat/ChatInputGroup.tsx
• surfsense_web/components/chat/SourceDetailSheet.tsx
• surfsense_web/components/sources/DocumentUploadTab.tsx
• surfsense_web/components/sources/YouTubeTab.tsx
• surfsense_web/contracts/types/document.types.ts
• surfsense_web/contracts/types/index.ts
• surfsense_web/hooks/index.ts
• surfsense_web/hooks/use-document-by-chunk.ts
• surfsense_web/hooks/use-document-types.ts
• surfsense_web/lib/apis/base-api.service.ts
• surfsense_web/lib/apis/documents-api.service.ts
• surfsense_web/lib/query-client/cache-keys.ts
|
|
||
| export const deleteDocumentMutationAtom = atomWithMutation((get) => { | ||
| const searchSpaceId = get(activeSearchSpaceIdAtom); | ||
| const authToken = localStorage.getItem("surfsense_bearer_token"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical SSR crash: Direct access to localStorage in Jotai atom initialization will cause a ReferenceError during server-side rendering in Next.js. Jotai atoms can be initialized on the server, but localStorage is only available in the browser environment. When this atom is first accessed during SSR, it will throw 'ReferenceError: localStorage is not defined', crashing the application.
Additionally, this authToken variable is only used in the 'enabled' condition (line 89) and is redundant because baseApiService already handles authentication via the bearerToken passed during initialization (see base-api.service.ts lines 296-298). The documentsApiService.deleteDocument() call on line 91 already uses the authenticated baseApiService.
This will cause production crashes when:
- The page is server-side rendered
- Any component using deleteDocumentMutationAtom is rendered on the server
- Next.js pre-renders pages at build time
Fix: Remove the localStorage access entirely as it's unnecessary, or wrap it in a typeof window !== 'undefined' check if needed for client-side logic.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
Description
Motivation and Context
FIX #
Screenshots
API Changes
Change Type
Testing Performed
Checklist
High-level PR Summary
This PR migrates the frontend document management system from custom React hooks to a Jotai and TanStack Query (jotai-tanstack-query) architecture. The changes introduce a new type-safe API service layer with Zod validation for document operations, replace existing hooks (
use-document-types,use-document-by-chunk) with Jotai atoms, and update all consuming components to use the new state management approach. Additionally, the backend includes improvements to Alembic migration scripts to make them idempotent and safer for reruns.⏱️ Estimated Review Time: 30-90 minutes
💡 Review Order Suggestion
surfsense_web/contracts/types/index.tssurfsense_web/contracts/types/document.types.tssurfsense_web/lib/apis/base-api.service.tssurfsense_web/lib/apis/documents-api.service.tssurfsense_web/lib/query-client/cache-keys.tssurfsense_web/atoms/documents/ui.atoms.tssurfsense_web/atoms/documents/document-query.atoms.tssurfsense_web/atoms/documents/document-mutation.atoms.tssurfsense_web/hooks/use-document-types.tssurfsense_web/hooks/use-document-by-chunk.tssurfsense_web/hooks/index.tssurfsense_web/app/dashboard/[search_space_id]/researcher/[[...chat_id]]/page.tsxsurfsense_web/components/chat/ChatInputGroup.tsxsurfsense_web/components/chat/SourceDetailSheet.tsxsurfsense_web/components/sources/DocumentUploadTab.tsxsurfsense_web/components/sources/YouTubeTab.tsxsurfsense_backend/alembic/versions/1_add_github_connector_enum.pysurfsense_backend/alembic/versions/40_move_llm_preferences_to_searchspace.pysurfsense_backend/alembic/versions/43_add_blocknote_fields_to_documents.py.gitignoresurfsense_backend/alembic/versions/1_add_github_connector_enum.pysurfsense_backend/alembic/versions/40_move_llm_preferences_to_searchspace.pysurfsense_backend/alembic/versions/43_add_blocknote_fields_to_documents.py.gitignore