fix: address code-review findings on the 2.16.0 branch #87

makiolaj · 2026-07-20T11:45:41Z

makiolaj commented

2026-07-20 11:45:41 +00:00

Code review of main...release/v2.16.0 (the per-event time zones, toolbar today-button, multi-day agenda and app-name work) surfaced ten findings. This fixes all of them.

Correctness

Recurring series anchor drifted by an hour (the significant one). buildEventUpdateValues moved a recurring event's series DTSTART by a millisecond delta measured at the edited occurrence. That delta bakes in whichever UTC offset applied on the occurrence's date — a different offset from the series anchor's whenever a DST boundary sits between them, and an entirely wrong one when the zone itself changed. Pinning a recurring event to another zone shifted the whole series.

Now the anchor moves by the wall-clock shift the user applied and is re-resolved in the event's (possibly new) zone, so "09:00" keeps meaning 09:00 at both ends. Three new tests pin it: a Berlin→Tokyo pin with a January anchor and a July occurrence, a time edit across a DST boundary, and a whole-day move. The pre-existing delta tests pass unchanged.

Zone abbreviations resolved at the wrong instant. The detail card and the edit form both resolved a pinned zone's abbreviation and offset at Instant.now() and printed them next to the event's time — a July event read CET · 10:00 AM when opened in January. Both now resolve at the event's own start.

Agenda labelling could disagree with grouping. agendaTimeSummary read a file-level private val zone fixed for the process lifetime, while grouping used the ViewModel's. After a device time-zone change the two diverge and a row can read "Starts …" under a day it doesn't start on. The zone now travels on AgendaUiState.Success; the constant is gone.

Week title hid the year across New Year. A week running Dec 28 – Jan 3 showed a bare "December" with four visible columns in the new year. Added a forceYear flag to formatCalendarTitle.

Performance

The ~600-entry zone catalogue (each entry a localized display name plus up to two ICU short-name lookups) was built inside composition when the picker opened. Now produceState + Dispatchers.Default; the device row's summary resolves on its own so it still renders complete on the first frame.
filterTimeZones re-normalized four strings per option per call — ~2400 NFD normalizations and regex passes per keystroke. Search keys are now normalized once at TimeZoneOption construction (in the class body, so they stay out of equals/copy), and the combining-mark Regex is hoisted out of the hot path.
The edit form's "your time" line recomputed timesIn on every recomposition — i.e. every keystroke in the title field. Now keyed on the fields it reads.
LauncherNameManager's getComponentEnabledSetting / setComponentEnabledSetting are binder round-trips that ran on the main thread (one of them in the SettingsViewModel initializer, while Settings was opening). Both moved off-thread; the app-name card highlights optimistically and settles on the real component state.

Cleanup

Duplicate Icons.Default.Public import in EventEditScreen.
Unused event_edit_timezone_clear string — it was being shipped to Weblate for translators to translate a string that can never render.

Beyond the ten findings

While rewriting the series-anchor logic I hit an adjacent pre-existing wart in the code I was touching: switching a recurring event to all-day shifts by a time-of-day and can land the anchor off a UTC midnight. Snapped it, with a test. Happy to split this out if you'd rather keep the PR strictly to the review findings.

Not addressed, deliberately: AgendaViewModel still caches its own zone at construction. It's recreated far more often than the process lives, so the window is much smaller than the file-level constant this removes, and fixing it properly means observing ACTION_TIMEZONE_CHANGED — bigger than this PR's scope.

Verification

./gradlew lint test assembleDebug green; scripts/check_translations.py clean.

Not verified on device. The time-zone and agenda changes in particular want an on-device look before 2.16.0 goes to main.

🤖 Generated with Claude Code

Code review of `main...release/v2.16.0` (the per-event time zones, toolbar today-button, multi-day agenda and app-name work) surfaced ten findings. This fixes all of them. ## Correctness **Recurring series anchor drifted by an hour (the significant one).** `buildEventUpdateValues` moved a recurring event's series `DTSTART` by a *millisecond* delta measured at the edited occurrence. That delta bakes in whichever UTC offset applied on the occurrence's date — a different offset from the series anchor's whenever a DST boundary sits between them, and an entirely wrong one when the zone itself changed. Pinning a recurring event to another zone shifted the whole series. Now the anchor moves by the **wall-clock** shift the user applied and is re-resolved in the event's (possibly new) zone, so "09:00" keeps meaning 09:00 at both ends. Three new tests pin it: a Berlin→Tokyo pin with a January anchor and a July occurrence, a time edit across a DST boundary, and a whole-day move. The pre-existing delta tests pass unchanged. **Zone abbreviations resolved at the wrong instant.** The detail card and the edit form both resolved a pinned zone's abbreviation and offset at `Instant.now()` and printed them next to the *event's* time — a July event read `CET · 10:00 AM` when opened in January. Both now resolve at the event's own start. **Agenda labelling could disagree with grouping.** `agendaTimeSummary` read a file-level `private val zone` fixed for the process lifetime, while grouping used the ViewModel's. After a device time-zone change the two diverge and a row can read "Starts …" under a day it doesn't start on. The zone now travels on `AgendaUiState.Success`; the constant is gone. **Week title hid the year across New Year.** A week running Dec 28 – Jan 3 showed a bare "December" with four visible columns in the new year. Added a `forceYear` flag to `formatCalendarTitle`. ## Performance - The ~600-entry zone catalogue (each entry a localized display name plus up to two ICU short-name lookups) was built **inside composition** when the picker opened. Now `produceState` + `Dispatchers.Default`; the device row's summary resolves on its own so it still renders complete on the first frame. - `filterTimeZones` re-normalized four strings per option per call — ~2400 NFD normalizations and regex passes **per keystroke**. Search keys are now normalized once at `TimeZoneOption` construction (in the class body, so they stay out of `equals`/`copy`), and the combining-mark `Regex` is hoisted out of the hot path. - The edit form's "your time" line recomputed `timesIn` on every recomposition — i.e. every keystroke in the title field. Now keyed on the fields it reads. - `LauncherNameManager`'s `getComponentEnabledSetting` / `setComponentEnabledSetting` are binder round-trips that ran on the main thread (one of them in the `SettingsViewModel` initializer, while Settings was opening). Both moved off-thread; the app-name card highlights optimistically and settles on the real component state. ## Cleanup - Duplicate `Icons.Default.Public` import in `EventEditScreen`. - Unused `event_edit_timezone_clear` string — it was being shipped to Weblate for translators to translate a string that can never render. ## Beyond the ten findings While rewriting the series-anchor logic I hit an adjacent pre-existing wart in the code I was touching: switching a *recurring* event to all-day shifts by a time-of-day and can land the anchor off a UTC midnight. Snapped it, with a test. Happy to split this out if you'd rather keep the PR strictly to the review findings. **Not addressed, deliberately:** `AgendaViewModel` still caches its own `zone` at construction. It's recreated far more often than the process lives, so the window is much smaller than the file-level constant this removes, and fixing it properly means observing `ACTION_TIMEZONE_CHANGED` — bigger than this PR's scope. ## Verification `./gradlew lint test assembleDebug` green; `scripts/check_translations.py` clean. **Not verified on device.** The time-zone and agenda changes in particular want an on-device look before 2.16.0 goes to main. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

makiolaj added 1 commit 2026-07-20 11:45:41 +00:00

fix: address code-review findings on the 2.16.0 branch

Translations / check (pull_request) Successful in 5s

Details

CI / ci (pull_request) Successful in 5m43s

Details

94355bf340

Correctness:
- Recurring writes: move the series DTSTART by the *wall-clock* shift applied
  to the edited occurrence and re-resolve it in the event's zone, instead of by
  a millisecond delta. The old delta baked in whichever UTC offset applied on
  the edited occurrence's date, so pinning a recurring event to another zone —
  or editing an occurrence on the far side of a DST boundary from the series
  anchor — shifted the whole series by an hour. Also snaps the anchor to a UTC
  midnight when the event becomes all-day.
- Detail card and edit form now resolve a pinned zone's abbreviation/offset at
  the *event's* instant, not at "now", so a July event no longer reads
  "CET · 10:00 AM" when opened in January.
- Agenda: the zone used to label multi-day rows now travels on
  AgendaUiState.Success rather than a process-lifetime file-level constant, so
  labelling can't disagree with the grouping after a device time-zone change.
- Week title: spell out the year when the week straddles New Year, via a new
  forceYear flag on formatCalendarTitle.

Performance:
- Build the ~600-entry zone catalogue off the main thread (produceState +
  Dispatchers.Default); resolve the device row's summary on its own so it still
  renders complete on the first frame.
- Pre-normalize each TimeZoneOption's search keys at construction, turning
  ~2400 NFD normalizations per keystroke into plain prefix/substring checks.
  Hoist the combining-mark Regex out of the hot path.
- Key the edit form's local-time line on the fields it reads instead of
  recomputing it on every keystroke.
- Move LauncherNameManager's PackageManager binder calls off the main thread.

Cleanup:
- Drop a duplicate Public icon import and the unused
  event_edit_timezone_clear string.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

makiolaj merged commit 997296e21e into release/v2.16.0

2026-07-20 11:52:21 +00:00

makiolaj deleted branch fix/code-review-2.16.0

2026-07-20 11:52:21 +00:00

makiolaj referenced this issue from a commit

2026-07-20 11:52:21 +00:00

Merge pull request 'fix: address code-review findings on the 2.16.0 branch' (!87) from fix/code-review-2.16.0 into release/v2.16.0

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: makiolaj/calendula#87