Grandiras/ATProto.NET
1
1
Fork 0
Code Issues Pull requests Projects Releases 9 Packages 8 Wiki Activity Actions

feat!: typed identifiers, AtDatetime and enumerators for app.bsky (2/4 of #119) #155

Merged
Grandiras merged 1 commit from issue-119-typed-bsky into main 2026-09-24 23:12:19 +00:00
Conversation 7 Commits 1 Files changed 33 +1255 -397
Grandiras commented 2026-09-24 23:09:15 +00:00
Owner
Copy link

Part of #119 (2/4: app.bsky).

Builds on #154 (part 1) and follows the conventions it wrote down in CONTRIBUTING.md. This converts every model and client under src/ATProtoNet/Lexicon/App/Bsky/** to typed identifiers, AtDatetime, IReadOnlyList<T> and ICursorPage<T>, and adds the Enumerate* methods.

How to review this

The change is large but mostly mechanical. Suggested order:

  1. tests/ATProtoNet.Tests/Conventions/TypedIdentifierGuardTests.cs: ATProtoNet.Lexicon.App.Bsky. is gone from PendingConversions, so the reflection guard now enforces this namespace. Eight new Exceptions entries each carry a reason: the generic uri-format link URLs, and searchPosts' since/until, which the Lexicon leaves format-less because they accept a bare date.
  2. tests/ATProtoNet.Tests/Lexicon/App/Bsky/TypedBskyClientTests.cs (new) shows the behaviour end to end: what goes out on the wire, what comes back typed, and how the enumerators walk pages. It also checks by reflection that every app.bsky type with a Cursor implements ICursorPage<T>.
  3. The *Client.cs files hold the real API decisions: parameter order, enumerator names, and which request types became internal.
  4. The *Models.cs files are find-and-replace: string → the typed identifier, List<T> → IReadOnlyList<T>, plus : ICursorPage<T> with an explicit Items. JSON names are unchanged.
  5. The rest are call sites: the Blazor components, one sample, the docs and the test fixtures (invalid CIDs such as "bafy1" became real ones, and invalid handles such as "alice" became alice.test).

What changed

Typed identifiers and AtDatetime (models)

  • Actor: ProfileView*.Did/Handle are Did/Handle, and indexedAt/createdAt are AtDatetime?. ViewerState.Blocking/Following/FollowedBy are AtUri?. ProfileRecord.CreatedAt is AtDatetime?.
  • Feed:
    • PostView.Uri/Cid/IndexedAt and GeneratorView.Uri/Cid/Did/IndexedAt are typed.
    • The viewer-state Like/Repost are AtUri?.
    • In ThreadgateRecord/PostgateRecord, Post is typed, and so are HiddenReplies and DetachedEmbeddingUris (IReadOnlyList<AtUri>).
    • NotFoundPost/BlockedPost/SkeletonFeedPost/DescribeFeedGenerator* are typed.
    • Every record's CreatedAt is required AtDatetime.
  • Graph:
    • FollowRecord/BlockRecord/ListItemRecord.Subject are Did, and ListBlockRecord.Subject, ListItemRecord.List and StarterPackRecord.List are AtUri.
    • The list and starter-pack views and ListViewerState.Blocked are typed.
    • Relationship.Did/Following/FollowedBy, NotFoundActor.Actor (AtIdentifier) and GetRelationshipsResponse.Actor are typed.
  • Notification: NotificationView.Uri/Cid/ReasonSubject/IndexedAt, ListNotificationsResponse.SeenAt and RegisterPushRequest.ServiceDid are typed.
  • Labeler: LabelerView*.Uri/Cid/IndexedAt, LabelerViewerState.Like and LabelerServiceRecord.CreatedAt are typed.
  • Embed / RichText / Video: VideoView.Cid, MentionFeature.Did and JobStatus.Did are typed.
  • Deliberately left as string (the guard lists them): ExternalInfo.Uri, ExternalViewInfo.Uri, LinkFeature.Uri and RichTextBuilder.Link(uri) are Lexicon uri, not at-uri. searchPosts since/until accept YYYY-MM-DD.

Clients: typed parameters and one parameter order

  • actor parameters take an AtIdentifier. Post, feed, list, thread-root and starter-pack parameters take an AtUri, cid takes a Cid?, seenAt takes an AtDatetime?, and LabelerClient.GetServicesAsync takes IEnumerable<Did>.
  • Array inputs are IEnumerable<T>: GetProfilesAsync, GetPostsAsync, GetFeedGeneratorsAsync, GetRelationshipsAsync(others), GetStarterPacksAsync, PutPreferencesAsync and GetServicesAsync.
  • Reordered methods, each now subject → filters → limit → cursor:
    • GetTimelineAsync(algorithm, limit, cursor)
    • GetAuthorFeedAsync(actor, filter, includePins, limit, cursor)
    • ListNotificationsAsync(priority, seenAt, limit, cursor)
  • RichTextBuilder.Mention(Handle, Did). Build() returns IReadOnlyList<Facet>? as a snapshot, so appending after Build() no longer changes a list you already hold.
  • Facade: part 1 had already typed LikeAsync/RepostAsync/FollowAsync/the delete-by-URI helpers and PostAsync's inputs. They now stamp createdAt with AtDatetime.Now(). Moving them to client.Bsky is #122.

ICursorPage<T> and Enumerate*

  • All 22 cursored app.bsky responses implement ICursorPage<T>, with Items explicit over the Lexicon-named list.
  • 25 new enumerators, all on the shared Pagination.EnumerateAsync, which stops on a null, empty or repeated cursor:
    • Actor: EnumerateSuggestionsAsync, EnumerateSearchActorsAsync
    • Feed:
      • timelines and feeds: EnumerateTimelineAsync, EnumerateAuthorFeedAsync, EnumerateFeedAsync, EnumerateListFeedAsync
      • likes, reposts and quotes: EnumerateActorLikesAsync, EnumerateLikesAsync, EnumerateRepostedByAsync, EnumerateQuotesAsync
      • feed generators: EnumerateActorFeedsAsync, EnumerateSuggestedFeedsAsync
      • search: EnumerateSearchPostsAsync
    • Graph:
      • followers and follows: EnumerateFollowersAsync, EnumerateFollowsAsync, EnumerateKnownFollowersAsync
      • blocks and mutes: EnumerateBlocksAsync, EnumerateMutesAsync
      • lists: EnumerateListsAsync, EnumerateListMembersAsync, EnumerateListBlocksAsync, EnumerateListMutesAsync
      • starter packs: EnumerateActorStarterPacksAsync, EnumerateSearchStarterPacksAsync
    • Notification: EnumerateNotificationsAsync
  • The issue named a core set. For consistency I also added list feed, known followers, list blocks/mutes, actor feeds, suggested feeds and actor/searched starter packs. getFeedSkeleton has no enumerator, because it is a feed generator's own endpoint.
  • Naming: Get* drops the verb (GetFollowers → EnumerateFollowers). Search* keeps it (EnumerateSearchPosts), so a search enumerator is never mistaken for a listing. getList → EnumerateListMembersAsync.

Internal request DTOs

PutPreferencesRequest, MuteActorRequest, MuteActorListRequest, MuteThreadRequest and UpdateSeenRequest are now internal. RegisterPushRequest stays public, because RegisterPushAsync accepts it.

Blazor (minimal; #136 rewires these)

  • FeedView parses its FeedSource string into AtIdentifier/AtUri and passes paging arguments by name.
  • PostCard.FormatTime takes the AtDatetime.
  • ProfileCard/PostCard read Handle.Value where they needed a string.
  • _Imports.razor adds ATProtoNet.Identity.

Breaking changes and migration

One Breaking changes bullet in CHANGELOG covers all of it:

  • Typed values: values from responses pass straight through. Parse literals where they enter your code: AtIdentifier.Parse("alice.bsky.social"), AtUri.Parse("at://…"), Did.Parse(…). A Did or Handle converts implicitly to AtIdentifier, and every typed value converts implicitly to string.
  • Timestamps: stamp records with AtDatetime.Now(), and read .Value / .TryGetValue(...) / .ToString().
  • Reordered methods: positional calls to GetTimelineAsync, GetAuthorFeedAsync and ListNotificationsAsync must pass the moved arguments by name.
  • Lists: List<T> → IReadOnlyList<T>. Copy ([.. page.Feed]) where you mutated.
  • Internal DTOs: call the client method instead of constructing the request type.
  • Strict parsing: an AppView response carrying an invalid identifier now throws XrpcResponseFormatException, the part 1 behaviour. A malformed datetime does not throw, because AtDatetime reads leniently.

Verification

  • dotnet build --no-incremental (whole solution, including samples and integration tests): 0 warnings, 0 errors.
  • Unit tests: 3175 passed, 0 failed, 5 skipped (3180 total), 14 of them new.
    • TypedBskyClientTests (new, 13 tests) checks:
      • the exact query strings, including repeated uris, seenAt sent with its offset, and typed mentions/author
      • the JSON bodies of updateSeen/muteActor/follow/createRecord
      • typed parsing of posts and notifications
      • the timeline enumerator stopping on a repeated cursor after two requests
      • the list-member and notification enumerators carrying their filters on every page
      • a canonical createdAt from PostAsync
      • byte-for-byte round-trip of a non-canonical createdAt
      • ICursorPage<T> on every cursored app.bsky type
    • RichTextBuilderTests gains a snapshot test.
    • The guard's three tests pass with App.Bsky removed from the pending list.
  • Integration tests compile. They are gated on a PDS and were not run.

Deferred / flagged

Wire-fidelity gaps noticed while converting. JSON names are unchanged here; these are for #123:

  • FeedClient.SearchPostsAsync(tag) is one string, but the Lexicon's tag is an array (repeated key).
  • Missing parameters: listNotifications reasons[] and getLists purposes[].
  • Missing fields:
    • LikeRecord/RepostRecord via
    • LabelerServiceRecord labels/reasonTypes/subjectTypes/subjectCollections
    • Relationship blocking/blockedBy/blockingByList/blockedByList
  • StandardLabelValues.NotAvailable is "!no-unauthenticated", the same value as NoUnauthenticated. Its doc says takedown, so the value looks wrong.
  • Still raw JsonElement rather than typed unions/refs: ThreadgateRecord.Allow, ListRecord.Labels, GetRelationshipsResponse.Relationships, FeedViewPost.Reason, LabelerView.Creator.
  • docs/blazor.md shows <ProfileCard Actor="…" />, but the component's parameter is Profile. This predates this PR; left to #136.

Shared-file edits outside app.bsky:

  • TypedIdentifierGuardTests.cs: removed only the App.Bsky pending line, and added the eight App.Bsky exceptions directly after Models.Label.Uri.
  • CHANGELOG.md: one bullet each under Breaking changes and Added.
  • docs/identity-types.md: the "In Models and Clients" sentence now lists app.bsky.* as converted. Parts 3 and 4 will edit the same sentence.
  • AtProtoClient.cs: the facade helpers' createdAt, plus an unused using removed.
  • tests/…/Http/EmptyResponseBodyTests.cs: only the app.bsky call sites. Its tools.ozone lines are untouched.

🤖 Generated with Claude Code

Part of #119 (2/4: app.bsky). Builds on #154 (part 1) and follows the conventions it wrote down in `CONTRIBUTING.md`. This converts every model and client under `src/ATProtoNet/Lexicon/App/Bsky/**` to typed identifiers, `AtDatetime`, `IReadOnlyList<T>` and `ICursorPage<T>`, and adds the `Enumerate*` methods. ## How to review this The change is large but mostly mechanical. Suggested order: 1. **`tests/ATProtoNet.Tests/Conventions/TypedIdentifierGuardTests.cs`**: `ATProtoNet.Lexicon.App.Bsky.` is gone from `PendingConversions`, so the reflection guard now enforces this namespace. Eight new `Exceptions` entries each carry a reason: the generic `uri`-format link URLs, and `searchPosts`' `since`/`until`, which the Lexicon leaves format-less because they accept a bare date. 2. **`tests/ATProtoNet.Tests/Lexicon/App/Bsky/TypedBskyClientTests.cs`** (new) shows the behaviour end to end: what goes out on the wire, what comes back typed, and how the enumerators walk pages. It also checks by reflection that every `app.bsky` type with a `Cursor` implements `ICursorPage<T>`. 3. **The `*Client.cs` files** hold the real API decisions: parameter order, enumerator names, and which request types became internal. 4. **The `*Models.cs` files** are find-and-replace: `string` → the typed identifier, `List<T>` → `IReadOnlyList<T>`, plus `: ICursorPage<T>` with an explicit `Items`. JSON names are unchanged. 5. The rest are call sites: the Blazor components, one sample, the docs and the test fixtures (invalid CIDs such as `"bafy1"` became real ones, and invalid handles such as `"alice"` became `alice.test`). ## What changed ### Typed identifiers and `AtDatetime` (models) - **Actor:** `ProfileView*.Did`/`Handle` are `Did`/`Handle`, and `indexedAt`/`createdAt` are `AtDatetime?`. `ViewerState.Blocking`/`Following`/`FollowedBy` are `AtUri?`. `ProfileRecord.CreatedAt` is `AtDatetime?`. - **Feed:** - `PostView.Uri`/`Cid`/`IndexedAt` and `GeneratorView.Uri`/`Cid`/`Did`/`IndexedAt` are typed. - The viewer-state `Like`/`Repost` are `AtUri?`. - In `ThreadgateRecord`/`PostgateRecord`, `Post` is typed, and so are `HiddenReplies` and `DetachedEmbeddingUris` (`IReadOnlyList<AtUri>`). - `NotFoundPost`/`BlockedPost`/`SkeletonFeedPost`/`DescribeFeedGenerator*` are typed. - Every record's `CreatedAt` is `required AtDatetime`. - **Graph:** - `FollowRecord`/`BlockRecord`/`ListItemRecord.Subject` are `Did`, and `ListBlockRecord.Subject`, `ListItemRecord.List` and `StarterPackRecord.List` are `AtUri`. - The list and starter-pack views and `ListViewerState.Blocked` are typed. - `Relationship.Did`/`Following`/`FollowedBy`, `NotFoundActor.Actor` (`AtIdentifier`) and `GetRelationshipsResponse.Actor` are typed. - **Notification:** `NotificationView.Uri`/`Cid`/`ReasonSubject`/`IndexedAt`, `ListNotificationsResponse.SeenAt` and `RegisterPushRequest.ServiceDid` are typed. - **Labeler:** `LabelerView*.Uri`/`Cid`/`IndexedAt`, `LabelerViewerState.Like` and `LabelerServiceRecord.CreatedAt` are typed. - **Embed / RichText / Video:** `VideoView.Cid`, `MentionFeature.Did` and `JobStatus.Did` are typed. - **Deliberately left as `string`** (the guard lists them): `ExternalInfo.Uri`, `ExternalViewInfo.Uri`, `LinkFeature.Uri` and `RichTextBuilder.Link(uri)` are Lexicon `uri`, not `at-uri`. `searchPosts` `since`/`until` accept `YYYY-MM-DD`. ### Clients: typed parameters and one parameter order - `actor` parameters take an `AtIdentifier`. Post, feed, list, thread-root and starter-pack parameters take an `AtUri`, `cid` takes a `Cid?`, `seenAt` takes an `AtDatetime?`, and `LabelerClient.GetServicesAsync` takes `IEnumerable<Did>`. - Array inputs are `IEnumerable<T>`: `GetProfilesAsync`, `GetPostsAsync`, `GetFeedGeneratorsAsync`, `GetRelationshipsAsync(others)`, `GetStarterPacksAsync`, `PutPreferencesAsync` and `GetServicesAsync`. - Reordered methods, each now subject → filters → `limit` → `cursor`: - `GetTimelineAsync(algorithm, limit, cursor)` - `GetAuthorFeedAsync(actor, filter, includePins, limit, cursor)` - `ListNotificationsAsync(priority, seenAt, limit, cursor)` - `RichTextBuilder.Mention(Handle, Did)`. `Build()` returns `IReadOnlyList<Facet>?` as a snapshot, so appending after `Build()` no longer changes a list you already hold. - Facade: part 1 had already typed `LikeAsync`/`RepostAsync`/`FollowAsync`/the delete-by-URI helpers and `PostAsync`'s inputs. They now stamp `createdAt` with `AtDatetime.Now()`. Moving them to `client.Bsky` is #122. ### `ICursorPage<T>` and `Enumerate*` - All 22 cursored `app.bsky` responses implement `ICursorPage<T>`, with `Items` explicit over the Lexicon-named list. - 25 new enumerators, all on the shared `Pagination.EnumerateAsync`, which stops on a null, empty or repeated cursor: - **Actor:** `EnumerateSuggestionsAsync`, `EnumerateSearchActorsAsync` - **Feed:** - timelines and feeds: `EnumerateTimelineAsync`, `EnumerateAuthorFeedAsync`, `EnumerateFeedAsync`, `EnumerateListFeedAsync` - likes, reposts and quotes: `EnumerateActorLikesAsync`, `EnumerateLikesAsync`, `EnumerateRepostedByAsync`, `EnumerateQuotesAsync` - feed generators: `EnumerateActorFeedsAsync`, `EnumerateSuggestedFeedsAsync` - search: `EnumerateSearchPostsAsync` - **Graph:** - followers and follows: `EnumerateFollowersAsync`, `EnumerateFollowsAsync`, `EnumerateKnownFollowersAsync` - blocks and mutes: `EnumerateBlocksAsync`, `EnumerateMutesAsync` - lists: `EnumerateListsAsync`, `EnumerateListMembersAsync`, `EnumerateListBlocksAsync`, `EnumerateListMutesAsync` - starter packs: `EnumerateActorStarterPacksAsync`, `EnumerateSearchStarterPacksAsync` - **Notification:** `EnumerateNotificationsAsync` - The issue named a core set. For consistency I also added list feed, known followers, list blocks/mutes, actor feeds, suggested feeds and actor/searched starter packs. `getFeedSkeleton` has no enumerator, because it is a feed generator's own endpoint. - Naming: `Get*` drops the verb (`GetFollowers` → `EnumerateFollowers`). `Search*` keeps it (`EnumerateSearchPosts`), so a search enumerator is never mistaken for a listing. `getList` → `EnumerateListMembersAsync`. ### Internal request DTOs `PutPreferencesRequest`, `MuteActorRequest`, `MuteActorListRequest`, `MuteThreadRequest` and `UpdateSeenRequest` are now internal. `RegisterPushRequest` stays public, because `RegisterPushAsync` accepts it. ### Blazor (minimal; #136 rewires these) - `FeedView` parses its `FeedSource` string into `AtIdentifier`/`AtUri` and passes paging arguments by name. - `PostCard.FormatTime` takes the `AtDatetime`. - `ProfileCard`/`PostCard` read `Handle.Value` where they needed a `string`. - `_Imports.razor` adds `ATProtoNet.Identity`. ## Breaking changes and migration One `Breaking changes` bullet in CHANGELOG covers all of it: - **Typed values:** values from responses pass straight through. Parse literals where they enter your code: `AtIdentifier.Parse("alice.bsky.social")`, `AtUri.Parse("at://…")`, `Did.Parse(…)`. A `Did` or `Handle` converts implicitly to `AtIdentifier`, and every typed value converts implicitly to `string`. - **Timestamps:** stamp records with `AtDatetime.Now()`, and read `.Value` / `.TryGetValue(...)` / `.ToString()`. - **Reordered methods:** positional calls to `GetTimelineAsync`, `GetAuthorFeedAsync` and `ListNotificationsAsync` must pass the moved arguments by name. - **Lists:** `List<T>` → `IReadOnlyList<T>`. Copy (`[.. page.Feed]`) where you mutated. - **Internal DTOs:** call the client method instead of constructing the request type. - **Strict parsing:** an AppView response carrying an invalid identifier now throws `XrpcResponseFormatException`, the part 1 behaviour. A malformed `datetime` does not throw, because `AtDatetime` reads leniently. ## Verification - `dotnet build --no-incremental` (whole solution, including samples and integration tests): **0 warnings, 0 errors**. - Unit tests: **3175 passed, 0 failed, 5 skipped** (3180 total), 14 of them new. - `TypedBskyClientTests` (new, 13 tests) checks: - the exact query strings, including repeated `uris`, `seenAt` sent with its offset, and typed `mentions`/`author` - the JSON bodies of `updateSeen`/`muteActor`/`follow`/`createRecord` - typed parsing of posts and notifications - the timeline enumerator stopping on a repeated cursor after two requests - the list-member and notification enumerators carrying their filters on every page - a canonical `createdAt` from `PostAsync` - byte-for-byte round-trip of a non-canonical `createdAt` - `ICursorPage<T>` on every cursored `app.bsky` type - `RichTextBuilderTests` gains a snapshot test. - The guard's three tests pass with `App.Bsky` removed from the pending list. - Integration tests compile. They are gated on a PDS and were not run. ## Deferred / flagged Wire-fidelity gaps noticed while converting. JSON names are unchanged here; these are for #123: - `FeedClient.SearchPostsAsync(tag)` is one `string`, but the Lexicon's `tag` is an array (repeated key). - Missing parameters: `listNotifications` `reasons[]` and `getLists` `purposes[]`. - Missing fields: - `LikeRecord`/`RepostRecord` `via` - `LabelerServiceRecord` `labels`/`reasonTypes`/`subjectTypes`/`subjectCollections` - `Relationship` `blocking`/`blockedBy`/`blockingByList`/`blockedByList` - `StandardLabelValues.NotAvailable` is `"!no-unauthenticated"`, the same value as `NoUnauthenticated`. Its doc says takedown, so the value looks wrong. - Still raw `JsonElement` rather than typed unions/refs: `ThreadgateRecord.Allow`, `ListRecord.Labels`, `GetRelationshipsResponse.Relationships`, `FeedViewPost.Reason`, `LabelerView.Creator`. - `docs/blazor.md` shows `<ProfileCard Actor="…" />`, but the component's parameter is `Profile`. This predates this PR; left to #136. Shared-file edits outside `app.bsky`: - `TypedIdentifierGuardTests.cs`: removed only the `App.Bsky` pending line, and added the eight `App.Bsky` exceptions directly after `Models.Label.Uri`. - `CHANGELOG.md`: one bullet each under Breaking changes and Added. - `docs/identity-types.md`: the "In Models and Clients" sentence now lists `app.bsky.*` as converted. Parts 3 and 4 will edit the same sentence. - `AtProtoClient.cs`: the facade helpers' `createdAt`, plus an unused `using` removed. - `tests/…/Http/EmptyResponseBodyTests.cs`: only the `app.bsky` call sites. Its `tools.ozone` lines are untouched. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
feat!: typed identifiers, AtDatetime and enumerators for app.bsky (part of #119)
All checks were successful
CI / pds-integration (pull_request) Successful in 26s
Details
CI / build-and-test (pull_request) Successful in 1m0s
Details
Sync Closures to GitHub / sync-closure (pull_request) Successful in 6s
Details
9640429d49 
Part 2 of 4 of the typed public API, following the conventions part 1 set
down in CONTRIBUTING.md, for Lexicon/App/Bsky/**:

- Models carry Did, Handle, AtIdentifier, AtUri, Cid and AtDatetime
  wherever the Lexicon field has that format: profile and post views,
  viewer-state record URIs, graph records, notifications, labeler views,
  facets and embeds. Lists are IReadOnlyList<T>. The generic uri-format
  link URLs and searchPosts' since/until, which accept a bare date, stay
  string and are listed in the guard's exceptions.
- Client methods take typed parameters in one order: subject, filters,
  limit, cursor. GetTimelineAsync, GetAuthorFeedAsync and
  ListNotificationsAsync are reordered. Array inputs are IEnumerable<T>,
  RichTextBuilder.Mention takes (Handle, Did), and five single-use
  request bodies are internal.
- Every cursored app.bsky response implements ICursorPage<T>. Enumerate*
  methods for the feeds, likes, reposts, quotes, graph lists,
  notifications, suggestions and searches go through the shared
  paginator, so a repeated cursor ends the walk.
- The facade's Bluesky helpers stamp createdAt with AtDatetime.Now(). The
  Blazor components, docs, sample and tests follow, and the reflection
  guard now enforces the app.bsky namespace.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
opencode-bot commented 2026-09-24 23:09:16 +00:00
Collaborator
Copy link

🤖 Reviewing this PR — the review will link the full session transcript.

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

🤖 Reviewing this PR — the review will link the full session transcript. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-155/run-2026-09-24T23-09-16-245Z.md) — updating every ~5 minutes while the review runs.
Grandiras commented 2026-09-24 23:10:09 +00:00
Author
Owner
Copy link

@claude /review — large and mostly mechanical; the description opens with How to review this.

@claude /review — large and mostly mechanical; the description opens with *How to review this*.
claude-bot commented 2026-09-24 23:10:14 +00:00
Collaborator
Copy link

🤖 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/593); 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/ATProto.NET/pr-155/run-593.md) — updating every ~5 minutes while the task runs.
claude-reviewer approved these changes 2026-09-24 23:11:21 +00:00
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

This is exactly what the description says it is: a large, mechanical typed-identifier conversion for app.bsky.*, following the pattern established in #154. I sampled ActorClient, FeedClient, GraphClient, NotificationClient, LabelerClient, RichTextBuilder, and the model files, plus the Blazor call sites and docs.

Things that check out:

  • Parameter reordering (GetTimelineAsync, GetAuthorFeedAsync, ListNotificationsAsync) is applied consistently at every call site, including the Blazor FeedView.razor, which correctly switches to named arguments where the positional order changed.
  • ICursorPage<T> explicit implementations consistently point Items at the Lexicon-named list (Likes, Feed, Followers, etc.), and the new Enumerate* methods wire the right response/item type pairs through Pagination.EnumerateAsync.
  • RichTextBuilder.Build() now returns a defensive copy ([.. _facets]), which actually fixes the aliasing bug called out in the PR description (appending after Build() no longer mutates a previously returned list).
  • The since/until search-post filters and the generic uri-format link fields are deliberately left as string, matching the stated Lexicon-format rationale, and the PutPreferencesRequest/MuteActorRequest/etc. becoming internal lines up with the call sites now only constructing them via the client methods.
  • The AtProtoClient.cs facade swap from AtProtoJsonDefaults.NowTimestamp() to AtDatetime.Now() is applied uniformly, and the unused using ATProtoNet.Serialization; removal is consistent with that.
  • CHANGELOG.md gets both a Breaking changes and an Added bullet in the same commit as required by CLAUDE.md.

The diff was truncated before TypedIdentifierGuardTests.cs, the new TypedBskyClientTests.cs, and the tail of the call-site/doc changes, so I couldn't directly verify those, but the described verification (0 warnings/errors, 3175 tests passing including 14 new ones covering wire format and enumerator behavior) is consistent with what I could see in the sampled files. No correctness or security issues found in the reviewable portion.

⏱ 0m 52s · 1 turns · tokens: 77.8k in / 5.3k out (+23.9k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is exactly what the description says it is: a large, mechanical typed-identifier conversion for `app.bsky.*`, following the pattern established in #154. I sampled `ActorClient`, `FeedClient`, `GraphClient`, `NotificationClient`, `LabelerClient`, `RichTextBuilder`, and the model files, plus the Blazor call sites and docs. Things that check out: - Parameter reordering (`GetTimelineAsync`, `GetAuthorFeedAsync`, `ListNotificationsAsync`) is applied consistently at every call site, including the Blazor `FeedView.razor`, which correctly switches to named arguments where the positional order changed. - `ICursorPage<T>` explicit implementations consistently point `Items` at the Lexicon-named list (`Likes`, `Feed`, `Followers`, etc.), and the new `Enumerate*` methods wire the right response/item type pairs through `Pagination.EnumerateAsync`. - `RichTextBuilder.Build()` now returns a defensive copy (`[.. _facets]`), which actually fixes the aliasing bug called out in the PR description (appending after `Build()` no longer mutates a previously returned list). - The `since`/`until` search-post filters and the generic `uri`-format link fields are deliberately left as `string`, matching the stated Lexicon-format rationale, and the `PutPreferencesRequest`/`MuteActorRequest`/etc. becoming `internal` lines up with the call sites now only constructing them via the client methods. - The `AtProtoClient.cs` facade swap from `AtProtoJsonDefaults.NowTimestamp()` to `AtDatetime.Now()` is applied uniformly, and the unused `using ATProtoNet.Serialization;` removal is consistent with that. - CHANGELOG.md gets both a `Breaking changes` and an `Added` bullet in the same commit as required by CLAUDE.md. The diff was truncated before `TypedIdentifierGuardTests.cs`, the new `TypedBskyClientTests.cs`, and the tail of the call-site/doc changes, so I couldn't directly verify those, but the described verification (0 warnings/errors, 3175 tests passing including 14 new ones covering wire format and enumerator behavior) is consistent with what I could see in the sampled files. No correctness or security issues found in the reviewable portion. --- ⏱ 0m 52s · 1 turns · tokens: 77.8k in / 5.3k out (+23.9k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-155/run-593.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:9640429d49ceba1e7ab25ba1c691e068cefc82b4 -->
src/ATProtoNet/Lexicon/App/Bsky/Notification/NotificationClient.cs
@ -67,3 +85,3 @@
/// <param name="cancellationToken">Cancellation token.</param>
public async Task UpdateSeenAsync(
string seenAt, CancellationToken cancellationToken = default)
AtDatetime seenAt, CancellationToken cancellationToken = default)
claude-reviewer commented 2026-09-24 23:11:21 +00:00
Collaborator
Copy link

Minor doc nit: <see cref="AtDatetime.Now"/> references a method without parentheses; typically this would be written AtDatetime.Now() for clarity in the cref, though it should still resolve. Not a build-breaking issue since CS1591 only flags missing docs, not invalid crefs here.

Minor doc nit: `<see cref="AtDatetime.Now"/>` references a method without parentheses; typically this would be written `AtDatetime.Now()` for clarity in the cref, though it should still resolve. Not a build-breaking issue since CS1591 only flags missing docs, not invalid crefs here.
claude-bot commented 2026-09-24 23:11:21 +00:00
Collaborator
Copy link

🔧 Let Claude fix this — tick the box and claude-bot picks up the review above, on this PR branch. Nothing runs until you do.

  • @claude-bot Address the findings of the latest review on this pull request.
🔧 **Let Claude fix this** — tick the box and claude-bot picks up the review above, on this PR branch. Nothing runs until you do. - [ ] @claude-bot Address the findings of the latest review on this pull request. <!-- claude-fix-button:155 -->
Grandiras deleted branch issue-119-typed-bsky 2026-09-24 23:12:20 +00:00
opencode-reviewer approved these changes 2026-09-24 23:15:39 +00:00
opencode-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

This is a clean, disciplined part-2 conversion. I verified the typings against the current upstream Lexicons (actor/graph/feed/notification defs and the getRelationships, searchPosts, listNotifications query schemas) and every typing decision matches the Lexicon format — including the deliberate string exceptions (ExternalInfo.Uri, LinkFeature.Uri are uri, searchPosts since/until are format-less because they accept a bare ISO date). The ICursorPage<T> implementations are consistent (all 22 cursored responses, explicit Items over the Lexicon-named list), all 25 enumerators go through the shared Pagination.EnumerateAsync with the null/empty/repeated-cursor stop, parameter orders follow the CONTRIBUTING.md rule (subject → filters → limit → cursor), and the newly internal request DTOs have no remaining external call sites. The reflection guard now covers App.Bsky with eight reasoned exceptions, and TypedBskyClientTests covers the wire format (repeated uris keys, seenAt as its original text), typed parsing, enumerator paging behavior (repeated cursor and per-page filters), the canonical createdAt from the facade, the non-canonical round-trip, and the interface on every cursored type. The RichTextBuilder.Build() snapshot fix is a genuine (small) bug fix. The Blazor/sample/docs call sites compile via the implicit string conversions, and docs and CHANGELOG match the code. No correctness issues found.

Non-blocking notes for the #123 wire-fidelity list the PR already maintains:

  • The upstream Lexicon for listNotifications now marks its priority parameter as "Deprecated: this parameter is ignored" and its seenAt parameter as "Deprecated: this parameter is unsupported and will cause an error." ListNotificationsAsync/EnumerateNotificationsAsync still expose both (they did before this PR too), and the doc comment for seenAt ("filter new notifications since") no longer matches upstream behaviour — sending seenAt will error against the live Bluesky AppView. Worth adding to the Deferred/flagged list alongside the missing reasons[].
  • GetUnreadCountAsync's seenAt likewise deserves a doc check against upstream when #123 touches this surface (the typed AtDatetime itself is correct there).

Everything else flagged in the description (missing fields, raw JsonElement unions, tag as array, StandardLabelValues.NotAvailable) is correctly deferred with reasons.

⏱ 6m 22s · 59 steps · $0.2245 · tokens: 343.3k in / 27.0k out (+5316.4k cache-read) · Go: 5h 2% · wk 99% · mo 82%
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a clean, disciplined part-2 conversion. I verified the typings against the current upstream Lexicons (actor/graph/feed/notification defs and the `getRelationships`, `searchPosts`, `listNotifications` query schemas) and every typing decision matches the Lexicon format — including the deliberate string exceptions (`ExternalInfo.Uri`, `LinkFeature.Uri` are `uri`, `searchPosts` `since`/`until` are format-less because they accept a bare ISO date). The `ICursorPage<T>` implementations are consistent (all 22 cursored responses, explicit `Items` over the Lexicon-named list), all 25 enumerators go through the shared `Pagination.EnumerateAsync` with the null/empty/repeated-cursor stop, parameter orders follow the CONTRIBUTING.md rule (subject → filters → limit → cursor), and the newly internal request DTOs have no remaining external call sites. The reflection guard now covers `App.Bsky` with eight reasoned exceptions, and `TypedBskyClientTests` covers the wire format (repeated `uris` keys, `seenAt` as its original text), typed parsing, enumerator paging behavior (repeated cursor and per-page filters), the canonical `createdAt` from the facade, the non-canonical round-trip, and the interface on every cursored type. The `RichTextBuilder.Build()` snapshot fix is a genuine (small) bug fix. The Blazor/sample/docs call sites compile via the implicit string conversions, and docs and CHANGELOG match the code. No correctness issues found. Non-blocking notes for the #123 wire-fidelity list the PR already maintains: - The upstream Lexicon for `listNotifications` now marks its `priority` parameter as "Deprecated: this parameter is ignored" and its `seenAt` parameter as "Deprecated: this parameter is unsupported and will cause an error." `ListNotificationsAsync`/`EnumerateNotificationsAsync` still expose both (they did before this PR too), and the doc comment for `seenAt` ("filter new notifications since") no longer matches upstream behaviour — sending `seenAt` will error against the live Bluesky AppView. Worth adding to the Deferred/flagged list alongside the missing `reasons[]`. - `GetUnreadCountAsync`'s `seenAt` likewise deserves a doc check against upstream when #123 touches this surface (the typed `AtDatetime` itself is correct there). Everything else flagged in the description (missing fields, raw `JsonElement` unions, `tag` as array, `StandardLabelValues.NotAvailable`) is correctly deferred with reasons. --- ⏱ 6m 22s · 59 steps · $0.2245 · tokens: 343.3k in / 27.0k out (+5316.4k cache-read) · Go: 5h 2% · wk 99% · mo 82% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-155/run-2026-09-24T23-09-16-245Z.md) <!-- opencode-reviewed-head:9640429d49ceba1e7ab25ba1c691e068cefc82b4 --> <!-- opencode-verdict:approve -->
src/ATProtoNet/Lexicon/App/Bsky/Notification/NotificationClient.cs
@ -26,3 +27,3 @@
public Task<ListNotificationsResponse> ListNotificationsAsync(
int? limit = null,
bool? priority = null,
AtDatetime? seenAt = null,
opencode-reviewer commented 2026-09-24 23:15:39 +00:00
Collaborator
Copy link

Non-blocking, for #123: upstream's app.bsky.notification.listNotifications now documents priority as "Deprecated: this parameter is ignored" and seenAt as "Deprecated: this parameter is unsupported and will cause an error", so sending seenAt will fail against the live Bluesky AppView. This parameter existed before this PR (as a string), so it is not introduced here, but the doc comment ("filter new notifications since…") no longer matches upstream and this pair belongs on the Deferred/flagged list next to the missing reasons[] parameter.

Non-blocking, for #123: upstream's `app.bsky.notification.listNotifications` now documents `priority` as "Deprecated: this parameter is ignored" and `seenAt` as "Deprecated: this parameter is unsupported and will cause an error", so sending `seenAt` will fail against the live Bluesky AppView. This parameter existed before this PR (as a `string`), so it is not introduced here, but the doc comment ("filter new notifications since…") no longer matches upstream and this pair belongs on the Deferred/flagged list next to the missing `reasons[]` parameter.
opencode-bot commented 2026-09-24 23:15:39 +00:00
Collaborator
Copy link

🔧 Let opencode-bot fix this — tick the box and opencode-bot picks up the review above, on this PR branch. Nothing runs until you do.

  • @opencode-bot Address the findings of the latest review on this pull request.
🔧 **Let opencode-bot fix this** — tick the box and opencode-bot picks up the review above, on this PR branch. Nothing runs until you do. - [ ] @opencode-bot Address the findings of the latest review on this pull request. <!-- opencode-fix-button:155 -->
Sign in to join this conversation.
Reviewers
No reviewers
claude-reviewer
opencode-reviewer
Labels
Clear labels   
breaking-change
Introduces a breaking API change

  
bug
Something is not working correctly

  
documentation
Documentation improvements

  
duplicate
This issue or PR already exists

  
enhancement
New feature or improvement

  
good first issue
Good for newcomers to the project

  
help wanted
Extra attention is needed

  
performance
Performance-related issue or improvement

  
question
Further information is requested

  
wontfix
This will not be worked on

No labels
breaking-change
bug
documentation
duplicate
enhancement
good first issue
help wanted
performance
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
5 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/ATProto.NET!155
No description provided.