Make Subscription.LastSeenAt optional (*time.Time)#151
Conversation
📝 WalkthroughWalkthroughMakes model.Subscription.LastSeenAt optional by changing its type from time.Time to *time.Time with ChangesSubscription LastSeenAt Optionality
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/model/model_test.go (1)
400-406: ⚡ Quick winUse the shared
roundTriphelper for the equality path.These two blocks reimplement the same marshal/unmarshal/deep-equal flow that
roundTripalready centralizes at the bottom of this file. Keeping thelastSeenAtomission assertions here makes sense, but the round-trip part should stay on the helper for consistency with the rest ofpkg/model/model_test.go.As per coding guidelines,
pkg/model/model_test.gomust verify all domain types marshal/unmarshal correctly via a genericroundTriphelper.Also applies to: 428-433
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/model/model_test.go` around lines 400 - 406, Replace the manual json.Marshal/json.Unmarshal/reflect.DeepEqual sequence used to test model.Subscription with the shared roundTrip helper: keep the assertions that verify lastSeenAt is omitted, but remove the explicit marshal/unmarshal/deep-equal block and instead call roundTrip with the subscription value (and destination if your helper requires it) so the equality path uses the centralized roundTrip helper; apply the same change to the other similar block later in the file that checks Subscription round-tripping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/model/model_test.go`:
- Around line 400-406: Replace the manual
json.Marshal/json.Unmarshal/reflect.DeepEqual sequence used to test
model.Subscription with the shared roundTrip helper: keep the assertions that
verify lastSeenAt is omitted, but remove the explicit
marshal/unmarshal/deep-equal block and instead call roundTrip with the
subscription value (and destination if your helper requires it) so the equality
path uses the centralized roundTrip helper; apply the same change to the other
similar block later in the file that checks Subscription round-tripping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ead07fbb-fc2b-499a-a25c-56954cc33606
📒 Files selected for processing (3)
docs/superpowers/specs/2026-04-30-optional-lastseenat-design.mdpkg/model/model_test.gopkg/model/subscription.go
Change LastSeenAt from time.Time to *time.Time with omitempty tags so "never seen" is represented as nil rather than the Go zero time. Mirrors the pattern already used by HistorySharedSince in the same struct and by ThreadSubscription.LastSeenAt. No production code reads or writes the field today, so existing creation sites continue to compile unchanged.
saurav-err-137
force-pushed
the
claude/optional-lastseenAt-field-FJGwl
branch
from
0fece90 to
1193d07
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/model/model_test.go (1)
411-431: ⚡ Quick winAdd BSON coverage for
LastSeenAtnil-omission and non-nil round-trip.The PR adds
omitemptyto the BSON tag (bson:"lastSeenAt,omitempty") — a behavioural change that affects MongoDB persistence (message-worker/store_mongo.gowrites"lastSeenAt": sub.LastSeenAt). The new subtests only verify JSON omission; neither the nil-drops-field nor the non-nil-round-trips claim is exercised at the BSON level. The design spec explicitly states both behaviours, so a test locking them in is warranted.✅ Suggested addition to
TestSubscriptionJSON+ t.Run("bson: lastSeenAt omitted when nil", func(t *testing.T) { + s := model.Subscription{ + ID: "s1", + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + RoomType: model.RoomTypeChannel, + SiteID: "site-a", + Roles: []model.Role{model.RoleMember}, + JoinedAt: time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), + } + data, err := bson.Marshal(&s) + require.NoError(t, err) + var raw bson.M + require.NoError(t, bson.Unmarshal(data, &raw)) + _, present := raw["lastSeenAt"] + assert.False(t, present, "lastSeenAt should be omitted from BSON when nil") + }) + + t.Run("bson: lastSeenAt round-trips when set", func(t *testing.T) { + lsa := time.Date(2026, 1, 2, 0, 0, 0, 0, time.UTC) + s := model.Subscription{ + ID: "s1", + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + RoomType: model.RoomTypeChannel, + SiteID: "site-a", + Roles: []model.Role{model.RoleMember}, + JoinedAt: time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), + LastSeenAt: &lsa, + } + data, err := bson.Marshal(&s) + require.NoError(t, err) + var dst model.Subscription + require.NoError(t, bson.Unmarshal(data, &dst)) + require.NotNil(t, dst.LastSeenAt) + assert.Equal(t, lsa.UTC(), dst.LastSeenAt.UTC()) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/model/model_test.go` around lines 411 - 431, Add BSON-level assertions for Subscription.LastSeenAt in the TestSubscriptionJSON tests: for the nil case, marshal the Subscription with bson.Marshal, unmarshal into map[string]any (or bson.M) and assert "lastSeenAt" is absent; for the non-nil case, set LastSeenAt, bson.Marshal then bson.Unmarshal back into a Subscription and assert LastSeenAt round-trips equal. Use the existing Subscription type, the test helper roundTrip pattern, and the bson package (e.g., bson.Marshal/bson.Unmarshal or bson.M) to mirror the JSON tests so the bson:"lastSeenAt,omitempty" behavior is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-04-30-optional-lastseenat-design.md`:
- Around line 20-22: The doc's assertion is wrong: ThreadSubscription.LastSeenAt
is a *time.Time pointer without `omitempty` (see the architectural note that
BSON nil must be encoded as explicit null); update the text to state that
Subscription should mirror the pointer pattern of ThreadSubscription (i.e., use
*time.Time to represent "never seen") but not apply `omitempty` there, and
clarify that only HistorySharedSince is intended to align with `omitempty`.
Reference ThreadSubscription.LastSeenAt, Subscription, and HistorySharedSince in
the corrected sentence so the distinction and rationale are unambiguous.
---
Nitpick comments:
In `@pkg/model/model_test.go`:
- Around line 411-431: Add BSON-level assertions for Subscription.LastSeenAt in
the TestSubscriptionJSON tests: for the nil case, marshal the Subscription with
bson.Marshal, unmarshal into map[string]any (or bson.M) and assert "lastSeenAt"
is absent; for the non-nil case, set LastSeenAt, bson.Marshal then
bson.Unmarshal back into a Subscription and assert LastSeenAt round-trips equal.
Use the existing Subscription type, the test helper roundTrip pattern, and the
bson package (e.g., bson.Marshal/bson.Unmarshal or bson.M) to mirror the JSON
tests so the bson:"lastSeenAt,omitempty" behavior is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29ea5c1c-a253-4dfc-8fbf-d2390e3557b9
📒 Files selected for processing (3)
docs/superpowers/specs/2026-04-30-optional-lastseenat-design.mdpkg/model/model_test.gopkg/model/subscription.go
✅ Files skipped from review due to trivial changes (1)
- pkg/model/subscription.go
| - The codebase already models the analogous "never seen" state on | ||
| `ThreadSubscription` as `*time.Time` with `omitempty`. Subscription should | ||
| follow the same convention. |
There was a problem hiding this comment.
Inaccurate claim about ThreadSubscription.LastSeenAt using omitempty.
ThreadSubscription.LastSeenAt is *time.Time without omitempty — and its source file carries an explicit architectural note:
"Never add omitempty: unreadThreadsPipeline relies on BSON encoding nil as explicit null, not a missing field."
This sentence should clarify that Subscription only mirrors the *time.Time pointer pattern from ThreadSubscription, while the omitempty alignment is specifically with HistorySharedSince.
📝 Suggested correction
-The codebase already models the analogous "never seen" state on
-`ThreadSubscription` as `*time.Time` with `omitempty`. Subscription should
-follow the same convention.
+The codebase already models the analogous "never seen" state on
+`ThreadSubscription` as `*time.Time` (though without `omitempty` there, as the
+pipeline relies on an explicit BSON null). For `Subscription`, both the pointer
+type and the `omitempty` behaviour mirror the sibling `HistorySharedSince` field.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - The codebase already models the analogous "never seen" state on | |
| `ThreadSubscription` as `*time.Time` with `omitempty`. Subscription should | |
| follow the same convention. | |
| - The codebase already models the analogous "never seen" state on | |
| `ThreadSubscription` as `*time.Time` (though without `omitempty` there, as the | |
| pipeline relies on an explicit BSON null). For `Subscription`, both the pointer | |
| type and the `omitempty` behaviour mirror the sibling `HistorySharedSince` field. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-04-30-optional-lastseenat-design.md` around lines
20 - 22, The doc's assertion is wrong: ThreadSubscription.LastSeenAt is a
*time.Time pointer without `omitempty` (see the architectural note that BSON nil
must be encoded as explicit null); update the text to state that Subscription
should mirror the pointer pattern of ThreadSubscription (i.e., use *time.Time to
represent "never seen") but not apply `omitempty` there, and clarify that only
HistorySharedSince is intended to align with `omitempty`. Reference
ThreadSubscription.LastSeenAt, Subscription, and HistorySharedSince in the
corrected sentence so the distinction and rationale are unambiguous.
3 participants
Summary
Changes
Subscription.LastSeenAtfrom a requiredtime.Timefield to an optional*time.Timefield, allowing it to benilwhen a subscription has not yet been viewed. This aligns with the existing pattern used byHistorySharedSincein the same struct andThreadSubscription.LastSeenAt.Changes
LastSeenAtfield type fromtime.Timeto*time.Timewithomitemptytags on both JSON and BSON, matching the pattern ofHistorySharedSinceTestSubscriptionJSONto:LastSeenAttimestamp in the "with optional fields set" sub-testlastSeenAtis omitted from JSON whennilrequireassertions for cleaner test codeImplementation Details
Subscription.LastSeenAt, so this is a pure compile-time change at the model layernil, the field is dropped from both JSON output and BSON documents via theomitemptytagshttps://claude.ai/code/session_01BBCq6GLGp2wdYua9Hbub3Y
Summary by CodeRabbit
Documentation
New Features
Tests