fix(browse): Fix issue where files would sometimes never load #365
Conversation
WalkthroughThis set of changes removes all React Query-based prefetching logic for file sources and folder contents, deletes the associated hooks, and refactors several components from client-side to server-side async components. Data fetching is now performed directly in server components, with error handling and UI loading states adjusted accordingly. Utility parsing logic is consolidated in a new function. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BrowsePage (server)
participant CodePreviewPanel/TreePreviewPanel (server)
participant DataService
User->>BrowsePage (server): Request page with params
BrowsePage (server)->>CodePreviewPanel/TreePreviewPanel: Render with explicit props
CodePreviewPanel/TreePreviewPanel->>DataService: await getRepoInfoByName / getFolderContents / getFileSource
DataService-->>CodePreviewPanel/TreePreviewPanel: Return data or error
CodePreviewPanel/TreePreviewPanel-->>BrowsePage (server): Rendered JSX
BrowsePage (server)-->>User: Send rendered page
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*`: Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (4)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
packages/web/src/features/fileTree/components/pureFileTreePanel.tsx (1)
54-60: Avoid mutating React state insidetransformTree
currentNode.isCollapsed = isCollapseddirectly mutates a node that still belongs to the previous state tree.
Although you later spread-copy the node, the mutation happens first, so the original tree is mutated before React receives the new tree, violating React’s immutability contract and risking subtle bugs (e.g. missed re-renders in memoised children).- if (currentNode.path === path) { - currentNode.isCollapsed = isCollapsed; - } - return currentNode; + if (currentNode.path === path) { + /* return a new object – never mutate the existing one */ + return { ...currentNode, isCollapsed }; + } + return currentNode;packages/web/src/app/[domain]/browse/[...path]/components/codePreviewPanel.tsx (1)
2-6: Remove unused imports from client-side version.These imports are no longer needed after the conversion to a server component:
useBrowseParams(now receives props)useQuery(now uses direct await)useDomain(now receives domain as prop)Loader2(loading handled by Suspense in parent)-import { useBrowseParams } from "@/app/[domain]/browse/hooks/useBrowseParams"; -import { useQuery } from "@tanstack/react-query"; -import { getFileSource } from "@/features/search/fileSourceApi"; -import { useDomain } from "@/hooks/useDomain"; -import { Loader2 } from "lucide-react"; +import { getFileSource } from "@/features/search/fileSourceApi";
🧹 Nitpick comments (5)
packages/web/src/features/fileTree/components/pureFileTreePanel.tsx (2)
95-98: Redundantkeyprop duplicationBoth the
React.Fragment(line 95) and the nestedFileTreeItemComponent(line 97) receive the samekey={node.path}.
The extra key on the child component is superfluous and can be safely removed to keep the markup tidy.-<React.Fragment key={node.path}> - <FileTreeItemComponent - key={node.path} +<React.Fragment key={node.path}> + <FileTreeItemComponent
112-113:renderTreedependency list may cause unnecessary re-creations
renderTreedepends onscrollAreaRef(passed to children) but the ref is omitted from the dependency array.
WhileuseRefobjects are stable, future changes (e.g. swapping touseStateor replacing the ref) would silently break memoisation.
For completeness and future-proofing, include it:- }, [path, onNodeClicked]); + }, [path, onNodeClicked, scrollAreaRef]);packages/web/src/app/[domain]/browse/hooks/utils.ts (2)
3-3: Fix typo in variable name.The variable name
sentinalIndexshould besentinelIndex(missing 'e').- const sentinalIndex = pathParam.search(/\/-\/(tree|blob)\//); + const sentinelIndex = pathParam.search(/\/-\/(tree|blob)\//);You'll also need to update the usage on lines 4, 8, and 13.
16-16: Fix typo in comment.The comment mentions
decodedURIComponentbut should bedecodeURIComponent.- // @note: decodedURIComponent is needed here incase the path contains a space. + // @note: decodeURIComponent is needed here in case the path contains a space.packages/web/src/app/[domain]/browse/[...path]/components/treePreviewPanel.tsx (1)
25-27: Enhance error handling with more descriptive messagesThe current error handling doesn't provide enough context about which operation failed or why.
if (isServiceError(folderContentsResponse) || isServiceError(repoInfoResponse)) { - return <div>Error loading tree preview</div> + const errorMessage = isServiceError(repoInfoResponse) + ? `Failed to load repository information for ${repoName}` + : `Failed to load folder contents at ${path}`; + return <div className="p-4 text-red-500">{errorMessage}</div> }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/web/src/app/[domain]/browse/[...path]/components/codePreviewPanel.tsx(2 hunks)packages/web/src/app/[domain]/browse/[...path]/components/pureTreePreviewPanel.tsx(1 hunks)packages/web/src/app/[domain]/browse/[...path]/components/treePreviewPanel.tsx(2 hunks)packages/web/src/app/[domain]/browse/[...path]/page.tsx(1 hunks)packages/web/src/app/[domain]/browse/components/fileSearchCommandDialog.tsx(0 hunks)packages/web/src/app/[domain]/browse/hooks/useBrowseParams.ts(1 hunks)packages/web/src/app/[domain]/browse/hooks/utils.ts(1 hunks)packages/web/src/app/[domain]/components/pathHeader.tsx(0 hunks)packages/web/src/ee/features/codeNav/components/exploreMenu/referenceList.tsx(0 hunks)packages/web/src/features/fileTree/components/fileTreeItemComponent.tsx(0 hunks)packages/web/src/features/fileTree/components/pureFileTreePanel.tsx(1 hunks)packages/web/src/hooks/usePrefetchFileSource.ts(0 hunks)packages/web/src/hooks/usePrefetchFolderContents.ts(0 hunks)
💤 Files with no reviewable changes (6)
- packages/web/src/features/fileTree/components/fileTreeItemComponent.tsx
- packages/web/src/app/[domain]/components/pathHeader.tsx
- packages/web/src/hooks/usePrefetchFolderContents.ts
- packages/web/src/hooks/usePrefetchFileSource.ts
- packages/web/src/app/[domain]/browse/components/fileSearchCommandDialog.tsx
- packages/web/src/ee/features/codeNav/components/exploreMenu/referenceList.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*`: Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
**/*: Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
packages/web/src/features/fileTree/components/pureFileTreePanel.tsxpackages/web/src/app/[domain]/browse/[...path]/components/pureTreePreviewPanel.tsxpackages/web/src/app/[domain]/browse/hooks/utils.tspackages/web/src/app/[domain]/browse/hooks/useBrowseParams.tspackages/web/src/app/[domain]/browse/[...path]/page.tsxpackages/web/src/app/[domain]/browse/[...path]/components/codePreviewPanel.tsxpackages/web/src/app/[domain]/browse/[...path]/components/treePreviewPanel.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (7)
packages/web/src/app/[domain]/browse/hooks/utils.ts (1)
2-37: LGTM - Well-structured utility function.The path parsing logic is well-implemented with proper error handling and clear separation of concerns. This centralizes the parsing logic effectively, supporting the broader refactoring goals.
packages/web/src/app/[domain]/browse/[...path]/components/pureTreePreviewPanel.tsx (1)
1-46: LGTM - Well-designed pure presentational component.This component provides a clean separation of concerns by accepting data as props while handling UI interactions through hooks. The use of
useCallbackfor the click handler is appropriate for performance optimization.packages/web/src/app/[domain]/browse/hooks/useBrowseParams.ts (1)
8-17: LGTM - Good refactoring to centralize parsing logic.The simplification of this hook by delegating to the utility function is a solid improvement. The memoization ensures the parsing only happens when the pathname changes.
packages/web/src/app/[domain]/browse/[...path]/page.tsx (2)
14-17: LGTM - Clean server component conversion.The conversion to an async server component with explicit props is well-executed. Using
decodeURIComponenton the joined path and the utility function for parsing is the correct approach.
20-25: LGTM - Appropriate loading state handling.The Suspense fallback with loading spinner provides good user feedback during data fetching. This aligns with the PR's goal of moving loading states to the server side.
packages/web/src/app/[domain]/browse/[...path]/components/codePreviewPanel.tsx (1)
21-40: LGTM - Excellent server component conversion.The conversion from React Query to direct
awaitcalls is well-implemented. The sequential data fetching, error handling withisServiceError, and explicit props pattern align perfectly with the PR's objectives to fix the loading issues.packages/web/src/app/[domain]/browse/[...path]/components/treePreviewPanel.tsx (1)
29-46: Clean separation of concerns!The refactoring to delegate rendering to
PureTreePreviewPanelwhile keeping data fetching in the server component is well-structured and aligns with Next.js best practices.
packages/web/src/app/[domain]/browse/[...path]/components/treePreviewPanel.tsx
Outdated
Show resolved
Hide resolved
|
todo: fix bug with navigating to repository root. |
Problem
We noticed that sometimes files would be stuck in a infinite loading state like the following:
This would happen randomly. Usually refreshing the page would fix it.
Solution
This PR moves the file source fetch from the client to the server. The downside of this approach is that all of the client side prefetching we were doing (with
usePrefetchFolderContents.tsandusePrefetchFileSource.ts) no longer works, so now users will always see the loading spinner.We should probably switch to links anyways (see #353), so we can probably rely on Next's built-in prefetching with the Link component..
Why was this happening in the first place?
TLDR: I'm not totally sure, but I think it's because we are using server actions incorrectly. See this issue.
I did some digging, and came across this issue that mentioned that server actions are not intended for fetching data and that you can get some unexpected behaviours when you do. This looks to be the case according to the react docs. It seems like this is one of those cases, but I'm not exactly sure why. I confirmed that this was the issue by switching over to using a
fetchcall and was unable to repro. Moving the data fetching to the server also fixed the issue.As a longer-term thing, it might make sense to move away from using server actions (either altogether or for specifically data fetching) to avoid issues like this. Ideally something with as good of a DX (or better). Some options I've come across:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores