Claude: changes for issue #85 #88
No reviewers
Labels
No labels
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/Ponente!88
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "claude/issue-85"
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?
Closes #85.
🤖 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.
💬 Verdict: comment — notes only, nothing blocking.
This is a large, carefully executed feature (the general
/calendar/allview) that follows the codebase's existing conventions closely: access control is re-checked throughBoardService.GetVisibleAsync/ResolveAsyncexactly 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 separateHashSet/Categoryabstraction to avoid id collisions,NavHeader's active-section/active-board logic is extended consistently, and there's substantial unit + e2e test coverage (CalendarScopeTests, newBoardLanes/MatchesBoardtests,calendar-all.spec.js). TheBoards.razorfix bundled in (loading into locals before assigning_boards/_access/_editstogether) 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.Parsematches its tests,BuildIcsAsync's three scope branches all funnel through viewer-checked resolution, and theFiltered/NoFilterrefactor inCalendar.razorlooks 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
@ -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;CalendarScope.Parseplaces no upper bound on how many comma-separated board ids theboard=query can carry (only dedup). For aNamedscope,BuildIcsAsyncresolves each id with a sequentialawait 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 thecalendar-feedrate limiter — a single request already pays the full cost before the limiter's window matters). Worth capping the id-list length inCalendarScope.Parse, or resolving the ids concurrently / with one batched visibility check instead of N sequentialResolveAsynccalls, so a crafted subscription URL can't turn one request into thousands of DB round trips.@claude-bot
🤖 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.
Addressed the review's finding on the
board=query and pushed toclaude/issue-85(commit48f6d04), which updates this PR.What was wrong
board=3,7was resolved one id at a time, andBoardService.ResolveAsyncis two round trips each (aGetAsynclookup, then aCanViewAsyncvisibility check) — awaited in series, before a single card was read. Nothing bounded the list, and/calendar/{token}.icsis anonymous: thecalendar-feedlimiter 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 sameBoardAccess.VisibleTo(viewer)predicate — and returns the boards in the order asked for, ornullif 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 = 100caps 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 writesallrather 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'sNamedbranch is now the single call, with the reasoning in a comment; the paragraph inCLAUDE.mdabout theboardquery 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, andboard=<real>,999999404s the whole feed rather than dropping the bad id.Verified
(The one
CS8604warning inCalendar.razoris pre-existing onmain, 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
@claude-reviewer
🤖 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.
✅ Verdict: approve — no blocking issues found.
This is a re-review. The previous request-changes finding —
CalendarScope.Parseallowing an unbounded comma-separated board-id list, withBuildIcsAsyncresolving each id via a sequentialawait 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 newBoardService.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.CalendarScopeTestsand the e2ecalendar-all.spec.jsboth exercise the cap, the dedup-before-cap behavior, and the 404 paths.Spot-checking the rest of this large diff (the
/calendar/allgeneral-calendar view, board-as-category legend/timeline lanes,CalendarScope/CalendarTimeline.BoardLanes/BoardPalette/BoardFilter.MatchesBoard, and theNavHeader/Calendar.razorscope plumbing) didn't turn up new correctness or security issues: access is consistently re-checked throughBoardService.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 accidentalWHERE ... IN ()semantics trap), and the order-preservingResolveManyAsync/CalendarScope.Parsebehavior matches what the tests assert. The bundledBoards.razorfix 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