Claude: changes for issue #78 #80
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!80
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "claude/issue-78"
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 #78.
A board is now either everyone's or a named few's. Board.Visibility says which, and BoardMember is the grant list the second one consults — a join table beside Board rather than a column on it, because a board is not owned. The switch exists so that an empty grant list never has to be interpreted. Read as "everyone" it turns taking the last person off a board into opening that board to the whole deployment, from a screen that was removing access; read as "nobody" it hides every board that predates the feature. One column settles both, and it defaults to Everyone — so the migration needs no backfill and nobody's board changes by deploying it. Nothing an ordinary member sees is new, which was the requirement. There is no lock icon and no greyed-out row: the switcher lists the boards you have, "/" lands on the first of those, and a board you were not given is simply not in the menu. The only screens written for this are empty states, because a link can still be pasted into a chat — "no such board", "there is, and it is not yours", and "you have none yet", told apart in the DOM by data-outcome rather than by their copy. The rule lives in one expression, BoardAccess.VisibleTo, so that the same predicate runs in Postgres and in the tests. Every read in BoardService comes in two — one taking a viewer, one ending in ForAdmin — and neither takes an optional one: a default of "no filter" is a hole that opens itself the first time a call site forgets an argument, which is why the old one-argument ResolveAsync is gone rather than kept. The two .ics endpoints mattered most here, being the only paths out of a board that are plain HTTP: a member could otherwise have walked board ids and read every deadline on the deployment out of the URL bar. Both build a viewer — one from the cookie principal, one from the DID behind the feed token — and 404 a board that is not theirs. Admins see every board, and the access page says so beside each admin in the list. They can grant themselves any board in one click and the admin pages have to be able to edit a board nobody shared with them, so hiding one would be theatre. /admin/access[/{id}] is the screen, the same shape as the columns and tags pages. Save is disabled until something differs from the database, which is also how the e2e suite knows a write landed without reading a toast in whichever language the run negotiated. Verified: dotnet build + the 165 unit tests (Debug and Release), the migration applied to a real Postgres, and the whole grant/revoke flow driven in Chromium against a local instance — including the existing boards.spec.js, board.spec.js, calendar.spec.js and filter.spec.js, which still pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>🤖 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: request changes — blocking issues below.
This is a large, well-documented feature (per-board visibility with an
Everyone/Restrictedswitch and aBoardMembergrant list). The core design is sound: the visibility rule lives in oneExpression<Func<Board,bool>>(BoardAccess.VisibleTo) that's compiled for unit tests and translated to SQL for real reads, admins always see everything,BoardPage/Calendarre-derive the viewer and re-resolve the board on every reload so a revoked grant lands on an open page, the two.icsendpoints (the only plain-HTTP paths onto a board) are re-checked per request,CardServicecorrectly stays untouched since there's no HTTP surface onto a card, the migration needs no backfill (Visibilitydefaults toEveryone),BoardMembercascades on both FKs (unlike the structuralRestrictFKs elsewhere, for a documented reason), and both locales/e2e/unit tests were extended together.One real gap:
NavHeader's own board switcher does not follow the "always re-check" discipline the rest of the feature is built around — see the inline finding. It cachesBoardViewer(in particular theIsAdminflag) for the life of the circuit and only refreshes the board list onBoardsChanged, which a role change (MemberService.SetRoleAsync) does not raise. A demoted admin therefore keeps seeing every board's name in the switcher, including boards restricted to other people, until their circuit fully reconnects — contrary to the PR's own stated goal that a board not shared with you is simply absent from the menu. Card/board content stays protected (the fresh checks inBoardPage/Calendarstill deny it), so this is a name/existence leak rather than a data leak, but it's a real hole in an otherwise carefully closed feature and should be easy to fix by havingNavHeaderrefresh its viewer alongside the board list.Everything else checked out:
BoardService's viewer-taking vsForAdminmethod split, theResolveAsync/CanViewAsynclogic and its threeBoardOutcomes,SetAccessAsync's diffing of grants (including dropping DIDs that aren't real members), the migration/model snapshot, the encryption-at-rest boundary (visibility and grants are plaintext by design, matching the rest of the schema's rule), and the en/de translation parity.⏱ 3m 32s · 15 turns · tokens: 123.0k in / 18.9k out (+1734.4k cache-read)
📜 Full session transcript
@claude-bot Please work on the review findings
🤖 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.
🤖 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.
Fixed and pushed to
claude/issue-78(commitdd25177).The finding
NavHeaderbuilt itsBoardVieweronce inOnInitializedAsyncand reused it for every refresh. The DID in it can't change under a live circuit, butIsAdmincan: it comes from the allowlist snapshot, andMemberService.SetRoleAsyncreloads that snapshot without ending the circuit —PersistingRevalidatingAuthStateProviderasks whether the member is still allowed, not whether they still administer. A demoted admin therefore kept the whole board list in their switcher, other people's restricted board names included, until they next reconnected.Two halves to the fix:
NavHeader.razor— the viewer is derived insideLoadBoardsAsync, so there is no cached copy left to go stale (the field is gone, not just refreshed).MemberService.SetRoleAsync— now firesNotifyBoardsChanged(). The snapshot was already reloaded there, but nothing re-read it on its own, so a demotion reached an open page only when something else happened to move the boards. A role change is an answer to "which boards are there, for me".Also
CLAUDE.md: a line under the access bullets on why the switcher re-asks who is looking, and the role change added to theBoardsChangedlist.Verification
I brought up the full compose stack (the dind daemon at the gateway, per the notes in
CLAUDE.md) and ran against it.dotnet build Ponente.slnclean;dotnet test165/165.a demotion takes the other boards out of the switcherinaccess.spec.js: two browser contexts — the member watches their switcher while an admin demotes them from the other session, and the board has to leave the menu with nothing reloading their page. I ran it against the unfixed code first and it fails there, so it pins the finding rather than the fix.access.spec.js3/3, four times running (three consecutive clean runs after the last change).Three bugs in
access.spec.jsitself that only showed up by running it — worth knowing since the review couldn't see them:e2eacc${Date.now()}= 19 characters, and the PDS refuses a first label over 18 (InvalidHandle: Handle too long). Both tests needing a second member failed at signup — the file had never passed against the current PDS image.deleteBoard's click in thefinallywas unretried, so a click landing before the circuit is up silently did nothing and left a board behind — which changes the nav header every other spec file asserts against.All three now use the retry pattern the rest of the file already documents.
One thing I did not touch: full-suite runs in this container are flaky under CPU contention (it shares 8 cores with other CI jobs). Baseline on the pristine commit: 24 failed / 141 passed; with my changes: 21 failed / 145 passed — different, overlapping sets, dominated by the same unretried-delete race in
boards.spec.js's own cleanup. That flake predates this PR, so I left it alone rather than widening the diff; it's a candidate for the same retry treatment if you want it.⏱ 41m 2s · 135 turns · tokens: 161.0k in / 39.2k out (+16293.1k cache-read)
📜 Full session transcript
✅ Verdict: approve — no blocking issues found.
This re-review confirms the one blocking issue from my previous pass is fixed.
NavHeader.LoadBoardsAsyncno longer treatsBoardVieweras circuit-lifetime state — it now re-derives the viewer (viaBoardViewer.For(state.User, Allowlist)) fresh on every call, includingIsAdmin, which comes straight from the allowlist snapshotMemberService.SetRoleAsyncreloads before notifying. AndSetRoleAsyncnow firesNotifyBoardsChanged()in addition toNotifyAllBoardsChanged(), specifically so a role change reachesNavHeader'sBoardsChangedsubscription and triggers that re-derivation. The newaccess.spec.jstest "a demotion takes the other boards out of the switcher" pins exactly this scenario with two browser contexts. The prior leak — a demoted admin's switcher continuing to show other members' restricted boards until reconnect — is closed.The rest of the feature is unchanged in substance from the last pass and remains sound: the single
BoardAccess.VisibleToexpression compiled for tests and translated to SQL for real reads, admins always seeing everything (by design, stated on screen),BoardPage/Calendarre-resolving on every reload and closing any open card modal when access is lost, the two.icsendpoints re-checking per request since they're the only plain-HTTP paths onto a board,BoardMembercascading on both FKs unlike the structuralRestrictFKs, the migration needing no backfill, and en/de translation parity for the new keys.No new issues found in this pass. Approving.
⏱ 0m 53s · 3 turns · tokens: 96.7k in / 4.7k out (+279.5k cache-read)
📜 Full session transcript