Claude: changes for issue #115 #128

Merged
Grandiras merged 2 commits from claude/issue-115 into main 2026-08-19 21:36:04 +00:00
Collaborator

Closes #115.

Closes #115.
Add a profile page for a member's own Atmosphere profile
All checks were successful
CI / build (pull_request) Successful in 34s
0762e03bb8
/profile shows the signed-in member's account and — where the deployment
asks for the permission — edits the two fields Ponente itself displays:
the display name and the avatar. It is the standard
app.bsky.actor.profile record at rkey "self", written with
com.atproto.repo.putRecord and uploadBlob, so a name set here is the name
every app on the network shows.

The permission is the interesting part. The narrowest scope the OAuth
specification can express for this is repo:app.bsky.actor.profile plus
blob:image/*, and no account server accepts it yet: measured today,
bsky.social and this deployment's own PDS both advertise only "atproto"
and the three transition:* scopes, and an unknown scope is rejected
rather than narrowed — so asking for it would break sign-in for everyone.
The only scope that works is transition:generic, which covers the whole
account. That trade belongs to the deployment, so it is
AtProto:ProfileScope (none | granular | generic), defaulting to none: the
page then still shows the account and refreshes Ponente's cached copy of
it, and says why the fields are missing. ProfileScope carries the
reasoning and docs/auth.md carries the measurement.

Also here: the record is merged as JSON rather than round-tripped through
a typed model, so a description, a banner or the self-labels behind "no
logged-out visibility" survive a rename; swapRecord makes a concurrent
change a message instead of an overwrite; MemberService gains
SetProfileAsync beside the renamed RefreshProfileAsync, because a member
clearing their own name means it and a lookup finding nothing does not;
and BoardNotifier.ProfileChanged lets the nav header replace the face in
the corner without a reload.

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/311); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-128/run-311.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 constructed feature (the /profile page, ProfileScope, ProfileService) with strong test coverage (ProfileScopeTests, ProfileRecordTests, profile.spec.js) and thorough documentation updates that follow the repo's conventions (Changelog entries in both locales, docs/auth.md/docs/orientation.md/docs/ui.md updated, BoardNotifier event wired for the new mutation, MemberService.RefreshProfileAsync vs SetProfileAsync split is well-reasoned and all call sites were updated consistently). The default-safe behavior (ProfileScopeMode.Parse failing towards None on any unrecognized value, scope read back from the granted token rather than assumed, JSON-merge instead of typed deserialization to avoid dropping description/banner/self-labels) is a good security posture and is well tested.

One real, if minor, inconsistency worth a look (see inline finding) in ProfileService.LoadAsync: when a token/client exists but reading the PDS record fails for a transient reason (not just "record not found"), the page falls back to nothing rather than to the cached Members row, unlike the no-token path which does fall back to the cache. Everything else I checked — scope-string parsing/matching (TrySplit/GrantsProfileWrite/GrantsBlob), the optimistic-concurrency swapRecord handling, avatar size/type validation, the NavHeader wiring of ProfileChanged, and the OAuth scope plumbing in Program.cs — looks correct and self-consistent with its own tests.


⏱ 2m 56s · 4 turns · tokens: 77.9k in / 17.2k out (+300.9k cache-read)
📜 Full session transcript

💬 **Verdict: comment** — notes only, nothing blocking. This is a large, carefully constructed feature (the `/profile` page, `ProfileScope`, `ProfileService`) with strong test coverage (`ProfileScopeTests`, `ProfileRecordTests`, `profile.spec.js`) and thorough documentation updates that follow the repo's conventions (Changelog entries in both locales, `docs/auth.md`/`docs/orientation.md`/`docs/ui.md` updated, `BoardNotifier` event wired for the new mutation, `MemberService.RefreshProfileAsync` vs `SetProfileAsync` split is well-reasoned and all call sites were updated consistently). The default-safe behavior (`ProfileScopeMode.Parse` failing towards `None` on any unrecognized value, scope read back from the granted token rather than assumed, JSON-merge instead of typed deserialization to avoid dropping `description`/`banner`/self-labels) is a good security posture and is well tested. One real, if minor, inconsistency worth a look (see inline finding) in `ProfileService.LoadAsync`: when a token/client exists but reading the PDS record fails for a transient reason (not just "record not found"), the page falls back to nothing rather than to the cached `Members` row, unlike the no-token path which does fall back to the cache. Everything else I checked — scope-string parsing/matching (`TrySplit`/`GrantsProfileWrite`/`GrantsBlob`), the optimistic-concurrency `swapRecord` handling, avatar size/type validation, the `NavHeader` wiring of `ProfileChanged`, and the OAuth scope plumbing in `Program.cs` — looks correct and self-consistent with its own tests. --- ⏱ 2m 56s · 4 turns · tokens: 77.9k in / 17.2k out (+300.9k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-128/run-311.md) <!-- claude-verdict:comment --> <!-- claude-reviewed-head:0762e03bb8716f76f465c5c6c3cfec196964d3f8 -->
@ -0,0 +158,4 @@
editable);
}
return new ProfileView(did, handle, null, null, client.PdsUrl, editable);
Collaborator

When a client can be built (the member holds some token) but ReadRecordAsync returns null for a reason other than "record genuinely doesn't exist" — e.g. the member's PDS is briefly unreachable, or any other exception caught by the generic catch (Exception ex) in ReadRecordAsync — this returns a ProfileView with DisplayName/AvatarUrl both null. That blanks out the identity block even though Ponente has a perfectly good cached copy in the Members row (used one branch down, in the no-token path, via member?.DisplayName/member?.AvatarUrl). A transient PDS hiccup would make a member's own profile page briefly show no name/picture instead of falling back to the same cache the rest of the app (and this page's own no-token branch) relies on. Worth deciding whether this should fall back to members.GetByDidAsync(did) the same way the client is null branch does below.

When a client can be built (the member holds *some* token) but `ReadRecordAsync` returns null for a reason other than "record genuinely doesn't exist" — e.g. the member's PDS is briefly unreachable, or any other exception caught by the generic `catch (Exception ex)` in `ReadRecordAsync` — this returns a `ProfileView` with `DisplayName`/`AvatarUrl` both null. That blanks out the identity block even though Ponente has a perfectly good cached copy in the `Members` row (used one branch down, in the no-token path, via `member?.DisplayName`/`member?.AvatarUrl`). A transient PDS hiccup would make a member's own profile page briefly show no name/picture instead of falling back to the same cache the rest of the app (and this page's own no-token branch) relies on. Worth deciding whether this should fall back to `members.GetByDidAsync(did)` the same way the `client is null` branch does below.
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/312); the reply will link the full session transcript. <!-- claude-run:comment --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/issue-128/run-312.md) — updating every ~5 minutes while the task runs.
Tell a profile read that failed apart from one that came back empty
All checks were successful
CI / build (pull_request) Successful in 39s
68aa2ee902
A transient PDS failure looked identical to an account with no profile
record: LoadAsync blanked the name and picture the app has cached, and
SaveAsync would have merged onto nothing.

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

Fixed the one finding, and it turned out to have a second, worse half on the write side.

The finding was right. ReadRecordAsync collapsed "this account has no profile record" and "the read didn't come back" into the same null, so a PDS hiccup blanked a member's own name and picture even though Members holds a perfectly good cached copy — the same cache the no-token branch one line below already falls back to.

The write side had the same confusion, with teeth. SaveAsync re-reads before merging. On a failed read it took existing as "no record yet", built a record from nothing, and only the swapRecord: null guard ("must not exist") stood between that and a write that dropped the member's description, banner and self-labels. It fails safe today, but by accident, and it reports the wrong reason (Conflict — "somebody else changed it") when the truth is "we couldn't reach your PDS". Since I was drawing the distinction anyway, I used it there too.

What changed in src/Ponente.Kanban/Services/ProfileService.cs:

  • ReadRecordAsync now returns RecordRead(State, Record, Cid) with Found / Absent / Unavailable instead of a nullable tuple.
  • LoadAsync: Absent still renders an empty profile (a new account — the cache can't know better than the repository); Unavailable falls back to members.GetByDidAsync, exactly as the no-token path does. That also keeps the display-name box filled, which matters: an empty box is what a later successful save would write, and a blip is not a request to delete a name.
  • SaveAsync: Unavailable returns Failed before the blob upload, rather than proceeding on a record it never read.
  • The classification is a public static, IsMissingRecord — only the account server saying so (RecordNotFound / InvalidRequest / 404 / 400) counts as absent; a timeout, a 502 or a DNS failure does not.
  • A body that parses to something other than a JSON object is now Unavailable rather than being treated as an empty profile — it arrives with a CID that would have let a from-scratch merge through the swap guard.

Tests: seven new cases in ProfileRecordTests pinning the classification both ways (the branch decision is pure and testable; LoadAsync itself needs a real client). dotnet build is clean apart from the pre-existing Calendar.razor CS8604, and dotnet test Ponente.sln passes 380/380, DocumentationTests included. The e2e suite I could not run — this container has neither podman nor docker, and per docs/auth.md the write path needs a real OAuth grant anyway.

docs/auth.md's "It reads the record, not the appview" bullet now records the distinction and why it exists. No changelog line: this is a fix inside the unreleased feature its own entry already describes.

Pushed to claude/issue-115 (68aa2ee). No new issues filed.


⏱ 4m 10s · 29 turns · tokens: 61.3k in / 16.1k out (+1550.6k cache-read)
📜 Full session transcript

Fixed the one finding, and it turned out to have a second, worse half on the write side. **The finding was right.** `ReadRecordAsync` collapsed "this account has no profile record" and "the read didn't come back" into the same `null`, so a PDS hiccup blanked a member's own name and picture even though `Members` holds a perfectly good cached copy — the same cache the no-token branch one line below already falls back to. **The write side had the same confusion, with teeth.** `SaveAsync` re-reads before merging. On a failed read it took `existing` as "no record yet", built a record from nothing, and only the `swapRecord: null` guard ("must not exist") stood between that and a write that dropped the member's description, banner and self-labels. It fails safe today, but by accident, and it reports the wrong reason (`Conflict` — "somebody else changed it") when the truth is "we couldn't reach your PDS". Since I was drawing the distinction anyway, I used it there too. What changed in `src/Ponente.Kanban/Services/ProfileService.cs`: - `ReadRecordAsync` now returns `RecordRead(State, Record, Cid)` with `Found` / `Absent` / `Unavailable` instead of a nullable tuple. - `LoadAsync`: `Absent` still renders an empty profile (a new account — the cache can't know better than the repository); `Unavailable` falls back to `members.GetByDidAsync`, exactly as the no-token path does. That also keeps the display-name box filled, which matters: an empty box is what a *later* successful save would write, and a blip is not a request to delete a name. - `SaveAsync`: `Unavailable` returns `Failed` before the blob upload, rather than proceeding on a record it never read. - The classification is a public static, `IsMissingRecord` — only the account server saying so (`RecordNotFound` / `InvalidRequest` / 404 / 400) counts as absent; a timeout, a 502 or a DNS failure does not. - A body that parses to something other than a JSON object is now `Unavailable` rather than being treated as an empty profile — it arrives with a CID that would have let a from-scratch merge through the swap guard. Tests: seven new cases in `ProfileRecordTests` pinning the classification both ways (the branch decision is pure and testable; `LoadAsync` itself needs a real client). `dotnet build` is clean apart from the pre-existing `Calendar.razor` CS8604, and `dotnet test Ponente.sln` passes 380/380, `DocumentationTests` included. The e2e suite I could not run — this container has neither `podman` nor `docker`, and per `docs/auth.md` the write path needs a real OAuth grant anyway. `docs/auth.md`'s "It reads the record, not the appview" bullet now records the distinction and why it exists. No changelog line: this is a fix inside the unreleased feature its own entry already describes. Pushed to `claude/issue-115` (`68aa2ee`). No new issues filed. --- ⏱ 4m 10s · 29 turns · tokens: 61.3k in / 16.1k out (+1550.6k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/issue-128/run-312.md)
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/313); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-128/run-313.md) — updating every ~5 minutes while the task runs.
claude-reviewer left a comment

Verdict: approve — no blocking issues found.

Re-review of the /profile feature PR. The one blocking issue from my previous pass — ProfileService.LoadAsync blanking a member's cached name/avatar on a transient PDS read failure instead of falling back to the Members cache — is now fixed: the RecordState.Unavailable path falls through to members.GetByDidAsync(did) the same way the no-token branch does, before returning the ProfileView. That was the only concrete defect I found last time, so this is good to merge.

I re-checked the rest of the surface for regressions or new issues: the UpdateProfileAsyncRefreshProfileAsync/SetProfileAsync split is applied consistently at every call site (NavHeader, Admin/Members.razor, Admin/RequestsPanel.razor, Program.cs /signup), BoardNotifier.ProfileChanged is subscribed/unsubscribed correctly in NavHeader, the swapRecord optimistic-concurrency guard and the RecordState.Unavailable short-circuit before blob upload in SaveAsync are both correct, ProfileScope's scope-string parsing/matching is exercised thoroughly by ProfileScopeTests and looks self-consistent, and the localization keys used in Profile.razor are all present in both en and de dictionaries. Docs (auth.md, orientation.md, ui.md), the changelog (both locales), and the env var plumbing (README.md, both .env.example files, both compose files) are all updated per the repo's conventions.

Two very minor, non-blocking observations, not worth holding the PR for:

  • In Profile.razor's SaveAsync, the ProfileSaveResult.NotPermitted case just silently reloads without setting _error. This is correct for the ordinary case (scope actually not granted — _view.Editable flips to a locked state that explains why). But there's an edge case where the granted scope allows writing yet ProfileService.CreateClientAsync still fails (e.g. a revoked/expired token) — SaveAsync returns NotPermitted there too, and the reload will still show the editable form (since Editable is derived from the granted scope string, not from whether a client could actually be built), so a click on Save would appear to silently do nothing with no explanation. Rare, and not a regression from this PR's stated goals.
  • A brand-new (no existing profile) member who opens /profile and clicks Save without touching either field will create a near-empty app.bsky.actor.profile record (just $type/createdAt), since the Unchanged short-circuit only applies when existing.Record is not null. Probably fine/intended, just noting it.

⏱ 2m 12s · 21 turns · tokens: 72.7k in / 9.9k out (+1702.2k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review of the `/profile` feature PR. The one blocking issue from my previous pass — `ProfileService.LoadAsync` blanking a member's cached name/avatar on a transient PDS read failure instead of falling back to the `Members` cache — is now fixed: the `RecordState.Unavailable` path falls through to `members.GetByDidAsync(did)` the same way the no-token branch does, before returning the `ProfileView`. That was the only concrete defect I found last time, so this is good to merge. I re-checked the rest of the surface for regressions or new issues: the `UpdateProfileAsync` → `RefreshProfileAsync`/`SetProfileAsync` split is applied consistently at every call site (`NavHeader`, `Admin/Members.razor`, `Admin/RequestsPanel.razor`, `Program.cs` `/signup`), `BoardNotifier.ProfileChanged` is subscribed/unsubscribed correctly in `NavHeader`, the `swapRecord` optimistic-concurrency guard and the `RecordState.Unavailable` short-circuit before blob upload in `SaveAsync` are both correct, `ProfileScope`'s scope-string parsing/matching is exercised thoroughly by `ProfileScopeTests` and looks self-consistent, and the localization keys used in `Profile.razor` are all present in both `en` and `de` dictionaries. Docs (`auth.md`, `orientation.md`, `ui.md`), the changelog (both locales), and the env var plumbing (`README.md`, both `.env.example` files, both compose files) are all updated per the repo's conventions. Two very minor, non-blocking observations, not worth holding the PR for: - In `Profile.razor`'s `SaveAsync`, the `ProfileSaveResult.NotPermitted` case just silently reloads without setting `_error`. This is correct for the ordinary case (scope actually not granted — `_view.Editable` flips to a locked state that explains why). But there's an edge case where the granted scope allows writing yet `ProfileService.CreateClientAsync` still fails (e.g. a revoked/expired token) — `SaveAsync` returns `NotPermitted` there too, and the reload will still show the editable form (since `Editable` is derived from the granted scope string, not from whether a client could actually be built), so a click on Save would appear to silently do nothing with no explanation. Rare, and not a regression from this PR's stated goals. - A brand-new (no existing profile) member who opens `/profile` and clicks Save without touching either field will create a near-empty `app.bsky.actor.profile` record (just `$type`/`createdAt`), since the `Unchanged` short-circuit only applies when `existing.Record is not null`. Probably fine/intended, just noting it. --- ⏱ 2m 12s · 21 turns · tokens: 72.7k in / 9.9k out (+1702.2k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-128/run-313.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:68aa2ee9021c1cf21503bb93268366eaba554484 -->
Grandiras deleted branch claude/issue-115 2026-08-19 21:36:04 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
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
Grandiras/Ponente!128
No description provided.