-
Notifications
You must be signed in to change notification settings - Fork 99
fix: Fix repo carousel thrashing #294
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
Conversation
""" WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant HomePage
participant getRepos
participant RepositorySnapshot
HomePage->>getRepos: Fetch repository data for domain
getRepos-->>HomePage: Return repos or error
HomePage->>RepositorySnapshot: Render with repos (empty array if error)
RepositorySnapshot->>RepositorySnapshot: Filter and display indexed repos based on indexedAt
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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 (
|
packages/web/src/app/[domain]/components/repositorySnapshot.tsx
Outdated
Show resolved
Hide resolved
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/web/src/app/[domain]/components/repositorySnapshot.tsx (1)
20-28
: Consider adding default value for repos prop.While the code handles the case where
repos
might be a service error, consider providing a default empty array for therepos
prop to handle cases where it might not be provided at all.- repos: RepositoryQuery[]; + repos?: RepositoryQuery[]; - repos: initialRepos, + repos: initialRepos = [],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/web/src/app/[domain]/components/repositorySnapshot.tsx
(3 hunks)packages/web/src/app/[domain]/page.tsx
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (10)
packages/web/src/app/[domain]/page.tsx (3)
13-14
: Good addition of required imports.These imports provide the necessary functionality to fetch repositories and handle service errors.
22-23
: Server-side data fetching implemented correctly.Fetching repository data server-side will provide initial data for hydration, preventing the loading skeleton from appearing on first page load.
41-44
: Good error handling and prop passing.The code correctly handles potential service errors by passing an empty array when errors occur, preventing error propagation to the client component.
packages/web/src/app/[domain]/components/repositorySnapshot.tsx (7)
18-18
: Good addition of type import.Importing the
RepositoryQuery
type ensures proper type checking for the new props.
20-23
: Interface correctly updated with new props.The interface now properly includes the
repos
prop that will be used for initial data.
25-28
: Good component parameter update with renamed prop.Renaming
repos
toinitialRepos
in the parameters makes it clear that these are initial values that may be updated.
31-36
: Query configuration properly updated for initial data.The changes correctly:
- Add
initialRepos
to the query key so React Query will refetch if initial data changes- Set
initialData
to the server-provided repositoriesThis ensures proper hydration and prevents unnecessary loading states.
46-49
: Good implementation of improved filtering logic.Changing the filter criteria to check for
indexedAt !== undefined
rather than the current indexing status is a good approach to prevent carousel thrashing. The comment explains the reasoning well.
50-68
: Improved conditional rendering with appropriate states.The conditional rendering logic has been improved to:
- Show a loading indicator when repositories are being indexed
- Show the empty state only when no repositories are indexed or in the indexing queue
This provides better user feedback about the system state.
78-78
: Fixed pluralization for better grammar.Good attention to detail by ensuring correct pluralization based on the number of repositories.
This PR fixes repo carousel thrashing by showing repos that have ever had a zoekt index (using
indexedAt !== undefined
) instead of relying on the current repo indexing status. It also adds hydration server side s.t., the skeleton does not appear on first page load.Fixes #293
Summary by CodeRabbit
New Features
Bug Fixes