Skip to content

[codex] Clamp popular tag limits#382

Merged
ralyodio merged 1 commit into
profullstack:masterfrom
jsdavid278-cyber:codex/popular-tags-limit
Jun 4, 2026
Merged

[codex] Clamp popular tag limits#382
ralyodio merged 1 commit into
profullstack:masterfrom
jsdavid278-cyber:codex/popular-tags-limit

Conversation

@jsdavid278-cyber
Copy link
Copy Markdown
Contributor

Fixes #360.

Summary:

  • replace the ad hoc popular-tags limit parser with the shared parsePaginationParam helper
  • make limit=0 clamp to the documented minimum of 1 instead of expanding to the default
  • make malformed numeric strings fall back to the default limit instead of being partially accepted
  • add focused route coverage for zero and malformed limits

Validation:

  • vitest run src/app/api/tags/popular/route.test.ts
  • tsc --noEmit

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Jun 4, 2026

Greptile Summary

This PR refactors the popular-tags limit parsing to use the shared parsePaginationParam helper, fixing two edge cases: limit=0 now correctly clamps to 1, and malformed strings like "7px" now fall back to the default (50) instead of being partially parsed via parseInt.

  • route.ts: Three-expression ad-hoc clamp replaced by a single parsePaginationParam call — the logic change is correct and well-scoped.
  • route.test.ts: Adds two new tests; the zero-limit test is effective, but the malformed-string test uses only 4 mock tags, which is fewer than the old partial-parse result of 7, so slice(0,7) and slice(0,50) produce identical output and the assertion cannot detect a regression.

Confidence Score: 4/5

The route change itself is correct and safe to merge; the only concern is in the new test file.

The malformed-limit test uses a mock dataset too small to distinguish the old partial-parse result (7) from the new default (50). This means the test passes even if the old buggy path is restored, leaving the 'malformed numeric strings fall back to default' guarantee unverified. Everything else — the helper delegation, zero clamping, and zero-limit test — is solid.

src/app/api/tags/popular/route.test.ts — the malformed-limit test needs a larger mock dataset to be meaningful.

Important Files Changed

Filename Overview
src/app/api/tags/popular/route.ts Replaces ad-hoc limit parser with shared parsePaginationParam helper; logic is correct and behavior improvements (zero-clamping, NaN fallback) are accurate.
src/app/api/tags/popular/route.test.ts New test file adds coverage for zero and malformed limits; the zero-limit test is correct, but the malformed-limit test uses too few mock tags (4) to distinguish old (limit=7) from new (limit=50) behavior, making it a false-positive guard.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[GET /api/tags/popular] --> B[searchParams.get limit]
    B --> C{parsePaginationParam}
    C -- null or empty --> D[default = 50]
    C -- NaN / non-finite --> D
    C -- valid number --> E[Math.trunc, clamp 1..200]
    D --> F[limit = 50]
    E --> G[limit = clamped value]
    F --> H[Fetch active gigs]
    G --> H
    H --> I[Build gigCountMap]
    I --> J[Fetch tag_follows]
    J --> K[Build followerCountMap]
    K --> L[Merge allTags Set]
    L --> M[Sort by follower_count desc, gig_count desc]
    M --> N[slice 0 .. limit]
    N --> O[NextResponse.json tags]
Loading

Reviews (1): Last reviewed commit: "Clamp popular tag limits" | Re-trigger Greptile

Comment on lines +63 to +70
expect(json.tags).toHaveLength(1);
expect(json.tags[0].tag).toBe("API");
});

it("uses the default limit for malformed numeric strings", async () => {
mockPopularTagsData();

const response = await GET(makeRequest({ limit: "7px" }));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Malformed-limit test doesn't distinguish old from new behavior

The mock supplies only 4 distinct tags, but the key difference between the old code (parseInt("7px", 10)7) and the new code (Number("7px")NaN → default 50) only becomes observable when there are more than 7 tags. With 4 tags, slice(0, 7) and slice(0, 50) both return the same 4 elements, so expect(json.tags).toHaveLength(4) passes regardless of which implementation is active. Reverting parsePaginationParam to the old ad-hoc expression would leave this test green while silently reintroducing the bug. The mock data needs at least 8 unique tags to make the assertion meaningful.

@ralyodio ralyodio merged commit 7c57621 into profullstack:master Jun 4, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clamp zero popular-tags limits instead of expanding to the default

2 participants