Repository navigation
fix(analytics): address the Flutter SDK review on the tracking helpers - #1977
Merged
Merged
Conversation
Four findings from the review on appwrite/sdk-for-flutter#336. `inactive` -> `resumed` (notification shade, app switcher preview) never reaches `hidden` or `paused`, so the engagement interval is still open. The handler restarted it anyway, discarding everything accrued before the interruption and emitting a foreground event for a background that never happened. A new interval now starts only once the previous one has ended. The emitter returns the request future, but the typedef declared `void`, so the surrounding try/catch only ever caught synchronous throws and a rejected request surfaced as an unhandled async error in the host app. The typedef is now `FutureOr<void>` and both helpers attach a handler to the future. `didRemove` fires for buried routes, and its `previousRoute` is the route below the removed one rather than the visible one, so cleaning up a middle route reported a screen view for a screen nobody saw. The observer now tracks the top route and emits only when the visible screen actually changes. The helpers were emitted into every SDK regardless of the spec, so an SDK generated without the analytics service shipped files referencing an `Analytics` class it never generates — which is what the review hit. File entries can now declare `requires`, and the analytics companions are gated on the service being present.
🟢 Tier S · Ready to merge
The pull request gates Flutter and Web analytics companions on the filtered analytics service and keeps their public exports aligned. It also preserves Flutter engagement intervals across inactive/resumed transitions, suppresses events for buried-route changes, and handles asynchronous emitter failures. Generation tests now check companion files and exports with analytics present and absent. Latest changes: The newest commits condition public exports on analytics availability, convert emitter futures to Future before swallowing errors, and add generation coverage for analytics companions.
📂 Walkthrough · 8
✅ Fixed since the last review · 3
Reviewed the commits since |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
Review follow-ups on the service gate. Skipping the helper files left the entry points still exporting them, so an SDK generated without analytics referenced missing modules and failed to build. `lib/<package>.dart` and `src/index.ts` now gate those exports on the same service, value and type exports alike. `catchError` on a typed future must return that future's type, so the empty handler would itself fail for an emitter returning `Future<Model>`. Both helpers narrow with `then<void>` before attaching the handler. Adds generation coverage for `requires`: the companions and their exports are absent when the spec has no analytics service and present when it does, so the contract cannot regress silently.
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.
Addresses the four analytics findings from the review on appwrite/sdk-for-flutter#336, fixed in the generator rather than in the generated repo.
Engagement lost on
inactive→resumedOpening and dismissing the notification shade (or the app-switcher preview) transitions
resumed→inactive→resumedwithout ever reachinghiddenorpaused. The interval is therefore still open, but the handler restarted it unconditionally — discarding every second accrued before the interruption and emitting anapp_foregroundedfor a background that never happened.A new interval now starts only once the previous one has ended.
Async emitter failures were unhandled
AnalyticsEventEmitterdeclaredvoid, but the documented wiring returnsanalytics.createEvent(...)— a future. Thevoidcontract discarded it, so the surroundingtry/catchonly ever caught synchronous throws, and a rejected request surfaced as an unhandled async error in the host app.The typedef is now
FutureOr<void>, and both helpers attach a handler to the returned future.didRemovereported screens nobody sawdidRemove'spreviousRouteis the route below the removed one, not necessarily the visible one, so removing a buried route during stack cleanup recorded a screen view for a screen the user never reached.The observer now tracks the top route and emits only when the visible screen actually changes.
didReplacecarried the identical flaw and got the same guard.Helpers shipped into SDKs with no analytics service
This is the root cause behind the review's "example references a non-existent
Analytics" comment, and it is a generator bug rather than a documentation one.Both helper templates were registered with
scope: default, so they were emitted into every generated SDK regardless of whether the spec carried an analytics service. An SDK generated from a spec without it got files naming anAnalyticsclass the SDK never generates.File entries can now declare
requires, andSDK.phpskips any entry whose required service is absent from the filtered services. The analytics companions are gated onanalytics.This affected Web as well —
src/services/analytics-tracking.tshad the same unconditional entry — so both are gated.Verification
php -lclean onSDK.php,Flutter.php,Web.php$filteredServicesconfirmed in scope at the new checkGenerated output has not been re-analyzed with
dart analyzeyet; theFutureOr/unawaitedchanges deserve that before merge.