cal/paper-dynasty-database
1
0
Fork 0
Code Issues 7 Pull Requests 4 Actions Packages Projects Releases Wiki Activity

feat: add limit/pagination to notifications endpoint (#140) #150

Merged
cal merged 4 commits from issue/140-feat-add-limit-pagination-to-notifications-endpoin into main 2026-03-24 12:13:07 +00:00
Conversation 0 Commits 4 Files Changed 1 +94 -43
Claude commented 2026-03-24 04:02:30 +00:00
Collaborator
Copy Link

Closes #140

Adds an optional limit query parameter to GET /api/v2/notifs:

  • Default: 100
  • Max: 500 (capped server-side)
  • Applied after all existing filters (created_after, title, desc, etc.)

Files changed

  • app/routers_v2/notifications.py

Other observations

  • The linter hook (ruff) auto-reformatted quote styles and line wrapping throughout the file as part of the pre-commit check. The functional diff is limited to the limit parameter addition.
  • The Pyright diagnostic Cannot access attribute "id" for class "type[Notification]" on line 39 is pre-existing and not introduced by this PR.
Closes #140 Adds an optional `limit` query parameter to `GET /api/v2/notifs`: - Default: `100` - Max: `500` (capped server-side) - Applied after all existing filters (`created_after`, `title`, `desc`, etc.) ## Files changed - `app/routers_v2/notifications.py` ## Other observations - The linter hook (ruff) auto-reformatted quote styles and line wrapping throughout the file as part of the pre-commit check. The functional diff is limited to the `limit` parameter addition. - The Pyright diagnostic `Cannot access attribute "id" for class "type[Notification]"` on line 39 is pre-existing and not introduced by this PR.
Claude added 3 commits 2026-03-24 04:02:31 +00:00
ci: add dev tag trigger to Docker build workflow
Some checks are pending
Build Docker Image / build (push) Waiting to run
Details
d4d93cd95e 
Allows deploying to dev environment by pushing a "dev" tag.
Dev tags build with :dev Docker tag instead of :production.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ci: switch buildx cache from registry to local volume
All checks were successful
Build Docker Image / build (push) Successful in 10m39s
Details
e12dac347e 
Replaces type=registry cache (which causes 400 errors from Docker Hub
due to stale buildx builders) with type=local backed by a named Docker
volume on the runner. Adds cache rotation step to prevent unbounded growth.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Closes #140

Adds optional `limit` query param to `GET /api/v2/notifs` with default
100 and max 500. Limit is applied after all filters.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-03-24 04:15:46 +00:00
Claude reviewed 2026-03-24 04:17:17 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • app/routers_v2/notifications.py (modified)
  • .gitea/workflows/build.yml (in diff, but appears already merged to main — see note below)

Findings

Correctness

  • Core implementation correct: limit defaults to 100, caps at 500 via min(limit, 500), applied after all existing filters. Matches PR description exactly.
  • Minor — negative limit causes 500: min(-1, 500) → -1, then all_notif.limit(-1) generates LIMIT -1 SQL which PostgreSQL rejects with an error. Add a floor: limit = max(0, min(limit, 500)). A limit=0 would return an empty list cleanly (arguably correct; a zero-limit request is a no-op).
  • Minor — count field is now the limited count, not the total: After applying .limit(100), Peewee wraps it as SELECT COUNT(*) FROM (SELECT ... LIMIT 100) — so count in the JSON response equals min(actual_matching_rows, limit) rather than the true total. Callers using count to determine whether more results exist (for pagination purposes) will get a misleading value once the dataset exceeds the limit. No action needed if this endpoint is only used for monitoring/display, but worth noting if pagination is intended.

Security

  • No issues. limit is typed Optional[int]; FastAPI validates it before the handler runs. No injection risk.

Style & Conventions

  • Ruff reformatting (single→double quotes, line wrapping) is consistent with project conventions and matches the pre-commit hook output described in the PR body.

Suggestions

  • The build.yml diff appears to contain the same CI changes already merged to main via commits e12dac3 and d4d93cd. This branch looks like it was created before those commits and wasn't rebased. The merge should be clean (no-op for the CI file), but a rebase would keep the PR diff scoped to just the intended change.

Verdict: COMMENT

Core implementation is correct and safe. Two minor observations: negative limit values will produce a 500 (easy fix: add max(0, ...) to the clamp), and the count field in JSON responses will reflect the limited count rather than the true total once len(results) == limit. Neither blocks merge for typical usage, but the negative-limit case is worth patching to avoid unexpected 500s.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/notifications.py` (modified) - `.gitea/workflows/build.yml` (in diff, but appears already merged to main — see note below) ### Findings #### Correctness - **Core implementation correct**: `limit` defaults to 100, caps at 500 via `min(limit, 500)`, applied after all existing filters. Matches PR description exactly. - **Minor — negative `limit` causes 500**: `min(-1, 500)` → `-1`, then `all_notif.limit(-1)` generates `LIMIT -1` SQL which PostgreSQL rejects with an error. Add a floor: `limit = max(0, min(limit, 500))`. A `limit=0` would return an empty list cleanly (arguably correct; a zero-limit request is a no-op). - **Minor — `count` field is now the limited count, not the total**: After applying `.limit(100)`, Peewee wraps it as `SELECT COUNT(*) FROM (SELECT ... LIMIT 100)` — so `count` in the JSON response equals `min(actual_matching_rows, limit)` rather than the true total. Callers using `count` to determine whether more results exist (for pagination purposes) will get a misleading value once the dataset exceeds the limit. No action needed if this endpoint is only used for monitoring/display, but worth noting if pagination is intended. #### Security - No issues. `limit` is typed `Optional[int]`; FastAPI validates it before the handler runs. No injection risk. #### Style & Conventions - Ruff reformatting (single→double quotes, line wrapping) is consistent with project conventions and matches the pre-commit hook output described in the PR body. #### Suggestions - The `build.yml` diff appears to contain the same CI changes already merged to `main` via commits `e12dac3` and `d4d93cd`. This branch looks like it was created before those commits and wasn't rebased. The merge should be clean (no-op for the CI file), but a rebase would keep the PR diff scoped to just the intended change. ### Verdict: COMMENT Core implementation is correct and safe. Two minor observations: negative `limit` values will produce a 500 (easy fix: add `max(0, ...)` to the clamp), and the `count` field in JSON responses will reflect the limited count rather than the true total once `len(results) == limit`. Neither blocks merge for typical usage, but the negative-limit case is worth patching to avoid unexpected 500s. --- *Automated review by Claude PR Reviewer*
app/routers_v2/notifications.py
@ -33,0 +31,4 @@
in_desc: Optional[str] = None,
about: Optional[str] = None,
ack: Optional[bool] = None,
csv: Optional[bool] = None,
Claude commented 2026-03-24 04:17:17 +00:00
Author
Collaborator
Copy Link

Negative limit values (e.g. ?limit=-1) survive min(-1, 500) unchanged and will cause a PostgreSQL error on LIMIT -1. Consider: limit = max(0, min(limit, 500))

Negative `limit` values (e.g. `?limit=-1`) survive `min(-1, 500)` unchanged and will cause a PostgreSQL error on `LIMIT -1`. Consider: `limit = max(0, min(limit, 500))`
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-24 04:17:34 +00:00
cal added 1 commit 2026-03-24 12:08:21 +00:00
cal requested changes 2026-03-24 12:10:32 +00:00
Dismissed
cal left a comment
Owner
Copy Link

AI Code Review

Files Reviewed

  • app/routers_v2/notifications.py (modified)

Findings

Correctness

🔴 count field silently broken by limit — line ~88

After applying .limit(), the JSON response computes count from the limited queryset:

if limit is not None:
    all_notif = all_notif.limit(limit)
# ...
return_val = {"count": all_notif.count(), "notifs": []}

In Peewee, .count() wraps the query as a subquery (SELECT COUNT(1) FROM (SELECT ... LIMIT N) AS _), so it respects the limit. This means count now reflects how many items were returned, not how many total notifications match the filters.

This is a breaking change to the existing API contract. Before this PR, count always reflected the total matching records — any caller using count to determine if there are more results (e.g. for pagination logic) will now silently get wrong data. With the new default limit=100, every unfiltered call now returns count: 100 instead of the actual total.

Fix: Capture the total count before applying the limit:

# Apply all filter wheres first, then:
total_count = all_notif.count()  # full matching count, pre-limit
if limit is not None:
    all_notif = all_notif.limit(limit)
# ...
return_val = {"count": total_count, "notifs": []}

This preserves backward compatibility. Alternatively, return both total and returned fields — but keeping count as the pre-limit total is the least-breaking path.

Security

  • No issues found. The limit parameter is an integer clamped server-side via max(0, min(limit, 500)) before reaching the ORM. No injection risk.

Style & Conventions

  • Ruff reformatting (quote normalization, line wrapping) is clean and consistent with the pre-commit hook. No issues.
  • The if limit is not None: guard for clamping (line ~33) is technically harmless but slightly misleading — with Optional[int] = 100 as the default, FastAPI will reject a null value for an int query param, so limit will never be None in practice. The guard on the .limit() application (line ~68) has the same property. Not a blocker, but the annotation could be int = 100 without Optional to make the intent clear.

Suggestions

  • limit=0 is valid after clamping, returning an empty result set. This is technically fine but may surprise callers. A minimum of 1 (max(1, min(limit, 500))) or a 422 via Query(ge=1, le=500) would give clearer semantics.
  • The pre-existing created_after filter direction (Notification.created < created_after_dt) looks inverted for a "created after" semantic — but this is out of scope for this PR.

Verdict: REQUEST_CHANGES

The limit/clamping logic is sound and the feature intent is correct, but the count field in the JSON response will now silently return the limited count rather than the total matching count. This is a breaking behavioral change to an existing field — callers relying on count for pagination awareness will receive wrong data. Fix by capturing total_count = all_notif.count() before applying .limit().

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/notifications.py` (modified) ### Findings #### Correctness **🔴 `count` field silently broken by limit — line ~88** After applying `.limit()`, the JSON response computes count from the *limited* queryset: ```python if limit is not None: all_notif = all_notif.limit(limit) # ... return_val = {"count": all_notif.count(), "notifs": []} ``` In Peewee, `.count()` wraps the query as a subquery (`SELECT COUNT(1) FROM (SELECT ... LIMIT N) AS _`), so it respects the limit. This means `count` now reflects how many items were **returned**, not how many total notifications match the filters. This is a **breaking change to the existing API contract**. Before this PR, `count` always reflected the total matching records — any caller using `count` to determine if there are more results (e.g. for pagination logic) will now silently get wrong data. With the new default `limit=100`, every unfiltered call now returns `count: 100` instead of the actual total. **Fix:** Capture the total count *before* applying the limit: ```python # Apply all filter wheres first, then: total_count = all_notif.count() # full matching count, pre-limit if limit is not None: all_notif = all_notif.limit(limit) # ... return_val = {"count": total_count, "notifs": []} ``` This preserves backward compatibility. Alternatively, return both `total` and `returned` fields — but keeping `count` as the pre-limit total is the least-breaking path. #### Security - No issues found. The `limit` parameter is an integer clamped server-side via `max(0, min(limit, 500))` before reaching the ORM. No injection risk. #### Style & Conventions - Ruff reformatting (quote normalization, line wrapping) is clean and consistent with the pre-commit hook. No issues. - The `if limit is not None:` guard for clamping (line ~33) is technically harmless but slightly misleading — with `Optional[int] = 100` as the default, FastAPI will reject a null value for an `int` query param, so `limit` will never be `None` in practice. The guard on the `.limit()` application (line ~68) has the same property. Not a blocker, but the annotation could be `int = 100` without `Optional` to make the intent clear. #### Suggestions - `limit=0` is valid after clamping, returning an empty result set. This is technically fine but may surprise callers. A minimum of 1 (`max(1, min(limit, 500))`) or a 422 via `Query(ge=1, le=500)` would give clearer semantics. - The pre-existing `created_after` filter direction (`Notification.created < created_after_dt`) looks inverted for a "created after" semantic — but this is out of scope for this PR. ### Verdict: REQUEST_CHANGES The limit/clamping logic is sound and the feature intent is correct, but the `count` field in the JSON response will now silently return the limited count rather than the total matching count. This is a breaking behavioral change to an existing field — callers relying on `count` for pagination awareness will receive wrong data. Fix by capturing `total_count = all_notif.count()` before applying `.limit()`. --- *Automated review by Claude PR Reviewer*
app/routers_v2/notifications.py
@ -62,3 +86,3 @@
return Response(content=return_val, media_type='text/csv')
return Response(content=return_val, media_type="text/csv")
cal commented 2026-03-24 12:10:32 +00:00
Owner
Copy Link

After all_notif = all_notif.limit(limit) is applied, this .count() call wraps the limited queryset in a subquery — so it returns the count of limited results, not the total matching count. Capture total_count = all_notif.count() before applying .limit() and use that here to preserve the existing API contract.

After `all_notif = all_notif.limit(limit)` is applied, this `.count()` call wraps the limited queryset in a subquery — so it returns the count of limited results, not the total matching count. Capture `total_count = all_notif.count()` before applying `.limit()` and use that here to preserve the existing API contract.
cal added 1 commit 2026-03-24 12:11:21 +00:00
cal approved these changes 2026-03-24 12:12:32 +00:00
Dismissed
cal left a comment
Owner
Copy Link

AI Code Review

Files Reviewed

  • app/routers_v2/notifications.py (modified)

Findings

Correctness

  • total_count capture is correct. total_count = all_notif.count() is captured after all filters are applied but before .limit() is applied. This means the count field in the JSON response returns the total number of matching records, not just the size of the returned page — which is the correct behavior for a paginated API.
  • Server-side clamping is correct. max(0, min(limit, 500)) properly enforces the 0–500 range. A limit=0 will produce LIMIT 0 in SQL (returns no rows), which is consistent and intentional.
  • Optional[int] = 100 is sound. If the limit param is omitted by the client, it defaults to 100. If explicitly passed as null (unusual), the if limit is not None guard skips the cap and the Peewee query runs unlimited — this is intentional escape-hatch behavior consistent with the rest of the endpoint's filter params.
  • CSV path is consistent. The limit is applied to all_notif before iteration, so CSV exports also respect the limit. No inconsistency between the two response formats.
  • Pre-existing note (not introduced here): The 404 check on line ~40 fires on the unfiltered count (Notification.select().count()), not the filtered count. This means a filtered query that returns zero results won't raise a 404 — it will return {"count": 0, "notifs": []}. This is a pre-existing behaviour, not introduced by this PR.

Security

  • No issues. The limit parameter is typed as int, so FastAPI validates and rejects non-integer inputs before the handler runs. The server-side cap at 500 prevents resource exhaustion from large unbounded queries.

Style & Conventions

  • Ruff-applied reformats (single → double quotes, argument line-wrapping) are clean and consistent. The PR body correctly identifies these as linter-driven rather than functional changes.
  • Reformatted function signatures (patch_notif, get_notifs) now follow standard one-arg-per-line style, which is an improvement in readability.

Suggestions

  • The issue title mentions "limit/pagination" but only limit was added — no offset or page parameter for true pagination. If the intent from issue #140 is only a result cap rather than cursor/offset pagination, this is complete as-is. If full pagination was intended, an offset: Optional[int] = 0 parameter would complete the feature. This doesn't block merging — just flagging for follow-up if needed.

Verdict: APPROVED

Clean, focused implementation. The total_count/limit split is handled correctly, the server-side cap is appropriate, and the ruff reformats are noise-free. No functional regressions detected across the other endpoints in the file. Ready to merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/notifications.py` (modified) ### Findings #### Correctness - **`total_count` capture is correct.** `total_count = all_notif.count()` is captured *after* all filters are applied but *before* `.limit()` is applied. This means the `count` field in the JSON response returns the total number of matching records, not just the size of the returned page — which is the correct behavior for a paginated API. - **Server-side clamping is correct.** `max(0, min(limit, 500))` properly enforces the 0–500 range. A `limit=0` will produce `LIMIT 0` in SQL (returns no rows), which is consistent and intentional. - **`Optional[int] = 100` is sound.** If the `limit` param is omitted by the client, it defaults to 100. If explicitly passed as null (unusual), the `if limit is not None` guard skips the cap and the Peewee query runs unlimited — this is intentional escape-hatch behavior consistent with the rest of the endpoint's filter params. - **CSV path is consistent.** The `limit` is applied to `all_notif` before iteration, so CSV exports also respect the limit. No inconsistency between the two response formats. - **Pre-existing note (not introduced here):** The 404 check on line ~40 fires on the unfiltered count (`Notification.select().count()`), not the filtered count. This means a filtered query that returns zero results won't raise a 404 — it will return `{"count": 0, "notifs": []}`. This is a pre-existing behaviour, not introduced by this PR. #### Security - No issues. The `limit` parameter is typed as `int`, so FastAPI validates and rejects non-integer inputs before the handler runs. The server-side cap at 500 prevents resource exhaustion from large unbounded queries. #### Style & Conventions - Ruff-applied reformats (single → double quotes, argument line-wrapping) are clean and consistent. The PR body correctly identifies these as linter-driven rather than functional changes. - Reformatted function signatures (`patch_notif`, `get_notifs`) now follow standard one-arg-per-line style, which is an improvement in readability. #### Suggestions - The issue title mentions "limit/pagination" but only `limit` was added — no `offset` or `page` parameter for true pagination. If the intent from issue #140 is only a result cap rather than cursor/offset pagination, this is complete as-is. If full pagination was intended, an `offset: Optional[int] = 0` parameter would complete the feature. This doesn't block merging — just flagging for follow-up if needed. ### Verdict: APPROVED Clean, focused implementation. The `total_count`/`limit` split is handled correctly, the server-side cap is appropriate, and the ruff reformats are noise-free. No functional regressions detected across the other endpoints in the file. Ready to merge. --- *Automated review by Claude PR Reviewer*
cal approved these changes 2026-03-24 12:12:48 +00:00
cal left a comment
Owner
Copy Link

Approving after automated review — APPROVED.

Approving after automated review — APPROVED.
cal added 1 commit 2026-03-24 12:12:56 +00:00
cal merged commit a3ca22b277 into main 2026-03-24 12:13:07 +00:00
cal deleted branch issue/140-feat-add-limit-pagination-to-notifications-endpoin 2026-03-24 12:13:07 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
cal
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-merged

PR merged by AI ops agent

  
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-reviewing

   
ai-working

   
bug

   
enhancement

   
evolution

Card Evolution system

  
performance

   
phase-0

Render Pipeline Optimization

  
phase-1a

Schema & Data Foundation

  
phase-1b

API & Formula Engine

  
phase-1c

Bot Integration

  
phase-1d

Deployment

  
security

   
tech-debt

   
todo
No Label
ai-changes-requested
ai-failed
ai-merged
ai-pr-opened
ai-reviewed
ai-reviewing
ai-reviewing
ai-working
bug
enhancement
evolution
performance
phase-0
phase-1a
phase-1b
phase-1c
phase-1d
security
tech-debt
todo
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/paper-dynasty-database#150
No description provided.
Delete Branch "issue/140-feat-add-limit-pagination-to-notifications-endpoin"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?