fix(ios): Avoid duplicate app-delegate on background engine#413
Open
cyclone-pk wants to merge 1 commit into
Open
fix(ios): Avoid duplicate app-delegate on background engine#413cyclone-pk wants to merge 1 commit into
cyclone-pk wants to merge 1 commit into
Conversation
|
To view this pull requests documentation preview, visit the following URL: docs.page/abausg/home_widget~413 Documentation is deployed and generated using docs.page. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughMake background engine setup re-entrant and dispatcher-aware: Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as HomeWidgetBackgroundWorker
participant Engine as FlutterEngine
participant Registrar as FlutterPluginRegistrar
participant Plugin as HomeWidgetPlugin
participant Continuation as PendingRunContinuation
Worker->>Worker: setupEngine(dispatcher)
Worker->>Engine: check existing engine & currentDispatcher
alt same dispatcher and isSetupCompleted == true
Worker-->>Worker: return early (no-op)
else dispatcher differs or setup incomplete
Worker->>Engine: clear method handler
Worker->>Engine: destroy context / teardown
Worker->>Engine: engine = nil, channel = nil, isSetupCompleted = false
Worker->>Continuation: resume any pending run continuations
Worker->>Engine: create headless FlutterEngine
Worker->>Registrar: registrar(for: engine)
Worker->>Plugin: HomeWidgetPlugin.registerChannels(with: Registrar)
Plugin->>Registrar: register method channel "home_widget"
Plugin->>Registrar: register event channel "home_widget/updates" (set handler)
Worker->>Engine: run(withEntrypoint: callbackName?, libraryURI: callbackLibrary?)
Worker->>Worker: set currentDispatcher = dispatcher, isSetupCompleted = true
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
cyclone-pk
force-pushed
the
fix/ios-408-duplicate-app-delegate
branch
from
c5d65bd to
d9843d6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
packages/home_widget/ios/home_widget/Sources/home_widget/HomeWidgetPlugin.swift (1)
41-50:⚠️ Potential issue | 🟠 Major
register(with:)can re-add the application delegate when called from a customregisterPluginscallback on the background engine.
HomeWidgetBackgroundWorker.swift(lines 66-74) delegates plugin registration toregisterPluginswhen that callback is set. If that callback registersHomeWidgetPluginby callingregister(with:)instead ofregisterChannels(with:), theisRunningInAppExtension()check at line 44 will not block theaddApplicationDelegate:call (since a background engine is not an app extension), causing the duplicate-delegate bug from issue#408to resurface. Theregister(with:)method needs to distinguish between main app and background engine contexts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home_widget/ios/home_widget/Sources/home_widget/HomeWidgetPlugin.swift` around lines 41 - 50, register(with:) can be invoked from a background engine and should not add the application delegate in that context; modify register(with:) to always call registerChannels(with: registrar) but only invoke registrar.perform("addApplicationDelegate:", with: instance) when running in the main app (not an app extension or a background engine). Implement an extra runtime check before performing the selector — for example ensure isRunningInAppExtension() == false AND that UIApplication.shared.delegate != nil (or otherwise detect main app context) — so that HomeWidgetPlugin.register(with:) never re-adds the app delegate when registration is delegated by HomeWidgetBackgroundWorker.registerPlugins.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/home_widget/ios/home_widget/Sources/home_widget/HomeWidgetBackgroundWorker.swift`:
- Around line 48-52: The early return guards against multiple registrations by
checking engine == nil, which prevents subsequent setupEngine(dispatcher:) calls
from replacing a stale or uninitialized engine; change this to key the fast-path
on both the dispatcher and the engine's startup state: track the last-registered
dispatcher (e.g. registeredDispatcher) and an engine-initialized flag (or use
engine.backgroundInitialized) and only short-circuit when engine != nil AND
registeredDispatcher == dispatcher AND the engine is fully initialized;
otherwise allow setupEngine(dispatcher:) to recreate or reinitialize the engine
(tearing down the old engine if necessary) so new dispatcher values or failed
initializations can recover.
---
Outside diff comments:
In
`@packages/home_widget/ios/home_widget/Sources/home_widget/HomeWidgetPlugin.swift`:
- Around line 41-50: register(with:) can be invoked from a background engine and
should not add the application delegate in that context; modify register(with:)
to always call registerChannels(with: registrar) but only invoke
registrar.perform("addApplicationDelegate:", with: instance) when running in the
main app (not an app extension or a background engine). Implement an extra
runtime check before performing the selector — for example ensure
isRunningInAppExtension() == false AND that UIApplication.shared.delegate != nil
(or otherwise detect main app context) — so that
HomeWidgetPlugin.register(with:) never re-adds the app delegate when
registration is delegated by HomeWidgetBackgroundWorker.registerPlugins.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 78816f5c-7b46-4773-ba78-cd6969acac90
📒 Files selected for processing (2)
packages/home_widget/ios/home_widget/Sources/home_widget/HomeWidgetBackgroundWorker.swiftpackages/home_widget/ios/home_widget/Sources/home_widget/HomeWidgetPlugin.swift
cyclone-pk
changed the title
cyclone-pk
force-pushed
the
fix/ios-408-duplicate-app-delegate
branch
from
d9843d6 to
8adde37
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/home_widget/ios/home_widget/Sources/home_widget/HomeWidgetBackgroundWorker.swift (1)
81-82: Consider guarding the registrar force-unwrap.
engine!.registrar(forPlugin:)returns an optional. While it should always succeed for "home_widget" on a running engine, a defensive guard would prevent a crash if something unexpected occurs.🛡️ Suggested defensive unwrap
- HomeWidgetPlugin.registerChannels( - with: engine!.registrar(forPlugin: "home_widget")!) + if let registrar = engine?.registrar(forPlugin: "home_widget") { + HomeWidgetPlugin.registerChannels(with: registrar) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home_widget/ios/home_widget/Sources/home_widget/HomeWidgetBackgroundWorker.swift` around lines 81 - 82, The call to engine!.registrar(forPlugin: "home_widget") is force-unwrapped and can crash if the registrar is nil; update the HomeWidgetPlugin.registerChannels call to guard the registrar using engine?.registrar(forPlugin: "home_widget") (or guard let registrar = engine?.registrar(forPlugin: "home_widget") else { ... }) and handle the nil case (log an error via the existing logger or return early) before passing the registrar into HomeWidgetPlugin.registerChannels so the app fails gracefully instead of crashing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/home_widget/ios/home_widget/Sources/home_widget/HomeWidgetBackgroundWorker.swift`:
- Around line 81-82: The call to engine!.registrar(forPlugin: "home_widget") is
force-unwrapped and can crash if the registrar is nil; update the
HomeWidgetPlugin.registerChannels call to guard the registrar using
engine?.registrar(forPlugin: "home_widget") (or guard let registrar =
engine?.registrar(forPlugin: "home_widget") else { ... }) and handle the nil
case (log an error via the existing logger or return early) before passing the
registrar into HomeWidgetPlugin.registerChannels so the app fails gracefully
instead of crashing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 47cc147a-ba45-4dd9-b10e-69f612f12e07
📒 Files selected for processing (2)
packages/home_widget/ios/home_widget/Sources/home_widget/HomeWidgetBackgroundWorker.swiftpackages/home_widget/ios/home_widget/Sources/home_widget/HomeWidgetPlugin.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/home_widget/ios/home_widget/Sources/home_widget/HomeWidgetPlugin.swift
HomeWidgetBackgroundWorker.setupEngine used HomeWidgetPlugin.register on the secondary FlutterEngine, which also calls addApplicationDelegate: and inserts another HomeWidgetPlugin into the host app's UIApplication delegate chain. That extra delegate disturbs URL/life-cycle propagation for other plugins (e.g. appsflyer_sdk deep-link/attribution callbacks). Split register into a channel-only registerChannels path and use it for the background engine so the background isolate still has the home_widget channel without polluting the main app's delegate chain. Also guard setupEngine so repeated registerBackgroundCallback calls do not leak engines. Closes ABausG#408
cyclone-pk
force-pushed
the
fix/ios-408-duplicate-app-delegate
branch
from
8adde37 to
ae95805
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HomeWidgetBackgroundWorker.setupEngine used HomeWidgetPlugin.register on the secondary FlutterEngine, which also calls addApplicationDelegate: and inserts another HomeWidgetPlugin into the host app's UIApplication delegate chain. That extra delegate disturbs URL/life-cycle propagation for other plugins (e.g. appsflyer_sdk deep-link/attribution callbacks).
Split register into a channel-only registerChannels path and use it for the background engine so the background isolate still has the home_widget channel without polluting the main app's delegate chain. Also guard setupEngine so repeated registerBackgroundCallback calls do not leak engines.
Closes #408
Description
Replace this text.
Checklist
exampleor documentation.Breaking Change?
Related Issues