Skip to content

Fix calendar event timezone projection - #521

Open
thomasluizon wants to merge 11 commits into
mainfrom
fix/ticket-526-calendar-tz
Open

thomasluizon wants to merge 11 commits into
mainfrom
fix/ticket-526-calendar-tz

Conversation

@thomasluizon

@thomasluizon thomasluizon commented Sep 13, 2026

Copy link
Copy Markdown
Owner

Closes #526

Summary

  • Project timed Google event dates and times into the account timezone.
  • Preserve Google all day dates and null times as floating values.
  • Persist both UTC boundary instants for stored auto sync suggestions.
  • Omit EndTime when EndUtc is unavailable, the local date changes, or the projected display minute does not advance.
  • Compare suggestion dates with account local today after projection.
  • Omit a projected event when its date moves and its unchanged recurrence rule contains BYDAY.

Server-side recurrence safety

Both calendar event feeds now reject the same unrepresentable shape. This includes stored suggestion rows written before this change.

Same-date BYDAY rules, date-shifted rules without BYDAY, and date-shifted nonrecurring events remain available unchanged. No RecurrenceRule value is rewritten.

Omission is the honest fallback, but it has a product cost. A Tokyo person with a UTC-scheduled weekly event can lose the ability to import it, and the list gives no reason.

Filed, not fixed

thomasluizon/orbit-tickets#569 tracks the richer product path. Its current body still proposes the rejected optional response field, so its scope needs correction before implementation.

The target is occurrence-aware handling inside the existing 60-day feed. A provable WEEKLY or DAILY rule with plain BYDAY and no ordinal, BYSETPOS, or BYMONTHDAY can be shifted when its first and last occurrence have the same projection offset.

Test evidence

  • Before changing tests, dotnet test tests/Orbit.Application.Tests --filter "FullyQualifiedName~GetCalendarEventsQueryHandlerTests|FullyQualifiedName~GetCalendarSyncSuggestionsQueryHandlerTests" passed 37 of 37 tests with the defect present.
  • Before the implementation, dotnet test tests/Orbit.Application.Tests --filter "FullyQualifiedName~Handle_RecurringByDayEventCrossingAccountDate_OmitsEvent|FullyQualifiedName~Handle_StoredByDaySuggestionCrossingAccountDate_OmitsEvent" failed both tests because each feed returned one date-shifted BYDAY event.
  • After the implementation, the focused query command passed 38 of 38 tests.
  • dotnet build Orbit.slnx completed with 0 errors and 15 existing warnings.
  • dotnet test --no-build --no-restore passed 6,262 tests: 3,408 Application, 2,237 Infrastructure, 585 Domain, and 32 Analyzer tests.
  • Changed C# files contain no bare // comments. The only match is an existing XML documentation line.

Assumptions

  • EndUtc remains the last optional CalendarEventItem field from the original implementation. Reordering existing positional fields was rejected.

Round 5: the refusal moves server side

Head 44611e5f.

The review was right and the earlier disposition on this pull request was wrong. Filing the refusal
as a client-side ticket protects only builds that ship after it, and the Android fleet lags through
the Play store, so every installed build would have kept importing the wrong schedule. A server-side
problem needs a server-side answer.

When the account-timezone projection moves an occurrence's DATE and the event's RecurrenceRule
carries a BYDAY term, the projected start and the pass-through rule disagree. That event is now
omitted from the import feed rather than offered.

HasUnrepresentableRecurrenceAfterProjection is deliberately narrow. It returns false unless BOTH
are true: the projected StartDate differs from the source StartDate, and the rule body carries a
BYDAY= term. An event whose projection leaves the date alone, and a recurring event with no
BYDAY, are both returned exactly as before. GetCalendarSyncSuggestionsQuery applies the same
test, so a stored suggestion row written before this change cannot resurrect the event on read.

ProjectRecurrenceRule stays deleted, and no RecurrenceRule value this endpoint emits differs
from the one Google returned.

The cost, stated rather than hidden

A person in Tokyo with a UTC-scheduled weekly event loses the ability to import it at all, and the
list gives no reason. That is worse than importing it correctly and better than importing it on the
wrong days, which is the only choice available inside this pull request's scope.

