I merged PR #207 in early October. Calendar feed ingestion. It worked. Sabine was reading my ICS feeds, Google Calendar was syncing, events showed up in the UI. Ship it.
Then I did a high-pass code review of the diff. Not pre-merge. Post-merge. Against main. Eight findings. Not edge cases—real bugs that would surface in production. Some already had.
The Findings
1. Week windows used UTC midnight instead of local timezone. A Sunday 8pm Central event (Monday 01:00 UTC) vanished from both weeks. Not in the previous week (ended Sunday midnight UTC = Saturday 7pm Central). Not in the current week (starts Monday midnight UTC = Sunday 7pm Central). The event fell into the gap.
2. Calendar IDs with # symbols weren't percent-encoded. Google's holiday calendar IDs look like 'en.usa#holiday@group.v.calendar.google.com'. httpx treats # as a URL fragment. The request hit /calendars/en.usa instead of the full ID. Silent 404.
3. Events sorted lexicographically by dtstart string instead of chronologically. '2026-04-10T14:00:00+00:00' (9am Central) sorts after '2026-04-10T10:00:00-05:00' (10am Central, same as 15:00 UTC) as strings. Agenda view showed 10am before 9am.
4. Events with NULL or empty dtend were silently dropped. The dtend comparison assumed a parseable datetime string. Missing value → epoch comparison bug → event treated as "ended before window start" → gone.
5. Events from paused/disabled subscriptions leaked into the response. The subscription lookup fell back to an empty dict when the sub wasn't in the active map. Events rendered as 'unassigned' instead of being filtered out.
6. All-day events rendered on the wrong day. The API sent them as bare 'YYYY-MM-DD' strings. new Date('2026-04-10') parses as UTC midnight. In US timezones, that's the previous calendar day. April 10 all-day event showed up on April 9.
7. Feed sync was full-replacement every time. Unchanged events re-imported. Removed events never deleted. No content hashing. No incremental logic.
8. Sync failures gave no signal. When get_event_mappings failed, the function returned success. No mappings_degraded flag. No exception chaining. Stack traces lost.
Why Post-Merge Review?
Because the feature worked. The tests passed. The PR diff looked reasonable in GitHub's line-by-line view. But reading the assembled code—the actual artifact on main—revealed patterns the diff obscured. The week window bug required seeing the UTC assumption in _fetch_supabase_events next to the local-timezone intent in the frontend. The sort bug required seeing string comparison against a mixed-offset event stream. The sync logic required reading the full flow.
Code review on the diff catches logic errors and style issues. Code review on main catches design problems that only emerge when you read the finished system.
The Fix
Five commits. 614 lines changed. 244 audit tests added. Every finding documented with a test that fails on main and passes after the fix.
_parse_week_start now takes a timezone parameter and converts to local midnight. Calendar IDs percent-encoded via urllib.parse.quote. Events sorted by datetime.fromisoformat(dtstart) with a fallback for bare dates. _row_ended_before treats missing dtend as 'still running'. Inactive subscriptions excluded from the sub_map. Frontend parseEventDate splits on 'T' and uses local midnight for all-day dates. sync_subscription hashes event content, skips unchanged imports, deletes stale mappings. Result includes mappings_degraded flag and exception chains use 'from exc'.
What's Next
Making post-merge review a standard practice. Not for every PR—just the ones that ship foundational functionality. The ones where the diff review focused on correctness but the assembled system needs a design pass.
The audit tests stay. They're regression guards now. If a future refactor breaks local timezone handling or reintroduces full-replacement sync, the test will catch it.
And I'm adding calendar ICS write-back to the roadmap. Now that the read path is solid, Sabine should be able to create events and block time, not just consume it.