packages/web/src/openapi/publicApiSchemas.ts (1)

1-58: LGTM: OpenAPI schema exports for commit authors are consistent.

The added public schemas are well-structured:

• query schema (for params),
• author object schema,
• response as an array of authors.

Please just make sure the route handler at GET /api/commits/authors wires these exact schemas when generating/validating OpenAPI responses/requests (so docs and runtime stay aligned).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/web/src/openapi/publicApiSchemas.ts` around lines 1 - 58, Ensure the
GET /api/commits/authors route handler uses the exported OpenAPI schemas: wire
publicListCommitAuthorsQuerySchema for request query validation,
publicCommitAuthorSchema for individual author objects, and
publicListCommitAuthorsResponseSchema for the response body/schema in the
OpenAPI generation and any runtime validation middleware (look for the route
handler referencing GET /api/commits/authors and update the OpenAPI/validator
config there to reference these three symbols).

packages/web/src/components/ui/avatar.tsx (2)

8-26: Potential ref regression: wrapper drops/changes ref behavior.

Avatar is now a plain function wrapper around AvatarPrimitive.Root with no forwardRef. If any call sites rely on passing ref to <Avatar /> (common for focus/scroll measurement), that ref may no longer work as before.

Please verify (1) whether <Avatar /> is used with ref={...} anywhere, and (2) whether React 19 in this repo actually treats ref as a normal prop for function components with the current TS setup.

If you don’t intend to support refs, consider changing the prop type to React.ComponentPropsWithoutRef<typeof AvatarPrimitive.Root> to prevent accidental ref usage.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/web/src/components/ui/avatar.tsx` around lines 8 - 26, The Avatar
wrapper drops ref forwarding and may break callers relying on ref; update Avatar
to either forward refs to AvatarPrimitive.Root using React.forwardRef
(preserving ref behavior for focus/measurement) or change the prop type to
React.ComponentPropsWithoutRef<typeof AvatarPrimitive.Root> to explicitly
disallow refs; locate the Avatar function and modify its signature and export to
use React.forwardRef (forward the ref into AvatarPrimitive.Root) or adjust the
props type as suggested, and ensure the data-size/data-slot/className spread
remain unchanged when applying the fix.

---

57-70: Badge sizing depends on group data on the ancestor.

AvatarBadge uses group-data-[size=...] /avatar:* selectors and assumes it’s rendered under an element with data-size and class="group/avatar". This is likely correct if call sites nest <AvatarBadge /> inside <Avatar />, but it’s worth ensuring the intended structure is documented/consistent.

If any call sites render AvatarBadge outside Avatar, badge sizing could break silently.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/web/src/components/ui/avatar.tsx` around lines 57 - 70, AvatarBadge
relies on ancestor group data selectors (group-data-[size=...]/avatar and
data-size on an element with class "group/avatar") so badge sizing will break if
it's rendered outside Avatar; update AvatarBadge to either accept an explicit
size prop (e.g., size: "sm" | "default" | "lg") and apply the corresponding size
classes when provided, or add a runtime check in AvatarBadge (useEffect) that
warns when no ancestor with data-size and class "group/avatar" is found so
callers know to nest it inside Avatar; also add a short doc comment on
AvatarBadge and update Avatar to pass size down if it renders AvatarBadge.

packages/web/src/features/git/listCommitsApi.ts (1)

78-107: Check correctness of --author/--grep option ordering and escaping assumptions.

The implementation correctly documents that --author and --grep are interpreted as git regex (POSIX BRE + GNU extensions) and that callers must escape metacharacters for literal matching.

One concern to double-check: you currently build sharedOptions into Object.entries(...), and while JS preserves insertion order, the presence/absence of author vs query can alter the relative order of --author/--grep vs --regexp-ignore-case. If ordering ever matters for parsing, it could be brittle. Tests cover some ordering, but not all combinations.

At minimum, ensure there’s test coverage for “author only”, “query only”, and “author+query+since/until” with exact argv order expectations where it matters.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/web/src/features/git/listCommitsApi.ts` around lines 78 - 107, The
ordering of --author/--grep vs their --regexp-ignore-case flag is brittle
because sharedOptions relies on Object.entries insertion order; change the arg
construction in listCommitsApi (the sharedOptions/logArgs logic) to explicitly
append flags in a stable sequence (e.g., if author present push '--author=...'
then push '--regexp-ignore-case', likewise for grep) rather than iterating
Object.entries, and add unit tests for the three scenarios ("author only",
"query only", and "author+query+since/until") asserting the exact logArgs/argv
order produced by the function so regressions are caught.

packages/web/src/app/(app)/browse/[...path]/components/codePreviewPanel.tsx (1)

16-30: Performance risk: extra git log call per code preview render.

codePreviewPanel now issues an additional listCommits(..., maxCount: 1) request for the current path and revisionName. Depending on how many CodePreviewPanel instances render per page, this could become a significant load on git operations.

If this component is rendered frequently (e.g., multiple open files, client transitions, pagination), consider:

• caching the “latest commit for this (repo, path, ref)” at the API or request layer, or
• computing it in a higher-level data fetch and passing it down.

Not necessarily blocker, but worth watching.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/web/src/app/`(app)/browse/[...path]/components/codePreviewPanel.tsx
around lines 16 - 30, CodePreviewPanel currently issues an additional
listCommits(...) call per render (using path and revisionName) which can be
heavy; to fix, remove or memoize that call by either (1) moving the
latest-commit lookup out of CodePreviewPanel into a higher-level loader or
parent component and pass the commit data down, or (2) add a short-lived cache
in the request layer/shared fetch helper so repeated listCommits(repo, path,
ref, maxCount:1) requests reuse results; update CodePreviewPanel to use the
already-fetched commit data or the cached fetch together with the existing
getFileSource and getRepoInfoByName calls to avoid per-instance git log calls.

packages/web/src/app/(app)/browse/[...path]/components/commitsPagination.tsx (1)

43-59: Prefer <button disabled> over <span aria-disabled="true"> for disabled prev/next.

A native disabled <button> is keyboard-skipped automatically and is consistently announced as disabled by screen readers; the current <span aria-disabled="true"> still receives focus traversal in some flows and relies entirely on visual styling. Minor a11y polish.

♻️ Suggested refactor

-            ) : (
-                <span className={cn(disabledClass)} aria-disabled="true">
-                    <ChevronLeft className="h-4 w-4" />
-                    Previous
-                </span>
-            )}
+            ) : (
+                <button type="button" disabled className={cn(disabledClass)}>
+                    <ChevronLeft className="h-4 w-4" />
+                    Previous
+                </button>
+            )}
@@
-            ) : (
-                <span className={cn(disabledClass)} aria-disabled="true">
-                    Next
-                    <ChevronRight className="h-4 w-4" />
-                </span>
-            )}
+            ) : (
+                <button type="button" disabled className={cn(disabledClass)}>
+                    Next
+                    <ChevronRight className="h-4 w-4" />
+                </button>
+            )}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/web/src/app/`(app)/browse/[...path]/components/commitsPagination.tsx
around lines 43 - 59, Replace the non-interactive disabled spans used for
Prev/Next with native disabled buttons so they are skipped by keyboard and
announced by assistive tech: where the code currently renders <span
className={cn(disabledClass)} aria-disabled="true"> for the previous and next
controls, change those branches to render <button type="button" disabled
className={cn(disabledClass)}> with the same children (ChevronLeft/ChevronRight
and text). Keep the enabled branches using Link (Link href={buildHref(page + 1,
extraParams)} className={linkClass}) and preserve linkClass, disabledClass,
hasNext/hasPrev (and page/extraParams) usage. Ensure no href is added to the
disabled button and retain the icon order/structure.

packages/web/src/app/(app)/browse/[...path]/page.tsx (1)

78-99: Consider whether to add searchParams validation, but note validation already exists at the API boundary.

In Next.js 16 App Router, searchParams values are string | string[] | undefined. The current type narrowing to string doesn't account for repeated query parameters (e.g., ?page=1&page=2 arrives as an array), which would be silently coerced by parseInt or template strings. However, similar page components in the codebase (signup, redeem, onboard) follow the same direct passthrough pattern. Since CommitsPanel ultimately calls the API route at packages/web/src/app/api/(server)/commits/route.ts, which validates via listCommitsQueryParamsSchema, the validation already occurs at the appropriate boundary. Adding Zod validation here would be stricter but would diverge from the established pattern where validation is centralized at API routes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/web/src/app/`(app)/browse/[...path]/page.tsx around lines 78 - 99,
The searchParams values in BrowsePage (BrowsePageProps -> searchParams) can be
string|string[]|undefined; update the coercion logic so you handle string[]
cases before parsing: for each param (searchParams.page, .author, .since,
.until) normalize to a single string by taking the first element when
Array.isArray(...) (or otherwise converting to string) and then parse page with
parseInt and assign author/since/until as undefined when empty; keep validation
at the API boundary but ensure BrowsePage doesn't silently coerce arrays (refer
to BrowsePage, searchParams, page, author, since, until, and
getBrowseParamsFromPathParam).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/web/src/app/(app)/browse/components/bottomPanel.tsx (1)

47-52: Minor: memoize fullHistoryHref.

Recomputed on every render; cheap but easy to stabilize:

♻️ Proposed tweak

-    const { repoName, revisionName, path } = useBrowseParams();
-    const fullHistoryHref = getBrowsePath({
-        repoName,
-        revisionName,
-        path,
-        pathType: 'commits',
-    });
+    const { repoName, revisionName, path } = useBrowseParams();
+    const fullHistoryHref = useMemo(
+        () => getBrowsePath({ repoName, revisionName, path, pathType: 'commits' }),
+        [repoName, revisionName, path],
+    );

(Add useMemo to the existing react import.)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/web/src/app/`(app)/browse/components/bottomPanel.tsx around lines 47
- 52, fullHistoryHref is recomputed on every render; wrap the call to
getBrowsePath in a useMemo to stabilize the value. Import useMemo from React
(add to the existing react import) and replace the direct assignment of
fullHistoryHref with a useMemo(() => getBrowsePath({ repoName, revisionName,
path, pathType: 'commits' }), [repoName, revisionName, path]); so the memo
depends on repoName, revisionName, and path and uses the existing getBrowsePath
reference in bottomPanel.tsx.

packages/web/src/app/(app)/browse/components/commitAuthors.ts (1)

5-13: Co-author trailers with leading whitespace will be missed.

Some commit conventions (and tools) emit indented trailers, e.g. when a commit body is wrapped/quoted. The regex anchors ^ directly to co-authored-by: and won't match Co-authored-by: …. If you want to be lenient, allow optional leading whitespace:

♻️ Proposed tweak

-    const regex = /^co-authored-by:\s*(.+?)\s*<(.+?)>\s*$/gim;
+    const regex = /^[ \t]*co-authored-by:\s*(.+?)\s*<(.+?)>\s*$/gim;

Not a blocker — standard git commit -s/--trailer-emitted trailers will already match.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/web/src/app/`(app)/browse/components/commitAuthors.ts around lines 5
- 13, The parseCoAuthors function's regex currently anchors to the line start
and misses indented trailers; update the regex used in parseCoAuthors to allow
optional leading whitespace (e.g. prefix with \s* after ^) so lines like " 
Co-authored-by: Name <email>" are matched; keep the existing case-insensitive
and global flags and ensure the rest of the capture groups (name/email) remain
unchanged so coAuthors.push({ name: match[1].trim(), email: match[2].trim() })
still works.

packages/web/src/app/(app)/browse/components/historyPanel.tsx (1)

51-66: Observer is reattached on every isFetchingNextPage flip.

Including isFetchingNextPage in the effect deps means the observer is destroyed and recreated each time a page starts/finishes fetching. It works, but you can drop it from the deps and read the latest value via a ref (or just data?.pages.length-style guard inside the callback) to avoid the churn. Optional.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/web/src/app/`(app)/browse/components/historyPanel.tsx around lines
51 - 66, The effect re-creates the IntersectionObserver whenever
isFetchingNextPage flips; remove isFetchingNextPage from the dependency array
and read its latest value via a ref instead. Create a ref like
isFetchingNextPageRef, update isFetchingNextPageRef.current whenever
isFetchingNextPage changes, then in the IntersectionObserver callback check
entries[0].isIntersecting && !isFetchingNextPageRef.current before calling
fetchNextPage; keep hasNextPage and fetchNextPage (and sentinelRef) in the
effect deps so the observer is only recreated when those truly change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/web/src/features/git/getPathTypeApi.ts (1)

75-82: Preserve original error via cause when re-throwing.

The wrapping Error discards the original stack trace, which makes diagnosing unexpected simple-git failures harder in production logs. Pass the original error through cause.

♻️ Proposed refactor

-            if (error instanceof Error) {
-                throw new Error(
-                    `Failed to resolve path type for ${repoName}:${path}: ${error.message}`,
-                );
-            }
-            throw new Error(
-                `Failed to resolve path type for ${repoName}:${path}: ${errorMessage}`,
-            );
+            throw new Error(
+                `Failed to resolve path type for ${repoName}:${path}: ${errorMessage}`,
+                error instanceof Error ? { cause: error } : undefined,
+            );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/web/src/features/git/getPathTypeApi.ts` around lines 75 - 82, The
thrown wrapper Errors in the path resolution logic lose the original error
stack; update both throw sites in getPathTypeApi (the branch checking error
instanceof Error and the fallback throwing with errorMessage) to include the
original error as the cause (use the Error constructor's { cause: ... } option),
so the original exception/stack is preserved when re-throwing from the function
that resolves path type (the two throw new Error(...) lines).