Return 400 for malformed notification settings JSON#469
Conversation
Greptile SummaryAdds a
Confidence Score: 4/5Safe to merge — the core fix is correct and well-scoped, with no risk of regressions to authenticated or happy-path flows. The The test file could benefit from additional cases covering valid-but-non-object JSON bodies and the happy path. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant PUT as PUT /api/notification-settings
participant Auth as getAuthContext
participant readJson as readJsonObject
participant DB as Supabase
Client->>PUT: PUT with JSON body
PUT->>Auth: getAuthContext(request)
Auth-->>PUT: null (unauthenticated)
PUT-->>Client: 401 Unauthorized
Client->>PUT: PUT with JSON body
PUT->>Auth: getAuthContext(request)
Auth-->>PUT: "{ user, supabase }"
PUT->>readJson: readJsonObject(request)
Note over readJson: request.json() throws OR result is not a plain object
readJson-->>PUT: null
PUT-->>Client: 400 Invalid JSON body
Client->>PUT: PUT with valid object body
PUT->>Auth: getAuthContext(request)
Auth-->>PUT: "{ user, supabase }"
PUT->>readJson: readJsonObject(request)
readJson-->>PUT: "Record<string, unknown>"
PUT->>DB: supabase.upsert(...)
DB-->>PUT: "{ data, error }"
PUT-->>Client: "200 { data } or 400 { error }"
Reviews (1): Last reviewed commit: "Handle malformed notification settings J..." | Re-trigger Greptile |
| it("returns 400 for malformed JSON before touching settings", async () => { | ||
| const response = await PUT(makePutRequest("{not valid json")); | ||
| const body = await response.json(); | ||
|
|
||
| expect(response.status).toBe(400); | ||
| expect(body.error).toBe("Invalid JSON body"); | ||
| expect(mocks.mockFrom).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Non-object JSON paths untested
readJsonObject has two distinct branches that both return null: a JSON parse failure and successfully-parsed JSON that is not an object (array, null, a string, a number). The single test only exercises the parse-failure path. Sending "null" or "[]" hits the !body || typeof body !== "object" || Array.isArray(body) guard and returns the same 400, but neither case is covered. If that guard were ever accidentally dropped, the upsert code would receive an unexpected value and these paths would silently regress.
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!
|
CI is green for PR #469. Verification:
uGig invoice evidence has been sent for this PR. |
Fixes #468.
Changes
Verification
corepack pnpm vitest run src/app/api/notification-settings/route.test.tscorepack pnpm tsc --noEmit