Normalize gig pagination params#397
Conversation
f691d14 to
5fa8835
Compare
Greptile SummaryThis PR replaces the inline
Confidence Score: 4/5Safe to merge; the normalization logic is correct for all tested and traced edge cases, and the Supabase range arguments remain bounded integers. The implementation correctly handles every meaningful edge case — null, whitespace, NaN, Infinity, fractions, negatives, zero, and oversized values — and the two new tests confirm the key paths. The only gap is that explicit page=0 / limit=0 input (a finite, non-NaN value that still gets clamped to 1 via Math.max) has no dedicated test, leaving that specific clamping path unverified. Both changed files are low risk; route.test.ts would benefit from adding a page=0 / limit=0 test case. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["searchParams.get('page' or 'limit')"] --> B{value null or whitespace?}
B -- "yes" --> C["use defaultValue (1 or 20)"]
B -- "no" --> D["Number(value)"]
D --> E{isFinite?}
E -- "no (NaN, Infinity)" --> C
E -- "yes" --> F["Math.trunc(parsed)"]
C --> F
F --> G["Math.max(1, truncated)"]
G --> H["Math.min(result, MAX_GIG_PAGE or MAX_GIG_LIMIT)"]
H --> I["integer in [1, max]"]
I --> J["offset = (page-1) * limit"]
J --> K["supabase.range(offset, offset + limit - 1)"]
Reviews (1): Last reviewed commit: "Normalize gig pagination params" | Re-trigger Greptile |
| it("truncates fractional page and limit values before querying", async () => { | ||
| const chain = chainResult({ data: null, error: null }); | ||
| chain.select = vi.fn().mockReturnValue(chain); | ||
| chain.range = vi.fn().mockResolvedValue({ data: [], error: null, count: 30 }); | ||
|
|
||
| mockFrom.mockReturnValue(chain); | ||
|
|
||
| const res = await GET(makeGetRequest({ page: "2.9", limit: "5.9" })); | ||
| const json = await res.json(); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| expect(chain.range).toHaveBeenCalledWith(5, 9); | ||
| expect(json.pagination.page).toBe(2); | ||
| expect(json.pagination.limit).toBe(5); | ||
| expect(json.pagination.totalPages).toBe(6); | ||
| }); | ||
|
|
||
| it("defaults invalid and non-positive pagination values before querying", async () => { | ||
| const chain = chainResult({ data: null, error: null }); | ||
| chain.select = vi.fn().mockReturnValue(chain); | ||
| chain.range = vi.fn().mockResolvedValue({ data: [], error: null, count: 0 }); | ||
|
|
||
| mockFrom.mockReturnValue(chain); | ||
|
|
||
| const res = await GET(makeGetRequest({ page: "-10", limit: "abc" })); | ||
| const json = await res.json(); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| expect(chain.range).toHaveBeenCalledWith(0, 19); | ||
| expect(json.pagination.page).toBe(1); | ||
| expect(json.pagination.limit).toBe(20); | ||
| }); |
There was a problem hiding this comment.
Missing test for explicit zero input clamping
The two new tests cover fractions and non-positive strings, but neither passes page=0 or limit=0 explicitly. Number("0") = 0 is finite and non-NaN, so the fallback to defaultValue is skipped — the value travels all the way to Math.max(1, Math.trunc(0)), which correctly returns 1. That path isn't exercised by any existing test, leaving a gap in the zero-input contract. A test asserting range(0, 19) when page="0" (and default limit=20) would close it.
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!
|
Closing in favor of a new PR that includes the bounties test fix from master. The gig pagination changes are correct and will be re-submitted. |
Normalizes /api/gigs pagination params before validation and querying. Fractional page/limit values are now truncated to integers, invalid values default safely, and the Supabase range plus pagination metadata stay integer and bounded.
Validation: