fix: error boundary, NavLink remount, env var standardization, useQuery refactor#247
Merged
DevanshuNEU merged 6 commits intoFeb 20, 2026
Merged
Conversation
Wraps AppRoutes in ErrorBoundary so any unhandled component error shows a recovery UI instead of a blank white screen. Displays the error message, a 'Try again' button (resets error state) and a 'Go home' button (navigates to /). Class component because React doesn't support error boundaries with hooks yet. Closes OPE-15
NavLink (OPE-28): - Moved NavLink from inside Sidebar body to module scope - Was creating a new component type every render, destroying state - Now receives active/showLabels/onClick as props instead of closures Docker env vars (OPE-12): - docker-compose.yml now uses SUPABASE_ANON_KEY (not SUPABASE_KEY) - Added SUPABASE_JWT_SECRET to backend container env - auth.py reads SUPABASE_ANON_KEY which was never passed in Docker - Frontend build args already used SUPABASE_ANON_KEY via different name Root .env.example (OPE-21): - Added SUPABASE_SERVICE_ROLE_KEY - Added VOYAGE_API_KEY (was in backend .env.example but not root)
supabase_service.py was falling back to SUPABASE_KEY which we just removed from docker-compose. Updated to fall back to SUPABASE_ANON_KEY which is now the standardized name across all config. Closes OPE-13 (startup validation already existed, just needed alignment)
Added useRepos hook to useCachedQuery.ts: - Uses React Query with refetchInterval: 30s (replaces setInterval) - Request deduplication, caching, loading/error states for free - Returns invalidate() function for manual refresh after mutations DashboardHome.tsx: - Removed manual useState for repos and reposLoading - Removed fetchRepos function and its useEffect + setInterval - Replaced all fetchRepos() calls with refreshRepos() (invalidate) - Reduced component by ~20 lines of manual data fetching code Closes OPE-30
|
@DevanshuNEU is attempting to deploy a commit to the Dev's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds startup environment validation and new env vars (SUPABASE_SERVICE_ROLE_KEY, VOYAGE_API_KEY), switches Supabase anon key usage, integrates validation into backend startup, adds frontend ErrorBoundary, refactors repo fetching to a React Query hook, and introduces Vitest-based test infra and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Startup
participant Lifespan as Lifespan Manager
participant Validator as validate_environment()
participant Logger as Logger Service
participant Process as Process
App->>Lifespan: Start application
Lifespan->>Validator: Call validate_environment()
Validator->>Validator: Check REQUIRED_VARS
alt Missing Required Variables
Validator->>Logger: Log error(s)
Validator->>Process: Exit(1)
else All Required Variables Set
Validator->>Validator: Check OPTIONAL_VARS
Validator->>Logger: Log warnings for missing optional vars
Validator->>Logger: Log validation success
Logger-->>Lifespan: Validation complete
end
sequenceDiagram
participant User as User
participant Child as Child Component
participant ErrorBoundary as ErrorBoundary
participant ErrorUI as Error Fallback UI
User->>Child: Interact / render
Child->>Child: Error thrown during render
Child-->>ErrorBoundary: Error propagates
ErrorBoundary->>ErrorBoundary: getDerivedStateFromError()
ErrorBoundary->>ErrorBoundary: componentDidCatch() (log)
ErrorBoundary->>ErrorUI: Render fallback UI with actions
ErrorUI-->>User: Show error + "Try again" / "Go home"
User->>ErrorUI: Click "Try again"
ErrorUI->>ErrorBoundary: Reset error state
ErrorBoundary->>Child: Re-render children
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
frontend/src/components/dashboard/DashboardHome.tsx (1)
33-33: Unused import:useInvalidateRepoCache.
useInvalidateRepoCacheis imported but never used in this component. Remove it to avoid confusion.Proposed fix
-import { useRepos, useInvalidateRepoCache } from '../../hooks/useCachedQuery' +import { useRepos } from '../../hooks/useCachedQuery'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/dashboard/DashboardHome.tsx` at line 33, Remove the unused import useInvalidateRepoCache from the import statement in the DashboardHome component so only useRepos is imported; update the import line that currently reads "import { useRepos, useInvalidateRepoCache } ..." to remove useInvalidateRepoCache and run lint/tests to verify no other references to useInvalidateRepoCache remain in DashboardHome.frontend/src/hooks/useCachedQuery.ts (1)
196-198: Consider memoizinginvalidateto provide a stable reference.
invalidatecreates a new closure on every render. If a consumer ever passes it as a prop or uses it in a dependency array, it would trigger unnecessary re-renders or effects. Wrapping it inuseCallbackis cheap insurance.Proposed fix
- const invalidate = () => queryClient.invalidateQueries({ queryKey: ['repos'] }) + const invalidate = useCallback( + () => queryClient.invalidateQueries({ queryKey: ['repos'] }), + [queryClient] + )(Add
useCallbackto the import from@tanstack/react-queryor import fromreact.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useCachedQuery.ts` around lines 196 - 198, The invalidate closure returned from useCachedQuery is recreated each render; wrap it in React's useCallback to memoize it (e.g., const invalidate = useCallback(() => queryClient.invalidateQueries({ queryKey: ['repos'] }), [queryClient])) and update the return to provide this memoized invalidate so consumers get a stable reference; add useCallback to the imports from 'react'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 22-23: The .env example uses inline comments on unquoted values
which python-dotenv will parse literally for SUPABASE_JWT_SECRET and
SUPABASE_SERVICE_ROLE_KEY; update the file by removing inline comments after the
values and instead place explanatory comments on the preceding line (or quote
the values) so the assigned values remain clean raw tokens (e.g., move "From
Project Settings -> API -> JWT Secret" and "From Project Settings -> API ->
service_role key" to their own comment lines above the SUPABASE_JWT_SECRET and
SUPABASE_SERVICE_ROLE_KEY entries).
In `@backend/config/startup_checks.py`:
- Line 12: The inline comment above REQUIRED_VARS is outdated; update it to
reflect the actual tuple shape used by REQUIRED_VARS (List[Tuple[str, str]]) —
i.e., two elements: env_var_name and description. Modify the comment near
REQUIRED_VARS in startup_checks.py to remove the ", required" part or replace
the whole comment with a correct description like "# (env_var_name,
description)". Ensure the comment text exactly matches the tuple structure used
by REQUIRED_VARS.
In `@frontend/package.json`:
- Around line 53-65: Add `@testing-library/dom` to devDependencies so
`@testing-library/react` no longer has an unmet peer; update frontend package.json
devDependencies to include "@testing-library/dom" with a version compatible with
"@testing-library/react@^16.3.2" (for example "^8.0.0") and then run the package
manager to install/update (this change relates to the entries
"@testing-library/react", "vitest", and "jsdom" in package.json).
In `@frontend/src/components/ErrorBoundary.tsx`:
- Line 35: The message "An unexpected error occurred. This has been logged." in
ErrorBoundary is misleading because componentDidCatch currently only calls
console.error; update componentDidCatch in the ErrorBoundary class to, when
SENTRY_DSN (or the app's front-end Sentry flag) is present, call
Sentry.captureException(error, { contexts: { react: { componentStack:
info.componentStack } } }) (import Sentry from '@sentry/browser' or the app's
Sentry wrapper and guard with process.env.SENTRY_DSN or the app config),
otherwise change the UI copy to remove "This has been logged." (e.g., "An
unexpected error occurred.") so it doesn't imply server-side logging.
- Around line 13-14: Replace the class-based ErrorBoundary with a functional
wrapper using the react-error-boundary package: create a functional
ErrorBoundary component that uses react-error-boundary's ErrorBoundary (or
useErrorBoundary) and accepts a FallbackComponent prop, moving the current
render fallback UI into that FallbackComponent; remove the class ErrorBoundary
and its lifecycle method componentDidCatch, and instead forward error/ info to
an actual error tracking call inside the onError handler you pass to
react-error-boundary. Also fix the user-facing text currently reading "This has
been logged" to accurately reflect whether errors are being sent (e.g., change
it to "An error occurred" if you don't integrate a tracking service) or
implement a real tracking call in the onError handler before leaving the message
as-is.
In `@frontend/src/hooks/useCachedQuery.ts`:
- Around line 176-194: Replace the inline repository type in the useQuery call
inside useCachedQuery (the const query = useQuery({...}) block) with the shared
Repository interface from frontend/src/types.ts to ensure fields like git_url
match DashboardHome usage; import Repository at the top of useCachedQuery.ts,
change the queryFn return typing to use Array<Repository>, and if additional API
fields (language, created_at, file_count, status) are required, extend the
Repository interface in types.ts rather than keeping a mismatched inline type.
---
Nitpick comments:
In `@frontend/src/components/dashboard/DashboardHome.tsx`:
- Line 33: Remove the unused import useInvalidateRepoCache from the import
statement in the DashboardHome component so only useRepos is imported; update
the import line that currently reads "import { useRepos, useInvalidateRepoCache
} ..." to remove useInvalidateRepoCache and run lint/tests to verify no other
references to useInvalidateRepoCache remain in DashboardHome.
In `@frontend/src/hooks/useCachedQuery.ts`:
- Around line 196-198: The invalidate closure returned from useCachedQuery is
recreated each render; wrap it in React's useCallback to memoize it (e.g., const
invalidate = useCallback(() => queryClient.invalidateQueries({ queryKey:
['repos'] }), [queryClient])) and update the return to provide this memoized
invalidate so consumers get a stable reference; add useCallback to the imports
from 'react'.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
frontend/src/components/dashboard/Sidebar.tsx (6)
33-85: RenameNavLinkto avoid shadowing react-router-dom's own export, and consider leveraging its built-in active state.react-router-dom v7 exports its own
NavLinkthat accepts aclassNamecallback({ isActive, isPending }) => ...for exactly this active-state styling use case. The locally definedfunction NavLinkat line 34 silently shadows that export — not a runtime bug today sinceNavLinkisn't imported from rr-dom here, but it's a naming hazard: any futureimport { NavLink } from 'react-router-dom'in this file would require aliasing, and readers may conflate the two.Recommended approach: rename the local helper (e.g.
SidebarNavLink) and, optionally, replace the internal<Link>branch with rr-dom'sNavLinkusing theclassNamecallback — which would also let you remove the manualisActive()function and theactiveprop threaded through every call site.♻️ Proposed rename + native active-state approach
+import { Link, NavLink, useLocation } from 'react-router-dom' -import { Link, useLocation } from 'react-router-dom'-// defined at module scope so React can reconcile it across renders -function NavLink({ +// defined at module scope so React can reconcile it across renders +function SidebarNavLink({ item, - active, showLabels, onClick, }: { item: NavItem - active: boolean showLabels: boolean onClick?: () => void }) { - const baseClasses = ` - flex items-center gap-3 px-3 py-2.5 rounded-lg transition-all group - ${active - ? 'bg-primary/10 text-primary' - : 'text-muted-foreground hover:text-foreground hover:bg-muted' - } - ` + const getClasses = (active: boolean) => + cn( + 'flex items-center gap-3 px-3 py-2.5 rounded-lg transition-all group', + active + ? 'bg-primary/10 text-primary' + : 'text-muted-foreground hover:text-foreground hover:bg-muted' + ) if (item.external) { return ( <a href={item.href} target="_blank" rel="noopener noreferrer" - className={baseClasses} + className={getClasses(false)} onClick={onClick} > ... </a> ) } return ( - <Link to={item.href} className={baseClasses} onClick={onClick}> + <NavLink + to={item.href} + className={({ isActive }) => getClasses(isActive)} + onClick={onClick} + > ... - </Link> + </NavLink> ) }Then remove the
isActivehelper fromSidebarand drop theactive={isActive(item.href)}prop from everySidebarNavLinkcall site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/dashboard/Sidebar.tsx` around lines 33 - 85, Rename the local helper function NavLink to avoid shadowing react-router-dom's NavLink (e.g., SidebarNavLink) and update all call sites; optionally replace the internal <Link> branch with react-router-dom's NavLink using its className callback to compute active styling so you can remove the isActive helper and stop passing active={isActive(...)} from Sidebar—adjust imports/usages accordingly (use NavLink from 'react-router-dom' for route-aware className callback and keep the external-anchor branch unchanged).
45-51: Prefercn()over a raw multi-line template literal forbaseClasses.The backtick string starts with
\nand ends with\n, producing a className value with leading/trailing whitespace. Browsers ignore it, but using the shadcncn()utility (typically@/lib/utils) is the idiomatic pattern in this stack for conditional Tailwind composition and avoids the artefact entirely.♻️ Proposed refactor using `cn()`
+import { cn } from '@/lib/utils' ... - const baseClasses = ` - flex items-center gap-3 px-3 py-2.5 rounded-lg transition-all group - ${active - ? 'bg-primary/10 text-primary' - : 'text-muted-foreground hover:text-foreground hover:bg-muted' - } - ` + const baseClasses = cn( + 'flex items-center gap-3 px-3 py-2.5 rounded-lg transition-all group', + active + ? 'bg-primary/10 text-primary' + : 'text-muted-foreground hover:text-foreground hover:bg-muted' + )As per coding guidelines, "Use shadcn/ui components over custom UI" — the
cnutility is shadcn's canonical class-merging helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/dashboard/Sidebar.tsx` around lines 45 - 51, Replace the multi-line template literal assigned to baseClasses with a call to the shadcn cn() utility (import from '@/lib/utils') to compose classes without leading/trailing whitespace and to handle the conditional active classes; locate the baseClasses declaration in Sidebar.tsx and rewrite it to pass the static classes and the active ? 'bg-primary/10 text-primary' : 'text-muted-foreground hover:text-foreground hover:bg-muted' expression into cn() so class merging follows project conventions.
99-103: Redundant first branch ingetWidthClass— bothmobileOpenand the default return the same string.Line 100 is effectively dead; removing it simplifies the function with no behavior change.
♻️ Proposed simplification
const getWidthClass = () => { - if (mobileOpen) return 'w-[var(--sidebar-width)]' if (collapsed) return 'w-[var(--sidebar-width-collapsed)]' return 'w-[var(--sidebar-width)]' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/dashboard/Sidebar.tsx` around lines 99 - 103, The getWidthClass function contains a redundant branch: the mobileOpen check returns the same string as the default case; remove that branch and simplify getWidthClass to only check collapsed (return 'w-[var(--sidebar-width-collapsed)]' when collapsed) and otherwise return the default 'w-[var(--sidebar-width)]' using the existing symbols mobileOpen, collapsed and function getWidthClass to locate and update the code.
99-103: Redundant first branch ingetWidthClass—mobileOpenand the default path return the same string.Line 100 is dead code: removing it produces identical runtime behavior and makes the intent (
collapsed → narrow, otherwise → full width) immediately legible.♻️ Proposed simplification
const getWidthClass = () => { - if (mobileOpen) return 'w-[var(--sidebar-width)]' if (collapsed) return 'w-[var(--sidebar-width-collapsed)]' return 'w-[var(--sidebar-width)]' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/dashboard/Sidebar.tsx` around lines 99 - 103, The first branch in getWidthClass is redundant because mobileOpen and the default return the same width; simplify getWidthClass by removing the mobileOpen branch and returning 'w-[var(--sidebar-width-collapsed)]' when collapsed is true, otherwise return 'w-[var(--sidebar-width)]' so the function clearly expresses "collapsed → narrow, otherwise → full width" (update the getWidthClass function and any related references to rely on collapsed/mobileOpen where needed).
45-51: Prefercn()over a raw multi-line template literal forbaseClasses.The backtick string starts with a newline and ends with trailing spaces. While browsers silently ignore extra whitespace in
className, shadcn'scnutility (typically available inlib/utils) is the idiomatic way to compose conditional Tailwind classes in this stack and avoids the whitespace artifact.♻️ Proposed refactor using `cn()`
+import { cn } from '@/lib/utils' ... - const baseClasses = ` - flex items-center gap-3 px-3 py-2.5 rounded-lg transition-all group - ${active - ? 'bg-primary/10 text-primary' - : 'text-muted-foreground hover:text-foreground hover:bg-muted' - } - ` + const baseClasses = cn( + 'flex items-center gap-3 px-3 py-2.5 rounded-lg transition-all group', + active + ? 'bg-primary/10 text-primary' + : 'text-muted-foreground hover:text-foreground hover:bg-muted' + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/dashboard/Sidebar.tsx` around lines 45 - 51, Replace the multi-line template literal assigned to baseClasses with the project's cn utility: import cn from "lib/utils" (or existing utils export) and build baseClasses via cn('flex items-center gap-3 px-3 py-2.5 rounded-lg transition-all group', active ? 'bg-primary/10 text-primary' : 'text-muted-foreground hover:text-foreground hover:bg-muted') to avoid the leading newline/trailing-space artifact and use the idiomatic class composition; update any references to baseClasses in Sidebar (component/JSX) accordingly.
33-85: RenameNavLinkto avoid shadowing react-router-dom's ownNavLinkexport.react-router-dom v7 exports its own
NavLinkcomponent. The local function at line 34 uses the same name, creating a naming hazard — any future import ofimport { NavLink } from 'react-router-dom'would require aliasing to avoid collision. Rename toSidebarNavLinkfor clarity.
⚠️ Regarding the refactor to use react-router-dom's NavLink: react-router-dom v7'sNavLinkdoes support aclassNamecallback accepting{ isActive }. However, the current code includes custom routing logic in theisActivehelper (line 90–97) that checks for/dashboardpathname AND/dashboard/repo/*prefixes. React Router's defaultNavLinkmatching won't replicate this custom logic. If adopting react-router-dom'sNavLink, ensure the routing rules still apply — either by adjusting the route config or retaining the custom matcher.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/dashboard/Sidebar.tsx` around lines 33 - 85, Rename the locally defined NavLink function to SidebarNavLink (update the function declaration and all internal references/usages) to avoid shadowing react-router-dom's NavLink export; ensure any existing imports or future imports of NavLink from 'react-router-dom' remain unambiguous. While renaming, preserve the existing custom isActive routing logic (the helper around lines 90–97) so the same dashboard and /dashboard/repo/* matching behavior continues; if you later switch to react-router-dom's NavLink, re-implement that matcher via its className/isActive callback or keep the custom matcher to maintain identical behavior.frontend/src/hooks/useCachedQuery.ts (1)
196-198: MemoizeinvalidatewithuseCallbackto stabilize the reference.
invalidateis recreated on every render. WhilequeryClientis stable, callers that putrefreshReposinuseEffectdeps will trigger spurious effects.♻️ Proposed refactor
+import { useQuery, useQueryClient, useCallback } from '@tanstack/react-query' - const invalidate = () => queryClient.invalidateQueries({ queryKey: ['repos'] }) + const invalidate = useCallback( + () => queryClient.invalidateQueries({ queryKey: ['repos'] }), + [queryClient] + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useCachedQuery.ts` around lines 196 - 198, The invalidate function is recreated every render; wrap it in React's useCallback to stabilize its reference (e.g., const invalidate = useCallback(() => queryClient.invalidateQueries({ queryKey: ['repos'] }), [queryClient])) and return that memoized invalidate from the same place that currently returns { ...query, invalidate } so callers can safely use it in effect deps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 35-37: The docker-compose backend service environment block is
missing the VOYAGE_API_KEY mapping so the container won't receive the .env
value; add a line like - VOYAGE_API_KEY=${VOYAGE_API_KEY} to the backend
service's environment list in docker-compose.yml (matching how COHERE_API_KEY
and SENTRY_DSN are handled) so startup_checks.py's OPTIONAL_VARS and
.env.example actually get propagated into the container.
In `@frontend/src/components/dashboard/DashboardHome.tsx`:
- Line 33: The import useInvalidateRepoCache in DashboardHome.tsx is unused;
remove it from the import statement (keep useRepos) or, if intended, wire it
into the component where cache invalidation is needed (e.g., call
useInvalidateRepoCache() after repo mutations). Locate the import line
referencing useInvalidateRepoCache and either delete that identifier from the
import list or add the appropriate invocation inside the DashboardHome component
(e.g., call the hook and use its invalidate function after repo changes).
---
Duplicate comments:
In @.env.example:
- Around line 22-23: The .env.example lines for SUPABASE_JWT_SECRET and
SUPABASE_SERVICE_ROLE_KEY trigger dotenv-linter's ValueWithoutQuotes rule;
update the example to wrap the values in quotes (e.g., change
SUPABASE_JWT_SECRET=your-jwt-secret to SUPABASE_JWT_SECRET="your-jwt-secret" and
SUPABASE_SERVICE_ROLE_KEY=eyJ... to SUPABASE_SERVICE_ROLE_KEY="eyJ...") so both
SUPABASE_JWT_SECRET and SUPABASE_SERVICE_ROLE_KEY are quoted.
In `@backend/config/startup_checks.py`:
- Line 12: The inline comment "# (env_var_name, description, required)" is
inconsistent with the actual variable shape used in startup_checks.py:
REQUIRED_VARS is declared as List[Tuple[str, str]]; either update the comment to
reflect the two-tuple shape (e.g. "# (env_var_name, description)") or change
REQUIRED_VARS and its usages to a three-tuple/typed structure (e.g.
List[Tuple[str, str, bool]] or a dataclass) if a "required" flag is intended;
adjust any references to REQUIRED_VARS (and functions that iterate it)
accordingly so the comment and type/signature match.
In `@frontend/package.json`:
- Around line 53-65: Add the missing peer dependency `@testing-library/dom` to
frontend package.json devDependencies so tests using `@testing-library/react`
(v16) can resolve its peer; update the devDependencies block to include
"@testing-library/dom" with a compatible version (match the
`@testing-library/react` major, e.g. latest v8/compatible), then run bun install
to install the new dependency.
In `@frontend/src/components/ErrorBoundary.tsx`:
- Line 35: The error message "An unexpected error occurred. This has been
logged." in the ErrorBoundary component is misleading because componentDidCatch
currently only calls console.error; either integrate a real reporter (e.g., call
Sentry.captureException(error, {extra: info}) inside componentDidCatch of
ErrorBoundary) or change the rendered string in ErrorBoundary's render (or
fallback UI) to a truthful message such as "An unexpected error occurred."
Update the code in the ErrorBoundary class: modify componentDidCatch to call the
chosen reporting API if you wire one up, or edit the JSX/text returned by render
to remove the "This has been logged" claim.
- Around line 13-14: Replace the class-based ErrorBoundary component with the
functional wrapper provided by react-error-boundary: remove the class export
ErrorBoundary (and its Props/State usage) and instead import { ErrorBoundary as
ReactErrorBoundary } from 'react-error-boundary', then export a functional
component named ErrorBoundary that accepts the same props, renders
<ReactErrorBoundary FallbackComponent={FallbackComponent}
onError={onError}>{children}</ReactErrorBoundary>, and implement any previously
in-class behavior (like logging or reset logic) via the onError prop or a small
functional FallbackComponent; reference the original ErrorBoundary class name
and any custom Fallback/handler functions so the replacement preserves existing
behavior.
In `@frontend/src/hooks/useCachedQuery.ts`:
- Around line 180-189: The inline cast returning repositories uses a mismatched
shape (url:string) causing selectedRepoData.git_url to be undefined; update the
return type in useCachedQuery's repository extraction to use the shared
Repository type (import Repository from the types module) and return
data.repositories as Repository[] so callers like DashboardHome.tsx
(selectedRepoData.git_url) get the correct git_url field.
---
Nitpick comments:
In `@frontend/src/components/dashboard/Sidebar.tsx`:
- Around line 33-85: Rename the local helper function NavLink to avoid shadowing
react-router-dom's NavLink (e.g., SidebarNavLink) and update all call sites;
optionally replace the internal <Link> branch with react-router-dom's NavLink
using its className callback to compute active styling so you can remove the
isActive helper and stop passing active={isActive(...)} from Sidebar—adjust
imports/usages accordingly (use NavLink from 'react-router-dom' for route-aware
className callback and keep the external-anchor branch unchanged).
- Around line 45-51: Replace the multi-line template literal assigned to
baseClasses with a call to the shadcn cn() utility (import from '@/lib/utils')
to compose classes without leading/trailing whitespace and to handle the
conditional active classes; locate the baseClasses declaration in Sidebar.tsx
and rewrite it to pass the static classes and the active ? 'bg-primary/10
text-primary' : 'text-muted-foreground hover:text-foreground hover:bg-muted'
expression into cn() so class merging follows project conventions.
- Around line 99-103: The getWidthClass function contains a redundant branch:
the mobileOpen check returns the same string as the default case; remove that
branch and simplify getWidthClass to only check collapsed (return
'w-[var(--sidebar-width-collapsed)]' when collapsed) and otherwise return the
default 'w-[var(--sidebar-width)]' using the existing symbols mobileOpen,
collapsed and function getWidthClass to locate and update the code.
- Around line 99-103: The first branch in getWidthClass is redundant because
mobileOpen and the default return the same width; simplify getWidthClass by
removing the mobileOpen branch and returning
'w-[var(--sidebar-width-collapsed)]' when collapsed is true, otherwise return
'w-[var(--sidebar-width)]' so the function clearly expresses "collapsed →
narrow, otherwise → full width" (update the getWidthClass function and any
related references to rely on collapsed/mobileOpen where needed).
- Around line 45-51: Replace the multi-line template literal assigned to
baseClasses with the project's cn utility: import cn from "lib/utils" (or
existing utils export) and build baseClasses via cn('flex items-center gap-3
px-3 py-2.5 rounded-lg transition-all group', active ? 'bg-primary/10
text-primary' : 'text-muted-foreground hover:text-foreground hover:bg-muted') to
avoid the leading newline/trailing-space artifact and use the idiomatic class
composition; update any references to baseClasses in Sidebar (component/JSX)
accordingly.
- Around line 33-85: Rename the locally defined NavLink function to
SidebarNavLink (update the function declaration and all internal
references/usages) to avoid shadowing react-router-dom's NavLink export; ensure
any existing imports or future imports of NavLink from 'react-router-dom' remain
unambiguous. While renaming, preserve the existing custom isActive routing logic
(the helper around lines 90–97) so the same dashboard and /dashboard/repo/*
matching behavior continues; if you later switch to react-router-dom's NavLink,
re-implement that matcher via its className/isActive callback or keep the custom
matcher to maintain identical behavior.
In `@frontend/src/hooks/useCachedQuery.ts`:
- Around line 196-198: The invalidate function is recreated every render; wrap
it in React's useCallback to stabilize its reference (e.g., const invalidate =
useCallback(() => queryClient.invalidateQueries({ queryKey: ['repos'] }),
[queryClient])) and return that memoized invalidate from the same place that
currently returns { ...query, invalidate } so callers can safely use it in
effect deps.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
1. docker-compose: add VOYAGE_API_KEY, COHERE_API_KEY, SENTRY_DSN to backend env (were in .env.example but never passed to container) 2. DashboardHome: remove unused useInvalidateRepoCache import 3. ErrorBoundary: remove misleading 'This has been logged' claim (we only console.error, no Sentry integration yet) 4. startup_checks.py: fix stale comment -- says 3-tuple, actual is 2-tuple (env_var_name, description) 5. package.json: add @testing-library/dom peer dep required by RTL v16 6. useCachedQuery: use shared Repository type instead of inline cast missing git_url field -- fixes undefined git_url in DashboardHome
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
docker-compose.yml (1)
35-43: SUPABASE_KEY → SUPABASE_ANON_KEY rename is a breaking change for existing deployments; add a migration note.The rename is semantically correct and aligns the compose file with
auth.pyandsupabase_service.py, and the three previously-missing optional vars (COHERE_API_KEY,VOYAGE_API_KEY,SENTRY_DSN) are now wired in — resolving the past review concern.However, any operator upgrading from a prior version who still has
SUPABASE_KEYin their.envwill find authentication silently broken (or loudly rejected at startup bystartup_checks.py). A short inline comment, or a note in the project README/migration guide, would prevent confusion:📝 Suggested inline comment
- SUPABASE_URL=${SUPABASE_URL} + # Renamed from SUPABASE_KEY — update your .env accordingly - SUPABASE_ANON_KEY=${SUPABASE_ANON_KEY}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 35 - 43, The docker-compose change renames SUPABASE_KEY → SUPABASE_ANON_KEY which breaks upgrades for operators who still use SUPABASE_KEY; add a brief inline comment next to the environment block in docker-compose.yml explaining the rename and instructing operators to update their .env, and add a note to the README/migration guide; optionally, to be extra safe, implement a fallback in startup_checks.py (or where envs are read in auth.py/supabase_service.py) to accept SUPABASE_KEY if SUPABASE_ANON_KEY is missing and log a deprecation warning so existing deployments don’t fail silently.frontend/src/components/dashboard/DashboardHome.tsx (1)
76-76: Consider disabling "Add" buttons during initial repo-list load.
reposdefaults to[]whilereposLoadingis true, sorepos.length >= MAX_FREE_REPOSevaluates tofalseduring the initial fetch. A user who is already at the limit could briefly see the buttons as enabled and attempt an add before the actual count is known.🔧 Suggested fix
- disabled={loading || repos.length >= MAX_FREE_REPOS} + disabled={loading || reposLoading || repos.length >= MAX_FREE_REPOS}Apply the same change to both the "Import from GitHub" button (line 319) and the "Add Repository" button (line 327).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/dashboard/DashboardHome.tsx` at line 76, The "Add" and "Import from GitHub" buttons can be enabled while repos are still loading because repos defaults to [], so update their disabled logic to also check the loading state: change the disabled condition to (reposLoading || repos.length >= MAX_FREE_REPOS) for both buttons referenced in DashboardHome (look for the "Import from GitHub" and "Add Repository" button elements) so they remain disabled during the initial fetch until the real repo count is known.frontend/src/test/ErrorBoundary.test.tsx (2)
1-1: Smoke-test comment claims "recovers on retry" but no such test exists.Line 1 declares the intent to test recovery ("recovers on retry"), yet there is no test case that clicks the Try again button and verifies the error state is cleared and children are rendered again. The three existing tests only cover rendering children, showing the fallback, and button presence — not the reset path.
Consider adding a test like:
it('recovers when Try again is clicked', async () => { const { rerender } = render( <ErrorBoundary> <ThrowingChild shouldThrow={true} /> </ErrorBoundary> ) expect(screen.getByText('Something went wrong')).toBeInTheDocument() fireEvent.click(screen.getByText('Try again')) rerender( <ErrorBoundary> <ThrowingChild shouldThrow={false} /> </ErrorBoundary> ) expect(screen.getByText('child rendered')).toBeInTheDocument() })This would also justify the
fireEventimport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/test/ErrorBoundary.test.tsx` at line 1, The test suite declares "recovers on retry" but lacks a test for the reset path; add a new test that renders <ErrorBoundary> with <ThrowingChild shouldThrow={true} />, asserts the fallback ("Something went wrong") is shown, clicks the "Try again" button (use fireEvent.click on the button), then rerenders <ErrorBoundary> with <ThrowingChild shouldThrow={false} /> and asserts the child content ("child rendered") is displayed; this verifies ErrorBoundary.reset logic and justifies the fireEvent import.
3-3:fireEventis imported but never used.
fireEventis imported from@testing-library/reactbut not referenced in any of the three test cases. Remove it to keep the import surface clean.♻️ Proposed fix
-import { render, screen, fireEvent } from '@testing-library/react' +import { render, screen } from '@testing-library/react'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/test/ErrorBoundary.test.tsx` at line 3, Remove the unused import "fireEvent" from the import statement in ErrorBoundary.test.tsx (currently imported from '@testing-library/react'); edit the import line that reads "import { render, screen, fireEvent } from '@testing-library/react'" to only include the used symbols (render, screen), then run the linter/tests to confirm no unused-import warnings remain.frontend/src/test/useGraphData.test.ts (1)
29-71: Consider sharing a singlerenderHookresult across the last four tests.Each of the four connected-graph tests independently calls
renderHook(() => useGraphData(mockApiData)), which re-runs the ForceAtlas2 layout algorithm every time. Extracting to abeforeAll/module-scoped variable eliminates 3 redundant layout computations.♻️ Suggested refactor
+import { beforeAll } from 'vitest' + +let graph: ReturnType<typeof useGraphData> + +beforeAll(() => { + const { result } = renderHook(() => useGraphData(mockApiData)) + graph = result.current +}) + describe('useGraphData', () => { it('returns null for empty data', () => { const { result } = renderHook(() => useGraphData(undefined)) expect(result.current).toBeNull() }) it('builds a graph with connected nodes, filters orphans', () => { - const { result } = renderHook(() => useGraphData(mockApiData)) - const graph = result.current! + expect(graph).not.toBeNull() // orphan.ts has 0 connections, should be filtered - expect(graph.order).toBe(3) + expect(graph!.order).toBe(3) // ... }) // same pattern for remaining tests })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/test/useGraphData.test.ts` around lines 29 - 71, The four tests repeatedly call renderHook(() => useGraphData(mockApiData)) causing redundant ForceAtlas2 runs; refactor by calling renderHook once in a beforeAll (or module-scoped) block, store result.current (graph) in a shared variable, and have each test read from that shared graph; update references to graph, attrs, etc., so tests use the shared graph instead of invoking renderHook in each it() (symbols: renderHook, useGraphData, mockApiData, graph, attrs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/test/ErrorBoundary.test.tsx`:
- Line 2: Remove the unused fireEvent import from the top import list in
ErrorBoundary.test.tsx and either use userEvent (preferred) or fireEvent if you
add interactive assertions; then add a test case named or matching "recovers on
retry" that mounts the ErrorBoundary (the same setup used in existing tests),
triggers the error state, finds and clicks the "Try again" button (the retry
control rendered by ErrorBoundary), and asserts the error is cleared and the
child content renders again to verify recovery behavior. Ensure you reference
the same ErrorBoundary test helpers and JSX used in the existing tests so the
new test reuses the current setup.
In `@frontend/src/test/useGraphData.test.ts`:
- Around line 30-37: Add an explicit null-check assertion before using the
non-null assertion on result.current in the useGraphData tests: call
expect(result.current).not.toBeNull() (or .toBeDefined()) immediately before the
line that reads result.current! and then access result.current. Do this in all
four connected-graph tests where result.current! is used so the test fails with
a clear Vitest assertion if useGraphData returns null.
In `@frontend/src/test/useMatrixData.test.ts`:
- Around line 42-49: Update the test "counts cross-directory deps only in
totalDeps" in frontend/src/test/useMatrixData.test.ts: correct the misleading
comment about edge counts (edge 3 is intra-directory under frontend/src so
cross-directory edges are 3, not 4) and tighten the assertion on
result.current.totalDeps from toBeGreaterThan(0) to an exact check (e.g.,
expect(data.totalDeps).toBe(3)) so the test verifies the deterministic
mockApiData output; reference the useDirectoryMatrix hook and mockApiData used
to produce result.current.
- Around line 19-20: Update the misleading comment before the edge { source:
'backend/routes/auth.py', target: 'backend/main.py' } to describe it as a
backend-internal back-edge that creates an intra-backend cycle (not a
cross-project dependency); explicitly note that this edge (Edge 4) forms a
bidirectional cycle with the existing backend → backend/routes edge (Edge 2) so
the cycle-detection test behaves as expected and any test data description
referring to "cross-project" should be replaced with "intra-backend back-edge"
or equivalent.
---
Duplicate comments:
In `@frontend/src/components/dashboard/DashboardHome.tsx`:
- Line 33: Remove the now-unused import symbol useInvalidateRepoCache from the
import list in DashboardHome.tsx and ensure only useRepos is imported; verify
that any references to useInvalidateRepoCache have been deleted or replaced so
there are no lingering usages or TypeScript errors related to that symbol.
---
Nitpick comments:
In `@docker-compose.yml`:
- Around line 35-43: The docker-compose change renames SUPABASE_KEY →
SUPABASE_ANON_KEY which breaks upgrades for operators who still use
SUPABASE_KEY; add a brief inline comment next to the environment block in
docker-compose.yml explaining the rename and instructing operators to update
their .env, and add a note to the README/migration guide; optionally, to be
extra safe, implement a fallback in startup_checks.py (or where envs are read in
auth.py/supabase_service.py) to accept SUPABASE_KEY if SUPABASE_ANON_KEY is
missing and log a deprecation warning so existing deployments don’t fail
silently.
In `@frontend/src/components/dashboard/DashboardHome.tsx`:
- Line 76: The "Add" and "Import from GitHub" buttons can be enabled while repos
are still loading because repos defaults to [], so update their disabled logic
to also check the loading state: change the disabled condition to (reposLoading
|| repos.length >= MAX_FREE_REPOS) for both buttons referenced in DashboardHome
(look for the "Import from GitHub" and "Add Repository" button elements) so they
remain disabled during the initial fetch until the real repo count is known.
In `@frontend/src/test/ErrorBoundary.test.tsx`:
- Line 1: The test suite declares "recovers on retry" but lacks a test for the
reset path; add a new test that renders <ErrorBoundary> with <ThrowingChild
shouldThrow={true} />, asserts the fallback ("Something went wrong") is shown,
clicks the "Try again" button (use fireEvent.click on the button), then
rerenders <ErrorBoundary> with <ThrowingChild shouldThrow={false} /> and asserts
the child content ("child rendered") is displayed; this verifies
ErrorBoundary.reset logic and justifies the fireEvent import.
- Line 3: Remove the unused import "fireEvent" from the import statement in
ErrorBoundary.test.tsx (currently imported from '@testing-library/react'); edit
the import line that reads "import { render, screen, fireEvent } from
'@testing-library/react'" to only include the used symbols (render, screen),
then run the linter/tests to confirm no unused-import warnings remain.
In `@frontend/src/test/useGraphData.test.ts`:
- Around line 29-71: The four tests repeatedly call renderHook(() =>
useGraphData(mockApiData)) causing redundant ForceAtlas2 runs; refactor by
calling renderHook once in a beforeAll (or module-scoped) block, store
result.current (graph) in a shared variable, and have each test read from that
shared graph; update references to graph, attrs, etc., so tests use the shared
graph instead of invoking renderHook in each it() (symbols: renderHook,
useGraphData, mockApiData, graph, attrs).
…eEvent import - Moved inline comments on SUPABASE_JWT_SECRET and SUPABASE_SERVICE_ROLE_KEY to their own lines above the values. python-dotenv parses everything after the = sign including inline comments when values are unquoted. - Removed unused fireEvent import from ErrorBoundary.test.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.env.example (1)
23-25: Consider reordering Supabase keys to satisfydotenv-linter.
dotenv-linterwarns thatSUPABASE_JWT_SECRETandSUPABASE_SERVICE_ROLE_KEYshould precedeSUPABASE_URL(alphabetical order:_A…→_J…→_S…→_U…). Functional impact is zero, but keeping the file linter-clean avoids noise in CI.♻️ Proposed reorder
# Supabase (Required) # Get from: https://app.supabase.com/project/_/settings/api -SUPABASE_URL=https://your-project.supabase.co SUPABASE_ANON_KEY=eyJ... # From Project Settings -> API -> JWT Secret SUPABASE_JWT_SECRET=your-jwt-secret # From Project Settings -> API -> service_role key SUPABASE_SERVICE_ROLE_KEY=eyJ... +SUPABASE_URL=https://your-project.supabase.co🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 23 - 25, Move the Supabase secret lines so they appear before SUPABASE_URL to satisfy dotenv-linter's alphabetical ordering: place SUPABASE_JWT_SECRET and SUPABASE_SERVICE_ROLE_KEY above the SUPABASE_URL entry (keep their values unchanged), ensuring the .env.example variables are ordered alphabetically by name to remove the linter warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.env.example:
- Around line 23-25: Move the Supabase secret lines so they appear before
SUPABASE_URL to satisfy dotenv-linter's alphabetical ordering: place
SUPABASE_JWT_SECRET and SUPABASE_SERVICE_ROLE_KEY above the SUPABASE_URL entry
(keep their values unchanged), ensuring the .env.example variables are ordered
alphabetically by name to remove the linter warning.
This was referenced Feb 23, 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
1 participant
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.
What
Focused bug fix and infrastructure sprint. Four distinct fixes, each its own commit.
Changes
1. React Error Boundary (OPE-15)
ErrorBoundary.tsxcomponent wrapping AppRoutes2. NavLink remount fix (OPE-28)
NavLinkfrom inside Sidebar body to module scope3. Docker env var standardization (OPE-12)
SUPABASE_ANON_KEY(wasSUPABASE_KEY)SUPABASE_JWT_SECRETto backend containerSUPABASE_ANON_KEYwhich was never set in Docker4. Supabase env var alignment
5. Replace fetch-in-useEffect with useQuery (OPE-30)
useReposhook in useCachedQuery.ts using React QueryFiles changed: 7
Lines: +350 / -90
Linear
Closes OPE-15, OPE-28, OPE-12, OPE-30
Verified: OPE-8 (already fixed), OPE-13 (already exists), OPE-29 (already done)
Summary by CodeRabbit
New Features
Tests
Chores