- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 140
 
Enhance Bob sync metadata and reminders integration #275
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.
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
base: master
Are you sure you want to change the base?
Enhance Bob sync metadata and reminders integration #275
Conversation
… Google config, add Auth window, implement FirebaseSyncService with idempotency, main-actor fixes for Reminders access, and logging. Update project to include new sources.
… keep canImport guards. Build verified.
…e and *.code-workspace
…issues.sh to open issues via gh when enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements comprehensive Bob sync metadata handling and Firebase integration for the Reminders MenuBar app. It transforms the application from a standalone reminders manager into a bidirectional sync tool that connects Apple Reminders with Firebase/Firestore, enabling cloud-based collaboration and cross-platform task management.
- Adds complete Firebase authentication and sync infrastructure with Google Sign-In support
 - Implements background sync scheduling and manual sync controls with comprehensive UI feedback
 - Introduces theme-to-calendar mapping system for automated reminder organization
 
Reviewed Changes
Copilot reviewed 37 out of 40 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description | 
|---|---|
| tools/swiftlint/LICENSE | Adds MIT license for SwiftLint tooling | 
| scripts/create_issues.sh | Automated script for creating GitHub issues tracking sync enhancements | 
| reminders-menubar/reminders_menubar.entitlements | Adds Firebase-required entitlements for network, reminders, and keychain access | 
| reminders-menubar/Views/Windows/ThemeCalendarMappingView.swift | New UI for mapping Bob themes to Reminders calendars | 
| reminders-menubar/Views/Windows/KeyboardShortcutView.swift | Updates keyboard shortcut configuration for Bob sync | 
| reminders-menubar/Views/Windows/FirebaseAuthView.swift | Complete Firebase authentication interface with Google Sign-In | 
| reminders-menubar/Views/SettingsBarView/SettingsBarSyncIndicator.swift | Sync status indicator with last sync timestamp | 
| reminders-menubar/Views/SettingsBarView/SettingsBarGearMenu.swift | Adds Bob sync menu with background sync controls | 
| reminders-menubar/Views/ContentView.swift | Integrates sync feedback toast notifications | 
| reminders-menubar/Services/* | Core sync services including Firebase integration, logging, and background scheduling | 
| reminders-menubar/Constants.swift | Updates bundle IDs and adds native Reminders integration toggle | 
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if ! command -v gh >/dev/null 2>&1; then | ||
| echo "Error: GitHub CLI (gh) is required. Install from https://cli.github.com/" >&2 | ||
| exit 1 | ||
| fi | 
    
      
    
      Copilot
AI
    
    
    
      Sep 30, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider adding a check for required environment variables or configuration before attempting to create issues. The script assumes GitHub CLI is authenticated and has proper repository access permissions.
| } | ||
| } | ||
| .labelsHidden() | ||
| Button("Add/Update") { addMapping() }.disabled((newTheme.trimmingCharacters(in: .whitespaces).isEmpty) || selectedListId == nil) | 
    
      
    
      Copilot
AI
    
    
    
      Sep 30, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Extract the validation logic into a computed property or function for better readability and reusability. The inline condition is complex and repeated logic appears elsewhere.
| private func loadAvailableThemes(force: Bool) async { | ||
| #if canImport(FirebaseFirestore) | ||
| if !FirebaseManager.shared.isConfigured { | ||
| FirebaseManager.shared.configureIfNeeded() | ||
| } | 
    
      
    
      Copilot
AI
    
    
    
      Sep 30, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The function is excessively long (100+ lines) and handles multiple responsibilities including network requests, error handling, caching, and UI updates. Consider breaking this into smaller, focused functions.
| // swiftlint:disable cyclomatic_complexity function_body_length type_body_length file_length | ||
| actor FirebaseSyncService { | 
    
      
    
      Copilot
AI
    
    
    
      Sep 30, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The SwiftLint disable comment indicates this service has grown too large and complex. Consider breaking this into smaller, focused services (e.g., TaskSyncService, ReminderImportService, ConflictResolutionService) to improve maintainability and testability.
| // - Updates fields both directions (title, due, completion) | ||
| // - Clears mappings for tasks whose reminders are gone | ||
| // - Best-effort delete: if task has deleted flag, remove reminder | ||
| func syncNow(targetCalendar preferredCalendar: EKCalendar?) async -> (created: Int, updated: Int, errors: [String]) { | 
    
      
    
      Copilot
AI
    
    
    
      Sep 30, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This function is extremely long (650+ lines) and handles numerous responsibilities including task fetching, conflict resolution, duplicate detection, and batch operations. This violates the single responsibility principle and makes testing and debugging difficult.
| private func rotateIfNeeded(_ url: URL) { | ||
| // Simple size-based rotation at ~1 MiB | ||
| let limit: UInt64 = 1 * 1024 * 1024 | 
    
      
    
      Copilot
AI
    
    
    
      Sep 30, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The rotation logic doesn't handle potential race conditions when multiple sync operations run concurrently. Consider adding file locking or atomic operations to prevent corruption during log rotation.
| scheduler.schedule { completion in | ||
| Task { | ||
| // Preflight | ||
| if FirebaseManager.shared.db != nil, await RemindersService.shared.hasFullRemindersAccess() { | ||
| _ = await FirebaseSyncService.shared.syncNow(targetCalendar: nil) | ||
| } | ||
| completion(.finished) | ||
| } | ||
| } | 
    
      
    
      Copilot
AI
    
    
    
      Sep 30, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Background sync errors are silently ignored. Consider logging failures and potentially adjusting the retry schedule based on error types to improve reliability.
| static let mainBundleId = "com.jc1.tech.bob" | ||
| static let launcherBundleId = "com.jc1.tech.bob.launcher" | 
    
      
    
      Copilot
AI
    
    
    
      Sep 30, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The bundle ID change suggests this is a fork or rebrand, but the hardcoded values should be configurable through build settings or Info.plist to support different deployment environments.
| static let mainBundleId = "com.jc1.tech.bob" | |
| static let launcherBundleId = "com.jc1.tech.bob.launcher" | |
| static var mainBundleId: String { | |
| return Bundle.main.bundleIdentifier ?? "-" | |
| } | |
| static var launcherBundleId: String { | |
| // Reads "LauncherBundleIdentifier" from Info.plist | |
| return Bundle.main.infoDictionary?["LauncherBundleIdentifier"] as? String ?? "-" | |
| } | 
| GCC_WARN_UNUSED_VARIABLE = YES; | ||
| LOCALIZATION_PREFERS_STRING_CATALOGS = YES; | ||
| MACOSX_DEPLOYMENT_TARGET = 11.0; | ||
| MACOSX_DEPLOYMENT_TARGET = 15.0; | 
    
      
    
      Copilot
AI
    
    
    
      Sep 30, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The deployment target was raised from 11.0 to 15.0, which significantly reduces device compatibility. This may be unnecessarily restrictive since many Firebase and sync features could work on earlier macOS versions.
| MACOSX_DEPLOYMENT_TARGET = 15.0; | |
| MACOSX_DEPLOYMENT_TARGET = 11.0; | 
Summary
Testing