Skip to content

Commit 4f5fb27

Browse files
committed
fix: address CodeRabbit review findings -- 6 issues
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
1 parent b9817f3 commit 4f5fb27

10 files changed

Lines changed: 199 additions & 17 deletions

File tree

backend/config/startup_checks.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
from services.observability import logger
1010

1111

12-
# (env_var_name, description, required)
12+
# (env_var_name, description)
1313
REQUIRED_VARS: List[Tuple[str, str]] = [
1414
("SUPABASE_URL", "Supabase project URL"),
1515
("SUPABASE_ANON_KEY", "Supabase anon/public key"),

docker-compose.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,9 @@ services:
3838
- API_KEY=${API_KEY}
3939
- BACKEND_API_URL=http://backend:8000
4040
- DISCORD_FEEDBACK_WEBHOOK=${DISCORD_FEEDBACK_WEBHOOK}
41+
- COHERE_API_KEY=${COHERE_API_KEY}
42+
- VOYAGE_API_KEY=${VOYAGE_API_KEY}
43+
- SENTRY_DSN=${SENTRY_DSN}
4144
- FORWARDED_ALLOW_IPS=*
4245
volumes:
4346
- ./backend/repos:/app/repos

frontend/bun.lock

Lines changed: 5 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

frontend/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050
"tailwindcss-animate": "^1.0.7"
5151
},
5252
"devDependencies": {
53+
"@testing-library/dom": "^10.4.1",
5354
"@testing-library/jest-dom": "^6.9.1",
5455
"@testing-library/react": "^16.3.2",
5556
"@testing-library/user-event": "^14.6.1",

frontend/src/components/ErrorBoundary.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ export class ErrorBoundary extends Component<Props, State> {
3232
Something went wrong
3333
</h1>
3434
<p className="text-muted-foreground">
35-
An unexpected error occurred. This has been logged.
35+
An unexpected error occurred.
3636
</p>
3737
{this.state.error && (
3838
<pre className="text-xs text-left bg-muted border border-border rounded-lg p-3 overflow-auto max-h-[120px] text-muted-foreground">

frontend/src/components/dashboard/DashboardHome.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import { UpgradeLimitModal } from '../UpgradeLimitModal'
3030
import type { Repository } from '../../types'
3131
import type { GitHubRepo } from '../../hooks/useGitHubRepos'
3232
import { API_URL } from '../../config/api'
33-
import { useRepos, useInvalidateRepoCache } from '../../hooks/useCachedQuery'
33+
import { useRepos } from '../../hooks/useCachedQuery'
3434

3535
const MAX_FREE_REPOS = 3
3636

frontend/src/hooks/useCachedQuery.ts

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import { useQuery, useQueryClient } from '@tanstack/react-query'
1212
import { getFromCache, saveToCache, invalidateRepoCache } from '../lib/cache'
1313
import { API_URL } from '../config/api'
14+
import type { Repository } from '../types'
1415

1516
// Stale time: 5 minutes (data considered fresh)
1617
const STALE_TIME = 5 * 60 * 1000
@@ -177,16 +178,7 @@ export function useRepos(apiKey: string | undefined) {
177178
queryKey: ['repos'],
178179
queryFn: async () => {
179180
const data = await fetchWithAuth(`${API_URL}/repos`, apiKey!)
180-
return (data.repositories || []) as Array<{
181-
id: string
182-
name: string
183-
url: string
184-
status: string
185-
file_count: number
186-
language: string
187-
branch: string
188-
created_at: string
189-
}>
181+
return (data.repositories || []) as Array<Repository>
190182
},
191183
enabled: !!apiKey,
192184
refetchInterval: 30_000,
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
// smoke test: ErrorBoundary renders fallback on error, recovers on retry
2+
import { describe, it, expect, vi } from 'vitest'
3+
import { render, screen, fireEvent } from '@testing-library/react'
4+
import { ErrorBoundary } from '@/components/ErrorBoundary'
5+
6+
function ThrowingChild({ shouldThrow }: { shouldThrow: boolean }) {
7+
if (shouldThrow) throw new Error('test explosion')
8+
return <div>child rendered</div>
9+
}
10+
11+
describe('ErrorBoundary', () => {
12+
// suppress console.error from React's error boundary logging
13+
const originalError = console.error
14+
beforeEach(() => { console.error = vi.fn() })
15+
afterEach(() => { console.error = originalError })
16+
17+
it('renders children when no error', () => {
18+
render(
19+
<ErrorBoundary>
20+
<div>hello</div>
21+
</ErrorBoundary>
22+
)
23+
expect(screen.getByText('hello')).toBeInTheDocument()
24+
})
25+
26+
it('renders fallback UI when child throws', () => {
27+
render(
28+
<ErrorBoundary>
29+
<ThrowingChild shouldThrow={true} />
30+
</ErrorBoundary>
31+
)
32+
expect(screen.getByText('Something went wrong')).toBeInTheDocument()
33+
expect(screen.getByText('test explosion')).toBeInTheDocument()
34+
})
35+
36+
it('shows Try again and Go home buttons', () => {
37+
render(
38+
<ErrorBoundary>
39+
<ThrowingChild shouldThrow={true} />
40+
</ErrorBoundary>
41+
)
42+
expect(screen.getByText('Try again')).toBeInTheDocument()
43+
expect(screen.getByText('Go home')).toBeInTheDocument()
44+
})
45+
})
Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
// smoke test: useGraphData transforms API response into a valid graphology graph
2+
import { describe, it, expect } from 'vitest'
3+
import { renderHook } from '@testing-library/react'
4+
import { useGraphData } from '@/components/DependencyGraph/GraphView/useGraphData'
5+
import type { DependencyApiResponse } from '@/components/DependencyGraph/types'
6+
7+
const mockApiData: DependencyApiResponse = {
8+
nodes: [
9+
{ id: 'src/App.tsx', label: 'App.tsx', language: 'typescript', import_count: 3 },
10+
{ id: 'src/utils.ts', label: 'utils.ts', language: 'typescript', import_count: 0 },
11+
{ id: 'src/types.ts', label: 'types.ts', language: 'typescript', import_count: 0 },
12+
{ id: 'src/orphan.ts', label: 'orphan.ts', language: 'typescript', import_count: 0 },
13+
],
14+
edges: [
15+
{ source: 'src/App.tsx', target: 'src/utils.ts' },
16+
{ source: 'src/App.tsx', target: 'src/types.ts' },
17+
],
18+
total_files: 4,
19+
total_dependencies: 2,
20+
}
21+
22+
describe('useGraphData', () => {
23+
it('returns null for empty data', () => {
24+
const { result } = renderHook(() => useGraphData(undefined))
25+
expect(result.current).toBeNull()
26+
})
27+
28+
it('builds a graph with connected nodes, filters orphans', () => {
29+
const { result } = renderHook(() => useGraphData(mockApiData))
30+
const graph = result.current!
31+
32+
// orphan.ts has 0 connections, should be filtered
33+
expect(graph.order).toBe(3)
34+
expect(graph.hasNode('src/App.tsx')).toBe(true)
35+
expect(graph.hasNode('src/utils.ts')).toBe(true)
36+
expect(graph.hasNode('src/types.ts')).toBe(true)
37+
expect(graph.hasNode('src/orphan.ts')).toBe(false)
38+
})
39+
40+
it('creates correct edges', () => {
41+
const { result } = renderHook(() => useGraphData(mockApiData))
42+
const graph = result.current!
43+
44+
expect(graph.size).toBe(2)
45+
expect(graph.hasEdge('src/App.tsx', 'src/utils.ts')).toBe(true)
46+
expect(graph.hasEdge('src/App.tsx', 'src/types.ts')).toBe(true)
47+
})
48+
49+
it('assigns node attributes: label, size, directory, color', () => {
50+
const { result } = renderHook(() => useGraphData(mockApiData))
51+
const graph = result.current!
52+
const attrs = graph.getNodeAttributes('src/App.tsx')
53+
54+
expect(attrs.label).toBe('App.tsx')
55+
expect(attrs.directory).toBe('src')
56+
expect(typeof attrs.size).toBe('number')
57+
expect(attrs.size).toBeGreaterThan(0)
58+
// Louvain or directory fallback should assign a color
59+
expect(attrs.color).toBeDefined()
60+
})
61+
62+
it('sets ForceAtlas2 positions (x, y are numbers)', () => {
63+
const { result } = renderHook(() => useGraphData(mockApiData))
64+
const graph = result.current!
65+
const attrs = graph.getNodeAttributes('src/App.tsx')
66+
67+
expect(typeof attrs.x).toBe('number')
68+
expect(typeof attrs.y).toBe('number')
69+
expect(Number.isFinite(attrs.x)).toBe(true)
70+
expect(Number.isFinite(attrs.y)).toBe(true)
71+
})
72+
})
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
// smoke test: useDirectoryMatrix builds correct adjacency matrix from API data
2+
import { describe, it, expect } from 'vitest'
3+
import { renderHook } from '@testing-library/react'
4+
import { useDirectoryMatrix } from '@/components/DependencyGraph/MatrixView/useMatrixData'
5+
import type { DependencyApiResponse } from '@/components/DependencyGraph/types'
6+
7+
const mockApiData: DependencyApiResponse = {
8+
nodes: [
9+
{ id: 'backend/main.py' },
10+
{ id: 'backend/routes/auth.py' },
11+
{ id: 'backend/services/db.py' },
12+
{ id: 'frontend/src/App.tsx' },
13+
{ id: 'frontend/src/utils.ts' },
14+
],
15+
edges: [
16+
{ source: 'backend/routes/auth.py', target: 'backend/services/db.py' },
17+
{ source: 'backend/main.py', target: 'backend/routes/auth.py' },
18+
{ source: 'frontend/src/App.tsx', target: 'frontend/src/utils.ts' },
19+
// cross-project dep
20+
{ source: 'backend/routes/auth.py', target: 'backend/main.py' },
21+
],
22+
total_files: 5,
23+
total_dependencies: 4,
24+
}
25+
26+
describe('useDirectoryMatrix', () => {
27+
it('returns null for empty data', () => {
28+
const { result } = renderHook(() => useDirectoryMatrix(undefined))
29+
expect(result.current).toBeNull()
30+
})
31+
32+
it('groups files into directories', () => {
33+
const { result } = renderHook(() => useDirectoryMatrix(mockApiData))
34+
const data = result.current!
35+
36+
expect(data.directories).toContain('backend')
37+
expect(data.directories).toContain('backend/routes')
38+
expect(data.directories).toContain('backend/services')
39+
expect(data.directories).toContain('frontend/src')
40+
})
41+
42+
it('counts cross-directory deps only in totalDeps', () => {
43+
const { result } = renderHook(() => useDirectoryMatrix(mockApiData))
44+
const data = result.current!
45+
46+
// all 4 edges are cross-directory (different dirs), intra-dir = 0
47+
// the matrix diagonal is self-deps which we exclude
48+
expect(data.totalDeps).toBeGreaterThan(0)
49+
})
50+
51+
it('detects circular dependencies between directories', () => {
52+
const { result } = renderHook(() => useDirectoryMatrix(mockApiData))
53+
const data = result.current!
54+
55+
// backend -> backend/routes and backend/routes -> backend = circular
56+
expect(data.totalCycles).toBeGreaterThanOrEqual(1)
57+
})
58+
59+
it('provides short labels for display', () => {
60+
const { result } = renderHook(() => useDirectoryMatrix(mockApiData))
61+
const data = result.current!
62+
63+
// short labels are the last segment of the dir path
64+
expect(data.shortLabels).toContain('routes')
65+
expect(data.shortLabels).toContain('services')
66+
expect(data.shortLabels).toContain('src')
67+
})
68+
})

0 commit comments

Comments
 (0)