Merged
Conversation
|
Cursor Agent can help with this pull request. Just |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtracts a client-side RecordingForm into Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RecordingForm as "RecordingForm (client)"
participant Fetch as "Browser Fetch"
participant Server as "POST /resources/calls/save"
participant Router as "React Router"
User->>RecordingForm: fill fields + attach audio, click submit
RecordingForm->>RecordingForm: client-side validate fields
alt validation fails
RecordingForm-->>User: display field errors
else validation passes
RecordingForm->>RecordingForm: convert audio -> dataURL, build request body
RecordingForm->>Fetch: POST to recordingFormActionPath (with headers)
Fetch->>Server: send request
Server->>Fetch: respond (success | validation error | redirect)
alt redirect
Fetch-->>RecordingForm: Location header
RecordingForm->>Router: trigger navigation/revalidation
Router->>User: navigate to target
else success without redirect
Fetch-->>RecordingForm: success response
RecordingForm-->>User: show success state / update UI
else server validation error
Fetch-->>RecordingForm: actionData/errors
RecordingForm-->>User: display server errors
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
content/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.test.browser.jsx (1)
53-57: Replace thesetTimeoutflush with a deterministicactawait.The
setTimeout(0)workaround is flaky and leaves a TODO in tests. You can flush the state update by awaiting the deferred promise insideactinstead.✅ Suggested change
- await defer.resolve() - await clickPromise - // TODO: figure out why this is necessary with the latest testing lib version 😵 - await new Promise((res) => setTimeout(res, 0)) + defer.resolve() + await act(async () => { + await defer.promise + }) + await clickPromiseIf you want, I can also help remove the TODO entirely by adjusting the test flow or component state handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@content/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.test.browser.jsx` around lines 53 - 57, Replace the flaky setTimeout flush with a deterministic act-wrapped await: import act from the test utils and wrap the async waits so the state updates are flushed inside act (e.g. use act(async () => { await defer.resolve(); await clickPromise; }) or at minimum await act(async () => await defer.resolve()); then remove the setTimeout workaround and the TODO; this targets the existing defer and clickPromise awaits and ensures screen.queryByText(/saving/i) is asserted after React state has been flushed.app/components/calls/recording-form.tsx (2)
231-236: Unsafe cast of server response JSON toRecordingFormData.Line 233 casts the parsed JSON to
RecordingFormDatawithout validation. If the server response shape ever diverges (e.g., a proxy error returning HTML-as-JSON, or an unexpected 4xx body), this could lead to subtle rendering bugs with undefined field accesses.Consider a lightweight guard (e.g., checking for
actionData.errorsoractionData.fieldsbefore assigning):♻️ Suggested guard
const actionData = await response.json().catch(() => null) - if (actionData && typeof actionData === 'object') { - setSubmissionData(actionData as RecordingFormData) + if ( + actionData && + typeof actionData === 'object' && + 'errors' in actionData + ) { + setSubmissionData(actionData as RecordingFormData) } else { setRequestError('Something went wrong submitting your recording.') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/calls/recording-form.tsx` around lines 231 - 236, The parsed JSON (actionData) is being unsafely cast to RecordingFormData before validation; update the submission handling in the recording-form component to perform a lightweight runtime guard on actionData (e.g., check for expected keys like errors, fields, or id) after the response.json().catch() and before calling setSubmissionData; if the guard passes, call setSubmissionData(actionData as RecordingFormData), otherwise call setRequestError('Something went wrong submitting your recording.') to avoid undefined-field runtime errors (refer to the actionData variable, setSubmissionData, setRequestError, and RecordingFormData).
73-99:URL.createObjectURLinuseMemomay produce a revoked URL in Strict Mode.In React 19 Strict Mode, effects fire cleanup+remount, but
useMemoreuses the cached value. This means the cleanup on line 98 revokes the blob URL, while the memoizedaudioURLstill references it — leaving the<audio>element with a deadsrcin development.A safer pattern is to manage both creation and cleanup inside
useEffectwith arefor state:♻️ Suggested refactor
- const audioURL = React.useMemo(() => { - return URL.createObjectURL(audio) - }, [audio]) + const [audioURL, setAudioURL] = React.useState<string | null>(null) - React.useEffect(() => { - return () => URL.revokeObjectURL(audioURL) - }, [audioURL]) + React.useEffect(() => { + const url = URL.createObjectURL(audio) + setAudioURL(url) + return () => URL.revokeObjectURL(url) + }, [audio])Then guard the
<audio>rendering onaudioURLbeing non-null (which it effectively already does on line 282).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/calls/recording-form.tsx` around lines 73 - 99, The memoized audioURL created with URL.createObjectURL inside the audioURL useMemo can be revoked by the cleanup effect (React.useEffect returning URL.revokeObjectURL) while the memo still returns the same (revoked) string in Strict Mode; fix by removing createObjectURL from useMemo and instead create and store the blob URL in state or a ref inside a React.useEffect that returns a cleanup which calls URL.revokeObjectURL on that stored value (use audio, audioURL, and the existing cleanup logic as identifiers), and keep the existing conditional rendering that only renders the <audio> when audioURL is non-null to avoid supplying a revoked src.vitest.browser.config.ts (1)
6-7: Fix ESLint import ordering.ESLint flags that the
vite-env-onlyimport should appear beforevite-tsconfig-paths.♻️ Suggested fix
-import tsconfigPaths from 'vite-tsconfig-paths' -import { envOnlyMacros } from 'vite-env-only' +import { envOnlyMacros } from 'vite-env-only' +import tsconfigPaths from 'vite-tsconfig-paths'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.browser.config.ts` around lines 6 - 7, ESLint import ordering is failing because the import of tsconfigPaths appears before envOnlyMacros; swap the two import statements so that envOnlyMacros from 'vite-env-only' is imported before tsconfigPaths from 'vite-tsconfig-paths' (i.e., ensure the envOnlyMacros import line is above the tsconfigPaths import line) to satisfy the linter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/utils/__tests__/call-kent-audio-storage.server.test.ts`:
- Around line 9-14: The test mutates module-scope environment variables
(process.env.R2_BUCKET, R2_ENDPOINT, R2_ACCESS_KEY_ID, R2_SECRET_ACCESS_KEY,
CALL_KENT_R2_BUCKET) which can leak across tests; wrap these assignments inside
a test suite lifecycle by saving the original values in beforeAll, set the mock
values there, and restore the originals in afterAll (or beforeEach/afterEach if
per-test isolation is preferred) so that the worker/global environment is
returned to its prior state after tests complete.
In `@app/utils/__tests__/fetch-json-with-retry-after.server.test.ts`:
- Around line 2-3: The import order violates the ESLint rule; move the vitest
import (beforeEach, expect, test, vi) so it appears before the local mswServer
import (mswServer) at the top of the test file; update the top lines so "import
{ beforeEach, expect, test, vi } from 'vitest'" precedes "import { mswServer }
from '#tests/msw-server.ts'".
In `@app/utils/__tests__/semantic-search-admin.server.test.ts`:
- Around line 4-10: The module-scope assignments to process.env.R2_BUCKET,
process.env.R2_ENDPOINT, process.env.R2_ACCESS_KEY_ID, and
process.env.R2_SECRET_ACCESS_KEY leak across tests; fix by moving these
assignments into a test lifecycle hook (e.g., beforeAll) where you first capture
the original values into local variables, set the mocked values, and then
restore the originals in afterAll (or use beforeEach/afterEach for per-test
isolation) so the environment is saved and restored and no mutation persists
outside this test file.
In `@app/utils/__tests__/x-server.test.ts`:
- Around line 2-3: ESLint requires module import ordering — move the named
import from 'vitest' (beforeEach, describe, expect, test, vi) so it appears
before the local/aliased import mswServer from '#tests/msw-server.ts'; update
the import block in app/utils/__tests__/x-server.test.ts to import { beforeEach,
describe, expect, test, vi } from 'vitest' first, then import { mswServer } from
'#tests/msw-server.ts' to satisfy the rule.
In `@mocks/msw-handlers.ts`:
- Around line 30-34: The image mock incorrectly decodes the base64 and returns
JSON; change the Buffer construction to Buffer.from(base64, 'base64') to decode
properly, and replace the HttpResponse.json(buffer) call with creating a raw
HttpResponse using the HttpResponse constructor (pass the buffer as the body and
set headers including 'Content-Type': 'image/webp' and an appropriate status),
mirroring how the fallback handler builds responses.
In `@vitest.config.ts`:
- Around line 4-7: ESLint wants the module import order fixed: move the
tsconfigPaths import to appear before importing symbols from 'vitest/config' so
that "tsconfigPaths" (import tsconfigPaths from 'vite-tsconfig-paths') precedes
"configDefaults" and "defineConfig" (import { configDefaults, defineConfig }
from 'vitest/config'); update the import order at the top of vitest.config.ts
accordingly while keeping the same imported identifiers (react, tsconfigPaths,
configDefaults, defineConfig).
---
Nitpick comments:
In `@app/components/calls/recording-form.tsx`:
- Around line 231-236: The parsed JSON (actionData) is being unsafely cast to
RecordingFormData before validation; update the submission handling in the
recording-form component to perform a lightweight runtime guard on actionData
(e.g., check for expected keys like errors, fields, or id) after the
response.json().catch() and before calling setSubmissionData; if the guard
passes, call setSubmissionData(actionData as RecordingFormData), otherwise call
setRequestError('Something went wrong submitting your recording.') to avoid
undefined-field runtime errors (refer to the actionData variable,
setSubmissionData, setRequestError, and RecordingFormData).
- Around line 73-99: The memoized audioURL created with URL.createObjectURL
inside the audioURL useMemo can be revoked by the cleanup effect
(React.useEffect returning URL.revokeObjectURL) while the memo still returns the
same (revoked) string in Strict Mode; fix by removing createObjectURL from
useMemo and instead create and store the blob URL in state or a ref inside a
React.useEffect that returns a cleanup which calls URL.revokeObjectURL on that
stored value (use audio, audioURL, and the existing cleanup logic as
identifiers), and keep the existing conditional rendering that only renders the
<audio> when audioURL is non-null to avoid supplying a revoked src.
In
`@content/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.test.browser.jsx`:
- Around line 53-57: Replace the flaky setTimeout flush with a deterministic
act-wrapped await: import act from the test utils and wrap the async waits so
the state updates are flushed inside act (e.g. use act(async () => { await
defer.resolve(); await clickPromise; }) or at minimum await act(async () =>
await defer.resolve()); then remove the setTimeout workaround and the TODO; this
targets the existing defer and clickPromise awaits and ensures
screen.queryByText(/saving/i) is asserted after React state has been flushed.
In `@vitest.browser.config.ts`:
- Around line 6-7: ESLint import ordering is failing because the import of
tsconfigPaths appears before envOnlyMacros; swap the two import statements so
that envOnlyMacros from 'vite-env-only' is imported before tsconfigPaths from
'vite-tsconfig-paths' (i.e., ensure the envOnlyMacros import line is above the
tsconfigPaths import line) to satisfy the linter.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (32)
app/components/calls/__tests__/submit-recording-form.test.browser.tsxapp/components/calls/recording-form.tsxapp/components/hooks/__tests__/use-countdown.test.browser.tsxapp/routes/calls_.admin/$callId.tsxapp/routes/calls_.record/new.tsxapp/routes/resources/calls/save.tsxapp/utils/__tests__/call-kent-audio-storage.server.test.tsapp/utils/__tests__/fetch-json-with-retry-after.server.test.tsapp/utils/__tests__/github-mdx-resolution.server.test.tsapp/utils/__tests__/semantic-search-admin.server.test.tsapp/utils/__tests__/semantic-search.server.test.tsapp/utils/__tests__/use-captured-route-error.test.browser.tsxapp/utils/__tests__/x-server.test.tscontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.jsxcontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.test.browser.jsxcontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/custom-hooks.test.browser.jsxcontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/timers.test.browser.jsxcontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/use-imperative-handle.test.browser.jsxcontent/blog/how-to-test-custom-react-hooks/__tests__/use-undo.helper.test.browser.jsxcontent/blog/how-to-test-custom-react-hooks/__tests__/use-undo.real-world.test.browser.jsxcontent/blog/how-to-test-custom-react-hooks/__tests__/use-undo.rthl.test.browser.jsxdocs/agents/project-context.mdmocks/__tests__/cloudflare.test.tsmocks/index.tsmocks/msw-handlers.tspackage.jsontests/msw-server.tstests/setup-backend.tstests/setup-browser.tstests/setup-env.tsvitest.browser.config.tsvitest.config.ts
💤 Files with no reviewable changes (2)
- content/blog/fix-the-not-wrapped-in-act-warning/tests/components.jsx
- app/components/hooks/tests/use-countdown.test.browser.tsx
Comment on lines
+9
to
14
| process.env.R2_BUCKET = 'mock-r2-bucket' | ||
| process.env.R2_ENDPOINT = 'https://mock.r2.cloudflarestorage.com' | ||
| process.env.R2_ACCESS_KEY_ID = 'MOCKR2ACCESSKEYID' | ||
| process.env.R2_SECRET_ACCESS_KEY = 'MOCKR2SECRETACCESSKEY' | ||
| process.env.CALL_KENT_R2_BUCKET = 'mock-call-kent-audio' | ||
|
|
There was a problem hiding this comment.
Isolate env changes to avoid cross-test leakage.
Module-scope env mutation affects the whole worker. Recommend scoping with beforeAll/afterAll and restoring previous values.
🧩 Suggested isolation pattern
-import { describe, expect, test } from 'vitest'
+import { describe, expect, test, beforeAll, afterAll } from 'vitest'
import {
deleteAudioObject,
getAudioBuffer,
getAudioStream,
putCallAudioFromBuffer,
} from '../call-kent-audio-storage.server.ts'
-process.env.R2_BUCKET = 'mock-r2-bucket'
-process.env.R2_ENDPOINT = 'https://mock.r2.cloudflarestorage.com'
-process.env.R2_ACCESS_KEY_ID = 'MOCKR2ACCESSKEYID'
-process.env.R2_SECRET_ACCESS_KEY = 'MOCKR2SECRETACCESSKEY'
-process.env.CALL_KENT_R2_BUCKET = 'mock-call-kent-audio'
+const originalEnv = {
+ R2_BUCKET: process.env.R2_BUCKET,
+ R2_ENDPOINT: process.env.R2_ENDPOINT,
+ R2_ACCESS_KEY_ID: process.env.R2_ACCESS_KEY_ID,
+ R2_SECRET_ACCESS_KEY: process.env.R2_SECRET_ACCESS_KEY,
+ CALL_KENT_R2_BUCKET: process.env.CALL_KENT_R2_BUCKET,
+}
+
+beforeAll(() => {
+ process.env.R2_BUCKET = 'mock-r2-bucket'
+ process.env.R2_ENDPOINT = 'https://mock.r2.cloudflarestorage.com'
+ process.env.R2_ACCESS_KEY_ID = 'MOCKR2ACCESSKEYID'
+ process.env.R2_SECRET_ACCESS_KEY = 'MOCKR2SECRETACCESSKEY'
+ process.env.CALL_KENT_R2_BUCKET = 'mock-call-kent-audio'
+})
+
+afterAll(() => {
+ for (const [key, value] of Object.entries(originalEnv)) {
+ if (typeof value === 'string') process.env[key] = value
+ else delete process.env[key]
+ }
+})📝 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.
Suggested change
| process.env.R2_BUCKET = 'mock-r2-bucket' | |
| process.env.R2_ENDPOINT = 'https://mock.r2.cloudflarestorage.com' | |
| process.env.R2_ACCESS_KEY_ID = 'MOCKR2ACCESSKEYID' | |
| process.env.R2_SECRET_ACCESS_KEY = 'MOCKR2SECRETACCESSKEY' | |
| process.env.CALL_KENT_R2_BUCKET = 'mock-call-kent-audio' | |
| import { describe, expect, test, beforeAll, afterAll } from 'vitest' | |
| import { | |
| deleteAudioObject, | |
| getAudioBuffer, | |
| getAudioStream, | |
| putCallAudioFromBuffer, | |
| } from '../call-kent-audio-storage.server.ts' | |
| const originalEnv = { | |
| R2_BUCKET: process.env.R2_BUCKET, | |
| R2_ENDPOINT: process.env.R2_ENDPOINT, | |
| R2_ACCESS_KEY_ID: process.env.R2_ACCESS_KEY_ID, | |
| R2_SECRET_ACCESS_KEY: process.env.R2_SECRET_ACCESS_KEY, | |
| CALL_KENT_R2_BUCKET: process.env.CALL_KENT_R2_BUCKET, | |
| } | |
| beforeAll(() => { | |
| process.env.R2_BUCKET = 'mock-r2-bucket' | |
| process.env.R2_ENDPOINT = 'https://mock.r2.cloudflarestorage.com' | |
| process.env.R2_ACCESS_KEY_ID = 'MOCKR2ACCESSKEYID' | |
| process.env.R2_SECRET_ACCESS_KEY = 'MOCKR2SECRETACCESSKEY' | |
| process.env.CALL_KENT_R2_BUCKET = 'mock-call-kent-audio' | |
| }) | |
| afterAll(() => { | |
| for (const [key, value] of Object.entries(originalEnv)) { | |
| if (typeof value === 'string') process.env[key] = value | |
| else delete process.env[key] | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/utils/__tests__/call-kent-audio-storage.server.test.ts` around lines 9 -
14, The test mutates module-scope environment variables (process.env.R2_BUCKET,
R2_ENDPOINT, R2_ACCESS_KEY_ID, R2_SECRET_ACCESS_KEY, CALL_KENT_R2_BUCKET) which
can leak across tests; wrap these assignments inside a test suite lifecycle by
saving the original values in beforeAll, set the mock values there, and restore
the originals in afterAll (or beforeEach/afterEach if per-test isolation is
preferred) so that the worker/global environment is returned to its prior state
after tests complete.
Comment on lines
+4
to
10
| // Tests load `.env` first (override=false), so keep this test self-contained in | ||
| // case the local `.env` has placeholder values like `MOCK_R2_ENDPOINT`. | ||
| process.env.R2_BUCKET = 'mock-r2-bucket' | ||
| process.env.R2_ENDPOINT = 'https://mock.r2.cloudflarestorage.com' | ||
| process.env.R2_ACCESS_KEY_ID = 'MOCKR2ACCESSKEYID' | ||
| process.env.R2_SECRET_ACCESS_KEY = 'MOCKR2SECRETACCESSKEY' | ||
|
|
There was a problem hiding this comment.
Avoid module-scope env mutation leaking across tests.
These assignments persist for the entire worker and can affect unrelated tests. Prefer saving/restoring env in beforeAll/afterAll (or similar) so this file stays isolated.
🧩 Suggested isolation pattern
-import { describe, expect, test } from 'vitest'
+import { describe, expect, test, beforeAll, afterAll } from 'vitest'
import { getSemanticSearchAdminStore } from '../semantic-search-admin.server.ts'
-// Tests load `.env` first (override=false), so keep this test self-contained in
-// case the local `.env` has placeholder values like `MOCK_R2_ENDPOINT`.
-process.env.R2_BUCKET = 'mock-r2-bucket'
-process.env.R2_ENDPOINT = 'https://mock.r2.cloudflarestorage.com'
-process.env.R2_ACCESS_KEY_ID = 'MOCKR2ACCESSKEYID'
-process.env.R2_SECRET_ACCESS_KEY = 'MOCKR2SECRETACCESSKEY'
+const originalEnv = {
+ R2_BUCKET: process.env.R2_BUCKET,
+ R2_ENDPOINT: process.env.R2_ENDPOINT,
+ R2_ACCESS_KEY_ID: process.env.R2_ACCESS_KEY_ID,
+ R2_SECRET_ACCESS_KEY: process.env.R2_SECRET_ACCESS_KEY,
+}
+
+beforeAll(() => {
+ // Tests load `.env` first (override=false), so keep this test self-contained in
+ // case the local `.env` has placeholder values like `MOCK_R2_ENDPOINT`.
+ process.env.R2_BUCKET = 'mock-r2-bucket'
+ process.env.R2_ENDPOINT = 'https://mock.r2.cloudflarestorage.com'
+ process.env.R2_ACCESS_KEY_ID = 'MOCKR2ACCESSKEYID'
+ process.env.R2_SECRET_ACCESS_KEY = 'MOCKR2SECRETACCESSKEY'
+})
+
+afterAll(() => {
+ for (const [key, value] of Object.entries(originalEnv)) {
+ if (typeof value === 'string') process.env[key] = value
+ else delete process.env[key]
+ }
+})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/utils/__tests__/semantic-search-admin.server.test.ts` around lines 4 -
10, The module-scope assignments to process.env.R2_BUCKET,
process.env.R2_ENDPOINT, process.env.R2_ACCESS_KEY_ID, and
process.env.R2_SECRET_ACCESS_KEY leak across tests; fix by moving these
assignments into a test lifecycle hook (e.g., beforeAll) where you first capture
the original values into local variables, set the mocked values, and then
restore the originals in afterAll (or use beforeEach/afterEach for per-test
isolation) so the environment is saved and restored and no mutation persists
outside this test file.
Comment on lines
+30
to
+34
| const base64 = | ||
| 'UklGRhoBAABXRUJQVlA4IA4BAABwCgCdASpkAEMAPqVInUq5sy+hqvqpuzAUiWcG+BsvrZQel/iYPLGE154ZiYwzeF8UJRAKZ0oAzLdTpjlp8qBuGwW1ntMTe6iQZbxzyP4gBeg7X7SH7NwyBcUDAAD+8MrTwbAD8OLmsoaL1QDPwEE+GrfqLQPn6xkgFHCB8lyjV3K2RvcQ7pSvgA87LOVuDtMrtkm+tTV0x1RcIe4Uvb6J+yygkV48DSejuyrMWrYgoZyjkf/0/L9+bAZgCam6+oHqjBSWTq5jF7wzBxYwfoGY7OdYZOdeGb4euuuLaCzDHz/QRbDCaIsJWJW3Jo4bkbz44AI/8UfFTGX4tMTRcKLXTDIviU+/u7UnlVaDQAA=' | ||
| const buffer = Buffer.from(base64) | ||
| return HttpResponse.json(buffer) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full file to understand context
cat -n mocks/msw-handlers.ts | head -120Repository: kentcdodds/kentcdodds.com
Length of output: 4243
🏁 Script executed:
# Search for any usage of the cloudinary blur endpoint
rg -n "w_100.*q_auto.*f_webp.*e_blur" --type=ts -C 3Repository: kentcdodds/kentcdodds.com
Length of output: 526
🏁 Script executed:
# Check imports to understand HttpResponse API being used
rg -n "import.*HttpResponse" mocks/ tests/ -A 2Repository: kentcdodds/kentcdodds.com
Length of output: 323
🏁 Script executed:
# Search for references to the cloudinary blur endpoint or the photoId parameter
rg -n "photoId|w_100.*blur" --type=ts --type=tsx -C 2Repository: kentcdodds/kentcdodds.com
Length of output: 96
🏁 Script executed:
# Search for any code that might be consuming this specific cloudinary endpoint
rg -n "res\.cloudinary\.com.*blur|blur:1000" --type=ts --type=tsxRepository: kentcdodds/kentcdodds.com
Length of output: 96
🏁 Script executed:
# Check if there are any tests that verify this handler behavior
rg -n "unsplash" tests/ mocks/ --type=ts -C 3Repository: kentcdodds/kentcdodds.com
Length of output: 526
Image mock has two bugs: wrong content-type and incorrect base64 decoding.
Line 32: Buffer.from(base64) is missing the 'base64' encoding parameter, treating the base64 string as UTF-8 bytes instead of decoding it.
Line 33: HttpResponse.json(buffer) returns Content-Type: application/json instead of the expected image/webp — use new HttpResponse() with explicit headers, following the pattern in the fallback handler above (lines 38–40).
Proposed fix
- const buffer = Buffer.from(base64)
- return HttpResponse.json(buffer)
+ const buffer = Buffer.from(base64, 'base64')
+ return new HttpResponse(buffer, {
+ status: 200,
+ headers: { 'Content-Type': 'image/webp' },
+ })📝 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.
Suggested change
| const base64 = | |
| 'UklGRhoBAABXRUJQVlA4IA4BAABwCgCdASpkAEMAPqVInUq5sy+hqvqpuzAUiWcG+BsvrZQel/iYPLGE154ZiYwzeF8UJRAKZ0oAzLdTpjlp8qBuGwW1ntMTe6iQZbxzyP4gBeg7X7SH7NwyBcUDAAD+8MrTwbAD8OLmsoaL1QDPwEE+GrfqLQPn6xkgFHCB8lyjV3K2RvcQ7pSvgA87LOVuDtMrtkm+tTV0x1RcIe4Uvb6J+yygkV48DSejuyrMWrYgoZyjkf/0/L9+bAZgCam6+oHqjBSWTq5jF7wzBxYwfoGY7OdYZOdeGb4euuuLaCzDHz/QRbDCaIsJWJW3Jo4bkbz44AI/8UfFTGX4tMTRcKLXTDIviU+/u7UnlVaDQAA=' | |
| const buffer = Buffer.from(base64) | |
| return HttpResponse.json(buffer) | |
| }, | |
| const base64 = | |
| 'UklGRhoBAABXRUJQVlA4IA4BAABwCgCdASpkAEMAPqVInUq5sy+hqvqpuzAUiWcG+BsvrZQel/iYPLGE154ZiYwzeF8UJRAKZ0oAzLdTpjlp8qBuGwW1ntMTe6iQZbxzyP4gBeg7X7SH7NwyBcUDAAD+8MrTwbAD8OLmsoaL1QDPwEE+GrfqLQPn6xkgFHCB8lyjV3K2RvcQ7pSvgA87LOVuDtMrtkm+tTV0x1RcIe4Uvb6J+yygkV48DSejuyrMWrYgoZyjkf/0/L9+bAZgCam6+oHqjBSWTq5jF7wzBxYwfoGY7OdYZOdeGb4euuuLaCzDHz/QRbDCaIsJW3Jo4bkbz44AI/8UfFTGX4tMTRcKLXTDIviU+/u7UnlVaDQAA=' | |
| const buffer = Buffer.from(base64, 'base64') | |
| return new HttpResponse(buffer, { | |
| status: 200, | |
| headers: { 'Content-Type': 'image/webp' }, | |
| }) | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mocks/msw-handlers.ts` around lines 30 - 34, The image mock incorrectly
decodes the base64 and returns JSON; change the Buffer construction to
Buffer.from(base64, 'base64') to decode properly, and replace the
HttpResponse.json(buffer) call with creating a raw HttpResponse using the
HttpResponse constructor (pass the buffer as the body and set headers including
'Content-Type': 'image/webp' and an appropriate status), mirroring how the
fallback handler builds responses.
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 (1)
app/utils/__tests__/use-captured-route-error.test.browser.tsx (1)
73-87:⚠️ Potential issue | 🟡 MinorSynchronous negative assertion may be a false positive given the async effect pattern.
The other two tests require
await expect.poll(...)to observemockCaptureExceptionbeing called, which indicates the call happens outside theactboundary thatawait render()flushes. If the 404 path also went through the same async scheduling (e.g. a microtask afteruseEffect), line 86 could pass vacuously — before any deferred call has had a chance to execute — masking a regression wherecaptureExceptionis incorrectly called for non-5xx responses.The library uses
actonly to flushuseEffectduring initial rendering and unmounting, but other use cases are handled by CDP andexpect.elementwhich both have built-in retry-ability. Since the positive tests needexpect.pollto detect the call, the call path is clearly deferred beyond the initial flush.Consider anchoring the negative assertion to a known stable state first:
🛡️ Proposed fix to stabilize the negative assertion
- await render(<TestComponent />) + const screen = await render(<TestComponent />) + // Wait for the component to be fully settled before asserting the negative + await expect.element(screen.getByText('hook called')).toBeVisible() expect(mockCaptureException).not.toHaveBeenCalled()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/use-captured-route-error.test.browser.tsx` around lines 73 - 87, The test "does not capture non-5xx route error responses" currently asserts mockCaptureException synchronously and may miss deferred calls; update the assertion to observe the async scheduling window (same pattern as the positive tests) by polling or awaiting a short timeout after render of TestComponent and then asserting mockCaptureException was not called; specifically, keep the setup using mockUseRouteError and mockIsRouteErrorResponse but replace the final synchronous expect(mockCaptureException).not.toHaveBeenCalled() with an await expect.poll or equivalent wait that verifies mockCaptureException remains uninvoked for the duration, ensuring any microtask/async effects that would call captureException are observed.
🧹 Nitpick comments (4)
content/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.test.browser.jsx (2)
92-95: Stale ESLint disable — no directconsole.*calls exist in this file.
vi.spyOn(console, 'error')does not triggerno-console; the comment was likely carried over from the originalcomponents.jsxand can be removed.🧹 Proposed cleanup
-/* -eslint - no-console: "off" -*/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@content/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.test.browser.jsx` around lines 92 - 95, Remove the stale ESLint disable block at the top (the /* eslint no-console: "off" */ comment) since this test file does not contain direct console.* calls; keep the vi.spyOn(console, 'error') usage as-is (it doesn't trigger no-console), and run the tests/linter to confirm no violations remain for the test file (look for vi.spyOn(console, 'error') in components.test.browser.jsx to locate the relevant test).
60-63: Prefer scopedscreenquery over rawdocument.body.textContent.Line 62 reaches outside the scoped
screenreturned byrender. Usingscreen.getByText(or aqueryByvariant) keeps the assertion consistent with the rest of the test and the browser-mode idiom.♻️ Proposed refactor
- await expect - .poll(() => document.body.textContent?.toLowerCase().includes('saving')) - .toBe(false) + await expect + .element(screen.getByText(/saving/i)) + .not.toBeVisible()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@content/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.test.browser.jsx` around lines 60 - 63, Replace the out-of-scope document lookup with a scoped Testing Library query: change the poll callback that uses document.body.textContent to use screen (returned by render) and a queryByText/getByText regex for "saving" (e.g., screen.queryByText(/saving/i)) and assert absence (query returns null) or use a boolean (!!screen.queryByText(/saving/i)) and expect(false); update the assertion in the test surrounding render()/screen usage so the check uses screen.queryByText(/saving/i) instead of document.body.textContent.app/components/calls/__tests__/submit-recording-form.test.browser.tsx (1)
296-340: Avoid accidental network calls when validation fails.Consider stubbing
fetchhere and asserting it’s not called. This makes the “no upload on client validation failure” intent explicit and prevents unexpected network activity if a regression slips in.♻️ Suggested tweak
try { + const fetchMock = vi.fn() + vi.stubGlobal('fetch', fetchMock as unknown as typeof fetch) const screen = await render( <RecordingForm audio={new Blob(['audio'])} intent="create-call" />, ) @@ await screen.getByRole('button', { name: 'Submit Recording' }).click() await expect.element(screen.getByText('Title is required')).toBeVisible() + expect(fetchMock).not.toHaveBeenCalled() const describedBy = titleInput?.getAttribute('aria-describedby') ?? '' expect(describedBy).toContain(`${titleId}-error`) expect(describedBy).toContain(`${titleId}-countdown`) } finally { createObjectURL.mockRestore() revokeObjectURL.mockRestore() + vi.unstubAllGlobals() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/calls/__tests__/submit-recording-form.test.browser.tsx` around lines 296 - 340, This test should explicitly stub the network to ensure no upload happens on client-side validation failure: before interacting with the form, stub/spy on global.fetch (or window.fetch) and ensure the stub is in place while rendering RecordingForm and during the user actions, then after clicking the "Submit Recording" button assert that fetch was not called; finally restore/cleanup the stub. Target the test that renders <RecordingForm audio={new Blob(['audio'])} intent="create-call" /> and add the fetch spy setup/teardown and the assertion that fetch was not invoked.content/blog/fix-the-not-wrapped-in-act-warning/__tests__/custom-hooks.test.browser.jsx (1)
12-22: Replace the IIFE with a plainasynctest function.The synchronous wrapper returning an async IIFE is functionally equivalent to just making the test
async, but adds an extra level of indentation and obscures intent. The canonicalvitest-browser-reactusage in its official docs uses a plainasync () =>test callback.♻️ Proposed simplification
-test('increment and decrement updates the count', () => { - return (async () => { - const { result, act } = await renderHook(() => useCount()) - - expect(result.current.count).toBe(0) - await act(() => result.current.increment()) - expect(result.current.count).toBe(1) - await act(() => result.current.decrement()) - expect(result.current.count).toBe(0) - })() +test('increment and decrement updates the count', async () => { + const { result, act } = await renderHook(() => useCount()) + + expect(result.current.count).toBe(0) + await act(() => result.current.increment()) + expect(result.current.count).toBe(1) + await act(() => result.current.decrement()) + expect(result.current.count).toBe(0) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@content/blog/fix-the-not-wrapped-in-act-warning/__tests__/custom-hooks.test.browser.jsx` around lines 12 - 22, The test "increment and decrement updates the count" currently returns an async IIFE; change the test callback to be an async function instead of returning an IIFE so it's more idiomatic and clearer. Update the test definition to `test('increment and decrement updates the count', async () => { ... })`, keep using renderHook(() => useCount()) and act(...) around result.current.increment()/decrement(), and remove the extra wrapper return/(() => { ... })() so the function itself is async.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@content/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.test.browser.jsx`:
- Around line 1-2: The imports are in the wrong order; move the vitest imports
before the vitest-browser-react import so test utilities come from 'vitest'
(test, expect, vi) first and then import render from 'vitest-browser-react';
update the import statements referencing render, test, expect, and vi
accordingly to ensure linting/import-order rules are satisfied.
In
`@content/blog/fix-the-not-wrapped-in-act-warning/__tests__/timers.test.browser.jsx`:
- Around line 63-70: The test uses fake timers with a promise-returning
checkStatus but calls the synchronous vi.advanceTimersByTime; replace that with
the async helper and await it (e.g., await vi.advanceTimersByTimeAsync(10000))
so microtask queues are flushed before asserting OrderStatus output; update the
test to await the async advance and keep references to OrderStatus and
checkStatus to locate the change.
In
`@content/blog/how-to-test-custom-react-hooks/__tests__/use-undo.helper.test.browser.jsx`:
- Around line 90-93: Replace the malformed block ESLint comment with a valid
one-line directive: remove the current multi-line comment and add a single-line
file-level ESLint directive such as /* eslint-disable max-statements */ (or /*
eslint max-statements: "off" */) at the top of the test file to properly disable
the rule; target the existing comment region where the broken block lives (the
test file's ESLint directive).
In
`@content/blog/how-to-test-custom-react-hooks/__tests__/use-undo.rthl.test.browser.jsx`:
- Around line 90-93: Replace the malformed block comment that attempts to
disable the eslint rule with a valid ESLint directive: remove the multi-line
block starting with "eslint" and instead add a one-line file-level directive
such as /* eslint max-statements: "off" */ (or use // eslint-disable-next-line
max-statements for a local line) in the test file use-undo.rthl.test.browser.jsx
so the max-statements rule is actually disabled for the intended scope.
---
Outside diff comments:
In `@app/utils/__tests__/use-captured-route-error.test.browser.tsx`:
- Around line 73-87: The test "does not capture non-5xx route error responses"
currently asserts mockCaptureException synchronously and may miss deferred
calls; update the assertion to observe the async scheduling window (same pattern
as the positive tests) by polling or awaiting a short timeout after render of
TestComponent and then asserting mockCaptureException was not called;
specifically, keep the setup using mockUseRouteError and
mockIsRouteErrorResponse but replace the final synchronous
expect(mockCaptureException).not.toHaveBeenCalled() with an await expect.poll or
equivalent wait that verifies mockCaptureException remains uninvoked for the
duration, ensuring any microtask/async effects that would call captureException
are observed.
---
Nitpick comments:
In `@app/components/calls/__tests__/submit-recording-form.test.browser.tsx`:
- Around line 296-340: This test should explicitly stub the network to ensure no
upload happens on client-side validation failure: before interacting with the
form, stub/spy on global.fetch (or window.fetch) and ensure the stub is in place
while rendering RecordingForm and during the user actions, then after clicking
the "Submit Recording" button assert that fetch was not called; finally
restore/cleanup the stub. Target the test that renders <RecordingForm audio={new
Blob(['audio'])} intent="create-call" /> and add the fetch spy setup/teardown
and the assertion that fetch was not invoked.
In
`@content/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.test.browser.jsx`:
- Around line 92-95: Remove the stale ESLint disable block at the top (the /*
eslint no-console: "off" */ comment) since this test file does not contain
direct console.* calls; keep the vi.spyOn(console, 'error') usage as-is (it
doesn't trigger no-console), and run the tests/linter to confirm no violations
remain for the test file (look for vi.spyOn(console, 'error') in
components.test.browser.jsx to locate the relevant test).
- Around line 60-63: Replace the out-of-scope document lookup with a scoped
Testing Library query: change the poll callback that uses
document.body.textContent to use screen (returned by render) and a
queryByText/getByText regex for "saving" (e.g., screen.queryByText(/saving/i))
and assert absence (query returns null) or use a boolean
(!!screen.queryByText(/saving/i)) and expect(false); update the assertion in the
test surrounding render()/screen usage so the check uses
screen.queryByText(/saving/i) instead of document.body.textContent.
In
`@content/blog/fix-the-not-wrapped-in-act-warning/__tests__/custom-hooks.test.browser.jsx`:
- Around line 12-22: The test "increment and decrement updates the count"
currently returns an async IIFE; change the test callback to be an async
function instead of returning an IIFE so it's more idiomatic and clearer. Update
the test definition to `test('increment and decrement updates the count', async
() => { ... })`, keep using renderHook(() => useCount()) and act(...) around
result.current.increment()/decrement(), and remove the extra wrapper return/(()
=> { ... })() so the function itself is async.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
app/components/calls/__tests__/submit-recording-form.test.browser.tsxapp/components/hooks/__tests__/use-countdown.test.browser.tsxapp/utils/__tests__/use-captured-route-error.test.browser.tsxcontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.test.browser.jsxcontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/custom-hooks.test.browser.jsxcontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/timers.test.browser.jsxcontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/use-imperative-handle.test.browser.jsxcontent/blog/how-to-test-custom-react-hooks/__tests__/use-undo.helper.test.browser.jsxcontent/blog/how-to-test-custom-react-hooks/__tests__/use-undo.real-world.test.browser.jsxcontent/blog/how-to-test-custom-react-hooks/__tests__/use-undo.rthl.test.browser.jsxpackage.jsontests/setup-browser.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/setup-browser.ts
- package.json
content/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.test.browser.jsx
Outdated
Show resolved
Hide resolved
content/blog/fix-the-not-wrapped-in-act-warning/__tests__/timers.test.browser.jsx
Show resolved
Hide resolved
content/blog/how-to-test-custom-react-hooks/__tests__/use-undo.helper.test.browser.jsx
Outdated
Show resolved
Hide resolved
content/blog/how-to-test-custom-react-hooks/__tests__/use-undo.rthl.test.browser.jsx
Outdated
Show resolved
Hide resolved
cursor
bot
force-pushed
the
cursor/test-categories-and-browser-mode-f948
branch
from
ad4d0bc to
c5f372c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
app/utils/__tests__/x-server.test.ts (1)
2-3:⚠️ Potential issue | 🟡 MinorFix import ordering to satisfy ESLint.
Reorder the imports so the
vitestimport comes before the local alias import.🧩 Proposed fix
-import { mswServer } from '#tests/msw-server.ts' -import { beforeEach, describe, expect, test, vi } from 'vitest' +import { beforeEach, describe, expect, test, vi } from 'vitest' +import { mswServer } from '#tests/msw-server.ts'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/x-server.test.ts` around lines 2 - 3, ESLint requires the vitest import to come before the local alias import; swap the import order so the line importing named helpers from 'vitest' (beforeEach, describe, expect, test, vi) appears above the import of mswServer (mswServer from '#tests/msw-server.ts'), preserving the same imported symbols and no other changes.app/utils/__tests__/call-kent-audio-storage.server.test.ts (1)
1-13:⚠️ Potential issue | 🟠 MajorIsolate env mutations to prevent cross-test leakage.
Module-scope
process.envchanges can leak into other tests; scope them inbeforeAll/afterAlland restore originals.✅ Suggested isolation pattern
-import { describe, expect, test } from 'vitest' +import { afterAll, beforeAll, describe, expect, test } from 'vitest' import { deleteAudioObject, getAudioBuffer, getAudioStream, putCallAudioFromBuffer, } from '../call-kent-audio-storage.server.ts' -process.env.R2_BUCKET = 'mock-r2-bucket' -process.env.R2_ENDPOINT = 'https://mock.r2.cloudflarestorage.com' -process.env.R2_ACCESS_KEY_ID = 'MOCKR2ACCESSKEYID' -process.env.R2_SECRET_ACCESS_KEY = 'MOCKR2SECRETACCESSKEY' -process.env.CALL_KENT_R2_BUCKET = 'mock-call-kent-audio' +const originalEnv = { + R2_BUCKET: process.env.R2_BUCKET, + R2_ENDPOINT: process.env.R2_ENDPOINT, + R2_ACCESS_KEY_ID: process.env.R2_ACCESS_KEY_ID, + R2_SECRET_ACCESS_KEY: process.env.R2_SECRET_ACCESS_KEY, + CALL_KENT_R2_BUCKET: process.env.CALL_KENT_R2_BUCKET, +} + +beforeAll(() => { + process.env.R2_BUCKET = 'mock-r2-bucket' + process.env.R2_ENDPOINT = 'https://mock.r2.cloudflarestorage.com' + process.env.R2_ACCESS_KEY_ID = 'MOCKR2ACCESSKEYID' + process.env.R2_SECRET_ACCESS_KEY = 'MOCKR2SECRETACCESSKEY' + process.env.CALL_KENT_R2_BUCKET = 'mock-call-kent-audio' +}) + +afterAll(() => { + for (const [key, value] of Object.entries(originalEnv)) { + if (typeof value === 'string') process.env[key] = value + else delete process.env[key] + } +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/call-kent-audio-storage.server.test.ts` around lines 1 - 13, The module-level process.env mutations in app/utils/__tests__/call-kent-audio-storage.server.test.ts should be moved into Vitest lifecycle hooks to avoid leaking into other tests: capture the original values of R2_BUCKET, R2_ENDPOINT, R2_ACCESS_KEY_ID, R2_SECRET_ACCESS_KEY, and CALL_KENT_R2_BUCKET in a variable, set the test-specific values in a beforeAll (or beforeEach) block, and restore the originals in an afterAll (or afterEach) block so the environment is reset; update the test file to use beforeAll/afterAll around the existing imports/use of deleteAudioObject/getAudioBuffer/getAudioStream/putCallAudioFromBuffer to ensure isolation.mocks/msw-handlers.ts (1)
30-34:⚠️ Potential issue | 🟡 MinorFix base64 decode + content-type for the Cloudinary image mock.
Buffer.from(base64)treats the string as UTF-8 bytes, andHttpResponse.json(buffer)returnsapplication/json, notimage/webp. Decode base64 and return a raw response with proper headers.🛠️ Proposed fix
- const buffer = Buffer.from(base64) - return HttpResponse.json(buffer) + const buffer = Buffer.from(base64, 'base64') + return new HttpResponse(buffer, { + status: 200, + headers: { 'Content-Type': 'image/webp' }, + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mocks/msw-handlers.ts` around lines 30 - 34, The mock currently does Buffer.from(base64) (which treats the string as UTF-8) and returns HttpResponse.json(buffer) (which sets application/json); change the decode to Buffer.from(base64, 'base64') using the existing base64 variable and return a raw binary response with the correct content-type header (e.g., replace the HttpResponse.json(buffer) call with returning a Response/new raw HttpResponse containing the decoded bytes and headers: { 'Content-Type': 'image/webp' }).app/utils/__tests__/fetch-json-with-retry-after.server.test.ts (1)
2-3: Import ordering (ESLint warning):vitestshould precede#tests/msw-server.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/fetch-json-with-retry-after.server.test.ts` around lines 2 - 3, Reorder the import statements so the named import from 'vitest' (beforeEach, expect, test, vi) comes before the aliased internal import mswServer from '#tests/msw-server.ts' to satisfy ESLint import ordering; update the top of the test file to import from 'vitest' first and then import mswServer without changing the imported symbols or test logic.
🧹 Nitpick comments (6)
docs/agents/project-context.md (1)
22-24: Document the auto-wired MSW backend test server in "Non-obvious caveats".The PR centralizes MSW handlers and adds
tests/setup-backend.tswhich automatically callslisten/reset/closeand resets Cloudflare mocks for every backend test. This means individual backend tests no longer need per-filesetupServerboilerplate — a non-obvious behavior that could trip up a future agent writing a new test. Per the project documentation guidelines, new sharp edges should be captured in the "Non-obvious caveats" section.📝 Suggested addition to the "Non-obvious caveats" section
- All external APIs are mocked via MSW when `MOCKS=true` (the default in dev). No real API keys are needed for local development; `.env.example` values are sufficient. +- Backend tests have MSW auto-wired via `tests/setup-backend.ts`: the shared + `mswServer` (from `tests/msw-server.ts`) is started/reset/closed automatically + around every test. Individual backend tests do **not** need to call + `setupServer()` themselves — import `mswServer` directly if you need to + override handlers within a specific test.Based on learnings: "If discovering a new sharp edge, workflow, or non-obvious project behavior, update the relevant docs in docs/agents/ so future agent runs are faster and more correct."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/agents/project-context.md` around lines 22 - 24, Add a note to the "Non-obvious caveats" section of docs/agents/project-context.md documenting that tests/setup-backend.ts auto-wires the MSW backend test server by calling listen/reset/close and resetting Cloudflare mocks for every backend test, so individual backend test files no longer need per-file setupServer boilerplate; mention the file name tests/setup-backend.ts and the behavior (auto listen/reset/close, Cloudflare mock reset) and a short example reminder to remove or not add per-file setupServer to avoid duplicate servers or unexpected state.app/components/calls/__tests__/episode-artwork-preview.test.browser.tsx (1)
81-81: Prefer the retry-ableexpect.elementassertion over the synchronous one-shotexpect().
expect(img.element().getAttribute('src')).not.toBe(initialSrc)snapshots the attribute once and asserts immediately. If React's re-render lags even slightly behind the timer flush, this can produce a false failure. The neighbouring assertion on line 76 already usesawait expect.element(img).toHaveAttribute(...), so consistency and resilience both point to the same fix.♻️ Proposed refactor
- expect(img.element().getAttribute('src')).not.toBe(initialSrc) + await expect.element(img).not.toHaveAttribute('src', initialSrc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/calls/__tests__/episode-artwork-preview.test.browser.tsx` at line 81, Replace the one-shot assertion that snapshots src with a retryable element assertion: instead of expect(img.element().getAttribute('src')).not.toBe(initialSrc), use the async assertion await expect.element(img).not.toHaveAttribute('src', initialSrc) (or await expect.element(img).toHaveAttribute('src', newValue) if applicable) so the check for img's src is retried until it changes; reference the img element, expect.element and toHaveAttribute/not.toHaveAttribute in the fix.app/components/calls/recording-form.tsx (2)
383-386:getStringFormValueis duplicated fromsave.tsx.Both files define the same two-line helper. If validation logic ever diverges (even subtly), the copies can drift. Extracting it to a shared utility (e.g.,
#app/utils/form.ts) would keep both consumers in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/calls/recording-form.tsx` around lines 383 - 386, The helper getStringFormValue is duplicated in recording-form.tsx and save.tsx; extract this function into a shared utility (e.g., create `#app/utils/form.ts` exporting getStringFormValue), update both recording-form.tsx and save.tsx to import getStringFormValue from that module, and remove the local duplicates so both components use the single exported function (ensure the exported function signature and return behavior match the current implementation).
42-56:isRecordingFormDataEqualwill silently miss any newRecordingFormDatafield.The function manually lists every field for comparison. If a new field is added to
RecordingFormData, theuseEffectguard on line 102 will still see the old and new data as equal, preventing the form state from resetting when it should.A JSON-stringify round-trip or a generic deep-equality utility would make this future-proof:
💡 Simpler alternative using JSON comparison
-function isRecordingFormDataEqual( - first?: RecordingFormData, - second?: RecordingFormData, -) { - if (first === second) return true - if (!first || !second) return false - return ( - first.fields.title === second.fields.title && - first.fields.notes === second.fields.notes && - first.errors.generalError === second.errors.generalError && - first.errors.audio === second.errors.audio && - first.errors.title === second.errors.title && - first.errors.notes === second.errors.notes - ) -} +function isRecordingFormDataEqual( + first?: RecordingFormData, + second?: RecordingFormData, +) { + if (first === second) return true + if (!first || !second) return false + return JSON.stringify(first) === JSON.stringify(second) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/calls/recording-form.tsx` around lines 42 - 56, The manual field-by-field comparison in isRecordingFormDataEqual is brittle and will miss new properties on RecordingFormData; replace its body with a robust deep equality check used by the useEffect guard (e.g., import and call a deep-equal util like lodash/isEqual or compare JSON.stringify(first) === JSON.stringify(second)) so the function compares the whole object graph of first and second (including nested fields and errors) instead of enumerating specific properties.package.json (2)
50-50:test:watchsilently skips browser tests.
npm run test:watchdelegates only totest:backend:watch, so changes to browser-test files produce no watch feedback. Consider either running both watchers concurrently or renaming this script totest:backend:watchto make the scope explicit.💡 Parallel-watch alternative
-"test:watch": "npm run test:backend:watch", +"test:watch": "concurrently \"npm run test:backend:watch\" \"npm run test:browser:watch\"",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 50, The package.json script "test:watch" currently only runs "test:backend:watch" and therefore silently skips browser tests; either update "test:watch" to run both watchers concurrently (e.g., spawn "test:backend:watch" and the browser watcher together using a parallel runner like "npm-run-all" or "concurrently") or rename "test:watch" to "test:backend:watch" to make the scope explicit; locate the "test:watch" entry in package.json and replace its value accordingly (referencing the "test:watch" and "test:backend:watch" script names when making the change).
165-165:vitestis independencies; browser plugins are indevDependencies— inconsistency.
vitest(line 165) is listed underdependencies, while the newly added@vitest/browser-playwrightandvitest-browser-reactland only indevDependencies. Ifvitestbeing independenciesis intentional (e.g., for a specific CI pattern), the browser plugins should follow the same classification; otherwise movevitesttodevDependencies.Also applies to: 197-197, 218-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 165, The package.json lists "vitest" under dependencies while "@vitest/browser-playwright" and "vitest-browser-react" are in devDependencies; make these classifications consistent by moving "vitest" into devDependencies (or alternatively move the browser plugins into dependencies if there is an intentional runtime/CI need). Update the single "vitest" entries (all occurrences) so they appear under the devDependencies section and remove them from dependencies, and ensure the related plugin entries "@vitest/browser-playwright" and "vitest-browser-react" remain in devDependencies if you choose the dev-only approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/calls/__tests__/episode-artwork-preview.test.browser.tsx`:
- Line 26: Replace the non-existent Vitest Browser Mode assertion toBeHidden()
with the supported negated visibility assertion: change await
expect.element(tooltip).toBeHidden() to await
expect.element(tooltip).not.toBeVisible() (apply the same replacement for the
second occurrence in the test), locating the calls by the tooltip variable in
episode-artwork-preview.test.browser.tsx.
In `@docs/agents/project-context.md`:
- Line 22: Add a note to the "Non-obvious caveats" section in
docs/agents/project-context.md explaining that backend tests auto-wire an MSW
server via tests/setup-backend.ts: it calls setupServer and automatically starts
(listen), resets handlers between tests, and resets the Cloudflare mock so
individual backend test files do not need to call setupServer or teardown
themselves; mention the file name tests/setup-backend.ts and the behaviors
(auto-listen, automatic reset, Cloudflare mock reset) so future contributors
know no per-file MSW boilerplate is required.
In `@vitest.browser.config.ts`:
- Around line 6-8: ESLint failing due to import order: move the import of
envOnlyMacros from 'vite-env-only' above the import of tsconfigPaths from
'vite-tsconfig-paths' so the file imports 'vite-env-only' before
'vite-tsconfig-paths'; keep the other imports (configDefaults, defineConfig)
unchanged and ensure the identifiers envOnlyMacros and tsconfigPaths remain
as-is.
---
Duplicate comments:
In `@app/utils/__tests__/call-kent-audio-storage.server.test.ts`:
- Around line 1-13: The module-level process.env mutations in
app/utils/__tests__/call-kent-audio-storage.server.test.ts should be moved into
Vitest lifecycle hooks to avoid leaking into other tests: capture the original
values of R2_BUCKET, R2_ENDPOINT, R2_ACCESS_KEY_ID, R2_SECRET_ACCESS_KEY, and
CALL_KENT_R2_BUCKET in a variable, set the test-specific values in a beforeAll
(or beforeEach) block, and restore the originals in an afterAll (or afterEach)
block so the environment is reset; update the test file to use
beforeAll/afterAll around the existing imports/use of
deleteAudioObject/getAudioBuffer/getAudioStream/putCallAudioFromBuffer to ensure
isolation.
In `@app/utils/__tests__/fetch-json-with-retry-after.server.test.ts`:
- Around line 2-3: Reorder the import statements so the named import from
'vitest' (beforeEach, expect, test, vi) comes before the aliased internal import
mswServer from '#tests/msw-server.ts' to satisfy ESLint import ordering; update
the top of the test file to import from 'vitest' first and then import mswServer
without changing the imported symbols or test logic.
In `@app/utils/__tests__/x-server.test.ts`:
- Around line 2-3: ESLint requires the vitest import to come before the local
alias import; swap the import order so the line importing named helpers from
'vitest' (beforeEach, describe, expect, test, vi) appears above the import of
mswServer (mswServer from '#tests/msw-server.ts'), preserving the same imported
symbols and no other changes.
In `@mocks/msw-handlers.ts`:
- Around line 30-34: The mock currently does Buffer.from(base64) (which treats
the string as UTF-8) and returns HttpResponse.json(buffer) (which sets
application/json); change the decode to Buffer.from(base64, 'base64') using the
existing base64 variable and return a raw binary response with the correct
content-type header (e.g., replace the HttpResponse.json(buffer) call with
returning a Response/new raw HttpResponse containing the decoded bytes and
headers: { 'Content-Type': 'image/webp' }).
---
Nitpick comments:
In `@app/components/calls/__tests__/episode-artwork-preview.test.browser.tsx`:
- Line 81: Replace the one-shot assertion that snapshots src with a retryable
element assertion: instead of
expect(img.element().getAttribute('src')).not.toBe(initialSrc), use the async
assertion await expect.element(img).not.toHaveAttribute('src', initialSrc) (or
await expect.element(img).toHaveAttribute('src', newValue) if applicable) so the
check for img's src is retried until it changes; reference the img element,
expect.element and toHaveAttribute/not.toHaveAttribute in the fix.
In `@app/components/calls/recording-form.tsx`:
- Around line 383-386: The helper getStringFormValue is duplicated in
recording-form.tsx and save.tsx; extract this function into a shared utility
(e.g., create `#app/utils/form.ts` exporting getStringFormValue), update both
recording-form.tsx and save.tsx to import getStringFormValue from that module,
and remove the local duplicates so both components use the single exported
function (ensure the exported function signature and return behavior match the
current implementation).
- Around line 42-56: The manual field-by-field comparison in
isRecordingFormDataEqual is brittle and will miss new properties on
RecordingFormData; replace its body with a robust deep equality check used by
the useEffect guard (e.g., import and call a deep-equal util like lodash/isEqual
or compare JSON.stringify(first) === JSON.stringify(second)) so the function
compares the whole object graph of first and second (including nested fields and
errors) instead of enumerating specific properties.
In `@docs/agents/project-context.md`:
- Around line 22-24: Add a note to the "Non-obvious caveats" section of
docs/agents/project-context.md documenting that tests/setup-backend.ts
auto-wires the MSW backend test server by calling listen/reset/close and
resetting Cloudflare mocks for every backend test, so individual backend test
files no longer need per-file setupServer boilerplate; mention the file name
tests/setup-backend.ts and the behavior (auto listen/reset/close, Cloudflare
mock reset) and a short example reminder to remove or not add per-file
setupServer to avoid duplicate servers or unexpected state.
In `@package.json`:
- Line 50: The package.json script "test:watch" currently only runs
"test:backend:watch" and therefore silently skips browser tests; either update
"test:watch" to run both watchers concurrently (e.g., spawn "test:backend:watch"
and the browser watcher together using a parallel runner like "npm-run-all" or
"concurrently") or rename "test:watch" to "test:backend:watch" to make the scope
explicit; locate the "test:watch" entry in package.json and replace its value
accordingly (referencing the "test:watch" and "test:backend:watch" script names
when making the change).
- Line 165: The package.json lists "vitest" under dependencies while
"@vitest/browser-playwright" and "vitest-browser-react" are in devDependencies;
make these classifications consistent by moving "vitest" into devDependencies
(or alternatively move the browser plugins into dependencies if there is an
intentional runtime/CI need). Update the single "vitest" entries (all
occurrences) so they appear under the devDependencies section and remove them
from dependencies, and ensure the related plugin entries
"@vitest/browser-playwright" and "vitest-browser-react" remain in
devDependencies if you choose the dev-only approach.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
app/components/calls/__tests__/episode-artwork-preview.test.browser.tsxapp/components/calls/__tests__/submit-recording-form.test.browser.tsxapp/components/calls/recording-form.tsxapp/components/hooks/__tests__/use-countdown.test.browser.tsxapp/routes/calls_.admin/$callId.tsxapp/routes/calls_.record/new.tsxapp/routes/resources/calls/save.tsxapp/utils/__tests__/call-kent-audio-storage.server.test.tsapp/utils/__tests__/fetch-json-with-retry-after.server.test.tsapp/utils/__tests__/github-mdx-resolution.server.test.tsapp/utils/__tests__/semantic-search-admin.server.test.tsapp/utils/__tests__/semantic-search.server.test.tsapp/utils/__tests__/use-captured-route-error.test.browser.tsxapp/utils/__tests__/x-server.test.tscontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.jsxcontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/components.test.browser.jsxcontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/custom-hooks.test.browser.jsxcontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/timers.test.browser.jsxcontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/use-imperative-handle.jsxcontent/blog/fix-the-not-wrapped-in-act-warning/__tests__/use-imperative-handle.test.browser.jsxcontent/blog/how-to-test-custom-react-hooks/__tests__/use-undo.helper.jsxcontent/blog/how-to-test-custom-react-hooks/__tests__/use-undo.helper.test.browser.jsxcontent/blog/how-to-test-custom-react-hooks/__tests__/use-undo.real-world.jsxcontent/blog/how-to-test-custom-react-hooks/__tests__/use-undo.real-world.test.browser.jsxcontent/blog/how-to-test-custom-react-hooks/__tests__/use-undo.rthl.test.browser.jsxdocs/agents/project-context.mdmocks/__tests__/cloudflare.test.tsmocks/index.tsmocks/msw-handlers.tspackage.jsontests/msw-server.tstests/setup-backend.tstests/setup-browser.tstests/setup-env.tsvitest.browser.config.tsvitest.config.ts
💤 Files with no reviewable changes (4)
- content/blog/how-to-test-custom-react-hooks/tests/use-undo.helper.jsx
- content/blog/how-to-test-custom-react-hooks/tests/use-undo.real-world.jsx
- content/blog/fix-the-not-wrapped-in-act-warning/tests/components.jsx
- content/blog/fix-the-not-wrapped-in-act-warning/tests/use-imperative-handle.jsx
🚧 Files skipped from review as they are similar to previous changes (12)
- app/utils/tests/semantic-search.server.test.ts
- tests/setup-browser.ts
- content/blog/fix-the-not-wrapped-in-act-warning/tests/timers.test.browser.jsx
- app/utils/tests/github-mdx-resolution.server.test.ts
- tests/msw-server.ts
- tests/setup-env.ts
- app/routes/calls_.admin/$callId.tsx
- content/blog/how-to-test-custom-react-hooks/tests/use-undo.helper.test.browser.jsx
- app/utils/tests/semantic-search-admin.server.test.ts
- app/routes/calls_.record/new.tsx
- content/blog/fix-the-not-wrapped-in-act-warning/tests/custom-hooks.test.browser.jsx
- vitest.config.ts
| expect(screen.getByRole('tooltip')).toHaveTextContent('If you check this') | ||
| await user.unhover(tooltipButton) | ||
| expect(screen.queryByRole('tooltip')).not.toBeInTheDocument() | ||
| await expect.element(tooltip).toBeHidden() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Vitest Browser Mode expect.element() assertion API toBeHidden toBeVisible documentation
💡 Result:
- Vitest Browser Mode
expect.element()Assertion API (official docs): see the Assertion API page (covers retriableexpect.element(locator)and lists matchers, includingtoBeVisible()). [1] toBeVisible()in Browser Mode (official docs):await expect.element(locator).toBeVisible(); Vitest defines “visible” as non-empty bounding box and notvisibility:hidden(note:opacity:0is considered visible by this definition). [1]toBeHidden()in Browser Mode: it’s not listed/supported in Vitest Browser Mode’s matcher list; use the negation instead:await expect.element(locator).not.toBeVisible(). [1]
For reference, @testing-library/jest-dom documents toBeVisible() (and does not document toBeHidden() either), with a different visibility definition than Vitest Browser Mode. [2]
Sources
[1] Vitest Browser Mode Assertion API: https://vitest.dev/api/browser/assertions.html
[2] @testing-library/jest-dom README (custom matchers, incl. toBeVisible): https://github.com/testing-library/jest-dom
🏁 Script executed:
cat -n app/components/calls/__tests__/episode-artwork-preview.test.browser.tsx | head -90Repository: kentcdodds/kentcdodds.com
Length of output: 3147
toBeHidden() does not exist in Vitest Browser Mode's expect.element() assertion API — TypeScript compilation fails.
The Vitest Browser Mode assertion set only supports toBeVisible() and its negation for visibility checks; toBeHidden() is not available. Both calls on lines 26 and 30 fail the TypeScript check, blocking the build.
Replace both occurrences with .not.toBeVisible().
Proposed fix
- await expect.element(tooltip).toBeHidden()
+ await expect.element(tooltip).not.toBeVisible()
await tooltipButton.hover()
await expect.element(tooltip).toHaveTextContent('If you check this')
await checkbox.hover()
- await expect.element(tooltip).toBeHidden()
+ await expect.element(tooltip).not.toBeVisible()📝 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.
Suggested change
| await expect.element(tooltip).toBeHidden() | |
| await expect.element(tooltip).not.toBeVisible() |
🧰 Tools
🪛 GitHub Check: ʦ TypeScript
[failure] 26-26:
Property 'toBeHidden' does not exist on type 'Promisify<Assertion<HTMLElement | SVGElement | null>>'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/calls/__tests__/episode-artwork-preview.test.browser.tsx` at
line 26, Replace the non-existent Vitest Browser Mode assertion toBeHidden()
with the supported negated visibility assertion: change await
expect.element(tooltip).toBeHidden() to await
expect.element(tooltip).not.toBeVisible() (apply the same replacement for the
second occurrence in the test), locating the calls by the tooltip variable in
episode-artwork-preview.test.browser.tsx.
| | Lint | `npm run lint` | | ||
| | Typecheck | `npm run typecheck` | | ||
| | Unit tests | `npm run test` | | ||
| | Unit tests | `npm run test` (runs backend + browser-mode tests) | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check package.json for test script definitions
cat package.json | grep -A 2 '"test"'Repository: kentcdodds/kentcdodds.com
Length of output: 239
🏁 Script executed:
# Check if there are multiple test-related scripts
rg '"test.*":' package.jsonRepository: kentcdodds/kentcdodds.com
Length of output: 621
🏁 Script executed:
# Read the docs/agents/project-context.md file to see its current state
cat -n docs/agents/project-context.mdRepository: kentcdodds/kentcdodds.com
Length of output: 2413
Document the MSW backend test server auto-wiring in "Non-obvious caveats".
The new tests/setup-backend.ts centralizes MSW server setup with auto-listen, reset, and Cloudflare mock reset on every backend test. This non-obvious behavior should be documented so future agent runs understand backend tests don't require per-file setupServer boilerplate. Per the coding guidelines, add an entry to the "Non-obvious caveats" section explaining this infrastructure change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/agents/project-context.md` at line 22, Add a note to the "Non-obvious
caveats" section in docs/agents/project-context.md explaining that backend tests
auto-wire an MSW server via tests/setup-backend.ts: it calls setupServer and
automatically starts (listen), resets handlers between tests, and resets the
Cloudflare mock so individual backend test files do not need to call setupServer
or teardown themselves; mention the file name tests/setup-backend.ts and the
behaviors (auto-listen, automatic reset, Cloudflare mock reset) so future
contributors know no per-file MSW boilerplate is required.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/utils/__tests__/x-server.test.ts (1)
1-3:⚠️ Potential issue | 🟡 MinorFix import order per ESLint warning.
mswshould be imported beforevitest, andvitestshould remain before#tests/msw-server.ts.Proposed fix
-import { describe, expect, test, vi } from 'vitest' -import { http, HttpResponse } from 'msw' -import { mswServer } from '#tests/msw-server.ts' +import { http, HttpResponse } from 'msw' +import { describe, expect, test, vi } from 'vitest' +import { mswServer } from '#tests/msw-server.ts'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/x-server.test.ts` around lines 1 - 3, Reorder the top-level imports so msw is imported before vitest: import { http, HttpResponse } from 'msw' should come first, then import { describe, expect, test, vi } from 'vitest', and leave import { mswServer } from '#tests/msw-server.ts' last; adjust the three import statements in the test file accordingly to satisfy the ESLint import order rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/utils/__tests__/fetch-json-with-retry-after.server.test.ts`:
- Around line 1-3: Reorder the imports so that the msw import (symbols http,
HttpResponse) comes before the vitest import (symbols beforeEach, expect, test,
vi); specifically, move "import { http, HttpResponse } from 'msw'" above "import
{ beforeEach, expect, test, vi } from 'vitest'" while keeping the mswServer
import unchanged to satisfy ESLint import-order rules.
---
Duplicate comments:
In `@app/utils/__tests__/x-server.test.ts`:
- Around line 1-3: Reorder the top-level imports so msw is imported before
vitest: import { http, HttpResponse } from 'msw' should come first, then import
{ describe, expect, test, vi } from 'vitest', and leave import { mswServer }
from '#tests/msw-server.ts' last; adjust the three import statements in the test
file accordingly to satisfy the ESLint import order rule.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/components/calls/__tests__/episode-artwork-preview.test.browser.tsxapp/routes/calls_.admin/$callId.tsxapp/routes/calls_.record/new.tsxapp/utils/__tests__/fetch-json-with-retry-after.server.test.tsapp/utils/__tests__/x-server.test.tsvitest.browser.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/routes/calls_.admin/$callId.tsx
- app/routes/calls_.record/new.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/utils/__tests__/fetch-json-with-retry-after.server.test.ts (1)
59-65:mswServer.use(...handlers)is registered for unit tests that don't need it.The three
getRetryDelayMsFromResponsetests (lines 85–115) are pure unit tests — no HTTP calls — yetbeforeEachregisters all five MSW handlers before each of them. Consider grouping the HTTP-dependent tests inside adescribeblock with their ownbeforeEach, so handler registration is scoped to tests that actually need it.♻️ Proposed restructure (sketch)
-beforeEach(() => { - mswServer.use(...handlers) - requestCount = 0 - always429Count = 0 - flaky500Count = 0 - networkErrorCount = 0 -}) - -test('fetchJsonWithRetryAfter waits Retry-After seconds...', ...) -test('fetchJsonWithRetryAfter throws after exhausting...', ...) -// ...remaining HTTP tests - test('getRetryDelayMsFromResponse parses Retry-After HTTP-date', ...) test('getRetryDelayMsFromResponse falls back to default delay...', ...) test('getRetryDelayMsFromResponse uses RateLimit-Reset...', ...) + +describe('fetchJsonWithRetryAfter', () => { + beforeEach(() => { + mswServer.use(...handlers) + requestCount = 0 + always429Count = 0 + flaky500Count = 0 + networkErrorCount = 0 + }) + + test('waits Retry-After seconds on 429 then retries', ...) + test('throws after exhausting 429 retries', ...) + // ...remaining HTTP tests +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/fetch-json-with-retry-after.server.test.ts` around lines 59 - 65, The global beforeEach currently calls mswServer.use(...handlers) which registers MSW handlers even for pure unit tests like the getRetryDelayMsFromResponse tests; move mswServer.use(...handlers) and the request-related counter resets into a dedicated describe block for HTTP-dependent tests (or add a nested beforeEach) so only tests that perform network calls register handlers, and leave the pure unit tests (e.g., tests referencing getRetryDelayMsFromResponse) outside that block with only the resets they need; update or remove the global beforeEach registration and ensure the HTTP tests call mswServer.use(...handlers) in their own beforeEach and cleanup appropriately.app/utils/__tests__/x-server.test.ts (1)
103-116: Consider consolidatingmswServer.use(tweetResultHandler)into abeforeEach.Every test in this
describeblock callsmswServer.use(tweetResultHandler)verbatim. Moving it to abeforeEachremoves the repetition and matches the pattern used infetch-json-with-retry-after.server.test.ts.♻️ Proposed refactor
-import { describe, expect, test, vi } from 'vitest' +import { beforeEach, describe, expect, test, vi } from 'vitest'describe('getTweetEmbedHTML', () => { + beforeEach(() => { + mswServer.use(tweetResultHandler) + }) + test('adds linked ellipsis when longform content is truncated', async () => { - mswServer.use(tweetResultHandler) const html = await getTweetEmbedHTML(test('prefers full note tweet text when available', async () => { - mswServer.use(tweetResultHandler) const html = await getTweetEmbedHTML(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/x-server.test.ts` around lines 103 - 116, The tests repeatedly call mswServer.use(tweetResultHandler); remove the duplication by registering tweetResultHandler in a beforeEach within the same describe block: add a beforeEach that calls mswServer.use(tweetResultHandler) so individual tests no longer need their own mswServer.use calls, keeping test behavior identical and matching the pattern used in fetch-json-with-retry-after.server.test.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/utils/__tests__/fetch-json-with-retry-after.server.test.ts`:
- Around line 59-65: The global beforeEach currently calls
mswServer.use(...handlers) which registers MSW handlers even for pure unit tests
like the getRetryDelayMsFromResponse tests; move mswServer.use(...handlers) and
the request-related counter resets into a dedicated describe block for
HTTP-dependent tests (or add a nested beforeEach) so only tests that perform
network calls register handlers, and leave the pure unit tests (e.g., tests
referencing getRetryDelayMsFromResponse) outside that block with only the resets
they need; update or remove the global beforeEach registration and ensure the
HTTP tests call mswServer.use(...handlers) in their own beforeEach and cleanup
appropriately.
In `@app/utils/__tests__/x-server.test.ts`:
- Around line 103-116: The tests repeatedly call
mswServer.use(tweetResultHandler); remove the duplication by registering
tweetResultHandler in a beforeEach within the same describe block: add a
beforeEach that calls mswServer.use(tweetResultHandler) so individual tests no
longer need their own mswServer.use calls, keeping test behavior identical and
matching the pattern used in fetch-json-with-retry-after.server.test.ts.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/utils/__tests__/fetch-json-with-retry-after.server.test.tsapp/utils/__tests__/x-server.test.tscontent/blog/how-to-test-custom-react-hooks/__tests__/use-undo.helper.test.browser.jsxcontent/blog/how-to-test-custom-react-hooks/__tests__/use-undo.rthl.test.browser.jsxvitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- vitest.config.ts
- content/blog/how-to-test-custom-react-hooks/tests/use-undo.helper.test.browser.jsx
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
content/blog/how-to-test-custom-react-hooks/__tests__/use-undo.rthl.test.browser.jsx
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
cursor
bot
force-pushed
the
cursor/test-categories-and-browser-mode-f948
branch
from
04f50e0 to
a90665d
Compare
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
bot
force-pushed
the
cursor/test-categories-and-browser-mode-f948
branch
from
a90665d to
0a5abe1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrate component tests to Vitest Browser Mode and configure backend tests with automatic MSW setup, establishing three distinct test categories.
Note
Medium Risk
Medium risk because it changes test infrastructure (Vitest runner configs, MSW lifecycle, and dependencies) and moves a key UI form component out of a route module, which could subtly affect imports and runtime behavior if any consumers were missed.
Overview
Introduces separate backend vs browser test suites: adds
vitest.browser.config.ts(Playwright-backed browser mode) and updatespackage.jsonscripts/docs to runtest:backend+test:browser, with backend tests now running innodeand excluding*.test.browser.*.Refactors Call Kent recording UI by extracting
RecordingForm(plusrecordingFormActionPathandgetNavigationPathFromResponse) intoapp/components/calls/recording-form.tsx, updating routes/tests to import from the new component, and moving the sharedgetStringFormValuehelper intoapp/utils/misc.ts.Consolidates MSW setup by centralizing handlers in
mocks/msw-handlers.ts, adding a sharedtests/msw-server.tsand automatic lifecycle hooks intests/setup-backend.ts, and updating several server tests to rely on this shared server instead of per-filesetupServerboilerplate. Browser/component tests are migrated from Testing Library/user-event tovitest-browser-reactlocators and expectations, and related devDependencies are swapped accordingly.Written by Cursor Bugbot for commit 0a5abe1. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation