Conflicting changes from parent repository#452
Conflicting changes from parent repository#452rnarciso wants to merge 34 commits intocoleam00:mainfrom
Conversation
The frontend was failing to communicate with the backend in development because it was constructing an absolute URL for API requests, bypassing the Vite proxy.
This change modifies the `getApiUrl` function in `src/config/api.ts` to return a relative path ('') in development mode. This ensures all API and WebSocket requests are correctly proxied by the Vite dev server, resolving the connection and CORS issues.
The React UI tests were failing in environments with older Node.js versions due to the use of `crypto.randomUUID()`, which is not universally available. This commit replaces the call to `crypto.randomUUID()` in `src/services/testService.ts` with `uuidv4()` from the `uuid` library. The `uuid` library and its types have been added to the project dependencies. This ensures compatibility and allows the UI tests to run correctly across different development environments.
Fix frontend api url
This commit addresses three issues: 1. A UI crash when clicking the "Dashboard" button without any test results. This was fixed by adding a null check for the test results summary. 2. The React UI tests not running. This was fixed by changing the test command in the vite config to use a non-interactive reporter and fixing the ANSI escape code stripping. 3. A layout issue in the dashboard where the "Test Summary" and "Coverage Analysis" boxes were displayed side-by-side. This was fixed by changing the grid layout to stack them vertically.
Fix Archon Unit Test Bugs
This commit addresses three issues: 1. A UI crash when clicking the "Dashboard" button without any test results. This was fixed by adding robust null checks, optional chaining, and default values for the test results summary and suites. 2. The React UI tests not running. This was fixed by changing the test command in the vite config to use the `dot` reporter, which is more reliable for streaming output. 3. A layout issue in the dashboard where the "Test Summary" and "Coverage Analysis" boxes were displayed side-by-side. This was fixed by changing the grid layout to stack them vertically.
Fix Archon Unit Test Bugs
The React UI tests were failing with the error "AssertionError: expected false to be true". This was caused by a bug in the `isLmConfigured` function in `archon-ui-main/src/utils/onboarding.ts`. The `hasValidCredential` helper function was not correctly handling encrypted credentials. It was checking for the presence of `cred.encrypted_value`, but it should have been checking if `cred.is_encrypted` is `true`. This change updates the `hasValidCredential` function to correctly handle encrypted credentials.
A test in `test/errors.test.tsx` was failing with the error `TestingLibraryElementError: Unable to find an accessible element with the role "alert"`.
This was caused by a flawed test that was not correctly handling asynchronous operations. The test was using `setTimeout` to simulate a delay, but it was not waiting for the delayed code to execute before trying to find the "alert" element.
This change fixes the test by using `vitest`'s fake timers (`vi.useFakeTimers()` and `vi.advanceTimersByTimeAsync()`) and `async/await` with `screen.findByRole('alert')`. This makes the test more robust and reliable.
Fix React UI test assertion error
A test in `test/errors.test.tsx` was failing with the error `ReferenceError: act is not defined`. This was caused by a missing import for the `act` function from `@testing-library/react`. The `act` function was used in the `timeout error simulation` test, but it was not imported. This change adds `act` to the import statement at the top of the file to resolve the error.
The `npm test` script was not generating coverage reports, which caused the test summary dashboard to show all zeros. This change updates the `test` script in `package.json` to include the `--coverage` flag. This ensures that running the standard `npm test` command will generate the necessary coverage reports for the UI.
The test results dashboard was showing all zeros because it was trying to fetch data from a backend API instead of the local report files generated by the test runner. The API was returning an empty response, which caused the dashboard to display incorrect data. This change modifies the `getTestResults` and `getCoverageData` functions in `testService.ts` to fetch the data directly from the static JSON files (`test-results.json` and `coverage-summary.json`). This ensures that the dashboard always reflects the results of the latest local test run.
The test results dashboard was showing "No Test Results Available" because of a mismatch between the data structure of the `test-results.json` file generated by `vitest` and the data structure expected by the dashboard component. This change updates the `getTestResults` function in `testService.ts` to transform the JSON data into the correct shape before passing it to the UI. This ensures that the summary and suite-level test results are displayed correctly.
…m/rnarciso/Archon into fix-react-ui-test-assertion-error
This commit addresses several issues causing React UI tests to fail. The test suite `errors.test.tsx` had a test that was consistently timing out. The test was intended to simulate a timeout using fake timers, but the timer mocking was not working correctly in the test environment. After several attempts to fix the timer mocking, the test was refactored to use real timers and rely on react-testing-library's async `findBy` queries. This makes the test more robust and independent of the tricky fake timer setup. The test suite `api.test.ts` had multiple failures related to API URL configuration. 1. The `getApiUrl` function had its logic for handling development ports removed, causing tests that relied on this logic to fail. The logic was restored. 2. A test checking for error-throwing behavior was failing because a module-level side effect was causing the `import()` to throw, which the test was not designed to catch. The test was updated to correctly assert that the promise from the dynamic import is rejected. 3. The `MCPClientService` was not being imported correctly in the tests because the class itself was not exported. The class is now exported, and a duplicate import was removed from the service file.
This commit addresses several issues causing React UI tests to fail. The test suite `errors.test.tsx` had a test that was consistently timing out. The test was intended to simulate a timeout using fake timers, but the timer mocking was not working correctly in the test environment. After several attempts to fix the timer mocking, the test was refactored to use real timers and rely on react-testing-library's async `findBy` queries. This makes the test more robust and independent of the tricky fake timer setup. The test suite `api.test.ts` had multiple failures related to API URL configuration. 1. The `getApiUrl` function had its logic for handling development ports removed, causing tests that relied on this logic to fail. The logic was restored. 2. A test checking for error-throwing behavior was failing because a module-level side effect was causing the `import()` to throw, which the test was not designed to catch. The test was updated to correctly assert that the promise from the dynamic import is rejected. 3. The `MCPClientService` was not being imported correctly in the tests because the class itself was not exported. The class is now exported, and a duplicate import was removed from the service file.
Fix failing React UI tests in errors.test.tsx and api.test.ts
Fix failing React UI tests
WalkthroughUpdates change test execution/streaming, test-data sourcing and normalization, API URL resolution, UI guards/layout, credential validation, a service export, and add a runtime UUID dependency; tests updated to align with new import/runtime behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client (UI)
participant S as Test Service
participant F as Browser Fetch
C->>S: getTestResults()
S->>F: GET /test-results/test-results.json
F-->>S: JSON payload
S->>S: transform → { summary, suites, timestamp }
S-->>C: Normalized results
C->>C: Render using safeSummary & guards
sequenceDiagram
autonumber
participant V as Vite Dev Server (SSE)
participant OS as Test Process (npx vitest)
participant B as Browser (Client)
B->>V: GET /test/coverage/stream (SSE)
V->>OS: spawn npx vitest run --coverage --reporter=dot --reporter=json
par streaming
OS-->>V: stdout/stderr chunks
V->>V: handleStream: split lines, strip ANSI, emit SSE "output"
V-->>B: SSE "output" events
end
OS-->>V: exit
V-->>B: SSE "done"
sequenceDiagram
autonumber
participant App as Module load
participant Env as Env Vars
participant Loc as window.location
App->>Env: Read VITE_API_URL
alt VITE_API_URL present
App-->>App: return VITE_API_URL
else
App->>Env: Read ARCHON_SERVER_PORT
alt Missing
App-->>App: throw "ARCHON_SERVER_PORT environment variable is required... Default value: 8181"
else Present
App->>Loc: Read protocol, hostname
App-->>App: return protocol + '//' + hostname + ':' + port
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Attempt to reconcile with changes from parent repository |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
archon-ui-main/src/utils/onboarding.ts (1)
31-39: Remove sensitive data from logs (API keys printed to console)The debug log prints credential values and encrypted_value, which can leak secrets to browser consoles, logs, or crash reports. Remove or redact immediately.
Apply this diff to redact while keeping useful diagnostics:
- console.log('🔎 isLmConfigured - API Keys:', apiKeyCreds map c => ({ - key: c.key, - value: c.value, - encrypted_value: c.encrypted_value, - is_encrypted: c.is_encrypted, - hasValidValue: !!(c.value && c.value !== 'null' && c.value !== null) - }))); + console.log('🔎 isLmConfigured - API Keys:', apiKeyCreds.map(c => ({ + key: c.key, + is_encrypted: !!c.is_encrypted, + value_present: typeof c.value === 'string' && c.value.trim() !== '' && c.value !== 'null' + })));archon-ui-main/package.json (1)
11-16: Mismatch: JSON test-results path vs summary scriptvitest.json reporter writes to public/test-results/test-results.json (per vitest.config.ts), but test:coverage:summary reads coverage/test-results.json. This will show “No test results found”.
Apply one of these diffs (Option A updates the script; Option B changes vitest output path to match the script).
Option A — fix the script to read from public/test-results/test-results.json:
- "test:coverage:summary": "echo '\\n📊 ARCHON TEST & COVERAGE SUMMARY\\n═══════════════════════════════════════\\n' && node -e \"try { const data = JSON.parse(require('fs').readFileSync('coverage/test-results.json', 'utf8')); const passed = data.numPassedTests || 0; const failed = data.numFailedTests || 0; const total = data.numTotalTests || 0; const suites = data.numTotalTestSuites || 0; console.log('Test Suites: ' + (failed > 0 ? '\\x1b[31m' + failed + ' failed\\x1b[0m, ' : '') + '\\x1b[32m' + (suites - failed) + ' passed\\x1b[0m, ' + suites + ' total'); console.log('Tests: ' + (failed > 0 ? '\\x1b[31m' + failed + ' failed\\x1b[0m, ' : '') + '\\x1b[32m' + passed + ' passed\\x1b[0m, ' + total + ' total'); console.log('\\n✨ Results saved to coverage/test-results.json'); } catch(e) { console.log('⚠️ No test results found. Run tests first!'); }\" || true", + "test:coverage:summary": "echo '\\n📊 ARCHON TEST & COVERAGE SUMMARY\\n═══════════════════════════════════════\\n' && node -e \"try { const data = JSON.parse(require('fs').readFileSync('public/test-results/test-results.json', 'utf8')); const passed = data.numPassedTests || 0; const failed = data.numFailedTests || 0; const total = data.numTotalTests || 0; const suites = data.numTotalTestSuites || 0; console.log('Test Suites: ' + (failed > 0 ? '\\x1b[31m' + failed + ' failed\\x1b[0m, ' : '') + '\\x1b[32m' + (suites - failed) + ' passed\\x1b[0m, ' + suites + ' total'); console.log('Tests: ' + (failed > 0 ? '\\x1b[31m' + failed + ' failed\\x1b[0m, ' : '') + '\\x1b[32m' + passed + ' passed\\x1b[0m, ' + total + ' total'); console.log('\\n✨ Results saved to public/test-results/test-results.json'); } catch(e) { console.log('⚠️ No test results found. Run tests first!'); }\" || true",Option B — change vitest config to write into coverage/test-results.json instead:
- "test:coverage:run": "vitest run --coverage --reporter=dot --reporter=json", + "test:coverage:run": "vitest run --coverage --reporter=dot --reporter=json",and in vitest.config.ts:
- outputFile: { - json: './public/test-results/test-results.json' - }, + outputFile: { + json: './coverage/test-results.json' + },Prefer Option A to align with your UI fetching /test-results/test-results.json.
🧹 Nitpick comments (9)
archon-ui-main/src/utils/onboarding.ts (1)
44-49: Verify new “is_encrypted implies valid” assumptionTrusting is_encrypted alone may produce false positives if the server sets the flag but the stored secret is missing/invalid. Confirm backend guarantees, or gate this with an additional server-side validation.
I can add a lightweight probe (e.g., HEAD/GET to a “validate credential” endpoint) or reintroduce a non-sensitive presence indicator returned by the backend. Want a follow-up patch?
archon-ui-main/package.json (1)
39-39: @types/uuid likely redundant with uuid v11 (built-in types)uuid has shipped its own TypeScript types for years; keeping @types/uuid can cause duplicate or conflicting declarations.
Apply this diff and run a type check:
"dependencies": { @@ - "uuid": "^11.1.0", + "uuid": "^11.1.0", @@ "devDependencies": { @@ - "@types/uuid": "^10.0.0",If TS complains, we can pin a compatible uuid version or adjust tsconfig’s types.
Also applies to: 49-49
archon-ui-main/test/errors.test.tsx (1)
1-2: Remove unused act importact isn’t used after the refactor. Drop it to keep imports clean.
-import { render, screen, fireEvent, act } from '@testing-library/react' -import { describe, test, expect, vi } from 'vitest' +import { render, screen, fireEvent } from '@testing-library/react' +import { describe, test, expect, vi } from 'vitest'archon-ui-main/src/services/testService.ts (2)
42-44: UUID import is fine; coordinate types with package.jsonSwitching to uuidv4 is reasonable for consistent ID generation across environments. See package.json comment about removing @types/uuid to avoid type duplication.
163-181: SSE robustness: advertise event-stream and allow long-lived connectionsSome servers gate SSE on Accept: text/event-stream; also consider keepalive to prevent premature termination on page transitions.
- const response = await fetch(endpoint, { + const response = await fetch(endpoint, { method: 'POST', headers: { - 'Content-Type': 'application/json', + 'Content-Type': 'application/json', + 'Accept': 'text/event-stream', } - }); + // keepalive can help in some user agents when navigating away + , keepalive: true + } as RequestInit);archon-ui-main/vite.config.ts (1)
168-170: Avoid per-chunk header flush; flush once after writeHeadCalling res.flushHeaders() on every data event is unnecessary and can hurt throughput. Flush once immediately after res.writeHead.
- if (res.flushHeaders) { - res.flushHeaders(); - } + // no-op: flush once after writeHead insteadAdd a one-time flush right after writeHead for this endpoint:
res.writeHead(200, { 'Content-Type': 'text/event-stream', 'Cache-Control': 'no-cache', 'Connection': 'keep-alive', 'Access-Control-Allow-Origin': '*', 'Access-Control-Allow-Headers': 'Content-Type', }); + if (typeof (res as any).flushHeaders === 'function') { + (res as any).flushHeaders(); + }archon-ui-main/src/services/mcpClientService.ts (2)
106-114: Reduce repetition in fetch + error parsing with a small helperEvery method repeats the same fetch/ok/json/error pattern and assumes JSON in error responses. Centralize this to avoid brittle JSON parsing on error bodies and to attach timeouts if needed.
Example helper you can add (outside this hunk):
async function request<T>(input: RequestInfo, init?: RequestInit): Promise<T> { const res = await fetch(input, init); if (!res.ok) { let detail: string | undefined; try { detail = (await res.json())?.detail; } catch { /* ignore */ } throw new Error(detail || `Request failed: ${res.status} ${res.statusText}`); } return res.json() as Promise<T>; }Then replace calls like:
return request<MCPClient[]>(`${this.baseUrl}/api/mcp/clients/`);Also applies to: 119-132, 137-146, 151-164, 169-180, 189-200, 205-216, 221-230, 235-248, 257-266, 271-284, 289-298, 303-314
399-407: Input validation: ensure ARCHON_MCP_PORT is numericCurrent check ensures presence only. If a non-numeric value is set, the constructed URL will be invalid.
Add a numeric validation and clearer error:
- const mcpPort = import.meta.env.ARCHON_MCP_PORT; + const mcpPort = import.meta.env.ARCHON_MCP_PORT; + if (!/^\d+$/.test(String(mcpPort))) { + throw new Error('ARCHON_MCP_PORT must be a numeric string (e.g., 8051)'); + }Also applies to: 409-413
archon-ui-main/test/config/api.test.ts (1)
198-223: Assertion focuses on URL construction; consider loosening the mock shapeThe mocked response shape doesn’t match MCPClient but the test only validates the POST target and body. That’s fine. If TS typing ever tightens here, you can mock to the MCPClient interface to avoid type noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
archon-ui-main/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
archon-ui-main/package.json(3 hunks)archon-ui-main/src/components/ui/TestResultDashboard.tsx(13 hunks)archon-ui-main/src/config/api.ts(1 hunks)archon-ui-main/src/services/mcpClientService.ts(1 hunks)archon-ui-main/src/services/testService.ts(3 hunks)archon-ui-main/src/utils/onboarding.ts(1 hunks)archon-ui-main/test/config/api.test.ts(1 hunks)archon-ui-main/test/errors.test.tsx(3 hunks)archon-ui-main/vite.config.ts(1 hunks)archon-ui-main/vitest.config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}: Remove dead code immediately; do not keep legacy/unused functions
Avoid comments that reference change history (e.g., LEGACY, CHANGED, REMOVED); keep comments focused on current functionality
Files:
archon-ui-main/src/utils/onboarding.tsarchon-ui-main/src/components/ui/TestResultDashboard.tsxarchon-ui-main/src/services/mcpClientService.tsarchon-ui-main/src/services/testService.tsarchon-ui-main/src/config/api.ts
archon-ui-main/src/components/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in archon-ui-main/src/components/
Files:
archon-ui-main/src/components/ui/TestResultDashboard.tsx
archon-ui-main/test/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place frontend tests under archon-ui-main/test/
Files:
archon-ui-main/test/config/api.test.tsarchon-ui-main/test/errors.test.tsx
archon-ui-main/src/services/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place API communication and business logic in archon-ui-main/src/services/
Files:
archon-ui-main/src/services/mcpClientService.tsarchon-ui-main/src/services/testService.ts
🪛 Biome (2.1.2)
archon-ui-main/vite.config.ts
[error] 164-164: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (15)
archon-ui-main/vitest.config.ts (1)
26-26: LGTM: bail explicitly disabled for full-result runsSetting bail: 0 is a good default for producing complete JSON and coverage artifacts even when tests fail.
archon-ui-main/test/errors.test.tsx (2)
39-39: Async refactor improves determinism of timeout testConverting to an async test with a proper awaitable query removes reliance on arbitrary setTimeout assertions.
64-65: Good use of findByRole with explicit timeoutThis awaits the alert appearance and avoids flakiness from fixed delays.
archon-ui-main/src/services/testService.ts (1)
262-292: Could you run a quick local dump of Vitest’s JSON reporter in v1.6.x and share the key structure? For example:# 1. Generate the report npx vitest --reporter=json > vitest-report.json # 2. Show top-level keys cat vitest-report.json | jq 'keys' # 3. Show suite-level keys for the first entry jq '.results[0] | keys' vitest-report.json # 4. Show assertion-level keys for the first test of the first suite jq '.results[0].assertionResults[0] | keys' vitest-report.json # 5. Check for timing fields on that suite jq '.results[0] | { startTime, endTime }' vitest-report.jsonThat will confirm whether the JSON shape uses “results” (not “testResults”), whether suites include
assertionResults, and if they havestartTime/endTime. Once we have those field names, we can finalize the safe-transform implementation.archon-ui-main/src/components/ui/TestResultDashboard.tsx (5)
75-101: Defensive summary handling is solidThe safeSummary wrapper and “No Test Results Available” empty state remove undefined access risks cleanly.
207-207: Confirm duration units (ms vs s)You divide by 1000 assuming summary.duration is milliseconds. If upstream changes units, display will be wrong.
Would you like me to add a small unit test to assert the expected unit and rendering?
218-231: Good UX: conditional failed-tests alert with pluralizationThe guard and message formatting are correct and avoid noise when there are no failures.
236-241: Robust guards in FailedTestsListThe null/shape checks and numeric failed filter prevent runtime errors with malformed data.
415-417: Correct gating for FailedTestsListRendering only when there are failures and suites exist avoids empty containers.
archon-ui-main/src/services/mcpClientService.ts (1)
96-96: Exporting MCPClientService improves reusability and testabilityPublic export is appropriate and matches the tests that import the class directly.
archon-ui-main/src/config/api.ts (3)
10-13: Priority override via VITE_API_URL is correctRespecting VITE_API_URL first is practical for Docker and remote environments.
21-28: Fail-fast in development when ARCHON_SERVER_PORT is missingThrowing here enforces a clear configuration contract, but it also makes importers fail at module-load time because of the exported API_FULL_URL/WS_URL constants.
Confirm this import-time failure is acceptable for DX. If not, we can lazily compute these values or guard the constants behind try/catch to log a warning instead of throwing.
30-33: Use window.location protocol/hostname for dev URLConstructing protocol://hostname:port avoids hard-coding and respects local HTTPS. Good.
archon-ui-main/test/config/api.test.ts (2)
50-54: Import-time error test matches new fail-fast behaviorAsserting a rejected dynamic import precisely captures the module-load failure when ARCHON_SERVER_PORT is absent.
175-183: Good negative test for missing ARCHON_MCP_PORTCovers the specific error messaging paths expected by createArchonClient().
| <div className={`grid gap-6 ${compact ? 'grid-cols-1' : 'grid-cols-1'}`}> | ||
| {/* Test Summary */} | ||
| <div> | ||
| <div className="flex-1"> | ||
| <TestSummaryCard results={results} isLoading={loading && !results} /> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
Grid layout condition is a no-op; compact has no effect
Both branches yield 'grid-cols-1', so the layout never expands. Likely a regression from the earlier xl breakpoint removal.
Fix the condition to restore non-compact layout:
- <div className={`grid gap-6 ${compact ? 'grid-cols-1' : 'grid-cols-1'}`}>
+ <div className={`grid gap-6 ${compact ? 'grid-cols-1' : 'md:grid-cols-2'}`}>Alternatively, if you intentionally want single-column below xl, preserve a wider layout at larger breakpoints:
- <div className={`grid gap-6 ${compact ? 'grid-cols-1' : 'grid-cols-1'}`}>
+ <div className={`grid gap-6 ${compact ? 'grid-cols-1' : 'grid-cols-1 xl:grid-cols-2'}`}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className={`grid gap-6 ${compact ? 'grid-cols-1' : 'grid-cols-1'}`}> | |
| {/* Test Summary */} | |
| <div> | |
| <div className="flex-1"> | |
| <TestSummaryCard results={results} isLoading={loading && !results} /> | |
| </div> | |
| <div className={`grid gap-6 ${compact ? 'grid-cols-1' : 'md:grid-cols-2'}`}> | |
| {/* Test Summary */} | |
| <div className="flex-1"> | |
| <TestSummaryCard results={results} isLoading={loading && !results} /> | |
| </div> |
🤖 Prompt for AI Agents
In archon-ui-main/src/components/ui/TestResultDashboard.tsx around lines 396 to
401, the grid class conditional is a no-op because both branches use
'grid-cols-1'; change the else branch to a wider column layout so compact
actually changes layout (for example use `${compact ? 'grid-cols-1' :
'grid-cols-2'}` or restore breakpoint-based classes like `grid-cols-1
md:grid-cols-2`/`grid-cols-1 xl:grid-cols-2` depending on desired responsive
behavior); update the JSX className string accordingly to reflect the
non-compact layout.
| const response = await fetch('/test-results/coverage/coverage-summary.json'); | ||
| if (!response.ok) { | ||
| throw new Error('Coverage data not available'); | ||
| } | ||
| return await response.json(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restore fallback for coverage data to avoid hard dependency on static files
If the static coverage JSON isn’t present (CI artifacts not published, first run, or running from a non-Vite host), the UI hard-fails. Add API fallback using existing endpoints.
- async getCoverageData(): Promise<any> {
- const response = await fetch('/test-results/coverage/coverage-summary.json');
- if (!response.ok) {
- throw new Error('Coverage data not available');
- }
- return await response.json();
- }
+ async getCoverageData(): Promise<any> {
+ try {
+ const response = await fetch('/test-results/coverage/coverage-summary.json');
+ if (response.ok) {
+ return await response.json();
+ }
+ throw new Error('static-not-found');
+ } catch {
+ // Fallback to API
+ try {
+ return await this.getVitestCoverage();
+ } catch {
+ // Final fallback to pytest coverage (still better than nothing)
+ return await this.getPytestCoverage();
+ }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await fetch('/test-results/coverage/coverage-summary.json'); | |
| if (!response.ok) { | |
| throw new Error('Coverage data not available'); | |
| } | |
| return await response.json(); | |
| } | |
| async getCoverageData(): Promise<any> { | |
| try { | |
| const response = await fetch('/test-results/coverage/coverage-summary.json'); | |
| if (response.ok) { | |
| return await response.json(); | |
| } | |
| throw new Error('static-not-found'); | |
| } catch { | |
| // Fallback to API | |
| try { | |
| return await this.getVitestCoverage(); | |
| } catch { | |
| // Final fallback to pytest coverage (still better than nothing) | |
| return await this.getPytestCoverage(); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In archon-ui-main/src/services/testService.ts around lines 251 to 256, the
function currently throws if the static coverage JSON fetch returns non-ok;
replace that hard-fail with a fallback: attempt the static fetch first, and if
response.ok is false (or the fetch throws), call the existing coverage API
endpoint (e.g. GET /api/test-results/coverage or the project’s coverage REST
endpoint) to retrieve the same summary, parse and return its JSON; ensure errors
are caught and only throw if both static file and API fallback fail, and
preserve awaiting/async behavior so the function always returns parsed JSON when
available.
| const testProcess = exec('npx vitest run --coverage --reporter=dot --reporter=json', { | ||
| cwd: process.cwd(), | ||
| env: { | ||
| ...process.env, | ||
| FORCE_COLOR: '1', | ||
| env: { | ||
| ...process.env, | ||
| FORCE_COLOR: '1', | ||
| CI: 'true', | ||
| NODE_ENV: 'test' | ||
| } // Enable color output and CI mode for cleaner output | ||
| NODE_ENV: 'test', | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Switch from exec to spawn and persist JSON reporter output to public/test-results/test-results.json
- exec buffers stdout/stderr and can truncate output for large test suites. spawn streams safely and avoids maxBuffer issues.
- Since you create public/test-results/, also direct Vitest’s JSON reporter to write there; otherwise testService.getTestResults() will never find test-results.json.
Apply this diff to use spawn and write the JSON report file deterministically:
- const testProcess = exec('npx vitest run --coverage --reporter=dot --reporter=json', {
- cwd: process.cwd(),
- env: {
- ...process.env,
- FORCE_COLOR: '1',
- CI: 'true',
- NODE_ENV: 'test',
- }
- });
+ const testProcess = spawn(
+ 'npx',
+ [
+ 'vitest',
+ 'run',
+ '--coverage',
+ '--reporter=dot',
+ '--reporter=json',
+ '--outputFile',
+ path.join('public', 'test-results', 'test-results.json'),
+ ],
+ {
+ cwd: process.cwd(),
+ env: {
+ ...process.env,
+ FORCE_COLOR: '1',
+ CI: 'true',
+ NODE_ENV: 'test',
+ },
+ shell: true,
+ windowsHide: true,
+ }
+ );Additional change needed outside this hunk (import spawn):
- import { exec } from 'child_process';
+ import { exec, spawn } from 'child_process';Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In archon-ui-main/vite.config.ts around lines 148 to 156, the code uses exec to
run Vitest which can truncate large outputs and doesn't persist the JSON
reporter output; replace exec with child_process.spawn (importing spawn at the
top of the file), run 'npx' as the command with args
['vitest','run','--coverage','--reporter=dot','--reporter=json'] and pipe
stdout/stderr, and ensure Vitest's JSON reporter writes to a deterministic file
by adding '--reporter-options','output=public/test-results/test-results.json'
(create the public/test-results directory beforehand or ensure it's created) so
the spawned process streams correctly and the JSON report is saved for
testService.getTestResults().
| const cleanLine = line.replace(/\x1b\[[0-9;]*m/g, ''); | ||
| res.write(`data: ${JSON.stringify({ type: 'output', message: cleanLine, timestamp: new Date().toISOString() })}\n\n`); |
There was a problem hiding this comment.
Fix Biome error: control character in RegExp; use Unicode escape
Biome flags the control character in the regex literal. Use a Unicode escape to represent ESC.
- const cleanLine = line.replace(/\x1b\[[0-9;]*m/g, '');
+ const cleanLine = line.replace(/\u001B\[[0-9;]*m/g, '');Optionally extract a shared constant at top-level for both stdout/stderr paths:
const ANSI_REGEX = /\u001B\[[0-9;]*m/g;
// usage: line.replace(ANSI_REGEX, '')🧰 Tools
🪛 Biome (2.1.2)
[error] 164-164: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🤖 Prompt for AI Agents
In archon-ui-main/vite.config.ts around lines 164 to 165, the regex uses the
control-character escape \x1b which Biome flags; replace it with the Unicode
escape \u001B (e.g. use /\u001B\[[0-9;]*m/g) and remove the inline literal to
avoid the control char; additionally, extract that regex to a top-level constant
(ANSI_REGEX) and reuse it for both stdout and stderr cleaning so both paths use
the same safe Unicode-escaped pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
archon-ui-main/vite.config.ts (2)
148-156: Use spawn for streaming robustness and persist JSON results to a deterministic file.exec buffers output (risking maxBuffer issues) and your UI/testService flow benefits from a stable JSON file. Switch to spawn and write the JSON reporter to public/test-results/test-results.json. This also aligns with the directory you already ensure exists.
Apply within this hunk:
- const testProcess = exec('npx vitest run --coverage --reporter=dot --reporter=json', { - cwd: process.cwd(), - env: { - ...process.env, - FORCE_COLOR: '1', - CI: 'true', - NODE_ENV: 'test', - } - }); + const testProcess = spawn( + 'npx', + [ + 'vitest', + 'run', + '--coverage', + '--reporter=dot', + '--reporter=json', + '--outputFile', + path.join('public', 'test-results', 'test-results.json'), + ], + { + cwd: process.cwd(), + env: { + ...process.env, + FORCE_COLOR: '1', + CI: 'true', + NODE_ENV: 'test', + }, + shell: true, + windowsHide: true, + } + );Additional change needed outside this hunk (top-level import):
// at the top of the file import { exec, spawn } from 'child_process';If you want to avoid keeping exec around, remove it as well.
I can also wire Vitest to always output JSON to that path via vitest.config.ts to reduce CLI coupling—say the word and I’ll draft it.
158-171: Fix Biome error: control character in regex; switch to Unicode escape and consider a shared constant.Biome flags \x1b in regex literals. Use \u001B instead.
Minimal fix in place:
- lines.forEach((line: string) => { - const cleanLine = line.replace(/\x1b\[[0-9;]*m/g, ''); + lines.forEach((line: string) => { + const cleanLine = line.replace(/\u001B\[[0-9;]*m/g, ''); res.write(`data: ${JSON.stringify({ type: 'output', message: cleanLine, timestamp: new Date().toISOString() })}\n\n`); });Optional consolidation outside this hunk:
// top-level const ANSI_REGEX = /\u001B\[[0-9;]*m/g; // here: const cleanLine = line.replace(ANSI_REGEX, '');
🧹 Nitpick comments (2)
archon-ui-main/test/config/api.test.ts (1)
44-44: Rename test to reflect the new expectation (throws when port is unset), not “uses default 8181.”The description now contradicts the assertion (we expect a throw, not defaulting). Rename to avoid misleading future readers and flaky assumptions.
Example rename outside this hunk:
it('should throw when ARCHON_SERVER_PORT is not set in development (mentions default 8181)', async () => { // ... });archon-ui-main/vite.config.ts (1)
174-175: Minor: Guard stream handlers to be defensive.spawn always provides streams, but adding null checks keeps this resilient if the implementation changes.
if (testProcess.stdout) handleStream(testProcess.stdout, res); if (testProcess.stderr) handleStream(testProcess.stderr, res);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
archon-ui-main/src/config/api.ts(1 hunks)archon-ui-main/test/config/api.test.ts(1 hunks)archon-ui-main/vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- archon-ui-main/src/config/api.ts
🧰 Additional context used
📓 Path-based instructions (1)
archon-ui-main/test/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place frontend tests under archon-ui-main/test/
Files:
archon-ui-main/test/config/api.test.ts
🪛 Biome (2.1.2)
archon-ui-main/vite.config.ts
[error] 164-164: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (1)
archon-ui-main/test/config/api.test.ts (1)
52-56: Good pivot to asserting the dynamic import rejection; behavior now matches the module’s top-level throw.Using await expect(import(...)).rejects.toThrow(...) correctly captures the load-time error from getApiUrl() and the combined regex keeps the assertion resilient. Also, vi.resetModules() in beforeEach prevents cache interference.
* Archon Orchestrator agent POC
* Fixing SSE race condition
* Fix discoverWorkflows destructuring after rebase
discoverWorkflows() return type changed from array to { workflows, errors }
on main. Updated 5 call sites in orchestrator-agent.ts and api.ts that were
still treating the return as an array, causing "{} is not iterable" errors.
Also passes expectedStream through web adapter for SSE race condition fix.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Fixing issue where project selection still invoked main agent orchestrator outside projects
* UI improvements and validate UI skill
* UI workflow flow improvements
* Fix type error and formatting after rebase from main
- Fix DirLoadResult usage in loader.ts (global workflows section)
- Apply Prettier formatting to files touched by rebase conflict resolution
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Deprecating the old handleMessage and command system officially
* Claude in Claude wasn't working, corrected env var
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* Archon Orchestrator agent POC
* Fixing SSE race condition
* Fix discoverWorkflows destructuring after rebase
discoverWorkflows() return type changed from array to { workflows, errors }
on main. Updated 5 call sites in orchestrator-agent.ts and api.ts that were
still treating the return as an array, causing "{} is not iterable" errors.
Also passes expectedStream through web adapter for SSE race condition fix.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Fixing issue where project selection still invoked main agent orchestrator outside projects
* UI improvements and validate UI skill
* UI workflow flow improvements
* Fix type error and formatting after rebase from main
- Fix DirLoadResult usage in loader.ts (global workflows section)
- Apply Prettier formatting to files touched by rebase conflict resolution
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Deprecating the old handleMessage and command system officially
* Claude in Claude wasn't working, corrected env var
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* Archon Orchestrator agent POC
* Fixing SSE race condition
* Fix discoverWorkflows destructuring after rebase
discoverWorkflows() return type changed from array to { workflows, errors }
on main. Updated 5 call sites in orchestrator-agent.ts and api.ts that were
still treating the return as an array, causing "{} is not iterable" errors.
Also passes expectedStream through web adapter for SSE race condition fix.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Fixing issue where project selection still invoked main agent orchestrator outside projects
* UI improvements and validate UI skill
* UI workflow flow improvements
* Fix type error and formatting after rebase from main
- Fix DirLoadResult usage in loader.ts (global workflows section)
- Apply Prettier formatting to files touched by rebase conflict resolution
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Deprecating the old handleMessage and command system officially
* Claude in Claude wasn't working, corrected env var
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Pull Request
Summary
Changes Made
Type of Change
Affected Services
Testing
Test Evidence
Checklist
Breaking Changes
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores