Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a TimeEntryStatus enum and status column, updates timer API handlers to use explicit statuses and ownership checks, resumes running timers in the TimerPage on mount, and introduces Jest config, test setup, and tests covering GET/POST/PUT timer flows and UI resume. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (TimerPage)
participant API as Server (app/api/timer)
participant DB as Database (Prisma)
Browser->>API: GET /api/timer
API->>DB: timeEntry.findFirst(status: RUNNING, include: client)
DB-->>API: runningEntry | null
API-->>Browser: 200 { runningEntry }
alt runningEntry returned
Browser->>Browser: set selectedClientId, activeEntryId, startTime, isRunning
end
Browser->>API: POST /api/timer { clientId, startTime }
API->>DB: timeEntry.findFirst(status: RUNNING, userId)
alt existing RUNNING
API-->>Browser: 409 { error: "A timer is already running" }
else no running
API->>DB: timeEntry.create({ status: RUNNING, ... })
DB-->>API: new timeEntry
API-->>Browser: 201 { timeEntry }
end
Browser->>API: PUT /api/timer/{id} { endTime }
API->>DB: timeEntry.findUnique(id)
alt not found or wrong user
API-->>Browser: 404 { error: "Time entry not found" }
else found
API->>DB: timeEntry.update(id, { status: COMPLETED, endTime })
DB-->>API: updated timeEntry
API-->>Browser: 200 { timeEntry }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 docstrings
🧪 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/timer/[id]/route.ts (1)
33-49:⚠️ Potential issue | 🟠 MajorHandle
P2025error to return correct 404 status on race condition.Lines 33–38 are non-atomic: if the time entry is deleted between the ownership check and the update call, Prisma throws
P2025but the catch block returns 500 instead of 404. Add explicit error handling to mapP2025to a 404 response.Suggested fix
-import { TimeEntryStatus } from '@prisma/client'; +import { Prisma, TimeEntryStatus } from '@prisma/client'; @@ - } catch (error) { + } catch (error: unknown) { + if (error instanceof Prisma.PrismaClientKnownRequestError && error.code === 'P2025') { + return Response.json({ error: 'Time entry not found' }, { status: 404 }); + } console.error('Error updating time entry:', error); return Response.json({ error: 'Internal server error' }, { status: 500 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/timer/`[id]/route.ts around lines 33 - 49, The update flow currently does a non-atomic check with prisma.timeEntry.findUnique followed by prisma.timeEntry.update, and if the row is deleted in between Prisma throws a P2025 but the catch block returns 500; update the catch in the route handler to detect Prisma.PrismaClientKnownRequestError (or inspect error.code === 'P2025') and return Response.json({ error: 'Time entry not found' }, { status: 404 }) for that case, otherwise keep the existing 500 handling and console.error; reference prisma.timeEntry.findUnique, prisma.timeEntry.update and the current catch block to locate where to add this mapping.
🧹 Nitpick comments (4)
__tests__/api/timer.put.test.ts (1)
37-42: Assert response status codes for route-contract coverage.Both cases verify body only. Add status assertions so regressions in HTTP semantics (e.g., 404 vs 500) are caught.
Suggested additions
const res = await PUT(req as Request, { params: Promise.resolve({ id: 'entry1' }) } as { params: Promise<{ id: string }> }); const json = await res.json(); + expect(res.status).toBe(200); expect(json.timeEntry).toBeDefined(); expect(json.timeEntry.status).toBe('COMPLETED'); @@ const res = await PUT(req as Request, { params: Promise.resolve({ id: 'doesnotexist' }) } as { params: Promise<{ id: string }> }); const json = await res.json(); + expect(res.status).toBe(404); expect(json.error).toBe('Time entry not found');Also applies to: 54-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/api/timer.put.test.ts` around lines 37 - 42, Tests currently only assert the response body; add explicit status assertions to catch HTTP-semantic regressions. After calling PUT(...) and before examining json, assert the response status (e.g., expect(res.status).toBe(200)) in the block that checks json.timeEntry.status === 'COMPLETED', and do the same for the other test case around lines 54-58; update the assertions to reference the same res variable returned by the PUT handler to ensure both status and body are validated.prisma/schema.prisma (1)
41-53: Add an index for the new running-timer lookup path.Given queries filter by
userId+status, add a composite index onTimeEntryto avoid table scans as entries grow.Suggested schema update
model TimeEntry { id String `@id` `@default`(cuid()) startTime DateTime endTime DateTime? status TimeEntryStatus `@default`(RUNNING) createdAt DateTime `@default`(now()) clientId String client Client `@relation`(fields: [clientId], references: [id], onDelete: Cascade) userId String user User `@relation`(fields: [userId], references: [id], onDelete: Cascade) + + @@index([userId, status]) }Also applies to: 55-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prisma/schema.prisma` around lines 41 - 53, Add a composite index on the TimeEntry model for the lookup path used in queries (userId + status): update the TimeEntry model in prisma/schema.prisma (the model named TimeEntry) to declare a model-level index (@@index) covering the userId and status fields so queries filtering by userId and status use the index and avoid table scans; include an explicit index name if desired for clarity (e.g., idx_timeentry_userid_status) and run prisma migrate to apply the change.jest.config.js (1)
6-10: Consider centralizing alias mappings to reduce maintenance drift.Lines 14–15 manually mirror the
tsconfig.jsonpaths (lines 21–23 intsconfig.json). Although currently in sync, this duplication can silently drift when aliases change. If these mappings must remain separate, document or automate the synchronization (e.g., via a shared configuration or build step) to maintain a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jest.config.js` around lines 6 - 10, The jest config currently duplicates tsconfig path aliases inside the '^.+\\.(ts|tsx)$' transformer ts-jest options, which risks drift; remove the inline paths object and instead make ts-jest read aliases from the single tsconfig.json (or load paths programmatically) so aliases are centralized—update the transformer config for 'ts-jest' to reference the project's tsconfig (or use a small helper that reads tsconfig.json and injects paths), or alternatively add automation/documentation to keep the ts-jest paths in sync with tsconfig.json; target the transformer config block for '^.+\\.(ts|tsx)$' and the tsconfig.json paths definitions when making this change.app/api/timer/route.ts (1)
89-98: Inconsistent status value type: useTimeEntryStatus.RUNNINGfor consistency.The POST handler uses the
TimeEntryStatus.RUNNINGenum (Line 46, 60), but the GET handler uses the string literal'RUNNING'. While Prisma accepts both, using the enum consistently improves type safety and maintainability.♻️ Proposed fix
const runningEntry = await prisma.timeEntry.findFirst({ where: { userId: user.id, - status: 'RUNNING', + status: TimeEntryStatus.RUNNING, }, include: { client: true, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/timer/route.ts` around lines 89 - 98, Replace the string literal status filter with the enum in the GET handler: in the prisma.timeEntry.findFirst call (the runningEntry query) use TimeEntryStatus.RUNNING instead of 'RUNNING' to match the POST handler and maintain type safety and consistency with the TimeEntryStatus enum.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/api/timer.post.test.ts`:
- Around line 20-26: The test's mocked module path is inconsistent: change the
requireMock call so the mocked prisma module is actually imported (replace
"../../app/lib/prisma" with the same module ID used in jest.mock, i.e.,
"@/lib/prisma") so that the destructured const { prisma: mockPrisma } refers to
the real Jest mock; update the requireMock invocation near the top of the test
(the statement that defines mockPrisma) to use "@/lib/prisma" to match the mock
declaration used elsewhere (see how timer.get.test.ts imports the same mock).
In `@__tests__/TimerPage.resume.test.tsx`:
- Around line 31-34: The assertion currently only checks for any colon-delimited
text using waitFor and screen.getByText, which can match "00:00:00"; update the
waitFor assertion to ensure the displayed time is not all zeros by matching a
regex that excludes "00:00:00" (for example use waitFor(() =>
expect(screen.getByText(/(?!00:00:00)\d{2}:\d{2}:\d{2}/)).toBeInTheDocument())),
i.e., replace the /:/ check with a stricter regex or an explicit
expect(...).not.toHaveTextContent('00:00:00') using the existing waitFor and
screen.getByText helpers.
In `@app/timer/page.tsx`:
- Around line 48-49: Clamp the computed elapsed seconds to non-negative before
updating state: when computing diff from Date.now() - s.getTime() (the current
elapsed calculation used before calling setSeconds), wrap the result with
Math.max(0, ...) so any negative value (due to clock skew or a future start
time) becomes 0, then call setSeconds with that clamped value.
In `@jest.setup.ts`:
- Around line 34-35: Guard the global fetch API shims with feature-detection
instead of unconditional overwrite: check typeof global.Request === 'undefined'
before assigning (global as unknown as { Request?: unknown }).Request =
TestRequest, and check typeof global.Response === 'undefined' before assigning
the Response mock; also update the Response mock used in the Response assignment
to include an .ok boolean and any other properties your code expects (e.g.,
status, json/text methods) so calls like if (!res.ok) in app/timer/page.tsx,
app/dashboard/page.tsx, app/invoices/page.tsx behave correctly.
In `@prisma/migrations/20260403085905_add_timeentry_status/migration.sql`:
- Line 5: The migration currently sets every TimeEntry.status to 'RUNNING' which
incorrectly marks entries with a non-null endTime as active; after adding the
"status" column (enum TimeEntryStatus) keep the default but run a corrective
UPDATE: set "status" = 'COMPLETED' WHERE "endTime" IS NOT NULL (targeting the
"TimeEntry" table and the "endTime" column), so existing completed rows are
marked correctly; ensure this UPDATE is executed in the same migration before
any NOT NULL constraints or assumptions about status.
---
Outside diff comments:
In `@app/api/timer/`[id]/route.ts:
- Around line 33-49: The update flow currently does a non-atomic check with
prisma.timeEntry.findUnique followed by prisma.timeEntry.update, and if the row
is deleted in between Prisma throws a P2025 but the catch block returns 500;
update the catch in the route handler to detect
Prisma.PrismaClientKnownRequestError (or inspect error.code === 'P2025') and
return Response.json({ error: 'Time entry not found' }, { status: 404 }) for
that case, otherwise keep the existing 500 handling and console.error; reference
prisma.timeEntry.findUnique, prisma.timeEntry.update and the current catch block
to locate where to add this mapping.
---
Nitpick comments:
In `@__tests__/api/timer.put.test.ts`:
- Around line 37-42: Tests currently only assert the response body; add explicit
status assertions to catch HTTP-semantic regressions. After calling PUT(...) and
before examining json, assert the response status (e.g.,
expect(res.status).toBe(200)) in the block that checks json.timeEntry.status ===
'COMPLETED', and do the same for the other test case around lines 54-58; update
the assertions to reference the same res variable returned by the PUT handler to
ensure both status and body are validated.
In `@app/api/timer/route.ts`:
- Around line 89-98: Replace the string literal status filter with the enum in
the GET handler: in the prisma.timeEntry.findFirst call (the runningEntry query)
use TimeEntryStatus.RUNNING instead of 'RUNNING' to match the POST handler and
maintain type safety and consistency with the TimeEntryStatus enum.
In `@jest.config.js`:
- Around line 6-10: The jest config currently duplicates tsconfig path aliases
inside the '^.+\\.(ts|tsx)$' transformer ts-jest options, which risks drift;
remove the inline paths object and instead make ts-jest read aliases from the
single tsconfig.json (or load paths programmatically) so aliases are
centralized—update the transformer config for 'ts-jest' to reference the
project's tsconfig (or use a small helper that reads tsconfig.json and injects
paths), or alternatively add automation/documentation to keep the ts-jest paths
in sync with tsconfig.json; target the transformer config block for
'^.+\\.(ts|tsx)$' and the tsconfig.json paths definitions when making this
change.
In `@prisma/schema.prisma`:
- Around line 41-53: Add a composite index on the TimeEntry model for the lookup
path used in queries (userId + status): update the TimeEntry model in
prisma/schema.prisma (the model named TimeEntry) to declare a model-level index
(@@index) covering the userId and status fields so queries filtering by userId
and status use the index and avoid table scans; include an explicit index name
if desired for clarity (e.g., idx_timeentry_userid_status) and run prisma
migrate to apply 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4cb7f5d-2f3e-41de-a21b-077147fcfffe
📒 Files selected for processing (12)
__tests__/TimerPage.resume.test.tsx__tests__/api/timer.get.test.ts__tests__/api/timer.post.test.ts__tests__/api/timer.put.test.tsapp/api/timer/[id]/route.tsapp/api/timer/route.tsapp/timer/page.tsxjest.config.jsjest.config.tsjest.setup.tsprisma/migrations/20260403085905_add_timeentry_status/migration.sqlprisma/schema.prisma
💤 Files with no reviewable changes (1)
- jest.config.ts
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
jest.setup.ts (1)
34-34:⚠️ Potential issue | 🟠 MajorRemove unconditional global
Requestoverride.Line 34 overwrites
global.Requestbefore the feature-detection block, so Line 66 becomes ineffective and nativeRequestis always clobbered.Proposed fix
-(global as unknown as { Request?: unknown }).Request = TestRequest; - // Basic env required by validateEnv and next-auth during tests process.env.DATABASE_URL = process.env.DATABASE_URL || 'postgresql://test:test@localhost:5432/test'; process.env.NEXTAUTH_SECRET = process.env.NEXTAUTH_SECRET || 'test-secret'; process.env.NEXTAUTH_URL = process.env.NEXTAUTH_URL || 'http://localhost:3000';#!/bin/bash # Verify there is no unconditional Request assignment left and only guarded assignment remains. rg -n -C2 '\bRequest\s*=\s*TestRequest' jest.setup.ts rg -n -C2 'if \(\(global as unknown as \{ Request\?: unknown \}\)\.Request === undefined\)' jest.setup.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jest.setup.ts` at line 34, Remove the unconditional override of global.Request so native Request isn't always clobbered: delete the direct assignment (global as unknown as { Request?: unknown }).Request = TestRequest and keep the feature-detection guarded assignment that checks if (global as unknown as { Request?: unknown }).Request === undefined before assigning TestRequest; ensure only the guarded path sets Request and that TestRequest is assigned there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jest.setup.ts`:
- Around line 21-31: The TestRequest.json() and text() implementations need to
match Fetch semantics: in json(), change the falsy check to strict `this._body
=== undefined` to allow empty-string bodies, and do not swallow parse errors—let
JSON.parse throw (remove the try/catch that returns the raw body) so malformed
JSON surfaces; in text(), always return a string (e.g., coerce `this._body` to a
string when resolving) so it never returns undefined. Reference: the
TestRequest.json() and TestRequest.text() methods and the `this._body` field.
---
Duplicate comments:
In `@jest.setup.ts`:
- Line 34: Remove the unconditional override of global.Request so native Request
isn't always clobbered: delete the direct assignment (global as unknown as {
Request?: unknown }).Request = TestRequest and keep the feature-detection
guarded assignment that checks if (global as unknown as { Request?: unknown
}).Request === undefined before assigning TestRequest; ensure only the guarded
path sets Request and that TestRequest is assigned there.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 785cb4ec-dc2c-4d9e-993a-cb4694b2ec76
📒 Files selected for processing (5)
__tests__/TimerPage.resume.test.tsx__tests__/api/timer.post.test.tsapp/timer/page.tsxjest.setup.tsprisma/migrations/20260403085905_add_timeentry_status/migration.sql
🚧 Files skipped from review as they are similar to previous changes (4)
- app/timer/page.tsx
- prisma/migrations/20260403085905_add_timeentry_status/migration.sql
- tests/TimerPage.resume.test.tsx
- tests/api/timer.post.test.ts
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 by CodeRabbit
New Features
Data
Tests
Chores