thomasluizon/orbit-tickets#569 is rescoped to close that gap properly: shift the provable subset
instead of omitting it. A FREQ=WEEKLY or FREQ=DAILY rule with a plain BYDAY, no ordinal
prefix, no BYSETPOS and no BYMONTHDAY, whose projection offset is identical for the first and
last occurrence inside the feed's existing 60-day bound, can be shifted correctly. That needs
occurrence expansion over the bounded window rather than the regex round 4 removed.

Test evidence for 44611e5f

Green, the Calendar suite at this head:

Aprovado!  – Com falha: 0, Aprovado: 171, Ignorado: 0, Total: 171

Red proved after the fact, by reverting only the two query files to 44611e5f~1:

Com falha! – Com falha: 2, Aprovado: 169, Ignorado: 0, Total: 171

Com falha Orbit.Application.Tests.Queries.Calendar.GetCalendarEventsQueryHandlerTests
          .Handle_RecurringByDayEventCrossingAccountDate_OmitsEvent
Com falha Orbit.Application.Tests.Queries.Calendar.GetCalendarSyncSuggestionsQueryHandlerTests
          .Handle_StoredByDaySuggestionCrossingAccountDate_OmitsEvent

Both paths fail, the live feed and the stored suggestion, which is what proves the filter is applied
in both places rather than only where it is easy. Restored with
git checkout 44611e5f -- src/Orbit.Application/Calendar/Queries/, after which
git status --porcelain printed nothing and the suite returned to 171 of 171.

@thomasluizon

Copy link
Copy Markdown
Owner Author

Approach: add EndUtc beside StartUtc in CalendarEventItem and resolve both instants in GoogleCalendarEventFetcher. Add one shared account-timezone projection for timed events, then call it from GetCalendarEventsQueryHandler and GetCalendarSyncSuggestionsQuery before filtering or returning values. Update RunCalendarAutoSyncCommand to consume StartUtc directly. Add focused unit coverage in the existing calendar query, suggestion, fetcher, and auto-sync test files. This keeps the wire contract unchanged and preserves Google all-day dates. I rejected client conversion because the response omits the instant and offset. I rejected converting all-day values because they are floating dates, not instants.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

This PR currently contains no implementation to review, so merging it would not fix the calendar timezone projection defect.

Reviewed changes Reviewed the complete diff at 31b8b5a and the current calendar timezone seams.

  • Branch state The sole commit is empty and the head tree matches main, with zero changed files.
  • Planned scope The PR discussion describes changes to event mapping, timezone projection, auto sync, and tests, but none are present at this head.

⚠️ The timezone fix is absent

The current head is tree-identical to main, so timed events retain the existing projection behavior and the stated defect remains. Push the planned implementation and focused unit coverage before this PR is merged.

Technical details
# The timezone fix is absent

## Affected sites
- PR head `31b8b5a2157bc52ff84f583b622b35836129df28` contains zero changed files.

## Required outcome
- Implement and test projection of timed calendar event start and end values into the account timezone.
- Preserve Google all-day values as floating dates.

Pullfrog  | Fix it ➔View workflow run | Using GPT Sol𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The projected end-time representation must be made safe for persisted suggestions and account-local midnight crossings before merge.

Reviewed changes Reviewed the calendar timezone implementation added since the prior empty-scaffold review.

  • Captured UTC boundaries: Added EndUtc extraction alongside the existing StartUtc value for timed and all-day Google events.
  • Projected account-local values: Converted timed start dates, start times, and end times before returning manual-import events.
  • Adjusted stored suggestions: Persisted the new end instant for future suggestions and projected deserialized events before local-date filtering and legacy-key matching.
  • Added regression coverage: Covered opposite-direction date crossings, UTC fallback, all-day preservation, UTC extraction, and projected suggestion filtering.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs Outdated
@thomasluizon

Copy link
Copy Markdown
Owner Author

