Skip to content

Keep last events on refresh errors #886

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
leits wants to merge 1 commit into from
+2 −5
Closed

Keep last events on refresh errors #886

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions MeetingBar/Core/Managers/EventManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,21 +174,18 @@ public class EventManager: ObservableObject {
return Deferred {
Future<([MBCalendar], [MBEvent]), Error> { 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(([], [])))
promise(.success(current))
Comment on lines +177 to +184
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

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

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

@leits leits Mar 4, 2026

Choose a reason for hiding this comment

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

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

Comment on lines +177 to +184
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}
Comment on lines 176 to 185
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jan 29, 2026

Choose a reason for hiding this comment

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

⚠️ 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.

}
}
}
.catch { error -> Empty<([MBCalendar], [MBEvent]), Never> in
NSLog("EventManager refresh failed: \(error)")
return Empty(completeImmediately: true)
}
.eraseToAnyPublisher()
}
// **important: hop back to the main run-loop before assigning**
Expand Down
Loading