Merged
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 client-side multi-step onboarding UI and supporting components, introduces onboarding types and validators, proxies several backend API endpoints through Next.js routes, exposes Changes
Sequence DiagramsequenceDiagram
participant User
participant Client as OnboardingPage
participant API as Next API Route
participant Backend as Backend API
User->>Client: Open onboarding page
Client->>Client: Hydration & auth check (activeKey / currentUser)
alt no access
Client->>User: Redirect to / or /settings/credentials
end
Client->>API: GET /api/organization?page=X
API->>Backend: GET /api/v1/organizations?page=X
Backend-->>API: organizations list
API-->>Client: organizations response
Client->>Client: Render OrganizationList (infinite scroll)
User->>Client: Select organization
Client->>API: GET /api/organization/{orgId}/projects
API->>Backend: GET /api/v1/projects/organization/{orgId}
Backend-->>API: projects list
API-->>Client: projects response
Client->>Client: Render ProjectList
User->>Client: Submit OnboardingForm
Client->>Client: Client-side validation
Client->>API: POST /api/onboard { payload }
API->>Backend: POST /api/v1/onboard { payload }
Backend-->>API: { success, data }
API-->>Client: { success, data }
alt success
Client->>Client: Show OnboardingSuccess (display api_key)
User->>Client: Copy API key
else failure
Client->>Client: Show errors/toast
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 8
🧹 Nitpick comments (2)
app/components/icons/common/EyeOffIcon.tsx (1)
1-3: Consider extracting sharedIconPropsinterface.The
IconPropsinterface is duplicated acrossEyeOffIcon.tsx,EyeIcon.tsx, andArrowLeftIcon.tsx. Consider extracting it to a shared location (e.g.,app/components/icons/types.ts) to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/icons/common/EyeOffIcon.tsx` around lines 1 - 3, Extract the duplicated IconProps interface into a shared module and update the icon components to import it: create a new file (e.g., app/components/icons/types.ts) exporting interface IconProps { className?: string }, then replace the local IconProps declarations in EyeOffIcon.tsx, EyeIcon.tsx and ArrowLeftIcon.tsx with an import of IconProps from that new module and update any component props types (e.g., EyeOffIcon) to use the shared IconProps.app/components/settings/onboarding/StepIndicator.tsx (1)
16-37: Consider adding accessibility attributes for screen readers.The step indicator conveys state visually but screen reader users won't know which step is active or completed. Consider adding
aria-current="step"for the active step:♿ Proposed accessibility enhancement
return ( - <div className="flex items-center gap-2"> + <div + className="flex items-center gap-2" + aria-current={active ? "step" : undefined} + > <div className={`w-7 h-7 rounded-full flex items-center justify-center text-xs font-semibold transition-colors ${🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/settings/onboarding/StepIndicator.tsx` around lines 16 - 37, The StepIndicator component currently conveys state only visually; update it to include accessibility attributes: add role="listitem" to the outer container and add an aria-label on the circle (or container) that includes the step number/label and state (e.g., "Step 2: Billing, current" or "Step 3: Payment, completed"), and set aria-current="step" when the active prop is true (omit or set to false otherwise) so screen readers can announce the active step; locate these changes in the StepIndicator component around the outer div and the circle div where completed/active/number/label are rendered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/`(main)/settings/onboarding/page.tsx:
- Around line 42-68: The effect handling organization load currently ignores the
error returned by usePaginatedList, causing failures to be treated like an empty
list; update the useEffect that reads isLoadingOrgs and organizations.length to
also read the hook's error (from usePaginatedList.error) and bail to an error
state instead of "form" when error is present—i.e., when error exists call
setView("error") (or a retryable error view) and ensure the view logic in this
component (view, setView) surfaces a retry button that triggers loadMore or a
refetch; make sure the dependency array includes the error value so the effect
reacts to load failures.
- Around line 83-103: The current handler clears projects, calls
setView("projects") and swallows all errors so failures look like an empty org;
fix by introducing an explicit projectsError state and only switch to the
"projects" view after a successful fetch: call setIsLoadingProjects(true) first,
perform the apiFetch inside try, on success call setProjects(result.data) and
setView("projects"), on failure setProjectsError(...) in the catch (and do not
call setView), and always clear setIsLoadingProjects(false) in finally;
reference the existing handlers setSelectedOrg, setView, setIsLoadingProjects,
setProjects and the apiFetch call to locate where to add the projectsError state
and the adjusted control flow.
In `@app/api/organization/`[orgId]/projects/route.ts:
- Around line 9-13: Validate and sanitize the orgId before interpolating it into
the apiClient call: ensure the route handler extracts orgId, check it against an
allowed pattern (e.g., UUID or numeric regex) and reject with a 400/appropriate
error if it fails, or else use a safe encoding (e.g., encodeURIComponent) before
passing to apiClient(`/api/v1/projects/organization/${orgId}`); update the
handler logic around the orgId extraction and the apiClient invocation to
perform this validation/sanitization so path traversal or query injection cannot
occur.
In `@app/components/Field.tsx`:
- Around line 31-56: Add an explicit id prop to the input and set the label's
htmlFor to that id, then wire the error paragraph to the input via
aria-describedby and mark the input aria-invalid when there is an error;
specifically update the JSX around the label, input and error in Field.tsx (the
elements using label, input, value, onChange, inputType, placeholder, disabled,
error) so the input receives id={idOrGenerated} (or accept an id prop), the
label uses htmlFor={idOrGenerated}, the input includes aria-describedby={error ?
`${idOrGenerated}-error` : undefined} and aria-invalid={!!error}, and the error
<p> uses id={`${idOrGenerated}-error`} so assistive tech associates the label
and validation message with the control.
In `@app/components/settings/onboarding/OrganizationList.tsx`:
- Around line 41-64: The scrollRef passed to useInfiniteScroll is on a
non-scrollable div so the hook never observes real scrolling; either attach
scrollRef to the actual scroll container used by the page wrapper or make this
list container scrollable by giving the element a constrained height and
overflow-y-auto so scrollRef.current is the real scroller; update the ref usage
(scrollRef) in OrganizationList.tsx and ensure useInfiniteScroll is still called
with that same ref so pagination advances correctly.
In `@app/components/settings/onboarding/ProjectList.tsx`:
- Around line 5-10: ProjectList currently only exposes onBack and renders each
project as a passive div so the parent can't know which project was chosen;
update the ProjectListProps interface to include an onSelectProject: (project:
Project) => void (or optionally onSelectProject?: ...) and then wire each
rendered project card in the ProjectList component to call
onSelectProject(project) via an interactive control (e.g., a button or clickable
card with onClick and keyboard accessibility handlers) so selection is
communicated to the parent; ensure the project render code that currently maps
over projects calls this new handler and that the prop name matches
ProjectListProps.
In `@app/lib/context/AuthContext.tsx`:
- Around line 52-70: The effect in AuthContext's useEffect keeps stale
currentUser when the active apiKey disappears or when apiFetch("/api/users/me",
apiKey) fails; update the effect so that when there is no apiKey or !isHydrated
you call setCurrentUser(null) to clear auth, and in the catch block call
setCurrentUser(null) (respecting the cancelled flag) so failures don't leave
prior user data; keep the existing cancelled boolean logic and only set state
when !cancelled, and reference the existing symbols useEffect, apiKeys,
isHydrated, apiFetch<User>("/api/users/me", apiKey), and setCurrentUser to
locate the change.
In `@app/lib/utils.ts`:
- Around line 164-167: Trim the email before testing so leading/trailing
whitespace doesn't cause valid addresses to fail: update isValidEmail to
normalize the input (e.g., const normalized = email.trim()) and run
EMAIL_REGEX.test(normalized) (keep using the existing EMAIL_REGEX and
MIN_PASSWORD_LENGTH symbols to locate the function).
---
Nitpick comments:
In `@app/components/icons/common/EyeOffIcon.tsx`:
- Around line 1-3: Extract the duplicated IconProps interface into a shared
module and update the icon components to import it: create a new file (e.g.,
app/components/icons/types.ts) exporting interface IconProps { className?:
string }, then replace the local IconProps declarations in EyeOffIcon.tsx,
EyeIcon.tsx and ArrowLeftIcon.tsx with an import of IconProps from that new
module and update any component props types (e.g., EyeOffIcon) to use the shared
IconProps.
In `@app/components/settings/onboarding/StepIndicator.tsx`:
- Around line 16-37: The StepIndicator component currently conveys state only
visually; update it to include accessibility attributes: add role="listitem" to
the outer container and add an aria-label on the circle (or container) that
includes the step number/label and state (e.g., "Step 2: Billing, current" or
"Step 3: Payment, completed"), and set aria-current="step" when the active prop
is true (omit or set to false otherwise) so screen readers can announce the
active step; locate these changes in the StepIndicator component around the
outer div and the circle div where completed/active/number/label are rendered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e643e87-173c-4687-bb3e-4be7e9ddf4a3
📒 Files selected for processing (22)
app/(main)/settings/onboarding/page.tsxapp/api/onboard/route.tsapp/api/organization/[orgId]/projects/route.tsapp/api/organization/route.tsapp/api/users/me/route.tsapp/components/Button.tsxapp/components/Field.tsxapp/components/Sidebar.tsxapp/components/icons/common/ArrowLeftIcon.tsxapp/components/icons/common/EyeIcon.tsxapp/components/icons/common/EyeOffIcon.tsxapp/components/icons/index.tsxapp/components/index.tsapp/components/settings/onboarding/OnboardingForm.tsxapp/components/settings/onboarding/OnboardingSuccess.tsxapp/components/settings/onboarding/OrganizationList.tsxapp/components/settings/onboarding/ProjectList.tsxapp/components/settings/onboarding/StepIndicator.tsxapp/components/settings/onboarding/index.tsapp/lib/context/AuthContext.tsxapp/lib/types/onboarding.tsapp/lib/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/(main)/settings/onboarding/page.tsx (2)
81-100:⚠️ Potential issue | 🟠 MajorProject request failures still look like an empty organization.
This handler switches to
"projects"and clears the list before the request succeeds, then swallows every exception. A 401/500/network error is therefore rendered as the same empty-state UI as a real org with no projects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`(main)/settings/onboarding/page.tsx around lines 81 - 100, fetchProjects currently clears projects, switches view to "projects", and swallows all exceptions, making request failures indistinguishable from a real empty org; change fetchProjects to preserve the previous projects/view until a successful response, set an explicit error state (e.g., setProjectsError or setIsProjectsLoadError) on non-success or caught exceptions, and only update setProjects and setView("projects") after verifying result.success and result.data; also ensure the finally block only flips setIsLoadingProjects(false) and that caught errors are logged or passed into the new error state so the UI can render an error message instead of the empty-state.
41-67:⚠️ Potential issue | 🟠 MajorStill need an explicit organization-load error path.
This branch only looks at
isLoadingOrgsandorganizations.length, so anyusePaginatedListfailure still falls through to"form". That makes an outage or missing-key state indistinguishable from a real empty org list and can send admins into duplicate-org creation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`(main)/settings/onboarding/page.tsx around lines 41 - 67, The effect handling org load doesn't handle errors: update the useEffect that watches isLoadingOrgs and organizations.length to also check the error state returned by usePaginatedList (e.g., isError or error) and setView to an explicit error view (e.g., setView("error")) when a load failure is detected; if the hook doesn't expose an error flag, extend usePaginatedList to return one (isError/error) and use that in the effect, ensuring you include the new dependency in the effect dependency array alongside isLoadingOrgs and organizations.length so the UI distinguishes real-empty lists from load failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/`(main)/settings/onboarding/page.tsx:
- Around line 32-50: The onboarding page currently starts fetching organizations
before the superuser auth check finishes; modify the logic so usePaginatedList
does not begin (and the UI stays in the "loading"/error auth state) until
useAuth reports auth resolved and the superuser check passes: use the isHydrated
and currentUser values from useAuth to gate the paginated load (e.g., add a
"skip" or "enabled" flag to the usePaginatedList call or move the hook into a
child component that is only rendered when isHydrated && currentUser?.role ===
'superuser'), and ensure view/activeKey remain "loading" or show an auth error
if the check fails instead of allowing setProjects/usePaginatedList to run
prematurely.
- Around line 81-115: The fetchProjects useCallback can allow stale responses to
overwrite a newer org's projects; modify fetchProjects to track the latest
request (e.g., add a requestsRef or an AbortControllerRef) so that each
invocation generates a unique token or abort signal, attach that to the apiFetch
call, and in the try/catch/finally only call setProjects and
setIsLoadingProjects if the token matches the latest ref (or apiFetch wasn't
aborted). Update fetchProjects (and any cleanup on handleBackToOrgs or unmount)
to cancel previous requests or ignore older completions so only the latest org's
response updates state.
---
Duplicate comments:
In `@app/`(main)/settings/onboarding/page.tsx:
- Around line 81-100: fetchProjects currently clears projects, switches view to
"projects", and swallows all exceptions, making request failures
indistinguishable from a real empty org; change fetchProjects to preserve the
previous projects/view until a successful response, set an explicit error state
(e.g., setProjectsError or setIsProjectsLoadError) on non-success or caught
exceptions, and only update setProjects and setView("projects") after verifying
result.success and result.data; also ensure the finally block only flips
setIsLoadingProjects(false) and that caught errors are logged or passed into the
new error state so the UI can render an error message instead of the
empty-state.
- Around line 41-67: The effect handling org load doesn't handle errors: update
the useEffect that watches isLoadingOrgs and organizations.length to also check
the error state returned by usePaginatedList (e.g., isError or error) and
setView to an explicit error view (e.g., setView("error")) when a load failure
is detected; if the hook doesn't expose an error flag, extend usePaginatedList
to return one (isError/error) and use that in the effect, ensuring you include
the new dependency in the effect dependency array alongside isLoadingOrgs and
organizations.length so the UI distinguishes real-empty lists from load
failures.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19c42398-9987-4f70-a0ec-e513a4dd07dc
📒 Files selected for processing (7)
app/(main)/configurations/page.tsxapp/(main)/configurations/prompt-editor/page.tsxapp/(main)/document/page.tsxapp/(main)/settings/onboarding/page.tsxapp/components/ConfigSelector.tsxapp/components/settings/onboarding/OnboardingSuccess.tsxapp/hooks/index.ts
✅ Files skipped from review due to trivial changes (4)
- app/(main)/configurations/prompt-editor/page.tsx
- app/(main)/document/page.tsx
- app/(main)/configurations/page.tsx
- app/hooks/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/settings/onboarding/OnboardingSuccess.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (4)
app/(main)/settings/onboarding/page.tsx (3)
100-110:⚠️ Potential issue | 🟠 MajorGate data fetching until superuser auth resolves.
This issue was previously flagged:
usePaginatedListruns immediately (line 79), butcurrentUseris populated asynchronously after hydration. The check at line 107 only redirects whencurrentUseris truthy and not a superuser—meaning regular users can see/interact with onboarding data until/api/users/mecompletes, and a failed user lookup leaves the page accessible indefinitely.Consider adding an
enabled/skipflag tousePaginatedListor rendering the paginated content only after auth resolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`(main)/settings/onboarding/page.tsx around lines 100 - 110, The paginated onboarding fetch (usePaginatedList) runs before auth resolves allowing non-superusers to see data; update the component to only call/use usePaginatedList after auth resolution by adding an enabled/skip condition tied to auth state (e.g., isHydrated && currentUser !== undefined && currentUser?.is_superuser) or by rendering the paginated list only when currentUser is known; ensure you treat a failed lookup (currentUser === null) as resolved and block access for non-superusers as well so the fetch never runs for unauthorized users.
112-135:⚠️ Potential issue | 🟠 MajorDiscard stale project responses and surface fetch errors.
Two previously flagged issues remain:
Stale responses: If a user opens org A, navigates back before completion, then opens org B, the first response can overwrite org B's list. Track requests with a ref or
AbortController.Error handling: The catch block at line 128 silently swallows errors, rendering failures as "no projects" with no retry option.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`(main)/settings/onboarding/page.tsx around lines 112 - 135, The fetchProjects function can be overwritten by stale responses and currently swallows errors; modify fetchProjects to (1) track each request (e.g., increment a local requestIdRef or use an AbortController stored in a ref) and ignore any response whose id/abort signal does not match the most recent request before calling setProjects/setIsLoadingProjects, and (2) replace the empty catch block by setting an error state (e.g., setProjectsError) so the UI can show the error and a retry action, ensuring you still clear isLoading in finally; reference fetchProjects, setSelectedOrg, setView, setIsLoadingProjects, setProjects and activeKey when implementing these changes.
73-82:⚠️ Potential issue | 🟠 MajorHandle
errorfromusePaginatedListto avoid conflating load failures with empty state.This issue was previously flagged:
usePaginatedListreturns anerrorfield (perapp/hooks/usePaginatedList.ts:30-38) but it's not destructured here. Any load failure falls through tosetView("form")at line 96, which is indistinguishable from "no organizations exist" and could trigger duplicate org creation during an outage.Also applies to: 90-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`(main)/settings/onboarding/page.tsx around lines 73 - 82, Destructure the error returned by usePaginatedList (e.g., const { items: organizations, isLoading: isLoadingOrgs, isLoadingMore, hasMore, loadMore, error } = usePaginatedList(...)) and change the logic around setView("form") (the code that switches to the org-creation form) so it only does that when there is no load error and organizations is empty; if error is present, set a distinct error state/view (e.g., setView("error") or show an error UI) or return early to avoid treating a fetch failure as "no organizations"—apply the same error check to the block around the other occurrence referenced (the 90–98 region) so failures and empty-state are not conflated.app/lib/context/AuthContext.tsx (1)
51-69:⚠️ Potential issue | 🟠 MajorClear
currentUserwhen API key disappears or fetch fails.This issue was previously flagged: the early return at line 53 and the silent catch at lines 61-63 both leave stale
currentUserin state. Components likeSidebarand the onboarding page rely oncurrentUser.is_superuserfor access control, so stale data can expose superuser-only UI to regular users after logout or key removal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/context/AuthContext.tsx` around lines 51 - 69, The effect currently returns early when no apiKey and silently ignores fetch errors, leaving a stale currentUser; update the useEffect in AuthContext so that when apiKeys[0]?.key is falsy you call setCurrentUser(null) before returning, and in the catch block call setCurrentUser(null) (but only if not cancelled) instead of silently swallowing the error; keep the cancelled guard so you never set state after unmount and continue using apiFetch<User> and setCurrentUser as the referenced symbols to locate the changes.
🧹 Nitpick comments (1)
app/(main)/settings/onboarding/page.tsx (1)
90-98: Addviewto dependency array or suppress the lint warning.The effect reads
viewat line 95 but omits it from the dependency array at line 98. This is likely intentional to avoid re-triggering, but it will cause anexhaustive-depslint warning. Either add an ESLint disable comment with justification, or refactor to use a ref for tracking whether the initial transition has occurred.♻️ Option: use a ref to track initial load
+import { useState, useEffect, useCallback, useRef } from "react"; @@ + const hasInitializedView = useRef(false); + useEffect(() => { if (isLoadingOrgs) { setView("loading"); + hasInitializedView.current = false; return; } - if (view === "loading") { + if (!hasInitializedView.current) { + hasInitializedView.current = true; setView(organizations.length > 0 ? "list" : "form"); } - }, [isLoadingOrgs, organizations.length]); + }, [isLoadingOrgs, organizations.length]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`(main)/settings/onboarding/page.tsx around lines 90 - 98, The effect in useEffect reads the view variable but doesn't include it in the dependency array which triggers exhaustive-deps; either add view to the deps or explicitly suppress the lint warning with a comment and justification, or refactor to track the initial transition with a ref. Locate the useEffect that checks isLoadingOrgs and sets setView("loading") / setView(organizations.length > 0 ? "list" : "form") and then either (a) add view to the dependency array so the effect becomes [isLoadingOrgs, organizations.length, view], (b) add an eslint-disable-next-line comment above the effect with a short reason like “intentional: only run on orgs load” to suppress the warning, or (c) replace the boolean logic with a useRef (e.g., initialTransitionRef) to ensure the effect runs only on the intended initial load and keep the current deps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/`(main)/settings/onboarding/page.tsx:
- Around line 100-110: The paginated onboarding fetch (usePaginatedList) runs
before auth resolves allowing non-superusers to see data; update the component
to only call/use usePaginatedList after auth resolution by adding an
enabled/skip condition tied to auth state (e.g., isHydrated && currentUser !==
undefined && currentUser?.is_superuser) or by rendering the paginated list only
when currentUser is known; ensure you treat a failed lookup (currentUser ===
null) as resolved and block access for non-superusers as well so the fetch never
runs for unauthorized users.
- Around line 112-135: The fetchProjects function can be overwritten by stale
responses and currently swallows errors; modify fetchProjects to (1) track each
request (e.g., increment a local requestIdRef or use an AbortController stored
in a ref) and ignore any response whose id/abort signal does not match the most
recent request before calling setProjects/setIsLoadingProjects, and (2) replace
the empty catch block by setting an error state (e.g., setProjectsError) so the
UI can show the error and a retry action, ensuring you still clear isLoading in
finally; reference fetchProjects, setSelectedOrg, setView, setIsLoadingProjects,
setProjects and activeKey when implementing these changes.
- Around line 73-82: Destructure the error returned by usePaginatedList (e.g.,
const { items: organizations, isLoading: isLoadingOrgs, isLoadingMore, hasMore,
loadMore, error } = usePaginatedList(...)) and change the logic around
setView("form") (the code that switches to the org-creation form) so it only
does that when there is no load error and organizations is empty; if error is
present, set a distinct error state/view (e.g., setView("error") or show an
error UI) or return early to avoid treating a fetch failure as "no
organizations"—apply the same error check to the block around the other
occurrence referenced (the 90–98 region) so failures and empty-state are not
conflated.
In `@app/lib/context/AuthContext.tsx`:
- Around line 51-69: The effect currently returns early when no apiKey and
silently ignores fetch errors, leaving a stale currentUser; update the useEffect
in AuthContext so that when apiKeys[0]?.key is falsy you call
setCurrentUser(null) before returning, and in the catch block call
setCurrentUser(null) (but only if not cancelled) instead of silently swallowing
the error; keep the cancelled guard so you never set state after unmount and
continue using apiFetch<User> and setCurrentUser as the referenced symbols to
locate the changes.
---
Nitpick comments:
In `@app/`(main)/settings/onboarding/page.tsx:
- Around line 90-98: The effect in useEffect reads the view variable but doesn't
include it in the dependency array which triggers exhaustive-deps; either add
view to the deps or explicitly suppress the lint warning with a comment and
justification, or refactor to track the initial transition with a ref. Locate
the useEffect that checks isLoadingOrgs and sets setView("loading") /
setView(organizations.length > 0 ? "list" : "form") and then either (a) add view
to the dependency array so the effect becomes [isLoadingOrgs,
organizations.length, view], (b) add an eslint-disable-next-line comment above
the effect with a short reason like “intentional: only run on orgs load” to
suppress the warning, or (c) replace the boolean logic with a useRef (e.g.,
initialTransitionRef) to ensure the effect runs only on the intended initial
load and keep the current deps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3dac0a6b-c732-4192-b09b-8dd2b2270bef
📒 Files selected for processing (5)
app/(main)/settings/onboarding/page.tsxapp/components/settings/onboarding/OrganizationList.tsxapp/components/settings/onboarding/ProjectList.tsxapp/lib/context/AuthContext.tsxeslint.config.mjs
✅ Files skipped from review due to trivial changes (1)
- eslint.config.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- app/components/settings/onboarding/OrganizationList.tsx
- app/components/settings/onboarding/ProjectList.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/lib/context/AuthContext.tsx (1)
51-63:⚠️ Potential issue | 🟠 MajorClear stale
currentUseron missing key and fetch failure.Line 53 returns early without resetting state, and Lines 61–63 swallow
/api/users/mefailures. This can keep stale auth data (includingis_superuser) after key removal or token failure.🐛 Proposed fix
useEffect(() => { const apiKey = apiKeys[0]?.key; - if (!apiKey || !isHydrated) return; + if (!isHydrated) { + setCurrentUser(null); + return; + } + if (!apiKey) { + setCurrentUser(null); + return; + } let cancelled = false; (async () => { try { const data = await apiFetch<User>("/api/users/me", apiKey); if (!cancelled) setCurrentUser(data); } catch { - // silently ignore — user info is non-critical + if (!cancelled) setCurrentUser(null); } })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/context/AuthContext.tsx` around lines 51 - 63, The effect in AuthContext's useEffect exits early when apiKeys[0]?.key is missing and swallows fetch errors, which can leave stale currentUser/is_superuser state; update the logic in the useEffect that reads apiKeys and calls apiFetch<User>("/api/users/me", apiKey) so that when no apiKey is present it explicitly clears currentUser via setCurrentUser(null), and modify the catch block around apiFetch to also call setCurrentUser(null) (respecting the cancelled flag) so failed fetches don't preserve stale auth info; keep the existing cancellation handling and only set state when cancelled is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/lib/context/AuthContext.tsx`:
- Around line 51-63: The effect in AuthContext's useEffect exits early when
apiKeys[0]?.key is missing and swallows fetch errors, which can leave stale
currentUser/is_superuser state; update the logic in the useEffect that reads
apiKeys and calls apiFetch<User>("/api/users/me", apiKey) so that when no apiKey
is present it explicitly clears currentUser via setCurrentUser(null), and modify
the catch block around apiFetch to also call setCurrentUser(null) (respecting
the cancelled flag) so failed fetches don't preserve stale auth info; keep the
existing cancellation handling and only set state when cancelled is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2b4d4d5-1534-4739-b129-b5be1119f9c8
📒 Files selected for processing (2)
app/components/settings/onboarding/OnboardingSuccess.tsxapp/lib/context/AuthContext.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/settings/onboarding/OnboardingSuccess.tsx
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
app/components/Button.tsx (1)
13-33: Align color styling with the component styling convention.
variantStylesencodes color tokens via Tailwind color classes. Project guidance for this path asks to keep Tailwind for layout/spacing and source colors from the sharedcolorsobject via inline styles.Based on learnings: Use Tailwind CSS for layout and spacing, and inline styles (referencing the
colorsobject) for colors instead of mixing multiple styling approaches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Button.tsx` around lines 13 - 33, variantStyles currently hardcodes Tailwind color classes; change it to only contain layout/spacing classes and move color values into inline style usage sourced from the shared colors object. Update the usage in the Button component (where variantStyles is consumed) to apply colors via style={{ backgroundColor: colors.primary, color: colors.onPrimary, borderColor: colors.border, ... }} (use appropriate token names for primary/outline/ghost/danger and disabled states) while keeping existing Tailwind classes for padding/rounded/hover layout; ensure you reference the variantStyles object and the ButtonVariant type when making the change so colors come from the shared colors export instead of Tailwind color classes.app/components/settings/onboarding/EditProjectModal.tsx (2)
80-126: Consider extracting shared form UI withAddProjectModal.Both modals share identical form fields, validation patterns, and button layouts. A
ProjectFormcomponent could reduce duplication, though the current implementation is acceptable given the distinct submit behaviors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/settings/onboarding/EditProjectModal.tsx` around lines 80 - 126, EditProjectModal duplicates AddProjectModal's form markup and behavior; extract the shared form into a new ProjectForm component that accepts props for form state and handlers (e.g., name, setName, nameError, description, setDescription, isActive, setIsActive, isSubmitting, onSubmit, onCancel) and reuse Field, Button layout, and validation logic; update AddProjectModal and EditProjectModal to render <ProjectForm ...> and wire their existing handleSubmit/handleClose (or equivalent) and state setters into the ProjectForm props so submit behaviors remain distinct.
99-112: Same accessibility improvement asAddProjectModalapplies here.Consider adding explicit
idandhtmlForattributes to the checkbox for improved screen reader support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/settings/onboarding/EditProjectModal.tsx` around lines 99 - 112, In EditProjectModal, improve checkbox accessibility by adding an explicit id on the checkbox input (e.g., id="project-active") and linking the label via htmlFor to that id instead of nesting; update the input using the existing state handlers isActive and setIsActive, and ensure the span text "Active" remains associated with the input so screen readers announce the control correctly.app/components/settings/onboarding/AddProjectModal.tsx (1)
99-111: Consider adding anidattribute to associate the checkbox with its label for better accessibility.The checkbox uses a wrapping
<label>which provides click association, but adding explicitidandhtmlForattributes would improve accessibility for screen readers.♻️ Optional accessibility improvement
<div> <label className="flex items-center gap-2.5 cursor-pointer"> <input type="checkbox" + id="project-active" checked={isActive} onChange={(e) => setIsActive(e.target.checked)} className="w-4 h-4 rounded" /> - <span className="text-sm text-text-primary">Active</span> + <label htmlFor="project-active" className="text-sm text-text-primary">Active</label> </label>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/settings/onboarding/AddProjectModal.tsx` around lines 99 - 111, Add an explicit id/htmlFor pairing to the Active checkbox to improve screen-reader accessibility: give the input an id (e.g. generated via React's useId or a unique string) and set the wrapping label's htmlFor to that id (while keeping the existing checked prop isActive and onChange setIsActive). Update the JSX around the input/label in AddProjectModal so the label uses htmlFor and the input has the matching id, ensuring uniqueness if multiple modals can be mounted.app/api/projects/[projectId]/route.ts (1)
9-18: Consider validatingprojectIdbefore forwarding to the backend.While the backend should validate the ID, adding basic validation here provides defense-in-depth against malformed requests.
🛡️ Optional validation
const { projectId } = await params; + if (!/^\d+$/.test(projectId)) { + return NextResponse.json( + { success: false, error: "Invalid project ID" }, + { status: 400 }, + ); + } const body = await request.json();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/projects/`[projectId]/route.ts around lines 9 - 18, Validate the extracted projectId from params before calling apiClient: ensure projectId is present and matches the expected format (e.g., UUID or numeric ID used by your backend) and return an early 400/422 response for invalid/missing IDs; update the route handler that reads const { projectId } = await params to perform this check and only call apiClient(`/api/v1/projects/${projectId}`, ...) when the ID is valid, otherwise short-circuit with an appropriate error response.app/components/settings/onboarding/UserList.tsx (2)
142-151: The check-circle icon may confuse users for inactive status.
CheckCircleIconis displayed for both active and inactive states. Consider using a different icon (e.g.,XCircleIconor no icon) for inactive users to visually distinguish the states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/settings/onboarding/UserList.tsx` around lines 142 - 151, The UI shows CheckCircleIcon for both active and inactive users which is confusing; update the rendering in UserList.tsx where CheckCircleIcon is used with the span that checks user.is_active so that it conditionally renders a different icon (e.g., XCircleIcon) or no icon when user.is_active is false, keeping the existing styling classes and the text {user.is_active ? "Active" : "Inactive"} intact; ensure you import the alternate icon (XCircleIcon) at the top and use a ternary or conditional to choose between CheckCircleIcon and XCircleIcon based on user.is_active.
130-140: Handle missingfull_namegracefully.If a user was added without a full name,
user.full_namemay be empty or undefined, resulting in an empty paragraph. Consider displaying the email or a fallback.🔧 Proposed fallback for missing name
<div> <p className="text-sm font-medium text-text-primary"> - {user.full_name} + {user.full_name || user.email} </p> - <p className="text-xs text-text-secondary mt-0.5"> - {user.email} - </p> + {user.full_name && ( + <p className="text-xs text-text-secondary mt-0.5"> + {user.email} + </p> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/settings/onboarding/UserList.tsx` around lines 130 - 140, The UI shows an empty paragraph when user.full_name is missing; in UserList.tsx update the JSX that renders user.full_name to use a fallback (e.g., user.email or a default label) so the <p> for the name always has content — replace the direct use of user.full_name with a conditional expression (e.g., user.full_name || user.email || 'Unknown user') when rendering the name next to formatRelativeTime(user.inserted_at).app/components/settings/onboarding/AddUserModal.tsx (1)
161-167: Consider adding an accessible label to the remove button.The remove button uses a visual
×character without an accessible label, which may not be clear to screen reader users.♿ Proposed accessibility improvement
{userRows.length > 1 && ( <button onClick={() => removeRow(index)} className="mt-2 p-1 text-text-secondary hover:text-red-500 transition-colors" + aria-label="Remove this user row" > × </button> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/settings/onboarding/AddUserModal.tsx` around lines 161 - 167, The remove button in AddUserModal uses only a visual "×" and lacks an accessible name; update the button (the one that calls removeRow(index) inside AddUserModal) to provide an accessible label—e.g., add an aria-label or title that describes the action (for example "Remove user" or "Remove user {index+1}") or include visually hidden text so screen readers can announce the purpose while preserving the existing visual ×.app/(main)/settings/onboarding/page.tsx (1)
139-152: Consider adding error feedback for project refresh failures.Similar to
fetchProjects, errors are silently swallowed. While less critical for a refresh operation, a toast notification would help users understand when the refresh failed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`(main)/settings/onboarding/page.tsx around lines 139 - 152, The refreshProjects function currently swallows errors; update its catch block to surface user feedback by showing the same toast/error notification used in fetchProjects (instead of silently ignoring failures). In the catch of refreshProjects (which calls apiFetch and setsProjects), call the existing toast/error helper the codebase uses for fetchProjects (reuse the same message pattern and include the caught error message) so users see a failure notification when the refresh fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/`(main)/settings/onboarding/page.tsx:
- Around line 193-205: The scrollRef must be attached to the real scrolling
container: move the scrollRef prop out of OrganizationList and attach it
directly to the div with class "flex-1 overflow-y-auto" (the scroll container
currently at the start of the block) so the ref points to the element that
actually scrolls; then remove the scrollRef prop from OrganizationList and
update OrganizationListProps (and any OrganizationList usage) to drop that prop
so infinite-scroll logic reads scroll position from the container ref you just
mounted.
In `@app/components/Button.tsx`:
- Around line 41-60: The Button component currently allows omission of the HTML
button type which defaults to "submit" in forms; update the Button
implementation (function Button) to supply a safe default type by setting the
rendered <button> element's type to "button" unless an explicit type is provided
(use props.type or a type prop default), ensuring the existing props spread
({...props}) can still override it; adjust the ButtonProps/default parameter or
the JSX attribute accordingly so non-submit actions don't accidentally submit
forms.
In `@app/components/settings/onboarding/OrganizationList.tsx`:
- Around line 1-6: This file (OrganizationList.tsx) uses the useAuth() React
hook (and likely other client-side behavior) but is missing the "use client"
directive; add the exact line "use client" as the very first statement of the
file (above imports) so the OrganizationList component and useAuth() run on the
client.
In `@app/components/settings/onboarding/UserList.tsx`:
- Around line 55-69: The fetchUsers function currently swallows errors; modify
fetchUsers (the useCallback that calls apiFetch in UserList.tsx) to surface
failures to the user by showing the same toast/error UI used in
handleRemoveUser: on catch call the toast/error helper (e.g., toast.error or the
project-wide notify function) with a clear message like "Failed to load users"
and the error details, and optionally set an error state (e.g., setFetchError)
so the component can render an error state instead of an empty list; keep
setIsLoading(false) in finally as-is.
---
Nitpick comments:
In `@app/`(main)/settings/onboarding/page.tsx:
- Around line 139-152: The refreshProjects function currently swallows errors;
update its catch block to surface user feedback by showing the same toast/error
notification used in fetchProjects (instead of silently ignoring failures). In
the catch of refreshProjects (which calls apiFetch and setsProjects), call the
existing toast/error helper the codebase uses for fetchProjects (reuse the same
message pattern and include the caught error message) so users see a failure
notification when the refresh fails.
In `@app/api/projects/`[projectId]/route.ts:
- Around line 9-18: Validate the extracted projectId from params before calling
apiClient: ensure projectId is present and matches the expected format (e.g.,
UUID or numeric ID used by your backend) and return an early 400/422 response
for invalid/missing IDs; update the route handler that reads const { projectId }
= await params to perform this check and only call
apiClient(`/api/v1/projects/${projectId}`, ...) when the ID is valid, otherwise
short-circuit with an appropriate error response.
In `@app/components/Button.tsx`:
- Around line 13-33: variantStyles currently hardcodes Tailwind color classes;
change it to only contain layout/spacing classes and move color values into
inline style usage sourced from the shared colors object. Update the usage in
the Button component (where variantStyles is consumed) to apply colors via
style={{ backgroundColor: colors.primary, color: colors.onPrimary, borderColor:
colors.border, ... }} (use appropriate token names for
primary/outline/ghost/danger and disabled states) while keeping existing
Tailwind classes for padding/rounded/hover layout; ensure you reference the
variantStyles object and the ButtonVariant type when making the change so colors
come from the shared colors export instead of Tailwind color classes.
In `@app/components/settings/onboarding/AddProjectModal.tsx`:
- Around line 99-111: Add an explicit id/htmlFor pairing to the Active checkbox
to improve screen-reader accessibility: give the input an id (e.g. generated via
React's useId or a unique string) and set the wrapping label's htmlFor to that
id (while keeping the existing checked prop isActive and onChange setIsActive).
Update the JSX around the input/label in AddProjectModal so the label uses
htmlFor and the input has the matching id, ensuring uniqueness if multiple
modals can be mounted.
In `@app/components/settings/onboarding/AddUserModal.tsx`:
- Around line 161-167: The remove button in AddUserModal uses only a visual "×"
and lacks an accessible name; update the button (the one that calls
removeRow(index) inside AddUserModal) to provide an accessible label—e.g., add
an aria-label or title that describes the action (for example "Remove user" or
"Remove user {index+1}") or include visually hidden text so screen readers can
announce the purpose while preserving the existing visual ×.
In `@app/components/settings/onboarding/EditProjectModal.tsx`:
- Around line 80-126: EditProjectModal duplicates AddProjectModal's form markup
and behavior; extract the shared form into a new ProjectForm component that
accepts props for form state and handlers (e.g., name, setName, nameError,
description, setDescription, isActive, setIsActive, isSubmitting, onSubmit,
onCancel) and reuse Field, Button layout, and validation logic; update
AddProjectModal and EditProjectModal to render <ProjectForm ...> and wire their
existing handleSubmit/handleClose (or equivalent) and state setters into the
ProjectForm props so submit behaviors remain distinct.
- Around line 99-112: In EditProjectModal, improve checkbox accessibility by
adding an explicit id on the checkbox input (e.g., id="project-active") and
linking the label via htmlFor to that id instead of nesting; update the input
using the existing state handlers isActive and setIsActive, and ensure the span
text "Active" remains associated with the input so screen readers announce the
control correctly.
In `@app/components/settings/onboarding/UserList.tsx`:
- Around line 142-151: The UI shows CheckCircleIcon for both active and inactive
users which is confusing; update the rendering in UserList.tsx where
CheckCircleIcon is used with the span that checks user.is_active so that it
conditionally renders a different icon (e.g., XCircleIcon) or no icon when
user.is_active is false, keeping the existing styling classes and the text
{user.is_active ? "Active" : "Inactive"} intact; ensure you import the alternate
icon (XCircleIcon) at the top and use a ternary or conditional to choose between
CheckCircleIcon and XCircleIcon based on user.is_active.
- Around line 130-140: The UI shows an empty paragraph when user.full_name is
missing; in UserList.tsx update the JSX that renders user.full_name to use a
fallback (e.g., user.email or a default label) so the <p> for the name always
has content — replace the direct use of user.full_name with a conditional
expression (e.g., user.full_name || user.email || 'Unknown user') when rendering
the name next to formatRelativeTime(user.inserted_at).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94979393-a265-4715-aca2-ca1e90f083a7
📒 Files selected for processing (21)
app/(main)/datasets/page.tsxapp/(main)/evaluations/[id]/page.tsxapp/(main)/knowledge-base/page.tsxapp/(main)/settings/credentials/page.tsxapp/(main)/settings/onboarding/page.tsxapp/(main)/speech-to-text/page.tsxapp/(main)/text-to-speech/page.tsxapp/api/projects/[projectId]/route.tsapp/api/projects/route.tsapp/components/Button.tsxapp/components/ConfigSelector.tsxapp/components/Modal.tsxapp/components/settings/onboarding/AddProjectModal.tsxapp/components/settings/onboarding/AddUserModal.tsxapp/components/settings/onboarding/EditProjectModal.tsxapp/components/settings/onboarding/OrganizationList.tsxapp/components/settings/onboarding/ProjectList.tsxapp/components/settings/onboarding/UserList.tsxapp/components/settings/onboarding/index.tsapp/components/speech-to-text/ModelComparisonCard.tsxapp/lib/types/onboarding.ts
💤 Files with no reviewable changes (7)
- app/(main)/evaluations/[id]/page.tsx
- app/(main)/datasets/page.tsx
- app/(main)/settings/credentials/page.tsx
- app/components/speech-to-text/ModelComparisonCard.tsx
- app/(main)/knowledge-base/page.tsx
- app/(main)/speech-to-text/page.tsx
- app/(main)/text-to-speech/page.tsx
✅ Files skipped from review due to trivial changes (2)
- app/components/Modal.tsx
- app/lib/types/onboarding.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/components/settings/onboarding/index.ts
- app/components/settings/onboarding/ProjectList.tsx
- app/components/ConfigSelector.tsx
vprashrex
approved these changes
Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue: #99
Summary:
Summary by CodeRabbit
New Features
Refactor
Chore