Review round approach: narrow CalendarEventItem.ProjectTo so a projected end is emitted only when EndUtc exists and both projected boundaries share the account-local date. Update the existing events and stored-suggestions query tests to prove legacy rows, midnight crossings, same-day intervals, and shared projection behavior. The implementation stays in GetCalendarEventsQuery.cs because both query handlers already use that projection. I rejected repairing legacy EndTime values because their source timezone is unavailable, and I rejected changing HabitInvariants because its clock-only guard is correct.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

A DST fall-back can still make a valid projected event fail bulk import.

Reviewed changes Reviewed the projected end-time hardening added since the prior Pullfrog review.

  • Omitted unavailable ends: Timed events without EndUtc now return no projected end time instead of retaining a source-timezone value.
  • Suppressed cross-day ranges: Projected ends now become null when their local date differs from the projected start date.
  • Strengthened coverage: Added exact regressions for legacy suggestion JSON and local-midnight crossings.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs Outdated
@thomasluizon

Copy link
Copy Markdown
Owner Author

Review round two approach: strengthen CalendarEventItem.ProjectTo so EndTime survives only when its account local date matches the start and its wall clock is strictly later. Add one New York fall back regression in GetCalendarEventsQueryHandlerTests. This keeps the shared projection authoritative for both queries and avoids changing the response contract or HabitInvariants.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The end time guard still permits an import invalid pair when distinct instants truncate to the same displayed minute.

Reviewed changes Reviewed the DST fallback hardening added since the prior Pullfrog review.

  • Rejected descending wall times: Omitted projected end times when the same date wall clock does not advance.
  • Added a fold regression: Covered a New York repeated hour event and asserted that its descending end becomes null.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs Outdated
@thomasluizon

Copy link
Copy Markdown
Owner Author

@pullfrog review

pullfrog[bot]
pullfrog Bot previously approved these changes Sep 16, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes Reviewed the minute-precision correction added since the prior Pullfrog review at cc86c61.

  • Aligned ordering with response precision: Compared projected HH:mm values so an end is retained only when the exact strings sent to clients form a valid range.
  • Covered boundary cases: Added regressions proving that distinct instants within one displayed minute omit the equal end while a one-minute event keeps its later end.

Pullfrog  | View workflow run | Using GPT Sol𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Projected recurring events can import on source-timezone weekdays instead of account-local weekdays when the conversion changes the calendar date.

Reviewed changes Reviewed the complete calendar timezone fix, including API projection, persisted suggestion compatibility, Google boundary mapping, shipped client consumption, and regression coverage.

  • UTC boundary capture Added EndUtc beside the existing optional StartUtc and populated both boundaries from Google event values.

  • Account timezone projection Centralized timed-event projection while preserving floating all-day dates.

  • Suggestion filtering Projected persisted events before account-local date filtering and legacy habit matching.

  • Import-safe end times Omitted unavailable, cross-day, descending, and same-minute end values that the habit invariant cannot represent.

  • Regression coverage Added focused tests for opposite timezone shifts, legacy rows, all-day events, midnight boundaries, DST folds, and minute precision.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs Outdated
@thomasluizon

Copy link
Copy Markdown
Owner Author

@pullfrog review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The existing recurrence weekday projection finding remains unresolved at this unchanged head. No additional Critical or High issues were found.

Reviewed changes Reviewed the complete calendar timezone implementation at 91da4ad, including API projection, persisted suggestion compatibility, Google mappings, and shipped web and mobile import behavior.

  • UTC boundary capture Added EndUtc beside the optional StartUtc and populated both boundaries from Google event values.
  • Account timezone projection Centralized timed event projection while preserving floating all day dates.
  • Suggestion handling Projected persisted events before account local date filtering and legacy habit matching.
  • Import safe ranges Omitted unavailable, cross day, descending, and same minute end values that the habit invariant cannot represent.
  • Regression coverage Added focused tests for opposite timezone shifts, legacy rows, all day events, midnight boundaries, DST folds, and minute precision.

Pullfrog  | Fix it ➔View workflow run | Using GPT Sol𝕏

@thomasluizon

Copy link
Copy Markdown
Owner Author

Approach for the current review round:

I will update CalendarEventItem.ProjectTo to clear RecurrenceRule only when a timed event moves to a different account local date. The API receives the raw master RRULE, while both clients parse BYDAY into imported habit weekdays. Rewriting that raw rule from one occurrence is unsafe because it may contain multiple or ordinal weekdays, and timezone date shifts can change across DST boundaries.

The production change will stay in src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs. The regression will stay in tests/Orbit.Application.Tests/Queries/Calendar/GetCalendarEventsQueryHandlerTests.cs. Both event queries already call the same ProjectTo method, so this keeps live events and stored suggestions aligned.

@thomasluizon

Copy link
Copy Markdown
Owner Author

@pullfrog review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The recurrence fix still silently turns date-shifting recurring Google events into one-time habits in shipped clients.

Reviewed changes Reviewed the complete calendar timezone projection implementation at ee8f3e4, including the latest recurrence handling and its client import impact.

  • UTC boundary capture: Added optional end instants alongside start instants and populated both from Google event boundaries.
  • Account timezone projection: Projected timed event dates and times while preserving floating all-day values.
  • Stored suggestions: Persisted UTC boundaries and projected deserialized suggestions before account-local filtering.
  • Import-safe ranges: Omitted unavailable, cross-day, descending, and same-minute end values.
  • Recurrence handling: Cleared source recurrence rules when projection changes the displayed date and added a focused regression.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Retaining recurrence from a single matching occurrence can still produce a wrong schedule after a daylight saving offset change.

Reviewed changes Reviewed the recurrence projection correction added since the prior Pullfrog review at 91da4ad.

  • Suppressed shifted recurrence: Cleared RecurrenceRule when the fetched occurrence moves to another account-local date.
  • Strengthened the boundary regression: Made the Tokyo projection fixture recurring and asserted that cross-date projection omits its rule.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs Outdated
@thomasluizon

Copy link
Copy Markdown
Owner Author

Round 2 approach: append the Google start timezone to CalendarEventItem, populate it in GoogleCalendarEventFetcher, and retain recurring rules only when four seasonal source-to-account projections keep the same local date. I will add one failing Lisbon regression, one stable companion, and verify the fetcher carries the SDK timezone. This avoids rewriting RRULE structure from one occurrence.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The recurrence stability work remains unsafe to merge: it retains rules whose projected local time changes by season, can miss transition-week date shifts, and now depends on an unverified Google response field. The existing recurrence threads remain open; one new external-interface finding is inline.

Reviewed changes Reviewed commit 3d59d9f since the prior Pullfrog review at ee8f3e4.

  • Carried the source timezone: Added optional StartTimeZone data from Google event starts and persisted it with newly created suggestions.
  • Sampled seasonal projections: Retained recurrence only when four source-to-account conversions keep the same calendar date.
  • Expanded recurrence coverage: Added Lisbon seasonal date cases and asserted the Google event mapper copies start.timeZone.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread src/Orbit.Infrastructure/Services/GoogleCalendarEventFetcher.cs Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Recurring imports can still lose recurrence or retain a seasonally wrong account-local time, and the load-bearing Google timezone field is not evidenced for this request shape.

Reviewed changes Reviewed all eight changed files at 3d59d9f, including Google event mapping, live and persisted projection, recurrence gating, and regression coverage.

  • UTC projection: Added optional start and end instants and projected timed events into the account timezone while preserving floating all-day dates.
  • Suggestion handling: Persisted new boundary data and projected stored suggestions before local-date filtering and legacy matching.
  • Import-safe ranges: Removed unsupported end times for missing boundaries, date crossings, repeated hours, and equal displayed minutes.
  • Recurrence gating: Added the Google start timezone and seasonal probes to decide whether to retain the source RRULE.
  • Regression coverage: Added focused query and fetcher tests for timezone boundaries, legacy JSON, DST behavior, and recurrence retention.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs Outdated
Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs Outdated
Comment thread src/Orbit.Infrastructure/Services/GoogleCalendarEventFetcher.cs Outdated
@thomasluizon

Copy link
Copy Markdown
Owner Author

Round 3 approach:

  • Replace recurrence suppression in CalendarEventItem.ProjectTo with BYDAY projection using the source date and projected account date.
  • Preserve ordinal prefixes and every non BYDAY term byte for byte.
  • Remove StartTimeZone and IsRecurrenceDateStable from the query, fetcher, and tests because recurrence projection no longer consumes Google timezone metadata.
  • Add red first query regressions for shifted, unchanged, ordinal, no BYDAY, Denver, and Lisbon cases.
  • Keep the existing UTC, all day, end time, and stored suggestion behavior unchanged.

This shape preserves recurring imports. Seasonal gating was rejected because a null rule silently creates a one time habit.

@thomasluizon

Copy link
Copy Markdown
Owner Author

@pullfrog review

2 similar comments
@thomasluizon

Copy link
Copy Markdown
Owner Author

@pullfrog review

@thomasluizon

Copy link
Copy Markdown
Owner Author

@pullfrog review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The new recurrence rewrite still cannot survive the shipped import path and can create rejected or materially different schedules.

Reviewed changes Reviewed the recurrence projection replacement added since the prior Pullfrog review at 3d59d9f.

  • Replaced seasonal gating: Retained every non-null RRULE and shifted each BYDAY token by the fetched occurrence's projected weekday delta.
  • Removed the external timezone dependency: Deleted StartTimeZone from the event DTO, fetch mapping, persisted payload, and tests.
  • Expanded recurrence examples: Added API-level assertions for multiple weekdays, intervals, ordinal weekdays, absent BYDAY, and additional timezone pairs.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs Outdated
@thomasluizon

Copy link
Copy Markdown
Owner Author

@pullfrog review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

A single projected occurrence cannot determine a stable habit weekday when source and account daylight saving rules differ; supported recurring imports can drift to the wrong day.

Reviewed changes Reviewed the complete calendar timezone projection at ff892dc, including persistence compatibility and shipped web and mobile import behavior.

  • UTC boundary capture: Added optional end instants beside start instants and populated both from Google event boundaries.
  • Account timezone projection: Projected timed dates and times while preserving floating all day events and import safe end ranges.
  • Stored suggestions: Persisted boundary data and projected deserialized suggestions before account local filtering.
  • Recurrence rewrite: Retained source recurrence and shifted BYDAY tokens from the fetched occurrence's weekday delta.
  • Regression coverage: Added focused tests for date crossings, legacy rows, repeated hours, minute precision, and recurrence rule shapes.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Deriving recurrence weekdays from one occurrence makes recurring imports wrong across DST boundaries that the source and account zones do not share.

Reviewed changes Reviewed the complete calendar timezone projection at ff892dc, including API and persisted suggestion projection, Google boundary mapping, recurrence translation, shipped client consumption, and regression coverage.

  • UTC boundary capture: Added optional EndUtc alongside StartUtc and populated both from Google event boundaries.
  • Account timezone projection: Projected timed dates and times while preserving floating all day values and import safe end ranges.
  • Suggestion handling: Persisted UTC boundaries and projected stored suggestions before account local filtering and legacy matching.
  • Recurrence translation: Shifted BYDAY tokens according to the sampled occurrence's projected weekday while retaining other rule terms.
  • Regression coverage: Added focused tests for timezone crossings, old suggestion JSON, DST folds, minute precision, and recurrence rule transformations.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The recurrence rewrite still emits schedules that diverge from the Google series across seasonal offset changes and for ordinal monthly rules.

Reviewed changes Reviewed the complete calendar timezone projection at ff892dc, including UTC boundary capture, account local projection, stored suggestions, Google mappings, recurrence rewriting, shipped web and mobile imports, and focused regression coverage.

  • UTC boundary capture Added EndUtc beside the existing StartUtc and mapped both Google event boundaries.
  • Account timezone projection Converted timed starts and ends before returning live events or persisted suggestions while preserving floating all day dates.
  • Import safe ranges Omitted unavailable, cross day, descending, and same minute end values that habit invariants cannot represent.
  • Recurrence projection Rotated every BYDAY token from the fetched occurrence's date delta while retaining other rule terms.
  • Suggestion filtering Compared projected event dates with account local today.
  • Regression coverage Added focused tests for timezone crossings, DST folds, minute precision, legacy JSON, and recurrence variants.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs Outdated
Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The existing recurrence projection finding remains unresolved. dayShift is still derived from one expanded occurrence and persisted as a fixed habit weekday, so the Denver to Phoenix example imports Tuesday from its July occurrence even though the same series projects to Wednesday in January.

Reviewed changes Reviewed all eight changed files at ff892dc, including the API projection, persisted suggestions, Google mappings, shipped web and mobile import behavior, and recurrence tests.

  • UTC boundary capture: Added optional EndUtc beside StartUtc and populated both boundaries from Google event values.
  • Account timezone projection: Converted timed starts and ends while preserving floating all day dates and omitting unrepresentable end times.
  • Suggestion handling: Persisted UTC boundaries and projected stored events before account local date filtering and legacy matching.
  • Recurrence translation: Shifted BYDAY tokens according to the fetched occurrence's source to account date delta.
  • Regression coverage: Added focused tests for timezone crossings, legacy rows, DST folds, minute precision, and recurrence rule shapes.

Pullfrog  | Fix it ➔View workflow run | Using GPT Sol𝕏

@thomasluizon

Copy link
Copy Markdown
Owner Author

Round 4 approach: remove the recurrence rewrite from src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs and return RecurrenceRule unchanged. Delete only recurrence transformation tests, retain or add pass-through coverage, and leave timezone date, time, end handling, stored suggestions, and fetch mapping intact. File the seasonal weekday representation limit through tools/create-ticket.mjs, blocked by #562, because Orbit habits carry no timezone and this endpoint cannot represent seasonal weekday changes safely.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The recurrence rollback still leaves a supported recurring import on the wrong account-local weekdays when timezone projection changes the occurrence date.

Reviewed changes Reviewed commit 8213458 since the prior Pullfrog review at ff892dc.

  • Removed recurrence rewriting: Deleted sampled BYDAY rotation, regex helpers, and the associated transformation behavior.
  • Restored rule pass-through: Preserved RecurrenceRule unchanged while continuing to project timed boundaries into the account timezone.
  • Narrowed regression coverage: Removed transformation-specific cases and made the cross-date Tokyo scenario assert unchanged recurrence.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs
@thomasluizon

Copy link
Copy Markdown
Owner Author

The body finding at head 82134581 is real and it is filed as thomasluizon/orbit-tickets#569, not fixed here.

The recurrence rollback still leaves a supported recurring import on the wrong account-local weekdays when timezone projection changes the occurrence date.

That is accurate. It is not a regression this pull request introduces and it is not fixable inside its scope.

Why it is not a regression. Before this pull request nothing was projected, so the rule and the start agreed with each other and both were wrong for the account. This pull request makes the start correct, which is ticket #526 in full, and leaves the rule as Google returned it.

Why the obvious fix stays deleted. Round 3 shipped exactly that fix, ProjectRecurrenceRule, which computed dayShift from the source and local weekdays and rewrote every BYDAY token. Four Pullfrog reviews found real defects in it and round 4 removed it, 136 deletions against 3 insertions. A whole-rule day shift is only correct for the simple weekly case; it is wrong for an ordinal token such as 2MO, for BYSETPOS, for BYMONTHDAY, and for any rule whose occurrences do not all shift by the same amount across a DST boundary. Making it correct means RRULE expansion, which is a different piece of work from a timezone projection.

What #569 decides instead. The endpoint reports that the recurrence cannot be projected, as a new optional append-only field, and the import review surface refuses the event visibly before the person taps import. That is the same answer thomasluizon/orbit-tickets#562 already settled for interval rules and ordinal weekday tokens: refuse visibly rather than import a schedule the person did not pick. No RecurrenceRule value this endpoint emits will ever differ from the one Google returned.

Every thread on this pull request is resolved. This comment disposes of the body-level finding, which carries no thread and therefore cannot be resolved through one.

@thomasluizon

Copy link
Copy Markdown
Owner Author

@pullfrog review

1 similar comment
@thomasluizon

Copy link
Copy Markdown
Owner Author

@pullfrog review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Date-shifting recurring events remain importable on the wrong account-local weekdays, and deferring rejection to an optional client field does not protect installed Android builds.

Reviewed changes Reviewed the complete calendar timezone implementation at 8213458, including API projection, persisted suggestions, Google boundary mapping, shipped client imports, and the proposed recurrence follow-up.

  • UTC boundary capture: Added optional EndUtc beside StartUtc and populated both from Google event boundaries.
  • Account timezone projection: Projected timed starts and ends for live events and stored suggestions while preserving floating all-day dates.
  • Import-safe ranges: Omitted unavailable, cross-day, descending, and same-minute end values that habit invariants cannot represent.
  • Suggestion filtering: Compared projected event dates with account-local today and retained compatibility with stored JSON that predates EndUtc.
  • Regression coverage: Added focused tests for timezone crossings, old suggestion rows, all-day values, DST folds, and minute precision.
  • Client compatibility: Verified the current web and Android import builders and shared calendar schemas against the changed response.

⚠️ The proposed deferral cannot protect installed clients

The current clients combine the newly projected StartDate and StartTime with source-timezone BYDAY values, and the accepted FREQ=DAILY;BYDAY=... shape is then scheduled by those unshifted days. A future optional unsupported-recurrence field is not inspected by the live-event importer and is absent from the parsed suggestion schema, so an API-first rollout still lets installed builds persist a materially different schedule.

Technical details
# Recurrence deferral is not backward-compatible

## Affected sites
- `src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs:43-51` projects the occurrence boundaries but leaves `RecurrenceRule` in the source calendar representation.
- `orbit-ui-mobile/packages/shared/src/utils/calendar-sync.ts:197-219` combines projected dates and times with weekdays parsed directly from that rule.
- `src/Orbit.Application/Habits/Services/HabitScheduleService.cs:654-655` filters future occurrences by the persisted source weekdays.
- `orbit-ui-mobile/packages/shared/src/types/calendar.ts:27-39` has no unsupported-recurrence field, and current auto-sync hooks parse suggestions through that schema.

## Required outcome
- Ensure date-shifting recurring events cannot be imported by currently supported clients until their account-local recurrence can be represented correctly.
- Define rollout sequencing that prevents installed clients which ignore any new optional field from importing the unsupported shape.

## Suggested approach
- Withhold or version-gate unsupported recurring events server-side until a compatible client build is live and enforceable, or return a server-safe account-local recurrence representation.

Pullfrog  | Fix it ➔View workflow run | Using GPT Sol𝕏

@thomasluizon

Copy link
Copy Markdown
Owner Author

You are right and my earlier disposition was wrong. Retracting it.

deferring rejection to an optional client field does not protect installed Android builds

That is the sentence that settles it. The mobile fleet lags through the Play store, so an optional response field plus a client-side refusal protects only builds that ship after it. Every installed build keeps importing the wrong schedule. A server-side problem needs a server-side answer, and I filed the wrong shape of ticket.

The refusal moves into this pull request. Round 5 is ordered on thomasluizon/orbit-tickets#526: when the account-timezone projection moves an occurrence's date and the event's RecurrenceRule carries a BYDAY term, the endpoint omits that event from the import feed rather than offering it. GetCalendarEventsQuery and GetCalendarSyncSuggestionsQuery both drop it, and a stored suggestion row that resolves to the same shape is filtered on read so it cannot resurrect. An event whose projection leaves the date alone, or whose rule carries no BYDAY, is returned exactly as it is today.

ProjectRecurrenceRule stays deleted. No RecurrenceRule value this endpoint emits will ever differ from the one Google returned.

The cost, stated plainly rather than hidden: a person in Tokyo with a UTC-scheduled weekly event loses the ability to import it at all, and the list gives no reason. That is worse than importing it correctly and better than importing it on the wrong days.

thomasluizon/orbit-tickets#569 is rescoped to close that gap properly: shift the provable subset instead of omitting it. A FREQ=WEEKLY or FREQ=DAILY rule with a plain BYDAY, no ordinal prefix, no BYSETPOS and no BYMONTHDAY, whose projection offset is identical for the first and last occurrence inside the feed's existing 60-day bound, can be shifted correctly. That needs occurrence expansion over the bounded window rather than the regex round 4 removed, which is why it is a separate piece of work and not this pull request.

pullfrog[bot]
pullfrog Bot previously approved these changes Sep 17, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes Reviewed the complete calendar timezone projection at 82134581, including persisted suggestion compatibility and shipped web and mobile import behavior.

  • UTC boundary projection: Captured Google start and end instants, projected timed events into the account timezone, and preserved floating all day dates.
  • Import safe ranges: Omitted unavailable, cross date, descending, and same minute end values that the habit contract cannot represent.
  • Stored suggestions: Persisted optional end instants and projected deserialized events before account local date filtering and legacy matching.
  • Recurrence scope: Preserved Google recurrence rules unchanged while keeping the unsupported seasonal weekday conversion outside this PR.
  • Regression coverage: Added exact tests for timezone crossings, old suggestion rows, all day events, midnight boundaries, DST folds, and minute precision.

Pullfrog  | View workflow run | Using GPT Sol𝕏

@thomasluizon

Copy link
Copy Markdown
Owner Author

Round 5 approach: add one CalendarEventItem predicate that detects a date shifted timed event whose unchanged recurrence rule contains a BYDAY term. Filter those projected items from GetCalendarEventsQuery and GetCalendarSyncSuggestionsQuery, including stored rows, while preserving same date BYDAY events, rules without BYDAY, and nonrecurring events. Tests will land in the two existing query handler test files. This keeps the Google rule byte identical and protects installed clients without adding a response field or restoring recurrence rewriting.

@sonarqubecloud

Copy link
Copy Markdown

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The new recurrence omission gate still admits BYDAY series that shift account-local dates after a timezone offset transition.

Reviewed changes Reviewed commit 44611e5 since the prior Pullfrog review at 8213458.

  • Gated unsafe recurrence: Omitted projected events when the sampled occurrence changes account-local date and its unchanged rule contains BYDAY.
  • Aligned stored suggestions: Applied the same omission to deserialized auto-sync suggestions before date and legacy-key filtering.
  • Added focused regressions: Covered cross-date omission in both feeds while retaining same-date and BYDAY-free recurrence shapes.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

internal bool HasUnrepresentableRecurrenceAfterProjection(CalendarEventItem projected)
{
if (RecurrenceRule is null
|| string.Equals(StartDate, projected.StartDate, StringComparison.Ordinal))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking only this occurrence's date still exposes BYDAY series whose projection changes after a later DST transition. A supported FREQ=DAILY;BYDAY=TH Lisbon 03:30 series can be admitted from a January occurrence as Thursday 00:30 in Sao Paulo, while its summer occurrences land Wednesday 23:30 and installed clients persist the stale Thursday schedule.

Technical details
# Same-date sampling does not prove recurrence stability

## Affected sites
- `src/Orbit.Application/Calendar/Queries/GetCalendarEventsQuery.cs:57-60` treats one same-date occurrence as sufficient proof that the series is representable.
- `tests/Orbit.Application.Tests/Queries/Calendar/GetCalendarEventsQueryHandlerTests.cs:234-260` codifies a Lisbon-to-Sao Paulo series whose account-local date relationship changes seasonally.
- `src/Orbit.Infrastructure/Services/GoogleCalendarEventFetcher.cs:106-116` keeps only the first expanded instance for each recurring master, so later transitions never reach this gate.
- `orbit-ui-mobile/packages/shared/src/utils/calendar-sync.ts:197-219` turns a quantity-one daily `BYDAY` rule into fixed habit `Days`.
- `src/Orbit.Application/Habits/Services/HabitScheduleService.cs:654-663` enforces those weekdays as a static schedule.

## Required outcome
- Do not expose a `BYDAY` series as importable unless its account-local weekday relationship remains valid across source and account offset transitions.
- Add a supported `FREQ=DAILY;BYDAY=...` regression spanning standard and daylight periods and verify the resulting habit schedule stays equivalent.

## Suggested approach
- Establish series-wide date stability before exposing any representative occurrence, or withhold the series when that cannot be proven.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant