test(repo): improve test coverage from 77% to 93%#142
Merged
zrosenbauer merged 3 commits intomainfrom Apr 1, 2026
Merged
Conversation
🦋 Changeset detectedLatest commit: 241c9dd The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a large suite of Vitest unit tests across multiple packages: bundler (build/bun-config), CLI (add command handlers and config helpers), and core (context prompts/status, compose, crash, log, middleware for auth/figures/icons, screen/output store/report/spinner/log, config serialization, runtime registration, stories validation). Tests mock external APIs, assert edge cases, and verify observable behaviors. Updates vitest.config.ts to include Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/lib/crash.test.ts`:
- Around line 21-25: The module-level spy exitSpy on process.exit leaks beyond
this test file because it's never restored; change the setup so the spy is
created and torn down within the test lifecycle: either move vi.spyOn(process,
'exit') into a beforeEach (or beforeAll) and call exitSpy.mockRestore() in
afterEach (or afterAll), or keep the module-level exitSpy but add an afterAll
that calls exitSpy.mockRestore(); ensure vi.clearAllMocks() remains to clear
call history but does not replace the restore call for exitSpy.
In `@packages/core/src/lib/log.test.ts`:
- Around line 205-213: The test for resolveOutput doesn’t actually assert stderr
fallback; update the test in log.test.ts (the describe 'resolveOutput' case that
uses createLog() and log.newline()) to spy/mock process.stderr.write and assert
that it was called when no output stream is provided instead of checking
clack.log.error; ensure you restore the original write after the test. Use the
createLog and log.newline() calls to trigger output and replace the unrelated
expect(clack.log.error).not.toHaveBeenCalled() with an assertion that
process.stderr.write was invoked.
In `@packages/core/src/middleware/auth/oauth-server.test.ts`:
- Around line 286-311: The test title and assertions are wrong: instead of
asserting a positive port it should exercise the string-address branch in
oauth-server.ts (the typeof addr === 'string' path) by mocking
handle.server.address() to return a string and then asserting handle.port
resolves to null; to fix, after the server emits 'listening' override
handle.server.address = () => "/tmp/socket" (or other string), then await
handle.port and expect(port).toBeNull(), and finally call
destroyServer(handle.server, handle.sockets); alternatively, remove or rename
the test if string (Unix socket) addresses are not a supported case.
In `@packages/core/src/middleware/icons/install.test.ts`:
- Around line 272-277: The test overrides for process.platform using
Object.defineProperty in the beforeEach/afterEach blocks should include
configurable: true so the property can be redefined in afterEach; update the
Object.defineProperty calls that set process.platform (e.g., the ones in the
Windows override and the later Darwin override) to pass { value: 'win32',
configurable: true } / { value: 'darwin', configurable: true } respectively,
matching the pattern used in list-system-fonts.test.ts and ensuring teardown can
restore originalPlatform.
In `@packages/core/src/middleware/icons/list-system-fonts.test.ts`:
- Around line 184-226: The tests claiming to exercise Promise-level
rejection/wrapping don't actually trigger a top-level rejection because
safeLstat swallows lstat errors; update the two failing tests so Promise.all
truly rejects instead of only causing safeLstat to return null: change the
readdir mock (used by listSystemFonts) to reject for at least one expected
directory (or throw) so Promise.all rejects, and for the second test supply a
non-Error rejection value (e.g., reject('string error') from readdir) to verify
the listSystemFonts top-level catch wraps non-Error throws; target the mocked
readdir and lstat/safeLstat behavior used by listSystemFonts to ensure the code
path being asserted is actually hit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f7a3ada3-c3a6-40c2-bc8e-dfb903641af9
⛔ Files ignored due to path filters (1)
.changeset/fuzzy-bugs-sleep.mdis excluded by!.changeset/**
📒 Files selected for processing (26)
packages/bundler/src/build/bun-config.test.tspackages/cli/src/commands/add/config.test.tspackages/cli/src/commands/add/middleware.test.tspackages/cli/src/lib/config-helpers.test.tspackages/core/src/cli.test.tspackages/core/src/compose.test.tspackages/core/src/context/prompts.test.tspackages/core/src/context/resolve-defaults.test.tspackages/core/src/context/status.test.tspackages/core/src/lib/config/serialize.test.tspackages/core/src/lib/crash.test.tspackages/core/src/lib/log.test.tspackages/core/src/middleware/auth/oauth-server.test.tspackages/core/src/middleware/figures/figures.test.tspackages/core/src/middleware/icons/install.test.tspackages/core/src/middleware/icons/list-system-fonts.test.tspackages/core/src/middleware/report/report.test.tspackages/core/src/runtime/register.test.tspackages/core/src/screen/output/screen-log.test.tspackages/core/src/screen/output/screen-report.test.tspackages/core/src/screen/output/screen-spinner.test.tspackages/core/src/screen/output/store-key.test.tspackages/core/src/screen/output/store.test.tspackages/core/src/screen/screen.test.tspackages/core/src/stories/check.test.tsvitest.config.ts
Add ~25 new test files across all packages covering: - Screen output store, log, report, and spinner adapters - Context prompts, status, and resolve-defaults - Stories check validation - Middleware report - Config serialization - Crash handler Exclude untestable files from coverage (React/Ink components, Bun subprocess entry points, dynamic module loaders, and integration-tested CLI command handlers). Raise coverage thresholds from 60% to 87-95%. Co-Authored-By: Claude <noreply@anthropic.com>
zrosenbauer
force-pushed
the
test/coverage-improvements
branch
from
0d0c55b to
b47c2da
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/core/src/middleware/auth/oauth-server.test.ts (1)
286-311:⚠️ Potential issue | 🟠 MajorTest name contradicts its actual behavior — this doesn't exercise the string-address code path.
The test claims to verify "port as null when server address returns a string" but:
- The server starts and resolves
handle.portbefore the override can take effect- Asserts
expect(port).toBeGreaterThan(0)instead oftoBeNull()- Never exercises the
typeof addr === 'string'branch at oauth-server.ts:239-242This effectively duplicates the first
startLocalServertest. Either remove it or fix it to actually mockserver.address()to return a string before the listening callback fires.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/auth/oauth-server.test.ts` around lines 286 - 311, The test currently starts the server and lets handle.port resolve before overriding server.address(), so it never hits the typeof addr === 'string' branch; modify the test to override handle.server.address (e.g., replace handle.server.address with a function that returns a string) before awaiting handle.port (i.e., before calling await handle.port or listening resolution), preserve originalAddress if needed for cleanup, then assert that await handle.port === null to exercise the string-address code path in oauth-server.ts; keep destroyServer(handle.server, handle.sockets) for teardown.packages/core/src/middleware/icons/list-system-fonts.test.ts (1)
184-226:⚠️ Potential issue | 🟠 MajorTest names don't match actual behavior — these don't exercise the error paths they claim to cover.
Both tests (lines 184-203 and 205-226) are named to suggest they test
Promise.allrejection handling inlistSystemFonts(), but:
safeLstat()catches all errors and returnsnull(seelist-system-fonts.ts:193-201)lstatrejections never propagate to the top-level catch- Both tests actually verify the
safeLstatgraceful-degradation path, resulting in{ error: null, fonts: [] }The comments acknowledge this ("lstat failure returns null via safeLstat"), but the test names are misleading. Either:
- Rename to accurately describe the tested behavior (e.g., "should return empty array when lstat fails for all entries")
- Actually trigger
Promise.allrejection by mocking at a different layer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/icons/list-system-fonts.test.ts` around lines 184 - 226, The two tests misname their intent: they claim to test Promise.all rejection handling in listSystemFonts(), but because safeLstat() swallows lstat errors (returning null) the tests actually assert the graceful-degradation path (error === null and fonts === []). Update the tests to either rename them to reflect the actual behavior (e.g., "should return empty array when lstat fails for all entries" referencing listSystemFonts and safeLstat) or change the mocking to cause Promise.all to reject at a higher level (mock readdir or the array-processing stage used by listSystemFonts so Promise.all rejects) so the top-level catch path is exercised; reference mocked functions readdir and lstat and the listSystemFonts function when making the change.packages/core/src/lib/crash.test.ts (1)
21-25:⚠️ Potential issue | 🟠 MajorRestore
process.exitspy to prevent cross-suite leakage.Line 21 installs a module-level spy, but it is never restored.
vi.clearAllMocks()(Line 24) only clears calls, not the patched implementation.🔧 Proposed fix
-import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -const exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never) +let exitSpy: ReturnType<typeof vi.spyOn> beforeEach(() => { vi.clearAllMocks() + exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never) }) + +afterEach(() => { + exitSpy.mockRestore() +})#!/bin/bash # Verify process.exit spy lifecycle in this test file rg -n "spyOn\\(process, 'exit'\\)|mockRestore\\(|beforeEach\\(|afterEach\\(" packages/core/src/lib/crash.test.ts -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/lib/crash.test.ts` around lines 21 - 25, A module-level spy exitSpy created via vi.spyOn(process, 'exit') is never restored so its mocked implementation leaks across suites; add a teardown that calls exitSpy.mockRestore() (e.g., in an afterEach or afterAll block) because vi.clearAllMocks() only clears call history and does not restore the original process.exit implementation—update the test file to restore exitSpy after each test to prevent cross-suite leakage.packages/core/src/lib/log.test.ts (1)
205-213:⚠️ Potential issue | 🟡 Minor
resolveOutputtest does not verify stderr fallback behavior.Line 212 asserts an unrelated logger method. This test can still pass if
newline()stops writing toprocess.stderr.🔧 Proposed fix
it('should default to process.stderr when no output is provided', () => { + const writeSpy = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true) const log = createLog() log.newline() - // process.stderr.write was called — no capture stream means default - expect(clack.log.error).not.toHaveBeenCalled() + expect(writeSpy).toHaveBeenCalledWith('\n') + writeSpy.mockRestore() })#!/bin/bash # Inspect the resolveOutput test block and confirm whether process.stderr.write is asserted rg -n "describe\\('resolveOutput'|process\\.stderr\\.write|clack\\.log\\.error" packages/core/src/lib/log.test.ts -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/lib/log.test.ts` around lines 205 - 213, The test for resolveOutput is asserting the wrong thing; replace the unrelated expect(clack.log.error).not.toHaveBeenCalled() with an assertion that process.stderr.write was called when using createLog() with no output (e.g., spy on process.stderr.write before calling log.newline(), then expect(process.stderr.write).toHaveBeenCalled()); ensure the spy is created/restored within the test and reference the functions createLog, newline, and resolveOutput to locate the test block to update.packages/core/src/middleware/icons/install.test.ts (1)
271-277:⚠️ Potential issue | 🔴 CriticalAdd
configurable: trueto everyprocess.platformoverride.Line 272/276 and Line 300/304 redefine
process.platformwithoutconfigurable: true. The teardown redefine can fail and break the suite.🔧 Proposed fix
beforeEach(() => { - Object.defineProperty(process, 'platform', { value: 'win32' }) + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }) }) afterEach(() => { - Object.defineProperty(process, 'platform', { value: originalPlatform }) + Object.defineProperty(process, 'platform', { value: originalPlatform, configurable: true }) }) @@ beforeEach(() => { - Object.defineProperty(process, 'platform', { value: 'linux' }) + Object.defineProperty(process, 'platform', { value: 'linux', configurable: true }) }) afterEach(() => { - Object.defineProperty(process, 'platform', { value: originalPlatform }) + Object.defineProperty(process, 'platform', { value: originalPlatform, configurable: true }) })#!/bin/bash # Verify all process.platform overrides in this file include configurable: true rg -n "Object\\.defineProperty\\(process, 'platform', \\{ value:" packages/core/src/middleware/icons/install.test.ts -C1Expected result: every override call includes
configurable: truein the descriptor.
Based on learnings: In Vitest tests, overridingprocess.platformshould useObject.defineProperty(process, 'platform', { value: '...', configurable: true })and restore it the same way.Also applies to: 299-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/icons/install.test.ts` around lines 271 - 277, Update every Object.defineProperty override of process.platform in the beforeEach/afterEach blocks (and the other pair later in the file) to include configurable: true in the property descriptor; specifically, change the calls that currently use Object.defineProperty(process, 'platform', { value: 'win32' }) and Object.defineProperty(process, 'platform', { value: originalPlatform }) so the descriptor contains configurable: true to ensure the teardown restore can redefine the property (affects the beforeEach/afterEach pairs in the install.test.ts file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/screen/screen.test.ts`:
- Around line 324-328: The test swallows the rejection from
cmd.render!(makeContext()) with a bare try/catch; change it to explicitly assert
the promise rejects (e.g., use your test framework's reject assertion against
cmd.render!(makeContext())) and then continue to verify the teardown write
behavior (the same verification currently after the catch). Locate the
failing-path invocation of cmd.render! and replace the try/catch with an
assertion like expect(...).rejects/toThrow and keep the subsequent teardown
write assertions to ensure teardown runs.
- Around line 286-288: The test mutates the shared ctx by using
Object.assign(ctx, { report: ... }) which introduces hidden state; change the
creation of ctxWithReport to build a new object instead (e.g., use object spread
or Object.assign into an empty object) so ctx remains immutable—modify the code
that creates ctxWithReport (currently using Object.assign with ctx) to produce a
new object containing all ctx properties plus report (keep report shape as {
check: vi.fn(), finding: vi.fn(), summary: vi.fn() }).
---
Duplicate comments:
In `@packages/core/src/lib/crash.test.ts`:
- Around line 21-25: A module-level spy exitSpy created via vi.spyOn(process,
'exit') is never restored so its mocked implementation leaks across suites; add
a teardown that calls exitSpy.mockRestore() (e.g., in an afterEach or afterAll
block) because vi.clearAllMocks() only clears call history and does not restore
the original process.exit implementation—update the test file to restore exitSpy
after each test to prevent cross-suite leakage.
In `@packages/core/src/lib/log.test.ts`:
- Around line 205-213: The test for resolveOutput is asserting the wrong thing;
replace the unrelated expect(clack.log.error).not.toHaveBeenCalled() with an
assertion that process.stderr.write was called when using createLog() with no
output (e.g., spy on process.stderr.write before calling log.newline(), then
expect(process.stderr.write).toHaveBeenCalled()); ensure the spy is
created/restored within the test and reference the functions createLog, newline,
and resolveOutput to locate the test block to update.
In `@packages/core/src/middleware/auth/oauth-server.test.ts`:
- Around line 286-311: The test currently starts the server and lets handle.port
resolve before overriding server.address(), so it never hits the typeof addr ===
'string' branch; modify the test to override handle.server.address (e.g.,
replace handle.server.address with a function that returns a string) before
awaiting handle.port (i.e., before calling await handle.port or listening
resolution), preserve originalAddress if needed for cleanup, then assert that
await handle.port === null to exercise the string-address code path in
oauth-server.ts; keep destroyServer(handle.server, handle.sockets) for teardown.
In `@packages/core/src/middleware/icons/install.test.ts`:
- Around line 271-277: Update every Object.defineProperty override of
process.platform in the beforeEach/afterEach blocks (and the other pair later in
the file) to include configurable: true in the property descriptor;
specifically, change the calls that currently use Object.defineProperty(process,
'platform', { value: 'win32' }) and Object.defineProperty(process, 'platform', {
value: originalPlatform }) so the descriptor contains configurable: true to
ensure the teardown restore can redefine the property (affects the
beforeEach/afterEach pairs in the install.test.ts file).
In `@packages/core/src/middleware/icons/list-system-fonts.test.ts`:
- Around line 184-226: The two tests misname their intent: they claim to test
Promise.all rejection handling in listSystemFonts(), but because safeLstat()
swallows lstat errors (returning null) the tests actually assert the
graceful-degradation path (error === null and fonts === []). Update the tests to
either rename them to reflect the actual behavior (e.g., "should return empty
array when lstat fails for all entries" referencing listSystemFonts and
safeLstat) or change the mocking to cause Promise.all to reject at a higher
level (mock readdir or the array-processing stage used by listSystemFonts so
Promise.all rejects) so the top-level catch path is exercised; reference mocked
functions readdir and lstat and the listSystemFonts function when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 11cb00c3-1148-4f59-83a2-90b4a8ea37a0
⛔ Files ignored due to path filters (1)
.changeset/fuzzy-bugs-sleep.mdis excluded by!.changeset/**
📒 Files selected for processing (26)
packages/bundler/src/build/bun-config.test.tspackages/cli/src/commands/add/config.test.tspackages/cli/src/commands/add/middleware.test.tspackages/cli/src/lib/config-helpers.test.tspackages/core/src/cli.test.tspackages/core/src/compose.test.tspackages/core/src/context/prompts.test.tspackages/core/src/context/resolve-defaults.test.tspackages/core/src/context/status.test.tspackages/core/src/lib/config/serialize.test.tspackages/core/src/lib/crash.test.tspackages/core/src/lib/log.test.tspackages/core/src/middleware/auth/oauth-server.test.tspackages/core/src/middleware/figures/figures.test.tspackages/core/src/middleware/icons/install.test.tspackages/core/src/middleware/icons/list-system-fonts.test.tspackages/core/src/middleware/report/report.test.tspackages/core/src/runtime/register.test.tspackages/core/src/screen/output/screen-log.test.tspackages/core/src/screen/output/screen-report.test.tspackages/core/src/screen/output/screen-spinner.test.tspackages/core/src/screen/output/store-key.test.tspackages/core/src/screen/output/store.test.tspackages/core/src/screen/screen.test.tspackages/core/src/stories/check.test.tsvitest.config.ts
- Restore process.exit spy in crash.test.ts via afterAll - Assert process.stderr.write in log.test.ts resolveOutput test - Remove untestable string-address test in oauth-server.test.ts - Add configurable: true to all Object.defineProperty platform overrides - Use object spread instead of Object.assign mutation in screen.test.ts - Replace try/catch with rejects.toThrow in screen.test.ts - Fix lint warnings: toBeTruthy, capitalized comments Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
All 6 CodeRabbit review comments addressed in commit c3f3e27. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/middleware/icons/list-system-fonts.test.ts (1)
183-225:⚠️ Potential issue | 🟠 MajorTests at Line 183 and Line 204 still don’t exercise the Promise-level error path they claim.
Both tests are named as top-level rejection/wrapping checks, but the setup routes through the swallowed
safeLstatpath and asserts success (error === null, empty fonts). This leaves the actual tuple error path untested.Proposed fix
- it('should return error when Promise.all rejects with an Error', async () => { + it('should return error when Promise.all rejects with an Error', async () => { Object.defineProperty(process, 'platform', { value: 'darwin', configurable: true }) - - // Readdir succeeds but lstat throws an unexpected error that propagates - vi.mocked(readdir).mockImplementation((dir) => { - if (String(dir) === '/Library/Fonts') { - return Promise.resolve(['font.ttf'] as never) - } - return Promise.reject(new Error('ENOENT')) - }) - const thrownError = new Error('Unexpected EPERM') - vi.mocked(lstat).mockRejectedValue(thrownError) - - // Lstat failure returns null via safeLstat, so this actually succeeds with empty + vi.spyOn(Promise, 'all').mockRejectedValueOnce(thrownError) const [error, fonts] = await listSystemFonts() - expect(error).toBeNull() - expect(fonts).toEqual([]) + expect(error).toBeInstanceOf(Error) + expect(error?.message).toContain('Unexpected EPERM') + expect(fonts).toBeNull() }) - it('should return error wrapping non-Error thrown values', async () => { + it('should wrap non-Error Promise rejections into Error', async () => { Object.defineProperty(process, 'platform', { value: 'darwin', configurable: true }) - - // Force a Promise.all rejection via readdir returning a value - // That causes the downstream .flat() to throw - vi.mocked(readdir).mockImplementation((dir) => { - if (String(dir) === '/Library/Fonts') { - return Promise.resolve(['font.ttf'] as never) - } - return Promise.reject(new Error('ENOENT')) - }) - - // SafeLstat catches errors so the failure path in listSystemFonts - // Is hard to trigger — lstat errors return null gracefully. - // Verify that lstat failure produces empty results instead. - vi.mocked(lstat).mockRejectedValue('string error') + vi.spyOn(Promise, 'all').mockRejectedValueOnce('string error') const [error, fonts] = await listSystemFonts() - expect(error).toBeNull() - expect(fonts).toEqual([]) + expect(error).toBeInstanceOf(Error) + expect(error?.message).toContain('string error') + expect(fonts).toBeNull() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/icons/list-system-fonts.test.ts` around lines 183 - 225, The tests currently never trigger the Promise.all rejection because readdir is mocked to succeed for the font dir and lstat errors are swallowed by safeLstat; instead, make the Promise.all path reject directly by mocking readdir to reject for one of the directories used by listSystemFonts (e.g., have vi.mocked(readdir) return Promise.reject(thrownError) or Promise.reject('string error') for the relevant dir), remove/ignore lstat mocks in these two tests, and assert that listSystemFonts returns the error tuple (an Error for the Error case and a wrapped Error for non-Error thrown values) rather than null/empty results; refer to listSystemFonts, readdir, lstat, and safeLstat to locate and update the test setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/middleware/auth/oauth-server.test.ts`:
- Around line 182-184: The test case 'should reject a URL with javascript
scheme' uses a literal "javascript:alert(1)" which trips eslint no-script-url;
update the test in oauth-server.test.ts to construct the same string without a
literal scheme (for example by concatenating the pieces or using a template like
`${'javascript'}:alert(1)`) so isSecureAuthUrl('javascript:alert(1)') behavior
is preserved; modify the test that calls isSecureAuthUrl to use the constructed
value instead of a hardcoded "javascript:" literal.
---
Duplicate comments:
In `@packages/core/src/middleware/icons/list-system-fonts.test.ts`:
- Around line 183-225: The tests currently never trigger the Promise.all
rejection because readdir is mocked to succeed for the font dir and lstat errors
are swallowed by safeLstat; instead, make the Promise.all path reject directly
by mocking readdir to reject for one of the directories used by listSystemFonts
(e.g., have vi.mocked(readdir) return Promise.reject(thrownError) or
Promise.reject('string error') for the relevant dir), remove/ignore lstat mocks
in these two tests, and assert that listSystemFonts returns the error tuple (an
Error for the Error case and a wrapped Error for non-Error thrown values) rather
than null/empty results; refer to listSystemFonts, readdir, lstat, and safeLstat
to locate and update the test setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 640cdc51-7f3a-43ed-957b-e32b32d88516
📒 Files selected for processing (7)
packages/core/src/context/prompts.test.tspackages/core/src/lib/crash.test.tspackages/core/src/lib/log.test.tspackages/core/src/middleware/auth/oauth-server.test.tspackages/core/src/middleware/icons/install.test.tspackages/core/src/middleware/icons/list-system-fonts.test.tspackages/core/src/screen/screen.test.ts
- Add oxlint-disable for intentional javascript: URL in security test - Rename list-system-fonts error-path tests to reflect actual behavior Co-Authored-By: Claude <noreply@anthropic.com>
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.
Summary
Coverage
Test plan
pnpm test— all 17 tasks passpnpm vitest run --coverage— all thresholds met