From c66264aabb861abbdea21e89f4d708d008020ba9 Mon Sep 17 00:00:00 2001 From: Damodar Lohani Date: Wed, 7 Oct 2026 12:26:01 +0545 Subject: [PATCH 1/2] fix(analytics): address the Flutter SDK review on the tracking helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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` 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. --- src/SDK/Language/Flutter.php | 2 ++ src/SDK/Language/Web.php | 1 + src/SDK/SDK.php | 7 ++++ .../lib/src/analytics_observer.dart.twig | 33 ++++++++++++++++--- .../lib/src/analytics_tracking.dart.twig | 16 ++++++++- 5 files changed, 54 insertions(+), 5 deletions(-) diff --git a/src/SDK/Language/Flutter.php b/src/SDK/Language/Flutter.php index a7ccea44a3..ede171775f 100644 --- a/src/SDK/Language/Flutter.php +++ b/src/SDK/Language/Flutter.php @@ -324,11 +324,13 @@ public function getFiles(): array [ 'scope' => 'default', 'destination' => '/lib/src/analytics_observer.dart', + 'requires' => 'analytics', 'template' => 'flutter/lib/src/analytics_observer.dart.twig', ], [ 'scope' => 'default', 'destination' => '/lib/src/analytics_tracking.dart', + 'requires' => 'analytics', 'template' => 'flutter/lib/src/analytics_tracking.dart.twig', ], [ diff --git a/src/SDK/Language/Web.php b/src/SDK/Language/Web.php index db8a921a24..9072f30fa8 100644 --- a/src/SDK/Language/Web.php +++ b/src/SDK/Language/Web.php @@ -66,6 +66,7 @@ public function getFiles(): array [ 'scope' => 'default', 'destination' => 'src/services/analytics-tracking.ts', + 'requires' => 'analytics', 'template' => 'web/src/services/analytics-tracking.ts.twig', ], [ diff --git a/src/SDK/SDK.php b/src/SDK/SDK.php index dd1480d0bc..9674c70e4f 100644 --- a/src/SDK/SDK.php +++ b/src/SDK/SDK.php @@ -1012,6 +1012,13 @@ public function generate(string $target): void continue; } + // A companion template that references a generated service must not + // ship when the spec carries no such service, or it names a class + // the SDK never generates and the package stops compiling. + if (isset($file['requires']) && !isset($filteredServices[$file['requires']])) { + continue; + } + if (!\in_array($file['scope'], ['copy', 'download'], true)) { $template = $this->twig->load($file['template']); /* @var $template TemplateWrapper */ } diff --git a/templates/flutter/lib/src/analytics_observer.dart.twig b/templates/flutter/lib/src/analytics_observer.dart.twig index ff9ec85a64..985479b148 100644 --- a/templates/flutter/lib/src/analytics_observer.dart.twig +++ b/templates/flutter/lib/src/analytics_observer.dart.twig @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter/widgets.dart'; /// Signature used to emit an analytics event from an [AnalyticsObserver] or @@ -27,7 +29,7 @@ import 'package:flutter/widgets.dart'; /// [propertyId] and [engagementTime] map to the endpoint's top-level params of /// the same name; both are null unless the helper has something to say about /// them, so a closure is free to ignore either. -typedef AnalyticsEventEmitter = void Function( +typedef AnalyticsEventEmitter = FutureOr Function( String name, { Map? props, String? propertyId, @@ -96,20 +98,33 @@ class AnalyticsObserver extends NavigatorObserver { this.propertyId, }) : nameExtractor = nameExtractor ?? defaultScreenNameExtractor; + /// The route the user is actually looking at. `didRemove` fires for buried + /// routes too, and its `previousRoute` is the route below the removed one + /// rather than the visible one, so the top has to be tracked explicitly. + Route? _topRoute; + @override void didPush(Route route, Route? previousRoute) { + _topRoute = route; _sendScreenView(route, previousRoute, trigger: 'push'); } @override void didReplace({Route? newRoute, Route? oldRoute}) { - if (newRoute != null) { - _sendScreenView(newRoute, oldRoute, trigger: 'replace'); + if (newRoute == null) { + return; } + if (oldRoute != null && !identical(oldRoute, _topRoute)) { + // A replace further down the stack leaves the visible screen alone. + return; + } + _topRoute = newRoute; + _sendScreenView(newRoute, oldRoute, trigger: 'replace'); } @override void didPop(Route route, Route? previousRoute) { + _topRoute = previousRoute; if (previousRoute != null) { _sendScreenView(previousRoute, route, trigger: 'pop'); } @@ -117,6 +132,11 @@ class AnalyticsObserver extends NavigatorObserver { @override void didRemove(Route route, Route? previousRoute) { + if (!identical(route, _topRoute)) { + // Removing a buried route never changes what is on screen. + return; + } + _topRoute = previousRoute; if (previousRoute != null) { _sendScreenView(previousRoute, route, trigger: 'remove'); } @@ -140,7 +160,12 @@ class AnalyticsObserver extends NavigatorObserver { props['previous'] = previousName; } try { - emit(eventName, props: props, propertyId: propertyId); + // The emitter usually returns the request future; without catching it a + // rejected request surfaces as an unhandled async error in the host app. + final pending = emit(eventName, props: props, propertyId: propertyId); + if (pending is Future) { + unawaited(pending.catchError((Object _) {})); + } } catch (_) { // Emitter failures must never propagate into the navigator stack — // swallow so analytics can't crash the app. diff --git a/templates/flutter/lib/src/analytics_tracking.dart.twig b/templates/flutter/lib/src/analytics_tracking.dart.twig index 760632a088..89dca4032b 100644 --- a/templates/flutter/lib/src/analytics_tracking.dart.twig +++ b/templates/flutter/lib/src/analytics_tracking.dart.twig @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter/widgets.dart'; import 'analytics_observer.dart'; @@ -123,12 +125,17 @@ class AnalyticsTracking with WidgetsBindingObserver { int? engagementTime, }) { try { - _emit( + // The emitter usually returns the request future; without catching it a + // rejected request surfaces as an unhandled async error in the host app. + final pending = _emit( name, props: props, propertyId: propertyId, engagementTime: engagementTime, ); + if (pending is Future) { + unawaited(pending.catchError((Object _) {})); + } } catch (_) { // Silent — see class docs. } @@ -177,6 +184,13 @@ class AnalyticsTracking with WidgetsBindingObserver { ); break; case AppLifecycleState.resumed: + // `inactive` -> `resumed` (notification shade, app switcher preview) + // never reaches `hidden`/`paused`, so the interval is still open. + // Restarting it would discard the engagement accrued before the + // interruption and emit a foreground event for no background. + if (_foregroundSince != null) { + break; + } _foregroundSince = DateTime.now(); event('app_foregrounded'); break; From 706975fc9cf5da8df4808b965adbea4efe1cc5e2 Mon Sep 17 00:00:00 2001 From: Damodar Lohani Date: Wed, 7 Oct 2026 12:34:10 +0545 Subject: [PATCH 2/2] fix(analytics): gate the companion exports and harden the error handler 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/.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`. Both helpers narrow with `then` 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. --- templates/flutter/lib/package.dart.twig | 2 + .../lib/src/analytics_observer.dart.twig | 4 +- .../lib/src/analytics_tracking.dart.twig | 4 +- templates/web/src/index.ts.twig | 4 ++ tests/generation/GenerationTest.php | 70 +++++++++++++++++++ 5 files changed, 82 insertions(+), 2 deletions(-) diff --git a/templates/flutter/lib/package.dart.twig b/templates/flutter/lib/package.dart.twig index 6fe340b2f2..f54d71f869 100644 --- a/templates/flutter/lib/package.dart.twig +++ b/templates/flutter/lib/package.dart.twig @@ -30,8 +30,10 @@ export 'src/upload_progress.dart'; export 'src/realtime_subscription.dart'; export 'src/realtime_message.dart'; export 'src/input_file.dart'; +{% if services['analytics'] is defined %} export 'src/analytics_observer.dart'; export 'src/analytics_tracking.dart'; +{% endif %} part 'query.dart'; part 'permission.dart'; diff --git a/templates/flutter/lib/src/analytics_observer.dart.twig b/templates/flutter/lib/src/analytics_observer.dart.twig index 985479b148..d79d0bac59 100644 --- a/templates/flutter/lib/src/analytics_observer.dart.twig +++ b/templates/flutter/lib/src/analytics_observer.dart.twig @@ -164,7 +164,9 @@ class AnalyticsObserver extends NavigatorObserver { // rejected request surfaces as an unhandled async error in the host app. final pending = emit(eventName, props: props, propertyId: propertyId); if (pending is Future) { - unawaited(pending.catchError((Object _) {})); + // `catchError` on a typed future must return that future's type, so an + // empty handler would itself throw. Narrow to `Future` first. + unawaited(pending.then((_) {}).catchError((Object _) {})); } } catch (_) { // Emitter failures must never propagate into the navigator stack — diff --git a/templates/flutter/lib/src/analytics_tracking.dart.twig b/templates/flutter/lib/src/analytics_tracking.dart.twig index 89dca4032b..0026e984a6 100644 --- a/templates/flutter/lib/src/analytics_tracking.dart.twig +++ b/templates/flutter/lib/src/analytics_tracking.dart.twig @@ -134,7 +134,9 @@ class AnalyticsTracking with WidgetsBindingObserver { engagementTime: engagementTime, ); if (pending is Future) { - unawaited(pending.catchError((Object _) {})); + // `catchError` on a typed future must return that future's type, so an + // empty handler would itself throw. Narrow to `Future` first. + unawaited(pending.then((_) {}).catchError((Object _) {})); } } catch (_) { // Silent — see class docs. diff --git a/templates/web/src/index.ts.twig b/templates/web/src/index.ts.twig index fb521a266c..e26d7ccf29 100644 --- a/templates/web/src/index.ts.twig +++ b/templates/web/src/index.ts.twig @@ -13,7 +13,9 @@ export { {{service.name | caseUcfirst}} } from './services/{{service.name | case {% endfor %} export { Realtime } from './services/realtime'; export { Push } from './services/push'; +{% if services['analytics'] is defined %} export { AnalyticsTracking } from './services/analytics-tracking'; +{% endif %} export type { Models, Payload, @@ -27,6 +29,7 @@ export type { SubscribeOptions, MessageCallback, } from './services/push'; +{% if services['analytics'] is defined %} export type { AnalyticsEventEmitter, AnalyticsEventOptions, @@ -34,6 +37,7 @@ export type { DownloadTrackingOptions, OutboundTrackingOptions, } from './services/analytics-tracking'; +{% endif %} export type { QueryTypes, QueryTypesList } from './query'; export { Permission } from './permission'; export { Role } from './role'; diff --git a/tests/generation/GenerationTest.php b/tests/generation/GenerationTest.php index 8330538e8d..db8dcb0bc8 100644 --- a/tests/generation/GenerationTest.php +++ b/tests/generation/GenerationTest.php @@ -293,6 +293,76 @@ public function testFixtureSelection(string $name): void } } + /** + * The analytics tracking helpers are hand-written companions to a generated + * service. Shipping them into an SDK whose spec has no analytics service + * leaves files, and entry-point exports, naming a class that is never + * generated — which is what broke appwrite/sdk-for-flutter#336. + */ + public function testAnalyticsCompanionsFollowTheService(): void + { + $companions = [ + 'flutter' => ['lib/src/analytics_observer.dart', 'lib/src/analytics_tracking.dart'], + 'web' => ['src/services/analytics-tracking.ts'], + ]; + $entrypoints = [ + 'flutter' => ['lib/packageName.dart', 'analytics_observer.dart'], + 'web' => ['src/index.ts', 'analytics-tracking'], + ]; + + foreach ($companions as $name => $paths) { + // The shared fixture carries no analytics service. + $without = $this->generate($name, 'client'); + foreach ($paths as $path) { + $this->assertArrayNotHasKey($path, $without, "{$name} shipped {$path} without the analytics service"); + } + [$entry, $token] = $entrypoints[$name]; + $this->assertArrayHasKey($entry, $without, "{$name} did not generate {$entry}"); + $this->assertStringNotContainsString($token, $without[$entry], "{$name} {$entry} exports a companion it did not generate"); + + $with = $this->generateWithAnalytics($name); + foreach ($paths as $path) { + $this->assertArrayHasKey($path, $with, "{$name} did not ship {$path} alongside the analytics service"); + } + $this->assertStringContainsString($token, $with[$entry], "{$name} {$entry} does not export the companion"); + } + } + + /** + * @return array + */ + private function generateWithAnalytics(string $name): array + { + $dir = self::OUTPUT . '/' . $name . '/analytics'; + $this->removeDirectory($dir); + + $document = \json_decode((string) \file_get_contents(self::FIXTURE), flags: JSON_THROW_ON_ERROR); + $operation = clone $document->paths->{'/ping'}->get; + $operation->tags = ['analytics']; + $operation->operationId = 'analyticsCreateEvent'; + $document->paths->{'/analytics/event'} = (object) ['get' => $operation]; + $document->tags[] = (object) ['name' => 'analytics', 'description' => 'Analytics']; + + $sdk = new SDK($this->language($name), Parser::parse(\json_encode($document, JSON_THROW_ON_ERROR))); + $sdk + ->setName('test') + ->setVersion('0.0.1') + ->setPlatform('client') + ->setNamespace('appwrite') + ->setTest('true') + ->generate($dir); + + $files = []; + $iterator = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($dir, FilesystemIterator::SKIP_DOTS)); + foreach ($iterator as $file) { + if ($file->isFile()) { + $files[\substr((string) $file->getPathname(), \strlen($dir) + 1)] = \strtolower((string) \file_get_contents($file->getPathname())); + } + } + + return $files; + } + public function testJavascriptClientOwnership(): void { $targets = [