[thomasluizon]

Closes #341

Summary

• Adds paired optional year and month query parameters to the existing recap route for closed calendar months.
• Preserves every rolling period request, the five period tokens, the free access path, and the /r/{code} referral route.
• Returns resolved closed month bounds and a real goal completion count derived from nondeleted goals in the user's timezone.
• Computes the first closed month response from the recap only historical occurrence path, then persists the complete response by user, DateFrom, and DateTo.
• Serves every later request from the stored row, including goal completions and the share link, so cadence edits and process restarts cannot change a previously opened month.
• Uses a transaction advisory lock and a unique database index to keep concurrent first requests on one immutable snapshot.
• Serializes first snapshot creation and full account reset on the same account scoped transaction lock.
• Makes reset wait for an in progress snapshot and delete it, while a recap that waits for reset reads only post reset data before storing.
• Clears the target user's stored recap snapshots inside the existing account reset transaction while preserving other users' rows.
• Restores live streak and legacy metric scheduling to their current DueDate alignment instead of the recap only historical anchor.
• Keeps RetrospectivePeriodRange and the retrospective payload unchanged, and documents that weekly consistency is ordered Monday through Sunday.

Persistence choice

Persistence was chosen over in process caching because habit cadence has no history. Recomputing after a cadence edit or process restart cannot reproduce the schedule that existed when the month was first opened. The stored row is therefore the durable source for that account and resolved calendar window.

Validation

• dotnet build Orbit.slnx completed with 0 errors.
• dotnet test Orbit.slnx passed all 5,838 tests.
• The focused recap and reset suite passed 19 tests.
• The coordinated race tests force both lock orders and prove no pre reset snapshot survives.
• The cadence change streak regression, cadence change closed recap snapshot, and account reset isolation cases pass.
• The additive PersistClosedMonthRecaps migration uses EF table and index operations and keys rows by UserId, DateFrom, and DateTo.

Assumptions

• The closed request uses year and month beside period=month; I rejected new parameter names because these match the route fields and keep shared links readable.
• The account's first localized calendar month is addressable even when signup occurred after its first day; I rejected requiring a full post signup month because the ticket says the first account month onward.
• A habit remains applicable on its localized deletion date; I rejected excluding the whole date because the habit can have applied and been completed before deletion that day.
• The habit model has no pause field or pause history; I used its persisted ScheduledStartDate, CreatedAtUtc, EndDate, and DeletedAtUtc lifecycle evidence and rejected inventing a pause history schema in this ticket.
• The ticket's transcribed Orbit Wrapped canvas requirements are the design authority in this run; DesignSync was unavailable and the work order prohibited browser access, so I rejected adding behavior not stated in the ticket.

[thomasluizon]

Intended approach

I will extend the existing gamification recap route with optional year and month query parameters. The controller will resolve either the unchanged rolling range or a new closed calendar month range after obtaining the user's local today and account creation instant. The closed resolver will live beside RetrospectivePeriodRange without modifying it.

I will add the resolved optional date bounds and goal completion count to RecapResponse, preserve the period token, and keep the existing /r/{code} referral link. GetRecapQueryHandler will count nondeleted goals by CompletedAtUtc within timezone converted UTC bounds. I will use a closed month memory cache keyed by user, year, and month with no expiration; the database remains the durable source, so any process restart can recompute every historical month.

The implementation will land in the gamification controller and application query, validator, a new closed month resolver, the goal read abstraction and implementation if the existing repository cannot express the count, dependency registration only if a new service requires it, and focused unit tests in the matching test projects. This keeps one route and one response contract. I rejected extending RetrospectivePeriodRange because its contract is rolling, adding another endpoint because GetRecapQuery already accepts date bounds, and changing RetrospectiveMetrics because that would alter the retrospective contract.

[pullfrog]

✅ No new issues found.

Reviewed changes I reviewed the initial scaffold commit at 9380b05; it contains no code or behavioral changes.

• Scaffold only: The single commit changes zero files, so no implementation, contract, or tests are present yet.

  ^{｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[pullfrog]

Important

Closed-month recaps can cache incorrect historical habit metrics and should be corrected before merge.

Reviewed changes I reviewed the closed-month recap implementation added since the prior Pullfrog review.

• Resolved closed months: Added paired year and month query parameters, local-calendar closure checks, account-age validation, and named failures while preserving rolling period requests.

• Expanded recap data: Added timezone-bounded goal completion counts, optional resolved date bounds, and calendar parameters in referral links.

• Cached closed responses: Added per-user, per-month in-memory snapshots without expiration while retaining database recomputation after process restart.

• Extended coverage and documentation: Added resolver, validator, handler, controller, timezone-boundary, free-access, and cache tests, plus the Monday-through-Sunday consistency contract.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

@pullfrog review

[pullfrog]

Important

The latest architecture-map refresh introduces no new defect, but the prior blocking finding remains unresolved: closed-month habit metrics still use each habit's live DueDate and can permanently cache incorrect historical values.

Reviewed changes I reviewed the complete branch at 5485ff5, including the generated architecture refresh added after the prior Pullfrog review.

• Closed-month routing: The existing recap endpoint accepts paired year and month parameters, resolves completed local calendar months, and preserves rolling-period behavior.
• Historical recap assembly: The query adds account-age checks, timezone-bounded goal completions, resolved date bounds, and calendar-aware referral links.
• Closed response caching: Each user-month response is stored in memory without expiration and returned on later requests.
• Tests and architecture map: Resolver, validator, handler, controller, timezone-boundary, plan-access, and cache coverage are accompanied by regenerated architecture artifacts.
• Cross-repo contract: The API additions are append-only; the current shared consumer still exposes only the rolling-period request and ignores the new response fields.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

Round 1 approach

I will add one historical occurrence path beside the live forward scheduler. It will align recurrence to ScheduledStartDate, bound activity by the localized creation, end, and deletion dates, and retain the live GetScheduledDates behavior unchanged. Closed month recap metrics will select that path and load soft deleted habits with their active logs so later deletion does not erase a month that already happened.

I will replace the calendar token cache key with the resolved DateFrom and DateTo values. The cache remains an in process permanent snapshot, and a deployment restart deliberately recomputes from the corrected historical path so values produced by the earlier DueDate based implementation do not survive.

The changes will land in HabitScheduleService, RetrospectiveMetricsCalculator, the recap handler, the generic read port needed to include filtered history, and focused application and infrastructure tests. I rejected changing GetScheduledDates because live callers require its moving DueDate cursor, and I rejected persisting recap snapshots because this ticket introduces no snapshot schema or close job.

[pullfrog]

Important

The historical fix prevents live DueDate advancement from corrupting recurring metrics, but it reintroduces deleted logs and misdates postponed one-time tasks.

Reviewed changes I reviewed ab46bdb, which replaces the previous closed-month scheduling path and broadens the historical schedule helper's callers.

• Anchored recurring history: Switched closed recaps to ScheduledStartDate based recurrence bounded by localized habit lifecycle dates.
• Restored deleted habit history: Added an unfiltered repository read so habits deleted after the requested month remain available to historical metrics.
• Unified historical scheduling: Reused the new historical path in streak scheduling and legacy habit-metric inference.
• Strengthened regressions: Added focused tests for advanced due dates, localized deletion bounds, ignored root filters, and byte-identical closed metrics.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

Round 2 approach

I will make active log filtering explicit in both the recap include and RetrospectiveMetricsCalculator so IgnoreQueryFilters can recover a deleted habit without recovering its deleted completion logs. I will add coverage proving a removed completion does not contribute to closed month totals or active days.

I will keep ScheduledStartDate as the recurrence anchor only for recurring habits. A one time task has one current scheduled occurrence, so its historical date will remain DueDate after postponement. I will add a focused scheduler regression for that case and leave recurring schedule behavior unchanged.

[thomasluizon]

@pullfrog review

[pullfrog]

✅ No new issues found.

Reviewed changes I reviewed the five-file a84f330 delta that addresses the two remaining historical recap regressions.

• Excluded removed activity: Added explicit soft-delete predicates to unfiltered log loading and every retrospective completion aggregation.

• Preserved postponed tasks: Historical one-time occurrences now use the current DueDate, while recurring schedules remain anchored to ScheduledStartDate.

• Added regression coverage: Added exact closed-recap and historical-schedule tests for unlogged completions and postponed one-time tasks.

  ^{｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[pullfrog]

Important

The new historical scheduler regresses streak calculations after a started habit changes cadence.

Reviewed changes I reviewed the complete branch at a84f330, including the closed-month fixes added since the prior Pullfrog review.

• Closed-month routing: The existing recap endpoint now accepts paired year and month parameters, resolves completed local calendar months, and preserves rolling-period behavior.
• Historical recap data: Closed recaps load deleted habit roots, exclude deleted logs, derive recurring occurrences from historical scheduling, count timezone-bounded goal completions, and return resolved date bounds.
• Caching and links: Closed responses are cached by user and resolved date window, and referral links preserve the requested calendar parameters.
• Shared scheduling: The historical occurrence helper now serves recap metrics, streak scheduling, and legacy habit-metric inference, with focused regression coverage.
• Contracts and generated artifacts: The response additions remain append-only for installed clients, while OpenAPI and architecture artifacts reflect the API changes.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

Round 3 approach

I will restore live streak scheduling to the existing forward occurrence path and reserve the historical scheduler for first time closed month recap computation. Its documentation will state the cadence history limitation and why stored snapshots are required.

I will add an additive closed recap snapshot entity and migration keyed by user, DateFrom, and DateTo. The recap handler will read that row first, compute the response only when no row exists, then persist the complete closed month payload including goal completions. Rolling requests will continue to bypass snapshot storage, and the closed month resolver will continue to reject an unfinished month before the handler can store anything.

The persistence change will land in the domain model, application persistence port and recap handler, infrastructure mapping and migration, and the existing focused test projects. I will add regressions for live streaks after cadence changes and byte identical closed month responses after cadence changes. I rejected memory caching because cadence has no history, so recomputation after a process restart cannot reproduce the original response.

[pullfrog]

Important

Account resets retain durable closed-month snapshots, so reset users can still receive progress data that should have been cleared.

Reviewed changes I reviewed the persistence and scheduling changes added in bf4e03a since the prior Pullfrog review.

• Persisted closed recaps: Replaced process-local caching with a database snapshot keyed by user and resolved month, protected by a transaction advisory lock and unique index.
• Restored live schedule alignment: Separated recap-only historical recurrence from live streak and legacy metric scheduling so current cadence uses the current DueDate anchor.
• Added persistence coverage: Added the snapshot entity and guards, EF configuration and migration, storage tests, immutable response regressions, and a cadence-change streak regression.

⚠️ Account resets retain persisted recaps

ResetAccountCommand delegates to AccountResetRepository.DeleteAllUserDataAsync, but that cleanup does not delete the new ClosedMonthRecaps rows. Because reset preserves the User row, requesting a previously opened month after reset returns the old immutable snapshot instead of the cleared account history.

Technical details

# Account resets retain closed month snapshots

## Affected sites
- `src/Orbit.Infrastructure/Persistence/AccountResetRepository.cs:8-152` does not delete rows from `ClosedMonthRecaps`.
- `src/Orbit.Infrastructure/Persistence/OrbitDbContext.cs:73,137-144` adds the user-owned snapshot table with cascade behavior that only runs when the user row is deleted.
- `src/Orbit.Application/Profile/Commands/ResetAccountCommand.cs:29-35` clears owned data while deliberately preserving the user row.
- `src/Orbit.Application/Gamification/Queries/GetRecapQuery.cs:76-82` returns a stored snapshot before reading the reset account's current data.

## Required outcome
- Remove the target user's closed month snapshots inside the account-reset transaction while preserving other users' rows.
- Add reset repository coverage proving a target snapshot is deleted and an unrelated user's snapshot remains.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

Round 4 approach

I will extend AccountResetRepository's existing user-owned data cleanup to delete ClosedMonthRecaps for the target user inside the same reset transaction. The user row remains intact, so the table's account deletion cascade cannot cover this reset path.

I will add repository coverage with one snapshot for the reset user and one for another user, then assert that reset removes only the target snapshot. This lands only in the reset repository and its persistence tests because the snapshot entity, route behavior, and migration are already correct.

[pullfrog]

Important

Account reset can still retain pre-reset recap data when a first-time recap is persisted concurrently.

Reviewed changes I reviewed the account-reset cleanup added since the prior Pullfrog review at bf4e03a.

• Deleted stored recaps: Added target-user ClosedMonthRecaps deletion inside the existing reset transaction.
• Added isolation coverage: Seeded snapshots for the reset user and another user, then asserted that only the target user's rows are removed.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

@pullfrog review

[pullfrog]

Important

Account reset can still retain and serve a pre-reset closed-month snapshot when first-time recap creation commits after the reset.

Reviewed changes I reviewed the complete branch at 1fe77fd, including the latest architecture refresh and merge from main.

• Closed-month routing: The recap endpoint accepts paired year and month parameters, resolves completed local calendar months, and preserves rolling-period requests.
• Durable immutable recaps: First-time responses are stored by user and resolved date window behind a transaction advisory lock and unique index.
• Historical metrics: Closed recaps recover deleted habit roots, exclude deleted logs, bound occurrences by localized lifecycle dates, and count goal completions using timezone-converted UTC bounds.
• Live scheduling isolation: Live streak and legacy metric paths retain current DueDate alignment while recap-only scheduling uses the historical anchor.
• Lifecycle cleanup: Account reset deletes existing recap rows for the target user, but it is not synchronized with concurrent snapshot creation.
• Contract review: The response additions and query parameters are additive; the current mobile shared schema ignores the new fields and has no paired open consumer PR.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

Round 3 approach

I will give closed month recap creation and account reset one shared advisory lock key derived only from the account id. Recap creation will keep its first storage probe as a fast path, then acquire the account lock and perform every source data read used for a new snapshot after that lock. Reset will acquire the same lock inside its existing transaction before any deletion.

The implementation will land in a shared application lock key helper, GetRecapQuery, ResetAccountCommand, and focused application tests. I will first add a coordinated race test that pauses recap creation at the lock, lets a full reset win, then releases recap creation and proves no pre reset figures are persisted. I will also cover the reverse order and assert the lock precedes reset deletion.

I rejected a conditional insert because the existing transaction advisory lock already supplies the required serialization primitive. I also rejected retaining the month in the lock key because reset must serialize with every first time recap creation for the account.

[pullfrog]

✅ No new issues found.

Reviewed changes I reviewed the two commits added since the prior Pullfrog review at 1fe77fd.

• Serialized recap creation and reset: Replaced month-specific recap locks with a shared per-user transaction advisory lock acquired before recap source reads and reset deletion.
• Covered both race orderings: Added coordinated tests proving reset removes an in-progress recap snapshot and a recap waiting for reset reads only post-reset data.
• Refreshed architecture artifacts: Updated the generated architecture maps to include the shared lock and race tests.

  ^{｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
92.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud