test(e2e): Playwright smoke gate — boot/auth + full escrow money path (A–F) - #27
Merged
Merged
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds Playwright E2E infrastructure: fixtures collecting console/page errors, shared helpers, a gated test-only auto-release API, deterministic ChangesE2E Testing Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
codeant-ai
Bot
added
the
size:XL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| <Button | ||
| size="sm" | ||
| className="w-full gap-1.5 bg-emerald-600 hover:bg-emerald-700" | ||
| data-testid="accept-applicant" |
There was a problem hiding this comment.
Suggestion: This test id is static inside a list, so multiple pending applications will render multiple elements with the same data-testid. Playwright getByTestId(...).click() is strict and will fail when more than one match exists, making the accept-applicant flow flaky or broken for jobs with multiple applicants. Make the test id unique per row (for example include applicant id/index) and update the helper to target the intended row explicitly. [logic error]
Severity Level: Major ⚠️
- ❌ Smoke test Section B fails when job has multiple applicants.
- ❌ Smoke test Sections C and F fail on ambiguous accept-applicant click.
- ⚠️ CI gate for escrow money path becomes flaky with realistic data.Steps of Reproduction ✅
1. Create a client job that receives at least two worker applications so that the
`applications` array rendered by `ClientJobDetail` at
`src/app/client/jobs/[id]/client-job-detail.tsx:1-56` contains multiple entries with
`app.status === "pending"` while `job.status === "open"`.
2. Visit `/client/jobs/:id` for that job so `ClientJobDetail` maps `applications.map((app)
=> ...)` and renders one `<Button data-testid="accept-applicant">` per pending application
at `client-job-detail.tsx:32-37`, resulting in multiple DOM elements sharing the same
`data-testid="accept-applicant"`.
3. Run the Playwright smoke tests in `e2e/smoke.spec.ts`, which import `acceptApplicant`
from `e2e/helpers.ts` at `e2e/smoke.spec.ts:2` and invoke it in several flows (e.g.
Section B at `e2e/smoke.spec.ts:14-18`, Section C at `e2e/smoke.spec.ts:53-57`, Section F
at `e2e/smoke.spec.ts:16-18`).
4. When `acceptApplicant(page, jobId)` executes (`e2e/helpers.ts:112-115`), it navigates
to `/client/jobs/${jobId}` and calls `page.getByTestId("accept-applicant").click()`.
Because the page now has multiple matches for this test id (one per pending applicant),
Playwright's strict locator mode treats this as an ambiguous locator and throws a strict
mode violation error instead of clicking, causing the smoke tests to fail before the
client-accepts-applicant step completes.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/client/jobs/[id]/client-job-detail.tsx
**Line:** 435:435
**Comment:**
*Logic Error: This test id is static inside a list, so multiple pending applications will render multiple elements with the same `data-testid`. Playwright `getByTestId(...).click()` is strict and will fail when more than one match exists, making the accept-applicant flow flaky or broken for jobs with multiple applicants. Make the test id unique per row (for example include applicant id/index) and update the helper to target the intended row explicitly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+15
to
+29
| await page.getByTestId("phone-input").fill(phone.replace(/^\+91/, "")) | ||
| await page.getByTestId("send-otp").click() | ||
|
|
||
| await expect(page).toHaveURL(/\/login\/verify/) | ||
|
|
||
| await page.getByTestId("otp-input").pressSequentially(DEMO_OTP, { delay: 50 }) | ||
|
|
||
| await expect(page).toHaveURL(/\/(client|worker|admin|onboarding)/) | ||
| } | ||
|
|
||
| export async function logout(page: Page) { | ||
| await page.locator("a[href$='/account']").click() | ||
| await page.getByTestId("sign-out").click() | ||
| await page.getByTestId("sign-out-confirm").click() | ||
| await expect(page).toHaveURL(/\/login/) |
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
The Playwright login/logout and wallet top-up helpers depend on data-testid selectors (phone-input, send-otp, otp-input, sign-out, sign-out-confirm, add-money, topup-amount, topup-confirm) that are not present anywhere in the current UI components, so the core smoke tests cannot drive these flows and will fail.
Suggestion: Add matching data-testid attributes to the login/verify forms, sign-out dialog, and TopUpDialog components, or update the helpers/tests to use stable role/name-based locators that match the existing markup.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** e2e/helpers.ts
**Line:** 15:29
**Comment:**
*CRITICAL: The Playwright login/logout and wallet top-up helpers depend on data-testid selectors (phone-input, send-otp, otp-input, sign-out, sign-out-confirm, add-money, topup-amount, topup-confirm) that are not present anywhere in the current UI components, so the core smoke tests cannot drive these flows and will fail.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Greptile SummaryThis PR adds a Playwright E2E smoke suite that exercises the full escrow money path (top-up → post job → apply → accept → fund → submit → approve) plus auth, idempotency, error display, form hardening, and auto-release gates. It also wires
Confidence Score: 5/5Safe to merge — all changes are additive test infrastructure and wiring-only The production-code changes are purely The Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Section A: Boot & Auth] --> A1[App loads at /]
A1 --> A2[Phone-OTP login — client + worker]
A2 --> A3[Protected route redirect check]
A3 --> B[Section B: Escrow Money Path]
B --> B1[Client tops up wallet ₹5000]
B1 --> B2[Client posts job + milestone ₹1000]
B2 --> B3[Worker applies at bid ₹1000]
B3 --> B4[Client accepts applicant]
B4 --> B5[Client funds milestone → escrow]
B5 --> B6[Worker submits milestone]
B6 --> B7[Client approves → money released]
B7 --> C[Section C: Idempotency]
C --> C1[Fresh accepted job ₹500]
C1 --> C2[Double-click Fund confirm]
C2 --> C3{Balance dropped by exactly ₹500?}
C3 -->|Yes| C4[✅ Idempotency holds]
C3 -->|No ₹1000 drop| C5[❌ Double-debit detected]
B7 --> D[Section D: Error Display]
D --> D1[Post job with budget ₹499999]
D1 --> D2[Fund button is disabled]
D2 --> D3[No raw SQL/Supabase text in body]
B7 --> E[Section E: Form Hardening]
E --> E1[Navigate to milestone amount input]
E1 --> E2[Wheel scroll — value unchanged]
E2 --> E3[Type bare minus — no NaN stored]
B7 --> F[Section F: Auto-release]
F --> F1{E2E_TEST_HOOKS=1?}
F1 -->|No| F2[⏭ Skipped]
F1 -->|Yes| F3[Fund + submit milestone]
F3 --> F4[POST /api/test/auto-release backdates auto_release_at -73h]
F4 --> F5[auto_release_milestones called]
F5 --> F6[Worker balance increased ✅]
Reviews (5): Last reviewed commit: "Update e2e-smoke.yml" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+273
to
+281
| const supabaseUrl = process.env.NEXT_PUBLIC_SUPABASE_URL ?? "" | ||
| const supabaseKey = process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY ?? "" | ||
| const msRes = await request.get( | ||
| `${supabaseUrl}/rest/v1/milestones?job_id=eq.${urlJobId}&select=id&status=eq.submitted`, | ||
| { headers: { apikey: supabaseKey, Authorization: `Bearer ${supabaseKey}` } }, | ||
| ) | ||
| const msData = await msRes.json() | ||
| const milestoneId: string = msData?.[0]?.id | ||
| expect(milestoneId, "should find a submitted milestone").toBeTruthy() |
There was a problem hiding this comment.
The milestone lookup uses the anon key directly against the Supabase REST API. If the milestones table's RLS policies require authentication (which is typical — milestones are user-scoped private data), the query returns an empty array rather than an error. msData?.[0]?.id would then be undefined, and the test would fail at expect(milestoneId, "should find a submitted milestone").toBeTruthy() with a message that points at the assertion rather than the underlying RLS block. Consider using an authenticated request or a data-milestone-id attribute on the DOM element to retrieve the ID without a separate REST call.
Comment on lines
+39
to
+46
| export async function expectRpcOk(page: Page, urlPart: string, act: () => Promise<void>) { | ||
| const [res] = await Promise.all([ | ||
| page.waitForResponse((r) => r.url().includes(urlPart) && r.request().method() === "POST"), | ||
| act(), | ||
| ]) | ||
| expect(res.status(), `${urlPart} should return 2xx`).toBeLessThan(400) | ||
| return res | ||
| } |
There was a problem hiding this comment.
expectRpcOk is exported but never called in the spec
The helper is fully implemented and documented but smoke.spec.ts never imports or uses it. If it's intended for future tests, a comment explaining its planned use would help; if it's dead code from an earlier design, removing it keeps the test surface clean.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+15
to
+20
| await page.getByTestId("phone-input").fill(phone.replace(/^\+91/, "")) | ||
| await page.getByTestId("send-otp").click() | ||
|
|
||
| await expect(page).toHaveURL(/\/login\/verify/) | ||
|
|
||
| await page.getByTestId("otp-input").pressSequentially(DEMO_OTP, { delay: 50 }) |
There was a problem hiding this comment.
Suggestion: These selectors don't exist in the current login UI (phone-form.tsx and verify-form.tsx have no matching data-testid attributes), so login() will time out before authentication starts. Update the helper to target existing stable selectors or add matching test IDs in the login forms. [api mismatch]
Severity Level: Critical 🚨
- ❌ All auth-dependent Playwright tests fail before OTP entry.
- ❌ Smoke gate cannot validate boot/auth paths at all.
- ⚠️ CI signal for login regressions becomes unusable.Steps of Reproduction ✅
1. Run the Playwright suite (e.g. `pnpm playwright test e2e/smoke.spec.ts`), which uses
the shared `login()` helper from `e2e/helpers.ts:12-22`.
2. In test `"phone-OTP login works for client and worker"` at `e2e/smoke.spec.ts:12-17`,
`login(page, DEMO.client)` is called, which navigates to `/login` and executes
`page.getByTestId("phone-input")` and `page.getByTestId("send-otp")` at
`e2e/helpers.ts:15-16`.
3. The login form implementation in `src/app/login/phone-form.tsx:39-58` renders the phone
input as `<Input id="phone" ... {...form.register("phone")}>` with no
`data-testid="phone-input"` or `"send-otp"` attributes, and
`src/app/login/verify/verify-form.tsx:71-120` similarly renders the OTP slots without
`data-testid="otp-input"`.
4. Because these `data-testid` attributes do not exist anywhere in the codebase (confirmed
by a project-wide grep for `data-testid="phone-input"`, `data-testid="send-otp"`, and
`data-testid="otp-input"`), Playwright cannot resolve these locators and the first
`login()` call times out, causing all tests that rely on `login()` to fail before
authentication.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** e2e/helpers.ts
**Line:** 15:20
**Comment:**
*Api Mismatch: These selectors don't exist in the current login UI (`phone-form.tsx` and `verify-form.tsx` have no matching `data-testid` attributes), so `login()` will time out before authentication starts. Update the helper to target existing stable selectors or add matching test IDs in the login forms.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+27
to
+28
| await page.getByTestId("sign-out").click() | ||
| await page.getByTestId("sign-out-confirm").click() |
There was a problem hiding this comment.
Suggestion: The account UI currently has no sign-out / sign-out-confirm test IDs, so this logout helper cannot find its targets and will fail every flow that switches users. Align selectors with the actual SignOutButton dialog controls or add those test IDs. [api mismatch]
Severity Level: Critical 🚨
- ❌ Any test that switches users fails on logout.
- ❌ Escrow path tests B–F cannot complete end-to-end.
- ⚠️ Prevents validating multi-role session handling.Steps of Reproduction ✅
1. Run `pnpm playwright test e2e/smoke.spec.ts`; tests call the shared `logout()` helper
from `e2e/helpers.ts:25-30` whenever switching between client and worker users (e.g.
`"phone-OTP login works for client and worker"` at `e2e/smoke.spec.ts:12-17`, Section
B/C/E/F tests).
2. `logout(page)` first clicks the account navigation link via
`page.locator("a[href$='/account']").click()` at `e2e/helpers.ts:26`, which matches
`/client/account` and `/worker/account` links defined in
`src/components/nav/client-nav-shell.tsx:10-12` and
`src/components/nav/worker-nav-shell.tsx:10-12`.
3. Once on the account page (`src/app/client/account/page.tsx:4-22` or
`src/app/worker/account/page.tsx:6-23`), the sign-out UI is rendered by `SignOutButton`
(`src/components/account/sign-out-button.tsx:21-69`), which exposes a `<Button>` and an
`AlertDialogAction` with visible text "Sign out" but no `data-testid="sign-out"` or
`data-testid="sign-out-confirm"`.
4. A repo-wide grep finds no `data-testid="sign-out"` or `"sign-out-confirm"` anywhere, so
`page.getByTestId("sign-out")` and `"sign-out-confirm"` at `e2e/helpers.ts:27-28` always
fail, causing every test that logs out to error at the first logout call.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** e2e/helpers.ts
**Line:** 27:28
**Comment:**
*Api Mismatch: The account UI currently has no `sign-out` / `sign-out-confirm` test IDs, so this logout helper cannot find its targets and will fail every flow that switches users. Align selectors with the actual `SignOutButton` dialog controls or add those test IDs.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+34
to
+36
| await page.getByTestId("add-money").click() | ||
| await page.getByTestId("topup-amount").fill("5000") | ||
| await page.getByTestId("topup-confirm").click() |
There was a problem hiding this comment.
Suggestion: The top-up test references test IDs that are not present in TopUpDialog, so the escrow path fails before funding begins. Use actual button/input selectors from the dialog or add these IDs in the component. [api mismatch]
Severity Level: Critical 🚨
- ❌ Escrow Section B cannot top up wallet in tests.
- ❌ Money-path smoke test fails before any funding.
- ⚠️ Idempotency/error-display sections lose realistic state.Steps of Reproduction ✅
1. Run `pnpm playwright test e2e/smoke.spec.ts` and focus on `"B · escrow money path"`,
test `"client tops up wallet"` at `e2e/smoke.spec.ts:30-41`.
2. After `login(page, DEMO.client)` and `page.goto("/client/wallet")` at
`e2e/smoke.spec.ts:31-32`, the client wallet view component `WalletView` is rendered from
`src/components/features/wallet-view.tsx:18-52`, which includes `<TopUpDialog />` when
`role === "client"` at line 48.
3. The dialog implementation in `src/components/features/topup-dialog.tsx:27-145` renders
the trigger button (`<Button ...>Add Money</Button>`) and an `<Input>` for the amount and
`<Button>`s for "Cancel" / "Add money", but none of these elements have
`data-testid="add-money"`, `data-testid="topup-amount"`, or `data-testid="topup-confirm"`.
4. Because these test IDs do not exist anywhere in the repo (confirmed by searching for
those exact `data-testid` values), `page.getByTestId("add-money")`, `"topup-amount"`, and
`"topup-confirm"` at `e2e/smoke.spec.ts:34-36` cannot locate elements, so the top-up test
fails before any wallet funding happens, blocking the rest of the escrow money-path
verification.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** e2e/smoke.spec.ts
**Line:** 34:36
**Comment:**
*Api Mismatch: The top-up test references test IDs that are not present in `TopUpDialog`, so the escrow path fails before funding begins. Use actual button/input selectors from the dialog or add these IDs in the component.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+103
to
+106
| await logout(page) | ||
| await login(page, DEMO.client) | ||
| await acceptApplicant(page, jobId) | ||
|
|
There was a problem hiding this comment.
Suggestion: The skip condition checks only truthiness, so values like "0" or "false" will run the test even though the API hook requires E2E_TEST_HOOKS === "1", causing deterministic 404 failures. Match the same strict condition as the server gate. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Misconfigured E2E_TEST_HOOKS causes deterministic failures in F.
- ⚠️ Client/server gating semantics for hooks are inconsistent.
- ⚠️ Debugging CI env issues becomes harder and error-prone.Steps of Reproduction ✅
1. The auto-release E2E test `"backdated milestone auto-releases"` is defined in
`e2e/smoke.spec.ts:101-222` and is guarded with `test.skip(!process.env.E2E_TEST_HOOKS,
"...")` at lines 103-106.
2. The corresponding API hook at `src/app/api/test/auto-release/route.ts:21-27` enables
privileged behavior only when `const TEST_HOOKS = process.env.E2E_TEST_HOOKS === "1"` and
returns 404 when `!TEST_HOOKS`.
3. Set the environment variable to a non-empty but non-"1" value, e.g. `E2E_TEST_HOOKS=0`
or `E2E_TEST_HOOKS=false`, and run `pnpm playwright test e2e/smoke.spec.ts`.
4. In this configuration, `process.env.E2E_TEST_HOOKS` is a non-empty string, so
`!process.env.E2E_TEST_HOOKS` is `false` and the test is NOT skipped, but on the server
`TEST_HOOKS` is `false` (since `"0" !== "1"`), so calls to `/api/test/auto-release` in the
test at `e2e/smoke.spec.ts:205-208` receive 404 instead of 2xx, causing deterministic test
failures even though the hook is effectively disabled according to server-side logic.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** e2e/smoke.spec.ts
**Line:** 103:106
**Comment:**
*Incorrect Condition Logic: The skip condition checks only truthiness, so values like `"0"` or `"false"` will run the test even though the API hook requires `E2E_TEST_HOOKS === "1"`, causing deterministic 404 failures. Match the same strict condition as the server gate.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+131
to
+137
| const fundRows = page.getByTestId("ledger-row-fund") | ||
| // There may be prior fund rows from Section B; assert at least 1 but not 2 new ones. | ||
| // Since C runs after B we assert the latest fund row shows ₹500 (our amount). | ||
| await expect(fundRows.first()).toBeVisible({ timeout: 10_000 }) | ||
| // Verify the top row matches our ₹500 fund (not a duplicate ₹500+₹500) | ||
| await expect(fundRows.first()).toContainText("500") | ||
| }) |
There was a problem hiding this comment.
Suggestion: This does not verify the stated requirement ("exactly one ledger row"): it only checks that the first fund row contains 500, which still passes when duplicate 500 rows are created. Assert the count delta for this operation (or uniquely match by job/milestone) to actually detect double-funding. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Double-funding bugs may ship undetected by suite.
- ⚠️ Idempotency guarantees for Fund are not actually enforced.
- ⚠️ CI green build may hide ledger duplication regressions.Steps of Reproduction ✅
1. The `"C · idempotency"` suite in `e2e/smoke.spec.ts:91-138` creates a new job, applies,
accepts, and then double-clicks the Fund button for milestone 0, relying on the
component's `inFlightRef` to guard against duplicate funds.
2. After funding, it navigates to `/client/wallet` (`e2e/smoke.spec.ts:93-107`) where
`WalletView` renders the ledger list; each fund-type entry sets
`data-testid="ledger-row-fund"` in `src/components/features/wallet-view.tsx:92-101`.
3. The test then executes the assertions at `e2e/smoke.spec.ts:131-137`:
- `const fundRows = page.getByTestId("ledger-row-fund")`
- `await expect(fundRows.first()).toBeVisible(...)`
- `await expect(fundRows.first()).toContainText("500")`.
4. If a future regression causes both clicks to write a `fund` ledger row of amount 500
(duplicate entries), the DOM will contain at least two `ledger-row-fund` items, each
containing `"500"`. The call to `.first()` still returns a node containing `"500"`, so
these assertions pass and the test does not detect the double-funding bug, despite the
test name and comments stating it should enforce "exactly one ledger row".Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** e2e/smoke.spec.ts
**Line:** 131:137
**Comment:**
*Incomplete Implementation: This does not verify the stated requirement ("exactly one ledger row"): it only checks that the first fund row contains `500`, which still passes when duplicate `500` rows are created. Assert the count delta for this operation (or uniquely match by job/milestone) to actually detect double-funding.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+48
to
+58
| const supabase = createSupabaseClient<Database>(supabaseUrl, serviceRoleKey, { | ||
| auth: { persistSession: false }, | ||
| }); | ||
|
|
||
| // 1. Backdate auto_release_at to 73 hours ago (past the 72h release window) | ||
| const backdated = new Date(Date.now() - 73 * 60 * 60 * 1000).toISOString(); | ||
| const { error: updateErr } = await supabase | ||
| .from("milestones") | ||
| .update({ auto_release_at: backdated }) | ||
| .eq("id", milestoneId) | ||
| .eq("status", "submitted"); // safety: only touch submitted milestones |
There was a problem hiding this comment.
Suggestion: This endpoint performs privileged service-role operations without any user authentication/authorization check; if the env gates are enabled, any caller who can reach the route can trigger escrow release actions. Require an authenticated trusted caller (or a signed internal secret/header) before running service-role updates. [security]
Severity Level: Critical 🚨
- ❌ Unauthenticated callers can trigger auto_release_milestones when hooks enabled.
- ❌ Service-role key is exercised on arbitrary user-supplied milestone IDs.
- ⚠️ Test/demo environments risk unintended escrow state changes.Steps of Reproduction ✅
1. The test hook endpoint `POST /api/test/auto-release` is implemented in
`src/app/api/test/auto-release/route.ts:17-73` and uses a Supabase service-role client
created at lines 48-50 (`createSupabaseClient<Database>(supabaseUrl, serviceRoleKey, {
auth: { persistSession: false } })`).
2. The only guards are environment-based: `DEMO_MODE = process.env.NEXT_PUBLIC_DEMO_MODE
=== "true"` and `TEST_HOOKS = process.env.E2E_TEST_HOOKS === "1"` at `route.ts:21-22`,
with a combined check `if (!DEMO_MODE || !TEST_HOOKS) return 404;` at lines 24-27.
3. When both env vars are set (as required to run the E2E test in
`e2e/smoke.spec.ts:101-222`), any HTTP client that can reach the app (not just Playwright)
can send `POST /api/test/auto-release` with `{ "milestoneId": "<some-submitted-id>" }` and
the handler will:
- Backdate `auto_release_at` for that milestone via service-role update at
`route.ts:52-58`, bypassing RLS.
- Invoke the privileged `auto_release_milestones` RPC at `route.ts:65-70`.
4. Because `NextRequest` is never authenticated and no additional secret or header is
validated, this privileged mutation is effectively exposed to any external caller whenever
DEMO_MODE and E2E_TEST_HOOKS are enabled, allowing unauthorized triggering of escrow
auto-release logic in that environment.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/test/auto-release/route.ts
**Line:** 48:58
**Comment:**
*Security: This endpoint performs privileged service-role operations without any user authentication/authorization check; if the env gates are enabled, any caller who can reach the route can trigger escrow release actions. Require an authenticated trusted caller (or a signed internal secret/header) before running service-role updates.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/smoke.spec.ts`:
- Around line 222-225: The skip predicate for the test currently only checks
truthiness of process.env.E2E_TEST_HOOKS which allows values like "0" to run and
doesn't ensure demo mode is enabled; update the test.skip condition (the call to
test.skip) to explicitly require both process.env.E2E_TEST_HOOKS === "1" and
process.env.DEMO_MODE === "1" (i.e. skip when process.env.E2E_TEST_HOOKS !== "1"
|| process.env.DEMO_MODE !== "1") so the test is only executed when the server
hook POST /api/test/auto-release is actually enabled.
- Around line 125-137: Before asserting visibility of the fund row, capture the
pre-action count from page.getByTestId("ledger-row-fund") (e.g., const before =
await fundRows.count()), perform the double-click flow, then re-query the fund
rows and assert finalCount === before + 1 to ensure exactly one new ledger
write, and additionally assert that the newly added row (e.g.,
fundRows.nth(finalCount - 1) or fundRows.last()) contains "500"; update the
expectations around fundRows.first()/toContainText to use the newly added row
after the count check.
In `@src/app/api/test/auto-release/route.ts`:
- Around line 54-58: The update currently only checks updateErr but not whether
any row was actually updated; change the logic after the supabase update call
(the supabase.from("milestones").update(...) result handling) to verify the
update affected a row (e.g., check result.data && result.data.length > 0 or an
affected-row count) for the given milestoneId before calling
auto_release_milestones(); if no row was matched, return an appropriate 4xx
response (or skip triggering auto_release_milestones()) instead of always
invoking auto_release_milestones().
🪄 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 Plus
Run ID: cd9d0797-2cbd-4fe1-a420-3976668bc22f
📒 Files selected for processing (12)
e2e/fixtures.tse2e/helpers.tse2e/smoke.spec.tssrc/app/api/test/auto-release/route.tssrc/app/client/jobs/[id]/client-job-detail.tsxsrc/app/client/jobs/[id]/milestones/client-milestones.tsxsrc/app/client/jobs/new/post-job-form.tsxsrc/app/client/page.tsxsrc/app/worker/jobs/[id]/milestones/worker-milestones.tsxsrc/app/worker/jobs/[id]/worker-job-detail.tsxsrc/components/features/wallet-view.tsxsrc/components/ui/status-badge.tsx
Comment on lines
+222
to
+225
| test.skip( | ||
| !process.env.E2E_TEST_HOOKS, | ||
| "requires E2E_TEST_HOOKS=1 and DEMO_MODE test hook: POST /api/test/auto-release", | ||
| ) |
There was a problem hiding this comment.
Make Section F skip logic match the server hook gate exactly.
Line 223 currently checks only truthiness. If E2E_TEST_HOOKS="0", this test still runs; it can also run when demo mode is off, then /api/test/auto-release returns 404.
Suggested fix
- test.skip(
- !process.env.E2E_TEST_HOOKS,
- "requires E2E_TEST_HOOKS=1 and DEMO_MODE test hook: POST /api/test/auto-release",
- )
+ const hooksEnabled = process.env.E2E_TEST_HOOKS === "1"
+ const demoModeEnabled = process.env.NEXT_PUBLIC_DEMO_MODE === "true"
+ test.skip(
+ !(hooksEnabled && demoModeEnabled),
+ "requires E2E_TEST_HOOKS=1 and NEXT_PUBLIC_DEMO_MODE=true for POST /api/test/auto-release",
+ )📝 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
| test.skip( | |
| !process.env.E2E_TEST_HOOKS, | |
| "requires E2E_TEST_HOOKS=1 and DEMO_MODE test hook: POST /api/test/auto-release", | |
| ) | |
| const hooksEnabled = process.env.E2E_TEST_HOOKS === "1" | |
| const demoModeEnabled = process.env.NEXT_PUBLIC_DEMO_MODE === "true" | |
| test.skip( | |
| !(hooksEnabled && demoModeEnabled), | |
| "requires E2E_TEST_HOOKS=1 and NEXT_PUBLIC_DEMO_MODE=true for POST /api/test/auto-release", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/smoke.spec.ts` around lines 222 - 225, The skip predicate for the test
currently only checks truthiness of process.env.E2E_TEST_HOOKS which allows
values like "0" to run and doesn't ensure demo mode is enabled; update the
test.skip condition (the call to test.skip) to explicitly require both
process.env.E2E_TEST_HOOKS === "1" and process.env.DEMO_MODE === "1" (i.e. skip
when process.env.E2E_TEST_HOOKS !== "1" || process.env.DEMO_MODE !== "1") so the
test is only executed when the server hook POST /api/test/auto-release is
actually enabled.
Comment on lines
+54
to
+58
| const { error: updateErr } = await supabase | ||
| .from("milestones") | ||
| .update({ auto_release_at: backdated }) | ||
| .eq("id", milestoneId) | ||
| .eq("status", "submitted"); // safety: only touch submitted milestones |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/app/api/test/auto-release/route.ts"
echo "== File =="
ls -l "$FILE"
echo "== Relevant excerpt (lines 35-110) =="
nl -ba "$FILE" | sed -n '35,110p'
echo "== Search for auto_release_milestones usage =="
rg -n "auto_release_milestones" "$FILE"
echo "== Search for .update({ auto_release_at: backdated }) =="
rg -n "update\(\{ *auto_release_at: *backdated" "$FILE"Repository: shaiksohelll/Pakka
Length of output: 261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/app/api/test/auto-release/route.ts"
echo "== File =="
ls -l "$FILE"
echo "== Relevant excerpt (lines 35-110) =="
awk 'NR>=35 && NR<=110 {printf "%d:%s\n", NR, $0}' "$FILE"
echo "== Search for auto_release_milestones usage =="
rg -n "auto_release_milestones" "$FILE" || true
echo "== Search for update({ auto_release_at: backdated }) =="
rg -n "update\(\{ *auto_release_at: *backdated" "$FILE" || trueRepository: shaiksohelll/Pakka
Length of output: 2142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "auto_release_milestones" . || true
fd -e sql -e psql -e js -e ts -e plpgsql -e py -e rb -e go . 2>/dev/null | head -n 50Repository: shaiksohelll/Pakka
Length of output: 9814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE_TS="src/app/api/test/auto-release/route.ts"
echo "## route.ts line count"
wc -l "$FILE_TS"
echo "## route.ts full content (numbered via awk)"
awk '{print NR":"$0}' "$FILE_TS"
echo "## auto_release_milestones latest migration(s) - show function body sections"
for f in \
supabase/migrations/20260604060000_auto_release_row_count_guard.sql \
supabase/migrations/20260521080000_pr19_review_fixes.sql \
supabase/migrations/202604300001_add_auto_release_and_idempotency.sql
do
if [ -f "$f" ]; then
echo "==== $f ===="
awk 'BEGIN{p=0} /create or replace function public\.auto_release_milestones\(/ {p=1} { if(p) print } /end\;\$\$/ { if(p){ exit } }' "$f" | sed -n '1,260p'
echo "==== end ===="
fi
done
echo "## E2E usage"
rg -n "auto-release" e2e/smoke.spec.ts e2e/fixtures.ts e2e/helpers.ts || true
rg -n "auto_release_milestones" e2e/smoke.spec.ts || trueRepository: shaiksohelll/Pakka
Length of output: 21513
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="e2e/smoke.spec.ts"
echo "== excerpt around F · auto-release =="
awk 'NR>=200 && NR<=320 {printf "%d:%s\n", NR, $0}' "$FILE" | sed -n '1,160p'Repository: shaiksohelll/Pakka
Length of output: 5003
Validate the milestone update matched a row before triggering global auto-release
route.ts updates milestones using id + status="submitted" but only checks updateErr; if 0 rows match, it still calls auto_release_milestones(), which releases any other submitted milestones whose auto_release_at < now(). The E2E test only asserts released > 0, so it can pass even when the requested milestone wasn’t backdated.
Suggested fix
- const { error: updateErr } = await supabase
+ const { data: updatedRows, error: updateErr } = await supabase
.from("milestones")
.update({ auto_release_at: backdated })
.eq("id", milestoneId)
- .eq("status", "submitted"); // safety: only touch submitted milestones
+ .eq("status", "submitted") // safety: only touch submitted milestones
+ .select("id");
if (updateErr) {
console.error("[test/auto-release] backdate error:", updateErr);
return NextResponse.json({ error: updateErr.message }, { status: 500 });
}
+ if (!updatedRows || updatedRows.length === 0) {
+ return NextResponse.json({ error: "Submitted milestone not found" }, { status: 404 });
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/test/auto-release/route.ts` around lines 54 - 58, The update
currently only checks updateErr but not whether any row was actually updated;
change the logic after the supabase update call (the
supabase.from("milestones").update(...) result handling) to verify the update
affected a row (e.g., check result.data && result.data.length > 0 or an
affected-row count) for the given milestoneId before calling
auto_release_milestones(); if no row was matched, return an appropriate 4xx
response (or skip triggering auto_release_milestones()) instead of always
invoking auto_release_milestones().
…ogin + idempotency specs; skip auto-release in CI
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XL
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e-smoke.yml:
- Around line 9-13: The workflow uses mutable action tags and leaves checkout
credentials persistent; update the three actions (actions/checkout,
pnpm/action-setup, actions/setup-node) to pinned commit SHAs instead of tag refs
(replace `@v4` with the corresponding immutable SHA for each action) and change
the checkout step (actions/checkout) to include persist-credentials: false to
disable token persistence; ensure any inputs like node-version or pnpm version
remain as before but reference the pinned action SHAs and keep the same
configuration keys (e.g., node-version, cache) when updating the action refs.
- Around line 17-19: The env block in the workflow uses incorrect secret
interpolation (`$ secrets.E2E_BASE_URL` and ` $
secrets.VERCEL_AUTOMATION_BYPASS_SECRET`); update the E2E_BASE_URL and
VERCEL_AUTOMATION_BYPASS_SECRET environment entries to use GitHub Actions secret
syntax `${{ secrets.E2E_BASE_URL }}` and `${{
secrets.VERCEL_AUTOMATION_BYPASS_SECRET }}` so Playwright receives the actual
runtime values.
🪄 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 Plus
Run ID: 341a8511-58d4-4089-8c01-921e7bc85f83
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.github/workflows/e2e-smoke.yml.gitignoree2e/smoke.spec.tspackage.jsonplaywright.config.tssrc/app/login/phone-form.tsxsrc/app/login/verify/verify-form.tsxsrc/components/account/sign-out-button.tsxsrc/components/features/topup-dialog.tsx
✅ Files skipped from review due to trivial changes (3)
- src/components/account/sign-out-button.tsx
- src/app/login/verify/verify-form.tsx
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/smoke.spec.ts
Comment on lines
+9
to
+13
| - uses: actions/checkout@v4 | ||
| - uses: pnpm/action-setup@v4 | ||
| with: { version: 10 } | ||
| - uses: actions/setup-node@v4 | ||
| with: { node-version: 20, cache: pnpm } |
There was a problem hiding this comment.
Harden workflow actions: pin by commit SHA and disable checkout credential persistence.
Line 9/10/12/20 use tag refs (@v4) instead of immutable SHAs, and checkout doesn’t set persist-credentials: false. This weakens CI supply-chain posture.
Suggested hardening pattern
- - uses: actions/checkout@v4
+ - uses: actions/checkout@<full-commit-sha>
+ with:
+ persist-credentials: false
- - uses: pnpm/action-setup@v4
+ - uses: pnpm/action-setup@<full-commit-sha>
- - uses: actions/setup-node@v4
+ - uses: actions/setup-node@<full-commit-sha>
- - uses: actions/upload-artifact@v4
+ - uses: actions/upload-artifact@<full-commit-sha>Also applies to: 20-20
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 9-9: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 9-9: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 10-10: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 12-12: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e-smoke.yml around lines 9 - 13, The workflow uses
mutable action tags and leaves checkout credentials persistent; update the three
actions (actions/checkout, pnpm/action-setup, actions/setup-node) to pinned
commit SHAs instead of tag refs (replace `@v4` with the corresponding immutable
SHA for each action) and change the checkout step (actions/checkout) to include
persist-credentials: false to disable token persistence; ensure any inputs like
node-version or pnpm version remain as before but reference the pinned action
SHAs and keep the same configuration keys (e.g., node-version, cache) when
updating the action refs.
Source: Linters/SAST tools
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XL
Sequence DiagramThis PR adds a DEMO-only backend endpoint that Playwright uses to trigger milestone auto-release by backdating the deadline and calling a privileged Supabase function, gated by environment flags. sequenceDiagram
participant SmokeTest as Smoke Test
participant Backend as Backend API
participant Supabase as Supabase
alt Demo mode or test hooks disabled
SmokeTest->>Backend: POST test auto release
Backend-->>SmokeTest: 404 Not found
else Flags enabled
SmokeTest->>Backend: POST test auto release with milestone id
Backend->>Supabase: Backdate milestone and run auto release function
Supabase-->>Backend: Return released count
Backend-->>SmokeTest: JSON with released count
end
Generated by CodeAnt AI |
| }; | ||
|
|
||
| export function StatusBadge({ variant, className }: StatusBadgeProps) { | ||
| export function StatusBadge({ variant, className, "data-testid": testId }: StatusBadgeProps) { |
There was a problem hiding this comment.
Suggestion: The new test-id forwarding is only applied in the non-null variant branch, so when the status is null/undefined the rendered fallback badge drops the provided data-testid. This breaks the component contract introduced in this PR and can cause flaky or failing selectors in flows where status data is temporarily missing. Pass the test id through in the fallback return path as well. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Fallback badges lack test-id, breaking selector-based tests.
- ⚠️ Playwright smoke flows brittle when status temporarily unavailable.Steps of Reproduction ✅
1. Note that `StatusBadge` is exported from `src/components/ui/status-badge.tsx:80` with
props `{ variant: StatusVariant | null | undefined; "data-testid"?: string }`, meaning
`null`/`undefined` are explicitly supported values for `variant` while still accepting a
test id.
2. In a caller (e.g., a unit test, Storybook story, or future UI code), render the
component with a missing status but a test id:
`render(<StatusBadge variant={null} data-testid="milestone-status-0" />);`
This directly invokes the `if (variant == null)` branch at `status-badge.tsx:81-34`.
3. Inside that branch (`status-badge.tsx:22-33` in the file view), the component returns
`<Badge variant="outline" className={...}>—</Badge>` without passing
`data-testid={testId}`, while the non-null branch (`status-badge.tsx:38-44`) does forward
`data-testid={testId}`.
4. When the rendered output is inspected (for example with Playwright
`page.getByTestId("milestone-status-0")` or React Testing Library
`screen.getByTestId("milestone-status-0")`), the query fails because the fallback badge
DOM node has no `data-testid` attribute despite the prop being provided, breaking the
test-id contract introduced in this PR (see the only current call with a test id at
`src/app/client/jobs/[id]/milestones/client-milestones.tsx:613`).Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/components/ui/status-badge.tsx
**Line:** 80:80
**Comment:**
*Incomplete Implementation: The new test-id forwarding is only applied in the non-null variant branch, so when the status is null/undefined the rendered fallback badge drops the provided `data-testid`. This breaks the component contract introduced in this PR and can cause flaky or failing selectors in flows where status data is temporarily missing. Pass the test id through in the fallback return path as well.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| const result = await requestOtpAction(values); | ||
| if (!result.success) { | ||
| toast.error(result.error ?? "Unable to send OTP."); | ||
| console.error("SEND OTP FAILED:", result.error); |
There was a problem hiding this comment.
Suggestion: Logging result.error in the browser exposes internal auth/provider error details in client-side console output, which is accessible to any user and conflicts with the goal of preventing raw backend text leakage. Remove the raw error payload from client logging or replace it with a sanitized constant message. [security]
Severity Level: Major ⚠️
- ⚠️ Browser console shows raw OTP send error messages.
- ⚠️ Internal auth details exposed in untrusted client environment.Steps of Reproduction ✅
1. Navigate to `/login` where `LoginPage` in `src/app/login/page.tsx:1-8` renders
`<LoginForm />` from `src/app/login/phone-form.tsx:15-18`.
2. Trigger an OTP send failure (for example by causing `supabase.auth.signInWithOtp` to
fail) so that `requestOtpAction` in `src/app/login/actions.ts:12-31` returns `{ success:
false, error: error.message }` at lines 26-27.
3. In `LoginForm`'s submit handler (`src/app/login/phone-form.tsx:25-31`), the failure
branch at lines 28-31 runs with `result.success === false`.
4. Line 29 `console.error("SEND OTP FAILED:", result.error);` logs the raw `result.error`
(directly derived from Supabase `error.message`) into the browser console, making the same
internal authentication/provider details visible to any user who opens developer tools.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/login/phone-form.tsx
**Line:** 29:29
**Comment:**
*Security: Logging `result.error` in the browser exposes internal auth/provider error details in client-side console output, which is accessible to any user and conflicts with the goal of preventing raw backend text leakage. Remove the raw error payload from client logging or replace it with a sanitized constant message.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if (!result.success) { | ||
| toast.error(result.error ?? "Unable to send OTP."); | ||
| console.error("SEND OTP FAILED:", result.error); | ||
| toast.error(result.error ?? "Failed to send OTP."); |
There was a problem hiding this comment.
Suggestion: This displays raw backend error text directly to end users via toast.error(result.error ...). requestOtpAction passes through error.message from Supabase, so internal provider/database details can leak in production UI instead of a sanitized user-facing message. Replace this with a fixed friendly message and keep detailed diagnostics only in controlled server-side logs. [security]
Severity Level: Major ⚠️
- ❌ Login OTP failures expose Supabase error text to users.
- ⚠️ Could reveal internal tables, columns, or provider messages.Steps of Reproduction ✅
1. Navigate to the login page `/login`, which is rendered by `src/app/login/page.tsx:1-8`
and uses `<LoginForm />` from `src/app/login/phone-form.tsx:15-18`.
2. In `LoginForm` (`src/app/login/phone-form.tsx:25-31`), submit the phone number so that
`handleSubmit` calls `requestOtpAction(values)` at line 27.
3. In `requestOtpAction` (`src/app/login/actions.ts:12-31`), the Supabase client created
at line 21 calls `supabase.auth.signInWithOtp` at lines 21-24; if this call fails, it
returns `{ success: false, error: error.message }` at lines 26-27, where `error.message`
comes directly from Supabase.
4. Back in `LoginForm`, the failure branch at `src/app/login/phone-form.tsx:28-31`
executes `toast.error(result.error ?? "Failed to send OTP.");`, which displays the raw
Supabase `error.message` string (potentially containing internal provider/database
details) directly to the end user in the toast.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/login/phone-form.tsx
**Line:** 30:30
**Comment:**
*Security: This displays raw backend error text directly to end users via `toast.error(result.error ...)`. `requestOtpAction` passes through `error.message` from Supabase, so internal provider/database details can leak in production UI instead of a sanitized user-facing message. Replace this with a fixed friendly message and keep detailed diagnostics only in controlled server-side logs.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| startTransition(async () => { | ||
| const result = await verifyOtpAction(values); | ||
| if (!result.success) { | ||
| console.error("OTP FAILED:", result.error); |
There was a problem hiding this comment.
Suggestion: This logs unsanitized OTP verification errors to the browser console. Since verifyOtpAction returns provider/database error.message strings, this can disclose internal implementation details to end users. Log only a sanitized message on the client (or move detailed logging to trusted server logs). [security]
Severity Level: Major ⚠️
- ⚠️ Browser console logs raw OTP verification error messages.
- ⚠️ May leak database or profile lookup failure details.Steps of Reproduction ✅
1. Navigate to `/login/verify?phone=...`, which is handled by `VerifyPage` in
`src/app/login/verify/page.tsx:1-15` and renders `<VerifyOtpForm phone={phone} />` from
`src/app/login/verify/verify-form.tsx:19-29`.
2. In `VerifyOtpForm`, submit the form so that the `submit` handler at
`src/app/login/verify/verify-form.tsx:41-51` calls `verifyOtpAction(values)` at line 43.
3. In `verifyOtpAction` (`src/app/login/actions.ts:51-115`), force an error in any backend
call: `supabase.auth.verifyOtp` at lines 63-67 can set `verifyError.message`,
`supabase.auth.getUser` at 73-80 can set `userError?.message`, or the `profiles` query at
82-90 can set `profileError.message`; each error path returns `{ success: false, error:
<backend message> }` at lines 69-70 or 88-90.
4. Back in `VerifyOtpForm`, when `!result.success` at
`src/app/login/verify/verify-form.tsx:44-47`, line 45 executes `console.error("OTP
FAILED:", result.error);`, logging the raw backend error string (including potential
database/profile implementation details) into the browser console where the end user can
read it.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/login/verify/verify-form.tsx
**Line:** 45:45
**Comment:**
*Security: This logs unsanitized OTP verification errors to the browser console. Since `verifyOtpAction` returns provider/database `error.message` strings, this can disclose internal implementation details to end users. Log only a sanitized message on the client (or move detailed logging to trusted server logs).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+158
to
+160
| // Fund button should be disabled (insufficient balance). | ||
| // The button has disabled attribute when walletBalance < m.amount. | ||
| await expect(page.getByTestId("fund-milestone-0")).toBeDisabled({ timeout: 10_000 }) |
There was a problem hiding this comment.
Suggestion: The test name says it verifies a friendly error toast for insufficient balance, but the implementation only checks that the fund button is disabled and never triggers/asserts the toast path. This means the intended error-display behavior is not actually tested and regressions in user-facing messaging can slip through. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Section D test misses regressions in insufficient-balance toast.
- ⚠️ User-facing error messaging unprotected from accidental changes.Steps of Reproduction ✅
1. The test `"insufficient balance shows a friendly toast, no raw SQL"` is defined at
`e2e/smoke.spec.ts:140-165` under describe block `"D · error display"` (138-166) and
claims in its name to verify a friendly toast.
2. The implementation logs in a client and creates a large-budget job via `postJob()`
(helpers at `e2e/helpers.ts:53-92`), then navigates to `/client/jobs/:id/milestones` at
lines 146-155, asserting only that `milestone-status-0` is visible (156) and that
`fund-milestone-0` is disabled using the snippet at 158-160.
3. The actual user-facing error toast for funding failures is produced in the client
milestones component at
`src/app/client/jobs/[id]/milestones/client-milestones.tsx:239-264`, where `handleFund()`
calls `fundMilestoneAction()` and shows `toast.error(result.error)` on failure (249-251)
and `toast.success("Milestone funded! Funds locked in escrow.")` on success (256-258).
4. Because the D test never clicks the disabled fund button or otherwise invokes
`handleFund()`, no toast path in `client-milestones.tsx` is executed; therefore, any
regression such as removing or changing the toast in `handleFund()` would not affect this
test, which would still pass as long as the button remains disabled and no raw SQL text
appears in `bodyText` (162-164).Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** e2e/smoke.spec.ts
**Line:** 158:160
**Comment:**
*Incomplete Implementation: The test name says it verifies a friendly error toast for insufficient balance, but the implementation only checks that the fund button is disabled and never triggers/asserts the toast path. This means the intended error-display behavior is not actually tested and regressions in user-facing messaging can slip through.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+273
to
+276
| const hookRes = await request.post("/api/test/auto-release", { | ||
| data: { milestoneId }, | ||
| }) | ||
| expect(hookRes.status(), "auto-release hook should return 2xx").toBeLessThan(300) |
There was a problem hiding this comment.
Suggestion: This test uses Playwright's standalone request fixture to call a protected API route, but that fixture does not share the browser session cookies from page. Since your middleware redirects unauthenticated requests, the hook call can be redirected to /login instead of executing, causing flaky/failing behavior when Section F is enabled. Use the authenticated page-bound request context (or explicitly pass auth) for this API call. [api mismatch]
Severity Level: Critical 🚨
- ❌ F · auto-release smoke test fails under E2E_TEST_HOOKS.
- ❌ Auto-release hook route never executes due to auth redirect.
- ⚠️ CI E2E gate unreliable for auto-release behavior.Steps of Reproduction ✅
1. In the Playwright suite `e2e/smoke.spec.ts:215-290`, enable Section F by running tests
with `E2E_TEST_HOOKS=1` so `test.skip` at lines 219-223 does not skip `"backdated
milestone auto-releases"`.
2. The test `F · auto-release` uses the Playwright fixtures `{ request, page }` and
authenticates only the `page` via `login(page, DEMO.client)` at lines 225-227, which sets
Supabase auth cookies in the browser context (see `e2e/helpers.ts:11-23`).
3. The same test later calls `request.post("/api/test/auto-release", { data: { milestoneId
} })` at `e2e/smoke.spec.ts:273-275` using the standalone `request` fixture, which does
not share the browser's cookies or session.
4. Next.js global middleware at `middleware.ts:10-24` runs for all non-public paths
(matcher at 26-27 includes `/api/test/auto-release`), calls `updateSession()`
(`src/lib/supabase/middleware.ts:5-33`), finds no `user` for the cookie-less `request`
context, and redirects unauthenticated calls to `/login` (19-21), causing
`hookRes.status()` at line 276 to be a 3xx redirect instead of the expected <300 success
from the API route, and preventing `src/app/api/test/auto-release/route.ts:24-73` from
executing.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** e2e/smoke.spec.ts
**Line:** 273:276
**Comment:**
*Api Mismatch: This test uses Playwright's standalone `request` fixture to call a protected API route, but that fixture does not share the browser session cookies from `page`. Since your middleware redirects unauthenticated requests, the hook call can be redirected to `/login` instead of executing, causing flaky/failing behavior when Section F is enabled. Use the authenticated page-bound request context (or explicitly pass auth) for this API call.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+24
to
+27
| export async function POST(req: NextRequest) { | ||
| // Double-gate: both flags must be set | ||
| if (!DEMO_MODE || !TEST_HOOKS) { | ||
| return NextResponse.json({ error: "Not found" }, { status: 404 }); |
There was a problem hiding this comment.
Suggestion: This route performs privileged service-role operations but only checks environment flags and does not authorize the caller in-handler. Any authenticated user in demo/test environments can invoke this endpoint to mutate escrow state. Add an explicit server-side auth/role check (or a secret test token) before running the update and RPC. [security]
Severity Level: Critical 🚨
- ❌ Any authenticated demo user can trigger service-role auto-release.
- ❌ Escrow milestones mutated outside intended admin-only controls.
- ⚠️ Demo or staging data integrity weakened during E2E runs.Steps of Reproduction ✅
1. The test hook endpoint `POST /api/test/auto-release` is implemented at
`src/app/api/test/auto-release/route.ts:1-74` and uses a Supabase service-role client
(`createSupabaseClient<Database>(supabaseUrl, serviceRoleKey, ...)` at 48-50) to bypass
RLS and call `auto_release_milestones` with full database privileges.
2. Access control in this route is limited to environment gates at 21-27: `DEMO_MODE` from
`NEXT_PUBLIC_DEMO_MODE` and `TEST_HOOKS` from `E2E_TEST_HOOKS`; when both are enabled, the
function accepts any incoming request and never inspects the caller identity.
3. Global Next.js middleware at `middleware.ts:10-24` applies to all non-public paths via
`matcher` at 26-27 (including `/api/test/auto-release`), authenticates the request via
Supabase (`updateSession()` in `src/lib/supabase/middleware.ts:5-33`), and redirects only
unauthenticated users to `/login` (19-21), meaning any authenticated client, worker, or
admin user can successfully reach this route.
4. With `NEXT_PUBLIC_DEMO_MODE="true"` and `E2E_TEST_HOOKS="1"` set (the configuration
required for the F smoke test in `e2e/smoke.spec.ts:219-223`), logging in through the
regular app (e.g., phone-OTP via `e2e/helpers.ts:11-23`) and then issuing a
`fetch("/api/test/auto-release", { method: "POST", body: JSON.stringify({ milestoneId })
})` from the browser will execute the service-role update and RPC in `route.ts:52-73`
without any additional server-side authorization or role check, allowing any authenticated
user in that environment to mutate escrow state.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/test/auto-release/route.ts
**Line:** 24:27
**Comment:**
*Security: This route performs privileged service-role operations but only checks environment flags and does not authorize the caller in-handler. Any authenticated user in demo/test environments can invoke this endpoint to mutate escrow state. Add an explicit server-side auth/role check (or a secret test token) before running the update and RPC.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+60
to
+70
| if (updateErr) { | ||
| console.error("[test/auto-release] backdate error:", updateErr); | ||
| return NextResponse.json({ error: updateErr.message }, { status: 500 }); | ||
| } | ||
|
|
||
| // 2. Trigger the auto-release cron function | ||
| const { data, error: rpcErr } = await supabase.rpc("auto_release_milestones"); | ||
|
|
||
| if (rpcErr) { | ||
| console.error("[test/auto-release] rpc error:", rpcErr); | ||
| return NextResponse.json({ error: rpcErr.message }, { status: 500 }); |
There was a problem hiding this comment.
Suggestion: Returning raw database error messages to clients leaks backend internals and can expose sensitive implementation details. Return a generic error message to the client and keep detailed database errors only in server logs. [security]
Severity Level: Major ⚠️
- ⚠️ Clients receive raw Postgres/Supabase error strings on failure.
- ⚠️ Internal schema details exposed via test auto-release endpoint.Steps of Reproduction ✅
1. The test-hook route at `src/app/api/test/auto-release/route.ts:24-73` performs a
Supabase `update()` on the `milestones` table (54-58) followed by an RPC call
`supabase.rpc("auto_release_milestones")` at 66 to trigger the cron-like release function.
2. If the update fails (e.g., due to a schema change, connection issue, or backend
constraint error), Supabase returns an `updateErr` object at 54-58; the handler checks `if
(updateErr)` at 60-62 and returns `NextResponse.json({ error: updateErr.message }, {
status: 500 })`, exposing the raw database error message to the client.
3. Similarly, if the RPC fails, Supabase sets `rpcErr` at 66; the handler's branch at
68-70 logs the error and returns `NextResponse.json({ error: rpcErr.message }, { status:
500 })`, again sending the underlying Postgres/Supabase error string back to the caller.
4. When Section F in `e2e/smoke.spec.ts:215-291` or any other caller hits `POST
/api/test/auto-release` and a backend error occurs, the JSON response body will include
these raw `updateErr.message` or `rpcErr.message` strings, leaking internal details such
as schema names, constraint names, or RLS policies instead of a generic user-safe message.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/test/auto-release/route.ts
**Line:** 60:70
**Comment:**
*Security: Returning raw database error messages to clients leaks backend internals and can expose sensitive implementation details. Return a generic error message to the client and keep detailed database errors only in server logs.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
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.
User description
What this adds
An always-on Playwright smoke test that re-verifies the core money path. Durable version of the CI-gate spec.
Coverage
-never writes NaNauto_release_atand runsauto_release_milestones()(double-gated, see below)Local result: 9 passed / 2 skipped by design (delete-account uses a disposable account; F skips unless
E2E_TEST_HOOKS=1).pnpm typecheckclean,pnpm lint0 errors.Safety
DEMO_MODE. No real auth/rate-limit was weakened.POST /api/test/auto-releaseis double-gated — refuses to run unlessNEXT_PUBLIC_DEMO_MODE=trueandE2E_TEST_HOOKS=1. Uses the service-role key only behind those gates.What's NOT in this PR (intentional, separate)
topup_walletambiguous-column bug-fix migration — gets its own money-RPC PR..github/workflows/e2e-smoke.yml) — added once theE2E_BASE_URLpreview secret is set.testids added
Wiring-only
data-testids across login, wallet, post-job wizard, job detail, and milestone components;StatusBadgenow forwardsdata-testid.Do not merge without a green review + human sign-off.
Summary by CodeRabbit
Tests
Chores
Test-only tools
CodeAnt-AI Description
Add a Playwright smoke suite for the core money flow and login paths
What Changed
Impact
✅ Fewer checkout-time money flow regressions✅ Clearer wallet and escrow failure handling✅ Earlier detection of broken login or milestone steps💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.