Claude: changes for issue #115 #128
No reviewers
Labels
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/Ponente!128
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "claude/issue-115"
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 #115.
🤖 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 constructed feature (the
/profilepage,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.mdupdated,BoardNotifierevent wired for the new mutation,MemberService.RefreshProfileAsyncvsSetProfileAsyncsplit is well-reasoned and all call sites were updated consistently). The default-safe behavior (ProfileScopeMode.Parsefailing towardsNoneon any unrecognized value, scope read back from the granted token rather than assumed, JSON-merge instead of typed deserialization to avoid droppingdescription/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 cachedMembersrow, 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-concurrencyswapRecordhandling, avatar size/type validation, theNavHeaderwiring ofProfileChanged, and the OAuth scope plumbing inProgram.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
@ -0,0 +158,4 @@editable);}return new ProfileView(did, handle, null, null, client.PdsUrl, editable);When a client can be built (the member holds some token) but
ReadRecordAsyncreturns 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 genericcatch (Exception ex)inReadRecordAsync— this returns aProfileViewwithDisplayName/AvatarUrlboth null. That blanks out the identity block even though Ponente has a perfectly good cached copy in theMembersrow (used one branch down, in the no-token path, viamember?.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 tomembers.GetByDidAsync(did)the same way theclient is nullbranch does below.🤖 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.
Fixed the one finding, and it turned out to have a second, worse half on the write side.
The finding was right.
ReadRecordAsynccollapsed "this account has no profile record" and "the read didn't come back" into the samenull, so a PDS hiccup blanked a member's own name and picture even thoughMembersholds 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.
SaveAsyncre-reads before merging. On a failed read it tookexistingas "no record yet", built a record from nothing, and only theswapRecord: nullguard ("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:ReadRecordAsyncnow returnsRecordRead(State, Record, Cid)withFound/Absent/Unavailableinstead of a nullable tuple.LoadAsync:Absentstill renders an empty profile (a new account — the cache can't know better than the repository);Unavailablefalls back tomembers.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:UnavailablereturnsFailedbefore the blob upload, rather than proceeding on a record it never read.IsMissingRecord— only the account server saying so (RecordNotFound/InvalidRequest/ 404 / 400) counts as absent; a timeout, a 502 or a DNS failure does not.Unavailablerather 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
ProfileRecordTestspinning the classification both ways (the branch decision is pure and testable;LoadAsyncitself needs a real client).dotnet buildis clean apart from the pre-existingCalendar.razorCS8604, anddotnet test Ponente.slnpasses 380/380,DocumentationTestsincluded. The e2e suite I could not run — this container has neitherpodmannordocker, and perdocs/auth.mdthe 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
🤖 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.
Re-review of the
/profilefeature PR. The one blocking issue from my previous pass —ProfileService.LoadAsyncblanking a member's cached name/avatar on a transient PDS read failure instead of falling back to theMemberscache — is now fixed: theRecordState.Unavailablepath falls through tomembers.GetByDidAsync(did)the same way the no-token branch does, before returning theProfileView. 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/SetProfileAsyncsplit is applied consistently at every call site (NavHeader,Admin/Members.razor,Admin/RequestsPanel.razor,Program.cs/signup),BoardNotifier.ProfileChangedis subscribed/unsubscribed correctly inNavHeader, theswapRecordoptimistic-concurrency guard and theRecordState.Unavailableshort-circuit before blob upload inSaveAsyncare both correct,ProfileScope's scope-string parsing/matching is exercised thoroughly byProfileScopeTestsand looks self-consistent, and the localization keys used inProfile.razorare all present in bothenanddedictionaries. Docs (auth.md,orientation.md,ui.md), the changelog (both locales), and the env var plumbing (README.md, both.env.examplefiles, both compose files) are all updated per the repo's conventions.Two very minor, non-blocking observations, not worth holding the PR for:
Profile.razor'sSaveAsync, theProfileSaveResult.NotPermittedcase just silently reloads without setting_error. This is correct for the ordinary case (scope actually not granted —_view.Editableflips to a locked state that explains why). But there's an edge case where the granted scope allows writing yetProfileService.CreateClientAsyncstill fails (e.g. a revoked/expired token) —SaveAsyncreturnsNotPermittedthere too, and the reload will still show the editable form (sinceEditableis 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./profileand clicks Save without touching either field will create a near-emptyapp.bsky.actor.profilerecord (just$type/createdAt), since theUnchangedshort-circuit only applies whenexisting.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