Claude: changes for issue #85 #88

Merged
Grandiras merged 2 commits from claude/issue-85 into main 2026-08-14 18:33:50 +00:00
Collaborator

Closes #85.

Closes #85.
Add a general calendar over every board a member can see
All checks were successful
CI / build (pull_request) Successful in 30s
3fddfd8e7d
/calendar/all is the same calendar page over all of them. A board is what
the app is organised by, but a due date is not — the card due on Tuesday is
due on Tuesday whichever board it is on, and reading five calendars to find
that out was the gap this closes.

The same component, not a second one: the month grid, the timeline, the
swipe and the entrance are all shared, and the scope decides only which
cards are loaded, what the legend's chips stand for and what the .ics links
ask for. It is read off the path, because /calendar/all is a literal segment
and cannot arrive as a route parameter — but it changes under a reused
component exactly as BoardId does, so it is handled beside it.

The category here is the board rather than the tag (CalendarTimeline.
BoardLanes, and the legend renders the boards): tags are per board, so five
boards' worth are five vocabularies that do not line up, and "which board is
this card from" is the question this view exists to answer and a tag cannot.
That keeps "the legend is the filter" true in both scopes. A board's colour
is derived from its id (BoardPalette) rather than stored — one view wanting
a colour is not a migration and an admin control — so two boards can share
one, and the legend, the lane and the tooltip name the board for that reason.

The board query on both .ics endpoints now carries the scope too
(CalendarScope): absent is the default board as before, `all` is every board
the viewer can see and stays dynamic, and `3,7` is exactly those. A named
board that has gone takes the whole feed with it rather than being dropped
from it, and a value that is none of those shapes is a 404 — the same
refusal of quiet substitution the single-board case already made.

Also fixes a pre-existing crash on /admin/boards: ReloadAsync published the
board list before _edits was filled, and ComponentBase renders after the
first await in OnInitializedAsync, so a slow second query made
_edits[board.Id] a KeyNotFoundException out of the prerender.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Collaborator

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/233); the reply will link the full session transcript. 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-88/run-233.md) — updating every ~5 minutes while the task runs.
claude-reviewer left a comment

💬 Verdict: comment — notes only, nothing blocking.

This is a large, carefully executed feature (the general /calendar/all view) that follows the codebase's existing conventions closely: access control is re-checked through BoardService.GetVisibleAsync/ResolveAsync exactly as the single-board calendar does, encryption/plaintext boundaries are respected, translations are added in both locales, the board-vs-tag filter split is modeled with a separate HashSet/Category abstraction to avoid id collisions, NavHeader's active-section/active-board logic is extended consistently, and there's substantial unit + e2e test coverage (CalendarScopeTests, new BoardLanes/MatchesBoard tests, calendar-all.spec.js). The Boards.razor fix bundled in (loading into locals before assigning _boards/_access/_edits together) is a legitimate, well-explained bugfix for a prerender race.

I didn't find correctness bugs in the parsing, filtering, or ICS-building logic — CalendarScope.Parse matches its tests, BuildIcsAsync's three scope branches all funnel through viewer-checked resolution, and the Filtered/NoFilter refactor in Calendar.razor looks like an equivalent restatement of the previous inline filtering rather than a behavior change.

One moderate, non-blocking concern below about unbounded board-id lists in the board= query parameter causing many sequential DB round trips per request.

🔧 Let Claude fix this — opens a prefilled task; press Create Issue to dispatch it.


⏱ 2m 51s · 3 turns · tokens: 103.0k in / 16.2k out (+262.8k cache-read)
📜 Full session transcript

💬 **Verdict: comment** — notes only, nothing blocking. This is a large, carefully executed feature (the general `/calendar/all` view) that follows the codebase's existing conventions closely: access control is re-checked through `BoardService.GetVisibleAsync`/`ResolveAsync` exactly as the single-board calendar does, encryption/plaintext boundaries are respected, translations are added in both locales, the board-vs-tag filter split is modeled with a separate `HashSet`/`Category` abstraction to avoid id collisions, `NavHeader`'s active-section/active-board logic is extended consistently, and there's substantial unit + e2e test coverage (`CalendarScopeTests`, new `BoardLanes`/`MatchesBoard` tests, `calendar-all.spec.js`). The `Boards.razor` fix bundled in (loading into locals before assigning `_boards`/`_access`/`_edits` together) is a legitimate, well-explained bugfix for a prerender race. I didn't find correctness bugs in the parsing, filtering, or ICS-building logic — `CalendarScope.Parse` matches its tests, `BuildIcsAsync`'s three scope branches all funnel through viewer-checked resolution, and the `Filtered`/`NoFilter` refactor in `Calendar.razor` looks like an equivalent restatement of the previous inline filtering rather than a behavior change. One moderate, non-blocking concern below about unbounded board-id lists in the `board=` query parameter causing many sequential DB round trips per request. 🔧 [Let Claude fix this](https://git.grandiras.net/Grandiras/Ponente/issues/new?title=Fix%20review%20findings%20on%20PR%20%2388&body=%40claude%20Address%20the%20findings%20of%20the%20latest%20review%20on%20pull%20request%20%2388.%0A%0A%3C!--%20claude-fix-pr%3A88%20--%3E) — opens a prefilled task; press **Create Issue** to dispatch it. --- ⏱ 2m 51s · 3 turns · tokens: 103.0k in / 16.2k out (+262.8k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-88/run-233.md) <!-- claude-verdict:comment --> <!-- claude-reviewed-head:3fddfd8e7d2cab5dd18ed45de408b6043b37cbbb -->
@ -333,0 +350,4 @@
var resolved = new List<Board>(scope.BoardIds.Count);
foreach (var id in scope.BoardIds)
{
if ((await boards.ResolveAsync(id, viewer, ct)).Board is not { } named) return null;
Collaborator

CalendarScope.Parse places no upper bound on how many comma-separated board ids the board= query can carry (only dedup). For a Named scope, BuildIcsAsync resolves each id with a sequential await boards.ResolveAsync(id, viewer, ct) — one DB round trip per id, awaited in series, before any card query runs. A caller can pack a few thousand distinct positive ints into one query string (comfortably within Kestrel's default request-line/header limits) and force that many sequential DB calls from a single request.

This is reachable from /calendar.ics (cookie-authed and allowlisted, but otherwise unthrottled beyond ordinary auth) and from /calendar/{token}.ics (anonymous, only mitigated by the calendar-feed rate limiter — a single request already pays the full cost before the limiter's window matters). Worth capping the id-list length in CalendarScope.Parse, or resolving the ids concurrently / with one batched visibility check instead of N sequential ResolveAsync calls, so a crafted subscription URL can't turn one request into thousands of DB round trips.

`CalendarScope.Parse` places no upper bound on how many comma-separated board ids the `board=` query can carry (only dedup). For a `Named` scope, `BuildIcsAsync` resolves each id with a sequential `await boards.ResolveAsync(id, viewer, ct)` — one DB round trip per id, awaited in series, before any card query runs. A caller can pack a few thousand distinct positive ints into one query string (comfortably within Kestrel's default request-line/header limits) and force that many sequential DB calls from a single request. This is reachable from `/calendar.ics` (cookie-authed and allowlisted, but otherwise unthrottled beyond ordinary auth) and from `/calendar/{token}.ics` (anonymous, only mitigated by the `calendar-feed` rate limiter — a single request already pays the full cost before the limiter's window matters). Worth capping the id-list length in `CalendarScope.Parse`, or resolving the ids concurrently / with one batched visibility check instead of N sequential `ResolveAsync` calls, so a crafted subscription URL can't turn one request into thousands of DB round trips.
Owner
@claude-bot
Author
Collaborator

🤖 On it — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 On it — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/234); the reply will link the full session transcript. 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/issue-88/run-234.md) — updating every ~5 minutes while the task runs.
Resolve a named-board feed in one query, and cap the list
All checks were successful
CI / build (pull_request) Successful in 38s
48f6d0404c
The review's finding on #88: `board=3,7` was resolved one id at a time, and
`ResolveAsync` is two round trips each — a lookup and a visibility check —
awaited in series before a single card was read. Nothing bounded the list, so
a few thousand ids fit comfortably in a query string, and `/calendar/{token}
.ics` is anonymous: the rate limiter bounds how often a crafted URL is served,
never what one request costs.

Both halves, because either alone leaves the other. BoardService.
ResolveManyAsync asks once, in the order asked for, and is all-or-nothing for
the same reason the loop was — a file carrying two boards where three were
named is the quiet substitution this app refuses one board at a time.
CalendarScope.MaxBoardIds caps the list at 100, far above anything the app
writes (the page names at most the boards a member can see, and past that it
writes `all`), and a longer one is refused rather than truncated, for the
reason a stray comma is.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Collaborator

Addressed the review's finding on the board= query and pushed to claude/issue-85 (commit 48f6d04), which updates this PR.

What was wrong

board=3,7 was resolved one id at a time, and BoardService.ResolveAsync is two round trips each (a GetAsync lookup, then a CanViewAsync visibility check) — awaited in series, before a single card was read. Nothing bounded the list, and /calendar/{token}.ics is anonymous: the calendar-feed limiter bounds how often a crafted URL is served, never what one request costs.

The fix — both halves

BoardService.ResolveManyAsync (src/Ponente.Kanban/Services/BoardService.cs:196) asks once — WHERE id = ANY(...) combined with the same BoardAccess.VisibleTo(viewer) predicate — and returns the boards in the order asked for, or null if any one of them is missing or not the viewer's. Semantics are identical to the loop it replaces (both cases were already a 404 there, deliberately indistinguishable), so nothing about who can read what changed; it's N sequential round trips → 1.

CalendarScope.MaxBoardIds = 100 caps the list at parse time. That's far above anything the app writes — the page names at most the boards a member can see, and past that it writes all rather than a list — so it's a ceiling on a hand-crafted URL, not a limit on anybody's calendar. A longer list is refused, not truncated, for the reason a stray comma already is: half the boards asked for is the same quiet substitution. Repeats don't count towards it (a board named twice is one board).

BuildIcsAsync's Named branch is now the single call, with the reasoning in a comment; the paragraph in CLAUDE.md about the board query gained a note on why the cap and the batched resolve exist.

Tests

  • CalendarScopeTests: a list exactly at the cap parses, one over is refused, repeats past the cap still parse as one board.
  • calendar-all.spec.js: the multi-id path now has end-to-end coverage it lacked — a list of both boards' ids returns 200 carrying both boards' cards, and board=<real>,999999 404s the whole feed rather than dropping the bad id.

Verified

dotnet build Ponente.sln   → Build succeeded, 0 errors
dotnet test  Ponente.sln   → Passed! Failed: 0, Passed: 194, Skipped: 0

(The one CS8604 warning in Calendar.razor is pre-existing on main, not from this branch.)

The Playwright suite I could not run here: this CI container has no podman/docker, so the compose stack the e2e suite needs can't come up. The new spec assertions are syntax-checked only; they'll want a real run against the stack before merge.


⏱ 82m 38s · 41 turns · tokens: 182.6k in / 16.1k out (+3578.2k cache-read)
📜 Full session transcript

Addressed the review's finding on the `board=` query and pushed to **`claude/issue-85`** (commit `48f6d04`), which updates this PR. ## What was wrong `board=3,7` was resolved one id at a time, and `BoardService.ResolveAsync` is two round trips each (a `GetAsync` lookup, then a `CanViewAsync` visibility check) — awaited in series, before a single card was read. Nothing bounded the list, and `/calendar/{token}.ics` is anonymous: the `calendar-feed` limiter bounds how *often* a crafted URL is served, never what one request costs. ## The fix — both halves **`BoardService.ResolveManyAsync`** (`src/Ponente.Kanban/Services/BoardService.cs:196`) asks once — `WHERE id = ANY(...)` combined with the same `BoardAccess.VisibleTo(viewer)` predicate — and returns the boards in the order asked for, or `null` if any one of them is missing or not the viewer's. Semantics are identical to the loop it replaces (both cases were already a 404 there, deliberately indistinguishable), so nothing about who can read what changed; it's N sequential round trips → 1. **`CalendarScope.MaxBoardIds = 100`** caps the list at parse time. That's far above anything the app writes — the page names at most the boards a member can see, and past that it writes `all` rather than a list — so it's a ceiling on a hand-crafted URL, not a limit on anybody's calendar. A longer list is **refused, not truncated**, for the reason a stray comma already is: half the boards asked for is the same quiet substitution. Repeats don't count towards it (a board named twice is one board). `BuildIcsAsync`'s `Named` branch is now the single call, with the reasoning in a comment; the paragraph in `CLAUDE.md` about the `board` query gained a note on why the cap and the batched resolve exist. ## Tests - `CalendarScopeTests`: a list exactly at the cap parses, one over is refused, repeats past the cap still parse as one board. - `calendar-all.spec.js`: the multi-id path now has end-to-end coverage it lacked — a list of both boards' ids returns 200 carrying both boards' cards, and `board=<real>,999999` 404s the whole feed rather than dropping the bad id. ## Verified ``` dotnet build Ponente.sln → Build succeeded, 0 errors dotnet test Ponente.sln → Passed! Failed: 0, Passed: 194, Skipped: 0 ``` (The one `CS8604` warning in `Calendar.razor` is pre-existing on `main`, not from this branch.) The Playwright suite I could **not** run here: this CI container has no podman/docker, so the compose stack the e2e suite needs can't come up. The new spec assertions are syntax-checked only; they'll want a real run against the stack before merge. --- ⏱ 82m 38s · 41 turns · tokens: 182.6k in / 16.1k out (+3578.2k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/issue-88/run-234.md)
Owner
@claude-reviewer
Author
Collaborator

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/235); the reply will link the full session transcript. 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-88/run-235.md) — updating every ~5 minutes while the task runs.
claude-reviewer left a comment

Verdict: approve — no blocking issues found.

This is a re-review. The previous request-changes finding — CalendarScope.Parse allowing an unbounded comma-separated board-id list, with BuildIcsAsync resolving each id via a sequential await boards.ResolveAsync(id, viewer, ct) — is fixed: CalendarScope.MaxBoardIds (100) now caps the list and refuses (rather than truncates) anything longer, and resolution goes through the new BoardService.ResolveManyAsync, a single batched query (Where(b => wanted.Contains(b.Id)).Where(BoardAccess.VisibleTo(viewer))) that also enforces all-or-nothing visibility (count mismatch → null → 404), matching the app's established "no quiet substitution" rule. CalendarScopeTests and the e2e calendar-all.spec.js both exercise the cap, the dedup-before-cap behavior, and the 404 paths.

Spot-checking the rest of this large diff (the /calendar/all general-calendar view, board-as-category legend/timeline lanes, CalendarScope/CalendarTimeline.BoardLanes/BoardPalette/BoardFilter.MatchesBoard, and the NavHeader/Calendar.razor scope plumbing) didn't turn up new correctness or security issues: access is consistently re-checked through BoardService.GetVisibleAsync/ResolveAsync/ResolveManyAsync, encrypted-column and locale conventions are followed, GetDatedCardsAsync's new multi-board overload short-circuits on an empty id list (avoiding an accidental WHERE ... IN () semantics trap), and the order-preserving ResolveManyAsync/CalendarScope.Parse behavior matches what the tests assert. The bundled Boards.razor fix from the prior pass is unchanged here. Good to merge.


⏱ 0m 55s · 3 turns · tokens: 95.0k in / 4.6k out (+259.2k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a re-review. The previous request-changes finding — `CalendarScope.Parse` allowing an unbounded comma-separated board-id list, with `BuildIcsAsync` resolving each id via a sequential `await boards.ResolveAsync(id, viewer, ct)` — is fixed: `CalendarScope.MaxBoardIds` (100) now caps the list and refuses (rather than truncates) anything longer, and resolution goes through the new `BoardService.ResolveManyAsync`, a single batched query (`Where(b => wanted.Contains(b.Id)).Where(BoardAccess.VisibleTo(viewer))`) that also enforces all-or-nothing visibility (count mismatch → null → 404), matching the app's established "no quiet substitution" rule. `CalendarScopeTests` and the e2e `calendar-all.spec.js` both exercise the cap, the dedup-before-cap behavior, and the 404 paths. Spot-checking the rest of this large diff (the `/calendar/all` general-calendar view, board-as-category legend/timeline lanes, `CalendarScope`/`CalendarTimeline.BoardLanes`/`BoardPalette`/`BoardFilter.MatchesBoard`, and the `NavHeader`/`Calendar.razor` scope plumbing) didn't turn up new correctness or security issues: access is consistently re-checked through `BoardService.GetVisibleAsync`/`ResolveAsync`/`ResolveManyAsync`, encrypted-column and locale conventions are followed, `GetDatedCardsAsync`'s new multi-board overload short-circuits on an empty id list (avoiding an accidental `WHERE ... IN ()` semantics trap), and the order-preserving `ResolveManyAsync`/`CalendarScope.Parse` behavior matches what the tests assert. The bundled `Boards.razor` fix from the prior pass is unchanged here. Good to merge. --- ⏱ 0m 55s · 3 turns · tokens: 95.0k in / 4.6k out (+259.2k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-88/run-235.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:48f6d0404cfeeb43ffcd10de7ebe2b094b677087 -->
Grandiras deleted branch claude/issue-85 2026-08-14 18:33:51 +00:00
Grandiras referenced this pull request from a commit 2026-08-14 18:33:51 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
3 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
Grandiras/Ponente!88
No description provided.