Keep last events on refresh errors

[leits]

Motivation

• Prevent losing the last known calendars and events when a refresh fetch fails by avoiding overwriting them with empty arrays.

Description

• In MeetingBar/Core/Managers/EventManager.swift capture the current self.calendars and self.events on the MainActor before creating the Future, and return that current tuple from the catch block instead of ([], []), and remove the downstream .catch that converted errors into an empty publisher.

Testing

• No automated tests were run.

---

Codex Task

Summary by CodeRabbit

• Bug Fixes
  • Enhanced error handling for calendar data retrieval. When fetching calendar information fails, the app now gracefully displays your previously cached calendars and events instead of an empty state, maintaining visibility to your data.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Error handling in EventManager's trigger pipeline was modified to return previously cached calendars and events on failure instead of empty arrays. A separate catch block was removed and error handling consolidated, with current values retrieved upfront as a fallback mechanism.

Changes

Cohort / File(s)	Summary
Error Handling Consolidation MeetingBar/Core/Managers/EventManager.swift	Modified trigger pipeline to cache current calendars and events at task start, then return cached data on Future failure instead of empty arrays. Consolidated separate catch block into existing Future error handler. Net change: +2/-5 lines.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 When futures fail and promises break,
The cache steps in for goodness sake,
No empty arrays, no loss of state—
Just cached data, returning late! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Keep last events on refresh errors' directly and concisely describes the main change: preventing loss of cached events when a refresh fails by returning previously cached data instead of empty arrays.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR modifies the EventManager's refresh error handling to preserve the last known calendars and events instead of clearing them when a fetch fails. This prevents data loss from transient network or API errors.

Changes:

• Capture current calendars and events state before attempting fetch operations
• Return captured state on error instead of empty arrays
• Remove redundant downstream .catch block that was also returning empty data

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

There's a potential race condition where the captured current state may become stale before being used. The current tuple is captured at the start of the Task, but if a concurrent refresh successfully completes and updates self.calendars and self.events on the main actor before this refresh fails, the error handler will overwrite the newer values with the older current snapshot.

This can occur when multiple refresh triggers fire in quick succession (e.g., manual refresh + timer trigger + defaults change). The second refresh could succeed and update the published properties, but then the first refresh's failure handler would revert them to the older state.

Consider either:

1. Not updating the properties at all on error (remove the .sink update when the values match current), or
2. Implementing a serial queue or cancellation mechanism to ensure only one refresh operation runs at a time

[leits]

@copilot open a new pull request to apply changes based on this feedback

[Copilot]

The new error-handling behavior (keeping last known values on refresh failure) lacks test coverage. The existing tests in EventManagerTests.swift only cover successful fetch scenarios and don't test what happens when fetchAllCalendars() or fetchEvents() throw errors.

Consider adding a test case that:

1. Sets up FakeEventStore to return initial successful data
2. Verifies the initial data is published
3. Modifies FakeEventStore to throw errors on subsequent fetches
4. Triggers a refresh
5. Verifies that calendars and events still contain the initial data (not empty arrays)

This would validate that the new fallback-to-current behavior works as intended.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

MeetingBar/Core/Managers/EventManager.swift (1)

170-189: Fix failure-type mismatch: flatMap closure return type must be Never, not Error.

The upstream trigger publisher (merged from defaultsPub, timerPub, and manualPub) has Failure == Never. When you declare the flatMap closure's return type as AnyPublisher<([MBCalendar], [MBEvent]), Never> on line 170, Combine requires the actual returned publisher to have Failure == Never as well. However, line 175 defines Future<([MBCalendar], [MBEvent]), Error>, which has Failure == Error. After .eraseToAnyPublisher() on line 189, this becomes AnyPublisher<..., Error>—a type mismatch that prevents compilation. Change the Future's failure type to Never:

Suggested fix

-                return Deferred {
-                    Future<([MBCalendar], [MBEvent]), Error> { promise in
+                return Deferred {
+                    Future<([MBCalendar], [MBEvent]), Never> { promise in
                         Task {
                             let current = await MainActor.run { (self.calendars, self.events) }
                             do {
                                 let cals = try await self.provider.fetchAllCalendars()
                                 let evts = try await self.fetchEvents(fromCalendars: cals)
                                 promise(.success((cals, evts)))
                             } catch {
                                 NSLog("EventManager refresh failed: \(error)")
                                 promise(.success(current))
                             }
                         }
                     }
                 }

🤖 Fix all issues with AI agents

In `@MeetingBar/Core/Managers/EventManager.swift`:
- Around line 176-185: The code captures a stale snapshot into `current` before
doing `provider.fetchAllCalendars()` and `fetchEvents(fromCalendars:)`, which
lets a slower failing refresh overwrite newer data; to fix, stop reading
`self.calendars`/`self.events` before the fetch and instead, inside the `catch`
block, obtain the latest values via `await MainActor.run { (self.calendars,
self.events) }` and pass those to `promise(.success(...))`, leaving the
happy-path success case unchanged.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Avoid stale fallback on overlapping refreshes.

Capturing current before the fetch means a slower failing refresh can overwrite newer data from a later successful refresh. Read current in the catch block instead, so the fallback always uses the latest cached values.

✅ Suggested fix

-                        Task {
-                            let current = await MainActor.run { (self.calendars, self.events) }
-                            do {
+                        Task {
+                            do {
                                 let cals = try await self.provider.fetchAllCalendars()
                                 let evts = try await self.fetchEvents(fromCalendars: cals)
                                 promise(.success((cals, evts)))
                             } catch {
                                 NSLog("EventManager refresh failed: \(error)")
+                                let current = await MainActor.run { (self.calendars, self.events) }
                                 promise(.success(current))
                             }
                         }

🤖 Prompt for AI Agents

In `@MeetingBar/Core/Managers/EventManager.swift` around lines 176 - 185, The code
captures a stale snapshot into `current` before doing
`provider.fetchAllCalendars()` and `fetchEvents(fromCalendars:)`, which lets a
slower failing refresh overwrite newer data; to fix, stop reading
`self.calendars`/`self.events` before the fetch and instead, inside the `catch`
block, obtain the latest values via `await MainActor.run { (self.calendars,
self.events) }` and pass those to `promise(.success(...))`, leaving the
happy-path success case unchanged.

[leits]

@copilot open a new pull request to apply changes based on the comments in this thread

[Copilot]

@leits I've opened a new pull request, #887, to work on those changes. Once the pull request is ready, I'll request review from you.

[Copilot]

@leits I've opened a new pull request, #895, to work on those changes. Once the pull request is ready, I'll request review from you.