Improve repository experience by adding a commits browser#177
Improve repository experience by adding a commits browser#177theMackabu wants to merge 10 commits intostylessh:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a protected "commits" splat route and UI with an infinite, paginated commits list, server fetcher and query keys/options, types and tab-store updates for "commits"; integrates path-scoped commit history into file/folder views and latest-commit bar; several UI, export, and package script tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Router as Commits Route
participant Loader as Route Loader
participant Prefetch as Query Prefetch
participant Server as getRepoCommits
participant GH as GitHub API
participant UI as RepoCommitsPage
participant Observer as IntersectionObserver
User->>Router: Navigate to /$owner/$repo/commits/$ (with _splat)
Router->>Loader: load()
Loader->>Prefetch: prefetch infinite commits (ref, path, perPage=30)
Prefetch->>Server: getRepoCommits(page=1, ref, path)
Server->>GH: REST GET /repos/{owner}/{repo}/commits?sha={ref}&path={path}&page=1
GH-->>Server: commits page
Server-->>Prefetch: normalized commits + nextPage
Loader-->>Router: return { ref, path }
Router->>UI: render RepoCommitsPage(ref, path)
UI->>Prefetch: useInfiniteQuery(commits)
Prefetch-->>UI: return cached page1
UI->>UI: render grouped commits + sentinel element
User->>Observer: scroll near sentinel
Observer->>Prefetch: request next page (page=2)
Prefetch->>Server: getRepoCommits(page=2, ref, path)
Server->>GH: REST GET /repos/.../commits?page=2
GH-->>Server: commits page2
Server-->>Prefetch: append commits + nextPage
Prefetch-->>UI: updated pages
UI->>User: display additional commits
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 (1)
apps/dashboard/src/lib/github.functions.ts (1)
9591-9591: Use the list cache tier for paginated commits.Line 9591 uses
githubCachePolicy.detail.staleTimeMs; this endpoint is a time-sensitive list and should likely usegithubCachePolicy.list.staleTimeMsfor fresher history updates.♻️ Suggested change
- freshForMs: githubCachePolicy.detail.staleTimeMs, + freshForMs: githubCachePolicy.list.staleTimeMs,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/lib/github.functions.ts` at line 9591, Replace the detail cache tier with the list cache tier for this paginated commits response: change the freshForMs assignment from githubCachePolicy.detail.staleTimeMs to githubCachePolicy.list.staleTimeMs so the commits list uses the shorter/fresher list stale time; update the object that sets freshForMs (the one referencing githubCachePolicy.detail.staleTimeMs) to reference githubCachePolicy.list.staleTimeMs instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard/package.json`:
- Line 10: The dev and build npm scripts in package.json use POSIX-only env
assignment (NODE_OPTIONS='...') which breaks on Windows; install cross-env as a
dev dependency and update the "dev" and "build" script values to prefix
NODE_OPTIONS with cross-env (i.e., use cross-env NODE_OPTIONS=... before vite
build/dev commands) so the environment variable is set in a cross-platform way;
modify the "dev" and "build" script entries in package.json accordingly and add
cross-env to devDependencies.
In `@apps/dashboard/src/components/repo/repo-commits-list.tsx`:
- Around line 129-135: The repo header backlink drops ref context; update the
Link in RepoCommitsList to include the currentRef so users stay on the same
branch/tag. Change the Link target and params to pass currentRef (e.g. include a
ref param or use the route segment that represents ref) by updating the Link's
to and params to something like params={{ owner, repo, ref: currentRef }} (or
the route's ref segment name) and ensure you reference the currentRef variable
used in this component so the backlink preserves the ref context.
- Around line 121-122: Don't unconditionally throw commitsQuery.error in the
RepoCommitsList component; that replaces existing pages on a transient "load
more" failure. Change the error handling so you still throw overviewQuery.error,
but only throw commitsQuery.error when it's a non-pagination failure (e.g.,
commitsQuery.isError && !commitsQuery.isFetchNextPageError). Remove the
unconditional throw of commitsQuery.error and instead surface
commitsQuery.isFetchNextPageError in the load-more UI (near the load-more
controls) with a retry affordance that calls commitsQuery.fetchNextPage so users
can retry without losing already-loaded commit pages.
In `@apps/dashboard/src/lib/github.functions.ts`:
- Around line 9575-9624: The handler for getRepoCommits currently swallows all
errors by appending .catch(() => ({ commits: [], nextPage: null })) to the
getCachedGitHubRequest call; instead remove that blanket catch and only handle
expected "no data" cases (e.g., 404/not-found) explicitly — let other errors
(rate limit, auth, network) propagate so they surface to the caller. Concretely:
in getRepoCommits, eliminate the final .catch(...) on the getCachedGitHubRequest
invocation and, if you must special-case a GitHub 404, catch and return an empty
commits page only when the error status/code indicates not found; otherwise
rethrow the error so callers and monitoring can detect failures. Ensure changes
reference getRepoCommits, getCachedGitHubRequest, and the mapData path for
locating the code to update.
---
Nitpick comments:
In `@apps/dashboard/src/lib/github.functions.ts`:
- Line 9591: Replace the detail cache tier with the list cache tier for this
paginated commits response: change the freshForMs assignment from
githubCachePolicy.detail.staleTimeMs to githubCachePolicy.list.staleTimeMs so
the commits list uses the shorter/fresher list stale time; update the object
that sets freshForMs (the one referencing githubCachePolicy.detail.staleTimeMs)
to reference githubCachePolicy.list.staleTimeMs instead.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b2d2656-14ec-41fe-a397-f3e53451a39a
📒 Files selected for processing (14)
apps/dashboard/package.jsonapps/dashboard/src/components/layouts/dashboard-tabs.tsxapps/dashboard/src/components/navigation/command-palette.tsxapps/dashboard/src/components/repo/code-explorer-toolbar.tsxapps/dashboard/src/components/repo/latest-commit-bar.tsxapps/dashboard/src/components/repo/repo-commits-list.tsxapps/dashboard/src/lib/github-revalidation.tsapps/dashboard/src/lib/github.functions.tsapps/dashboard/src/lib/github.query.tsapps/dashboard/src/lib/github.types.tsapps/dashboard/src/lib/query-client.tsxapps/dashboard/src/lib/tab-store.tsapps/dashboard/src/routeTree.gen.tsapps/dashboard/src/routes/_protected/$owner/$repo/commits.$.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/dashboard/src/components/repo/code-file-view.tsx (1)
540-548: Consider centralizing commits-link construction.This URL-param composition now exists in multiple components; extracting a small shared helper/component would reduce future drift (labeling,
_splatshape, aria text).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/repo/code-file-view.tsx` around lines 540 - 548, The commits link construction is duplicated; extract it into a small shared helper or component (e.g., a buildCommitsLink(owner, repo, currentRef, path) utility or a CommitsLink React component) and replace the inline Link usage in code-file-view (the Link with params={{ owner, repo, _splat: `${currentRef}/${path}` }}, aria-label="View commits", and visible label "History"). The shared helper/component should return the same route shape (to="/$owner/$repo/commits/$" and _splat `${currentRef}/${path}`) and preserve aria-label and styling props so all callers (current and other components) use the single source to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/dashboard/src/components/repo/code-file-view.tsx`:
- Around line 540-548: The commits link construction is duplicated; extract it
into a small shared helper or component (e.g., a buildCommitsLink(owner, repo,
currentRef, path) utility or a CommitsLink React component) and replace the
inline Link usage in code-file-view (the Link with params={{ owner, repo,
_splat: `${currentRef}/${path}` }}, aria-label="View commits", and visible label
"History"). The shared helper/component should return the same route shape
(to="/$owner/$repo/commits/$" and _splat `${currentRef}/${path}`) and preserve
aria-label and styling props so all callers (current and other components) use
the single source to avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b02b90f-e4a9-4e74-9f11-baaf774e0580
📒 Files selected for processing (5)
apps/dashboard/src/components/layouts/dashboard-tabs.tsxapps/dashboard/src/components/repo/code-file-view.tsxapps/dashboard/src/components/repo/folder-view.tsxapps/dashboard/src/components/repo/latest-commit-bar.tsxapps/dashboard/src/components/repo/repo-commits-list.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/dashboard/src/components/repo/folder-view.tsx
- apps/dashboard/src/components/repo/repo-commits-list.tsx
- apps/dashboard/src/components/layouts/dashboard-tabs.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/dashboard/src/lib/github.functions.ts (1)
9672-9698:⚠️ Potential issue | 🟠 MajorUse
clampPage/clampPerPagehere.The current math still lets
NaN,Infinity, and fractional values through, sopage/perPagecan become invalid in both the cache params and thelistCommitscall.🛠️ Proposed fix
- const perPage = Math.min(Math.max(data.perPage ?? 15, 1), 30); - const page = Math.max(data.page ?? 1, 1); + const perPage = Math.min(clampPerPage(data.perPage, 15), 30); + const page = clampPage(data.page);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/lib/github.functions.ts` around lines 9672 - 9698, Replace the ad-hoc Math.min/Math.max sanitization for perPage and page with the existing clampPerPage and clampPage helpers so NaN, Infinity and fractional values are normalized; call clampPerPage(data.perPage) to produce perPage and clampPage(data.page) to produce page and use those sanitized values in the params passed to getCachedGitHubRequest and in the context.octokit.rest.repos.listCommits call (keeping the same namespace/signal keys like githubRevalidationSignalKeys.repoCode(data) and freshForMs = githubCachePolicy.detail.staleTimeMs).
🧹 Nitpick comments (3)
apps/dashboard/src/components/repo/repo-commits-list.tsx (1)
303-305: Use a more descriptive avatar alt label.Line 304 can be slightly clearer for assistive tech by labeling this as an avatar.
🪄 Small accessibility tweak
- alt={commit.author.login} + alt={`${commit.author.login} avatar`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/repo/repo-commits-list.tsx` around lines 303 - 305, The avatar image's alt text (in repo-commits-list.tsx where src={commit.author.avatarUrl}) is currently just the username; update the alt prop to a more descriptive label for assistive tech such as "Avatar of {commit.author.login}" or "{commit.author.login} avatar" so screen readers clearly announce that this is an avatar image.apps/dashboard/src/lib/command-palette/use-command-items.ts (1)
208-225: Extract repository item construction into a shared helper.Line 208 duplicates the same repo-to-
CommandItemmapping used in Line 95, which increases drift risk when metadata/keywords change.♻️ Proposed refactor
+function buildRepoCommandItem(repo: UserRepoSummary): CommandItem { + return { + id: `repo:${repo.id}`, + label: repo.fullName, + group: "Repositories", + icon: CodeIcon, + keywords: [repo.name, repo.owner, repo.language ?? ""].filter(Boolean), + action: { + type: "navigate", + to: `/${repo.owner}/${repo.name}`, + }, + meta: { + language: repo.language, + stars: repo.stars, + updatedAt: repo.updatedAt ?? undefined, + }, + }; +} ... if (repos) { for (const repo of repos) { - dynamicItems.push({ - id: `repo:${repo.id}`, - label: repo.fullName, - group: "Repositories", - icon: CodeIcon, - keywords: [repo.name, repo.owner, repo.language ?? ""].filter(Boolean), - action: { - type: "navigate", - to: `/${repo.owner}/${repo.name}`, - }, - meta: { - language: repo.language, - stars: repo.stars, - updatedAt: repo.updatedAt ?? undefined, - }, - }); + dynamicItems.push(buildRepoCommandItem(repo)); } } ... for (const repo of result.repos) { - items.push({ - id: `repo:${repo.id}`, - label: repo.fullName, - group: "Repositories", - icon: CodeIcon, - keywords: [repo.name, repo.owner, repo.language ?? ""].filter(Boolean), - action: { - type: "navigate", - to: `/${repo.owner}/${repo.name}`, - }, - meta: { - language: repo.language, - stars: repo.stars, - updatedAt: repo.updatedAt ?? undefined, - }, - }); + items.push(buildRepoCommandItem(repo)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/lib/command-palette/use-command-items.ts` around lines 208 - 225, Extract the duplicated repo-to-CommandItem mapping into a single helper (e.g., createRepoCommandItem or buildRepoCommandItem) and use it wherever repos are mapped to command items (the places that currently do items.push({...}) for result.repos). The helper should accept the repo object and return an object with id: `repo:${repo.id}`, label: repo.fullName, group: "Repositories", icon: CodeIcon, keywords: [repo.name, repo.owner, repo.language ?? ""].filter(Boolean), action: { type: "navigate", to: `/${repo.owner}/${repo.name}` }, and meta: { language: repo.language, stars: repo.stars, updatedAt: repo.updatedAt ?? undefined }; replace both in-file mappings with calls to this helper to avoid duplication and keep metadata/keywords consistent.apps/dashboard/src/lib/github.functions.ts (1)
444-469: Extract the shared repo-to-summary projection.
mapGithubSearchRepoToUserRepoSummarynow duplicates almost all ofmapGithubRestRepoToUserRepoSummary. Any future field tweak here is easy to miss in one path and not the other.♻️ Suggested shape
+function mapRepoToUserRepoSummary(repo: { + id?: number; + name?: string | null; + full_name?: string | null; + description?: string | null; + stargazers_count?: number | null; + forks_count?: number | null; + language?: string | null; + updated_at?: string | null; + created_at?: string | null; + private?: boolean | null; + visibility?: string | null; + html_url?: string | null; + owner?: { login?: string | null } | null; +}): UserRepoSummary { + const visibility: UserRepoSummary["visibility"] = + repo.visibility === "internal" + ? "internal" + : repo.visibility === "private" || repo.private + ? "private" + : "public"; + + return { + id: repo.id ?? 0, + name: repo.name ?? "", + fullName: repo.full_name ?? "", + description: repo.description ?? null, + stars: repo.stargazers_count ?? 0, + forks: repo.forks_count ?? 0, + language: repo.language ?? null, + updatedAt: repo.updated_at ?? null, + createdAt: repo.created_at ?? null, + isPrivate: Boolean(repo.private), + visibility, + url: repo.html_url ?? "", + owner: repo.owner?.login ?? "", + }; +} + function mapGithubRestRepoToUserRepoSummary( repo: AuthenticatedUserRepo | ListForUserRepo | GetRepoItem, ): UserRepoSummary { - ... + return mapRepoToUserRepoSummary(repo); } function mapGithubSearchRepoToUserRepoSummary( repo: SearchRepoItem, ): UserRepoSummary { - ... + return mapRepoToUserRepoSummary(repo); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/lib/github.functions.ts` around lines 444 - 469, Duplicate repo-to-summary logic exists between mapGithubSearchRepoToUserRepoSummary and mapGithubRestRepoToUserRepoSummary; extract the shared projection into a single helper used by both. Create a helper (e.g., buildUserRepoSummaryBase or projectRepoToSummaryBase) that accepts a repo-like object and returns the common fields (id, name, fullName, description, stars, forks, language, updatedAt, createdAt, isPrivate, url, owner and visibility resolution logic if it is identical); then replace the body of mapGithubSearchRepoToUserRepoSummary and mapGithubRestRepoToUserRepoSummary to call that helper and apply any mapper-specific overrides. Ensure you reference and use the existing symbols mapGithubSearchRepoToUserRepoSummary and mapGithubRestRepoToUserRepoSummary so callers are unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/dashboard/src/lib/github.functions.ts`:
- Around line 9672-9698: Replace the ad-hoc Math.min/Math.max sanitization for
perPage and page with the existing clampPerPage and clampPage helpers so NaN,
Infinity and fractional values are normalized; call clampPerPage(data.perPage)
to produce perPage and clampPage(data.page) to produce page and use those
sanitized values in the params passed to getCachedGitHubRequest and in the
context.octokit.rest.repos.listCommits call (keeping the same namespace/signal
keys like githubRevalidationSignalKeys.repoCode(data) and freshForMs =
githubCachePolicy.detail.staleTimeMs).
---
Nitpick comments:
In `@apps/dashboard/src/components/repo/repo-commits-list.tsx`:
- Around line 303-305: The avatar image's alt text (in repo-commits-list.tsx
where src={commit.author.avatarUrl}) is currently just the username; update the
alt prop to a more descriptive label for assistive tech such as "Avatar of
{commit.author.login}" or "{commit.author.login} avatar" so screen readers
clearly announce that this is an avatar image.
In `@apps/dashboard/src/lib/command-palette/use-command-items.ts`:
- Around line 208-225: Extract the duplicated repo-to-CommandItem mapping into a
single helper (e.g., createRepoCommandItem or buildRepoCommandItem) and use it
wherever repos are mapped to command items (the places that currently do
items.push({...}) for result.repos). The helper should accept the repo object
and return an object with id: `repo:${repo.id}`, label: repo.fullName, group:
"Repositories", icon: CodeIcon, keywords: [repo.name, repo.owner, repo.language
?? ""].filter(Boolean), action: { type: "navigate", to:
`/${repo.owner}/${repo.name}` }, and meta: { language: repo.language, stars:
repo.stars, updatedAt: repo.updatedAt ?? undefined }; replace both in-file
mappings with calls to this helper to avoid duplication and keep
metadata/keywords consistent.
In `@apps/dashboard/src/lib/github.functions.ts`:
- Around line 444-469: Duplicate repo-to-summary logic exists between
mapGithubSearchRepoToUserRepoSummary and mapGithubRestRepoToUserRepoSummary;
extract the shared projection into a single helper used by both. Create a helper
(e.g., buildUserRepoSummaryBase or projectRepoToSummaryBase) that accepts a
repo-like object and returns the common fields (id, name, fullName, description,
stars, forks, language, updatedAt, createdAt, isPrivate, url, owner and
visibility resolution logic if it is identical); then replace the body of
mapGithubSearchRepoToUserRepoSummary and mapGithubRestRepoToUserRepoSummary to
call that helper and apply any mapper-specific overrides. Ensure you reference
and use the existing symbols mapGithubSearchRepoToUserRepoSummary and
mapGithubRestRepoToUserRepoSummary so callers are unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c59c7a1-1093-410b-9b90-66c1d8bac972
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
apps/dashboard/package.jsonapps/dashboard/src/components/repo/repo-commits-list.tsxapps/dashboard/src/lib/command-palette/use-command-items.tsapps/dashboard/src/lib/github.functions.tsapps/dashboard/src/lib/github.types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dashboard/package.json
- apps/dashboard/src/lib/github.types.ts
|
@claude review |
whys this stuck |
|
this looks nice, can we size down avatar? also remove skeleton as load more state, just add spinner in bottom side. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/dashboard/src/lib/github.functions.ts (1)
418-469: Extract a shared repo-summary mapper.These two functions now duplicate nearly the entire
UserRepoSummarymapping. A small shared helper would keep the REST and search shapes from drifting the next time this model changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/lib/github.functions.ts` around lines 418 - 469, Both mapGithubRestRepoToUserRepoSummary and mapGithubSearchRepoToUserRepoSummary duplicate the same mapping logic; extract a shared helper (e.g., buildUserRepoSummaryFromRaw) that takes a minimal common input (an object with id, name, full_name, description, stargazers_count, forks_count, language, updated_at, created_at, private, visibility, html_url and owner?.login) and returns a UserRepoSummary using the existing visibility logic and field fallbacks; then have mapGithubRestRepoToUserRepoSummary (for AuthenticatedUserRepo | ListForUserRepo | GetRepoItem) and mapGithubSearchRepoToUserRepoSummary (for SearchRepoItem) normalize their shapes into that helper (taking care that owner may be optional in SearchRepoItem and repo.private may be absent) and delegate the final mapping to it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard/src/components/repo/repo-commits-list.tsx`:
- Around line 69-71: The current useMemo flattens pages directly
(commitsQuery.data?.pages.flatMap(...)) which can produce duplicate commit SHAs
when page boundaries shift; change the useMemo that defines commits to merge
pages while de-duplicating by commit.sha (e.g., iterate pages in order and add
commits to a Map or Set keyed by sha, keeping the first occurrence to preserve
order) so the resulting commits array contains unique SHAs and avoids duplicate
React keys.
- Around line 141-143: The current unconditional throw of overviewQuery.error
causes an unnecessary error boundary when cached data exists; change the logic
around overviewQuery to only throw overviewQuery.error if overviewQuery.data is
null/undefined (i.e., no cached overview). Specifically update the check using
overviewQuery, overviewQuery.error and overviewQuery.data so that you only
rethrow the error when there is no overviewQuery.data, while keeping the
existing commitsQuery.isError handling as-is.
In `@apps/dashboard/src/lib/github.functions.ts`:
- Around line 9672-9673: Replace the manual Math.min/Math.max clamping with the
project's clamp helpers to correctly handle NaN, Infinity and float truncation:
use clampPerPage(data.perPage) for perPage and clampPage(data.page) for page
instead of Math.min/Math.max, since identityValidator does no runtime checks and
malformed query params can leak into Octokit and cache keys; update the
assignment sites where perPage and page are set (currently using data.perPage
and data.page) to call clampPerPage and clampPage respectively.
- Around line 9721-9724: The catch block handling commit listing currently only
checks for RequestError with status === 404; expand this to also treat status
=== 409 (Conflict) as a non-error case so empty repositories return { commits:
[], nextPage: null }. Update the catch logic that references RequestError and
error.status to test for (error.status === 404 || error.status === 409) and
return the empty result for both statuses, leaving other errors to rethrow or
propagate unchanged.
---
Nitpick comments:
In `@apps/dashboard/src/lib/github.functions.ts`:
- Around line 418-469: Both mapGithubRestRepoToUserRepoSummary and
mapGithubSearchRepoToUserRepoSummary duplicate the same mapping logic; extract a
shared helper (e.g., buildUserRepoSummaryFromRaw) that takes a minimal common
input (an object with id, name, full_name, description, stargazers_count,
forks_count, language, updated_at, created_at, private, visibility, html_url and
owner?.login) and returns a UserRepoSummary using the existing visibility logic
and field fallbacks; then have mapGithubRestRepoToUserRepoSummary (for
AuthenticatedUserRepo | ListForUserRepo | GetRepoItem) and
mapGithubSearchRepoToUserRepoSummary (for SearchRepoItem) normalize their shapes
into that helper (taking care that owner may be optional in SearchRepoItem and
repo.private may be absent) and delegate the final mapping to it.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 02ed296e-f4d1-4124-bfec-a6a102eb394d
📒 Files selected for processing (7)
apps/dashboard/src/components/pulls/detail/pull-detail-header.tsxapps/dashboard/src/components/repo/code-file-view.tsxapps/dashboard/src/components/repo/commits-link.tsxapps/dashboard/src/components/repo/latest-commit-bar.tsxapps/dashboard/src/components/repo/repo-commits-list.tsxapps/dashboard/src/lib/github.functions.tsapps/dashboard/src/lib/protected-auth-cache.ts
✅ Files skipped from review due to trivial changes (2)
- apps/dashboard/src/components/pulls/detail/pull-detail-header.tsx
- apps/dashboard/src/lib/protected-auth-cache.ts
| const commits = useMemo( | ||
| () => commitsQuery.data?.pages.flatMap((page) => page.commits) ?? [], | ||
| [commitsQuery.data], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "repo-commits-list.tsx" | head -5Repository: stylessh/diffkit
Length of output: 119
🏁 Script executed:
wc -l ./apps/dashboard/src/components/repo/repo-commits-list.tsxRepository: stylessh/diffkit
Length of output: 123
🏁 Script executed:
cat -n ./apps/dashboard/src/components/repo/repo-commits-list.tsxRepository: stylessh/diffkit
Length of output: 14351
🏁 Script executed:
find . -type f -name "github.query*" | head -5Repository: stylessh/diffkit
Length of output: 101
🏁 Script executed:
wc -l ./apps/dashboard/src/lib/github.query.tsRepository: stylessh/diffkit
Length of output: 105
🏁 Script executed:
rg -n "githubRepoCommitsQueryOptions" ./apps/dashboard/src/lib/github.query.ts -A 30Repository: stylessh/diffkit
Length of output: 936
🏁 Script executed:
rg -n "getRepoCommits" ./apps/dashboard/src/lib/github.query.ts -B 5 -A 20 | head -100Repository: stylessh/diffkit
Length of output: 1453
🏁 Script executed:
find . -type f -name "github.functions*" | head -5Repository: stylessh/diffkit
Length of output: 105
🏁 Script executed:
rg -n "^export.*getRepoCommits|^function getRepoCommits|^const getRepoCommits" ./apps/dashboard/src/lib/github.functions.ts -A 25 | head -100Repository: stylessh/diffkit
Length of output: 1149
🏁 Script executed:
rg -n "^export.*getRepoCommits|^function getRepoCommits|^const getRepoCommits" ./apps/dashboard/src/lib/github.functions.ts -A 50 | head -150Repository: stylessh/diffkit
Length of output: 1973
🏁 Script executed:
rg -n "^export.*getRepoCommits|^function getRepoCommits|^const getRepoCommits" ./apps/dashboard/src/lib/github.functions.ts -A 75 | head -200Repository: stylessh/diffkit
Length of output: 2785
De-duplicate merged pages to avoid duplicate keys when page boundaries shift.
With offset-based GitHub pagination, new commits arriving between fetchNextPage() calls can shift page boundaries, causing the same commit SHA to appear across multiple pages. Flattening pages without de-duplication would render duplicate keys at line 231, causing React warnings. Normalize by sha while merging pages to prevent this.
Suggested fix
- const commits = useMemo(
- () => commitsQuery.data?.pages.flatMap((page) => page.commits) ?? [],
- [commitsQuery.data],
- );
+ const commits = useMemo(() => {
+ const seen = new Set<string>();
+ const merged: RepoCommitSummary[] = [];
+
+ for (const page of commitsQuery.data?.pages ?? []) {
+ for (const commit of page.commits) {
+ if (seen.has(commit.sha)) continue;
+ seen.add(commit.sha);
+ merged.push(commit);
+ }
+ }
+
+ return merged;
+ }, [commitsQuery.data]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const commits = useMemo( | |
| () => commitsQuery.data?.pages.flatMap((page) => page.commits) ?? [], | |
| [commitsQuery.data], | |
| const commits = useMemo(() => { | |
| const seen = new Set<string>(); | |
| const merged: RepoCommitSummary[] = []; | |
| for (const page of commitsQuery.data?.pages ?? []) { | |
| for (const commit of page.commits) { | |
| if (seen.has(commit.sha)) continue; | |
| seen.add(commit.sha); | |
| merged.push(commit); | |
| } | |
| } | |
| return merged; | |
| }, [commitsQuery.data]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/src/components/repo/repo-commits-list.tsx` around lines 69 -
71, The current useMemo flattens pages directly
(commitsQuery.data?.pages.flatMap(...)) which can produce duplicate commit SHAs
when page boundaries shift; change the useMemo that defines commits to merge
pages while de-duplicating by commit.sha (e.g., iterate pages in order and add
commits to a Map or Set keyed by sha, keeping the first occurrence to preserve
order) so the resulting commits array contains unique SHAs and avoids duplicate
React keys.
| if (overviewQuery.error) throw overviewQuery.error; | ||
| if (commitsQuery.isError && !commitsQuery.isFetchNextPageError) { | ||
| throw commitsQuery.error; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the specific file
find . -name "repo-commits-list.tsx" -type fRepository: stylessh/diffkit
Length of output: 119
🏁 Script executed:
# Check the file structure to understand the repo
git ls-files | grep -E "(dashboard|repo)" | head -20Repository: stylessh/diffkit
Length of output: 835
🏁 Script executed:
# Look for the specific lines mentioned
if [ -f "apps/dashboard/src/components/repo/repo-commits-list.tsx" ]; then
wc -l "apps/dashboard/src/components/repo/repo-commits-list.tsx"
fiRepository: stylessh/diffkit
Length of output: 121
🏁 Script executed:
# Read the specific lines and surrounding context
sed -n '130,155p' "apps/dashboard/src/components/repo/repo-commits-list.tsx"Repository: stylessh/diffkit
Length of output: 1055
🏁 Script executed:
# Also check the imports to understand what useQuery is being used
head -30 "apps/dashboard/src/components/repo/repo-commits-list.tsx"Repository: stylessh/diffkit
Length of output: 1074
🏁 Script executed:
# Let's look for where overviewQuery is initialized to understand its usage
grep -n "useQuery\|useInfiniteQuery" "apps/dashboard/src/components/repo/repo-commits-list.tsx" | head -20Repository: stylessh/diffkit
Length of output: 210
🏁 Script executed:
# Get more context around the error handling section and how data is used
sed -n '1,180p' "apps/dashboard/src/components/repo/repo-commits-list.tsx" | tail -50Repository: stylessh/diffkit
Length of output: 1867
🏁 Script executed:
# Search for where overviewQuery.data is actually used in the render
rg "overviewQuery\.(data|error)" "apps/dashboard/src/components/repo/repo-commits-list.tsx" -A 2 -B 2Repository: stylessh/diffkit
Length of output: 395
🏁 Script executed:
# Let's see the full initialization of overviewQuery and the data usage
sed -n '50,120p' "apps/dashboard/src/components/repo/repo-commits-list.tsx"Repository: stylessh/diffkit
Length of output: 1825
🏁 Script executed:
# Search for how repoData is used throughout the component
rg "repoData" "apps/dashboard/src/components/repo/repo-commits-list.tsx" -nRepository: stylessh/diffkit
Length of output: 207
🏁 Script executed:
# Let's check how repoData is actually used in the JSX return
sed -n '145,220p' "apps/dashboard/src/components/repo/repo-commits-list.tsx"Repository: stylessh/diffkit
Length of output: 2757
🏁 Script executed:
# Also check React Query documentation to confirm the behavior - search for any comments or notes in the codebase about this pattern
rg "isFetchNextPageError\|background.*refetch\|cached.*data" "apps/dashboard/src/components/repo/" -iRepository: stylessh/diffkit
Length of output: 42
🏁 Script executed:
# Let's verify the exact lines and understand how the component falls back on missing data
sed -n '138,200p' "apps/dashboard/src/components/repo/repo-commits-list.tsx"Repository: stylessh/diffkit
Length of output: 2239
🌐 Web query:
TanStack React Query useQuery behavior cached data background refetch error
💡 Result:
In TanStack React Query's useQuery, when a background refetch fails due to an error (e.g., triggered by window focus, mount if stale, or refetchInterval), the query retains the previously cached successful data while setting status to 'error'. The component continues to render the stale cached data alongside the error object. Key states after background refetch error: - status: 'error' - data: stale data from last successful fetch - error: the error from the failed refetch - isError: true - isRefetchError: true (if during refetch) - isFetching: true (during fetch), then idle This optimistic behavior prioritizes showing stale data over no data. Initial fetches without prior cache show loading then error with data: undefined. Retries apply on initial failure (default 3 attempts). Failed background refetches do not trigger retries by default unless configured. To control: - Use retry: false to disable retries. - retryOnMount: false prevents retry on mount after error. - staleTime: Infinity prevents background refetches. - refetchOnWindowFocus: false etc. to disable triggers. - Manually refetch with refetch {throwOnError: true} to handle errors differently. Official docs confirm: status='error' means query attempt resulted in error, but prior data persists if available (13,3,5). Maintainer TkDodo: "React Query will not throw away cached data. If a background refetch fails... data: " (1).
Citations:
- 1: Using cached data after a request has failed TanStack/query#5780
- 2: https://react-query.tanstack.com/reference/useQuery.md
- 3: https://tanstack.com/query/latest/docs/framework/react/guides/query-retries
- 4: https://tanstack.com/query/v4/docs/framework/react/reference/useQuery
- 5: Respecting staletime for errors TanStack/query#4956
- 6: https://react-query.tanstack.com/guides/important-defaults
- 7: https://tanstack.dev/query/v5/docs/framework/react/reference/useQuery
- 8: https://tanstack.com/query/v5/docs/react/guides/background-fetching-indicators
- 9: https://tanstack.com/query/v5/docs/framework/react/guides/query-retries
Only throw the overview error when there is no cached overview data.
useQuery retains overviewQuery.data after a background refetch fails. Throwing overviewQuery.error unconditionally bounces users into the error boundary even though the page has fallback UI and can render with cached data. The component already gracefully handles missing repoData at line 196 with a fallback display.
Suggested fix
-if (overviewQuery.error) throw overviewQuery.error;
+if (overviewQuery.error && !overviewQuery.data) {
+ throw overviewQuery.error;
+}
if (commitsQuery.isError && !commitsQuery.isFetchNextPageError) {
throw commitsQuery.error;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/src/components/repo/repo-commits-list.tsx` around lines 141 -
143, The current unconditional throw of overviewQuery.error causes an
unnecessary error boundary when cached data exists; change the logic around
overviewQuery to only throw overviewQuery.error if overviewQuery.data is
null/undefined (i.e., no cached overview). Specifically update the check using
overviewQuery, overviewQuery.error and overviewQuery.data so that you only
rethrow the error when there is no overviewQuery.data, while keeping the
existing commitsQuery.isError handling as-is.
| } catch (error) { | ||
| if (error instanceof RequestError && error.status === 404) { | ||
| return { commits: [], nextPage: null }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/dashboard/src/lib/github.functions.ts | sed -n '9700,9750p'Repository: stylessh/diffkit
Length of output: 1973
🏁 Script executed:
cat -n apps/dashboard/src/lib/github.functions.ts | sed -n '9650,9730p'Repository: stylessh/diffkit
Length of output: 2877
🏁 Script executed:
rg "409" apps/dashboard/src/lib/github.functions.ts -B 3 -A 3Repository: stylessh/diffkit
Length of output: 306
🏁 Script executed:
rg "unborn|empty.*repo|empty.*ref" apps/dashboard/src/lib/github.functions.ts -i -B 2 -A 2Repository: stylessh/diffkit
Length of output: 42
🏁 Script executed:
fd -t f "github" apps/dashboard --type f | grep -i test | head -20Repository: stylessh/diffkit
Length of output: 451
🏁 Script executed:
rg "getRepoCommits" apps/dashboard/src/lib/*.test.ts -B 2 -A 5Repository: stylessh/diffkit
Length of output: 42
🌐 Web query:
GitHub API repos.listCommits empty repository unborn ref error code 409 404
💡 Result:
The GitHub REST API endpoint GET /repos/{owner}/{repo}/commits (repos.listCommits) returns a 409 Conflict error with the message "Git Repository is empty." when the repository has no commits. This occurs for completely empty repositories (no initial commit or branches). The official documentation lists 409 as a possible response code for this endpoint. A 404 Not Found may occur if the repository itself does not exist or you lack access to it. No sources explicitly mention an "unborn ref" error message for this endpoint. "Unborn ref" is a Git concept for a branch ref pointing to no commits yet, which may appear in other contexts like git log on unborn branches locally, or potentially in other GitHub API endpoints involving refs (e.g., git/refs), but not confirmed for repos.listCommits. If you're seeing "unborn ref," it might be from client-side Git operations, a different endpoint like GET /repos/{owner}/{repo}/commits/{ref} with an invalid ref, or a wrapper library. To handle empty repositories: - Check response status: expect 409 and empty array [] for 200 on repos with commits. - To check if empty beforehand, attempt GET /repos/{owner}/{repo}/git/refs/heads/{default-branch} (404 if no branch) or use repository metadata like default_branch presence. Example curl for empty repo (from GitHub issue): curl https://api.github.com/repos/gr2m/empty-repo-for-testing/commits returns HTTP 409 with {"message": "Git Repository is empty.", "documentation_url": "https://docs.github.com/rest/reference/repos#list-commits"}
Citations:
- 1: [Schema Inaccuracy]
GET /repos/{owner}/{repo}/commitscan respond with 409 if repository is empty github/rest-api-description#385 - 2: https://docs.github.com/v3/repos/commits
- 3: https://docs.github.com/en/rest/commits/commits?apiVersion=2026-03-10
Handle empty repositories by catching both 404 and 409 errors.
The GitHub API returns a 409 Conflict error when listing commits on an empty repository (as opposed to 404 which indicates repo not found or access denied). The current code only catches 404, which causes empty repositories to throw an error instead of rendering an empty commit history.
Suggested fix
- } catch (error) {
- if (error instanceof RequestError && error.status === 404) {
+ } catch (error) {
+ if (
+ error instanceof RequestError &&
+ (error.status === 404 || error.status === 409)
+ ) {
return { commits: [], nextPage: null };
}
throw error;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| if (error instanceof RequestError && error.status === 404) { | |
| return { commits: [], nextPage: null }; | |
| } | |
| } catch (error) { | |
| if ( | |
| error instanceof RequestError && | |
| (error.status === 404 || error.status === 409) | |
| ) { | |
| return { commits: [], nextPage: null }; | |
| } | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/src/lib/github.functions.ts` around lines 9721 - 9724, The
catch block handling commit listing currently only checks for RequestError with
status === 404; expand this to also treat status === 409 (Conflict) as a
non-error case so empty repositories return { commits: [], nextPage: null }.
Update the catch logic that references RequestError and error.status to test for
(error.status === 404 || error.status === 409) and return the empty result for
both statuses, leaving other errors to rethrow or propagate unchanged.
# Conflicts: # apps/dashboard/src/components/layouts/dashboard-tabs.tsx # apps/dashboard/src/components/pulls/detail/pull-detail-header.tsx # apps/dashboard/src/lib/github.functions.ts # apps/dashboard/src/lib/tab-store.ts
2 participants
changes
major changes:
minor tweak:
testing
pnpm --filter dashboard check-types ✅pnpm exec biome check --write ... ✅Screenshots
Summary by CodeRabbit
New Features
Style
Chores