Skip to content

JMCP: Papers - jmcp/make-the-date-cutoff-a-query-parameter-UqhwXRms#40

Closed
landigf wants to merge 1 commit intomainfrom
jmcp/make-the-date-cutoff-a-query-parameter-UqhwXRms
Closed

JMCP: Papers - jmcp/make-the-date-cutoff-a-query-parameter-UqhwXRms#40
landigf wants to merge 1 commit intomainfrom
jmcp/make-the-date-cutoff-a-query-parameter-UqhwXRms

Conversation

@landigf
Copy link
Copy Markdown
Owner

@landigf landigf commented Mar 25, 2026

Here's the review:


Review: Make date cutoff a query parameter

Change summary

  • apps/web/app/feed/page.tsx: Adds since?: string to searchParams, passes it through to repository.getFeed().
  • packages/db/src/index.ts: Extends the getFeed interface and DemoRepository implementation to accept since, filtering out papers with createdAt before that date.
  • packages/auth/package.json / package-lock.json: Bumps drizzle-orm from ^0.44.5 to ^0.45.1 (removes duplicate nested install). Unrelated housekeeping.

Validation confidence: High

The change is small, mechanical, and follows the existing pattern (same approach as the query param). The filtering logic is straightforward.

Risks

  1. No validation on since value. If a user passes ?since=garbage, new Date("garbage").getTime() returns NaN, and NaN < sinceMs is always false, so the filter silently passes everything through — effectively a no-op. This is safe but could surprise callers expecting an error. Consider adding a guard (if (sinceMs && !Number.isNaN(sinceMs))).

  2. Only the DemoRepository is patched. If there's a production/SQL implementation of PapersRepository, it won't filter by since until updated. The interface is updated, so TypeScript will catch this at compile time — but confirm npm run check passes.

  3. Drizzle bump is unrelated. Mixing dependency bumps with feature changes makes rollback harder. Low risk but worth noting.

  4. No tests added. A unit test for the since filter on DemoRepository.getFeed would be cheap insurance, especially given the NaN edge case above.

@landigf
Copy link
Copy Markdown
Owner Author

landigf commented Mar 25, 2026

Closing: will integrate valuable features directly on main to avoid cascade merge conflicts.

@landigf landigf closed this Mar 25, 2026
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.

1 participant