Repository navigation
Conversation
SnowLukin
changed the title
|
thanks @SnowLukin, this is very exciting! we'll take a look at this 🙌 |
dustinbyrne
left a comment
dustinbyrne
left a comment
There was a problem hiding this comment.
looking great! i reviewed this with my agent and some findings below. we're happy to help get this over the line too - let us know if you'd like us to resolve any of these. thanks again!
Agent notes
The overall desktop implementation looks reasonable, but I would address the asynchronous client/consent/reset boundaries before shipping, and settle the persistence scope before applications start depending on it. I reproduced the event-routing, consent, stale-flags, bootstrap, exposure and close behaviors through the public Dart entry points with unchanged SDK source and loopback HTTP. These tests ran in a standalone harness on macOS, not native Windows/Linux. The storage-scope observation is source-supported; Windows FFI still needs direct runtime coverage.
Shared-debt follow-up: exposure invalidation
The exposure tracker retains all previously seen values across reloads. I reproduced two exposures for true→false→true, and no new exposure when experiment metadata changed without changing the value. This conflicts with the canonical tracker invalidation rule and is shared with other SDKs. I would track a coordinated follow-up rather than make a broad dedupe redesign a gate for this PR; unchanged-state reloads should remain deduplicated.
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
Advisory review follow-up: adding the beforeSend reuse suggestion. The two blocking findings already have inline comments, so they are not duplicated here.
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
Non-blocking desktop persistence performance consideration.
| void _writeAtomically(File file, String contents) { | ||
| final tmp = File('${file.path}.tmp'); | ||
| try { | ||
| tmp.writeAsStringSync(contents); |
There was a problem hiding this comment.
suggestion: Consider asynchronous persistence to avoid blocking the UI isolate — these synchronous file writes/renames, together with queue reads/deletions, run on the calling isolate (normally Flutter's UI isolate). Slow disks, antivirus scanning or event bursts could delay frames; a Future-returning capture/flush method or timer does not move synchronous IO off that isolate. Dart already offers async writeAsString/readAsBytes/rename/delete APIs; awaiting them would yield to UI work without requiring a custom worker isolate. The change would need serialized snapshot writes (the temp filename is shared), coordinated queue reads/deletions, and explicit persistence-completion semantics for capture, consent and close. Alternatively, profile representative Windows/Linux workloads and document acceptance of the current tradeoff. Reproduction: not reproduced — no desktop frame-timing benchmark was run; this is a potential performance risk, not a measured UI-stall defect.
There was a problem hiding this comment.
I kept disk I/O synchronous in this update and focused on correctness and directory discovery. Async persistence remains deferred; it needs coordinated writes and clear completion semantics for consent and shutdown.
| import 'dart:math'; | ||
| import 'dart:typed_data'; | ||
|
|
||
| /// Generates a UUID version 7 (RFC 9562): a millisecond Unix timestamp |
There was a problem hiding this comment.
or we vendor this one https://github.com/daegalus/dart-uuid/blob/main/lib/v7.dart which is the most used in the dart lang
copying the license and header on top of the file
There was a problem hiding this comment.
I kept the current generator because queue filenames rely on UUIDs sorting in generation order, including within one millisecond and when the clock moves backwards. The linked implementation uses a random tail and does not provide that ordering guarantee, so replacing it directly would change queue ordering.
|
i have a linux machine and can test this on linux EoW |
|
@PostHog/team-client-libraries any of you have a WIN machine? |
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
Non-blocking suggestion for desktop application-directory discovery.
|
I've fixed the event, consent and feature-flag issues and added regression tests, including coverage for storage recovery and pending events across close/setup. Directory discovery now uses the official Windows/Linux I left disk I/O synchronous and focused this update on correctness and directory discovery. Maintainer edits are enabled, so feel free to push adjustments directly to this branch. |
|
Regarding the shared exposure-invalidation follow-up: I left metadata-aware exposure invalidation unchanged, following your suggestion to handle it as a coordinated SDK follow-up. The opted-out-read bug is fixed separately in this PR. |
close() now makes one attempt to send the queued events, bounded by two seconds, before it closes the client. A setup() that follows waits for that close, so the next client gets the storage lock and the queue on disk instead of falling back to memory.
Without beforeSend callbacks the event was still awaited, so an identify(), reset() or opt-out made right after an unawaited capture, screen or captureException ran first, and the event was dropped as belonging to another user. Now it is captured synchronously, as on the other platforms. An event dropped after its callbacks is now logged in debug mode.
Where /etc/localtime is a copy rather than a link into the zoneinfo directory, as in many containers, $timezone was left out. Debian-based systems still name the zone in /etc/timezone, which is now read as a fallback.
The public close() docs now say that desktop sends the queue first, bounded by two seconds, and what happens to the events left unsent. The core client docs no longer promise that queued events survive close() with an in-memory storage.
An event whose async beforeSend callbacks finished after close() and before the next setup() completed found no client and was dropped, although it belongs to the same project and user. It now waits for the setup in progress. A repeated close() also waits until the queue is sent instead of returning at once.
The three capture paths no longer repeat the wait for a setup in progress: the helper that applies the callbacks does it, and the call sites only await when callbacks ran.
PostHog always answers /flags/?v=2 with the flags key. The v1 fallback is kept for servers that ignore v=2, matching posthog-js, iOS, Android and Python, and now says so.
Tear-downs run in reverse order, so a directory created after the platform was deleted while the platform still held its lock file open. Windows refuses that deletion, which failed two setup tests there. The directory is now created first.
|
Since CI here is waiting for approval, ran it on the fork. Windows and Linux are green, including a new integration test that runs the SDK inside the example app on both (context props, flag person properties, close, restart with a queued event): https://github.com/SnowLukin/posthog-flutter/actions/runs/37109227395 Two setup tests were failing on Windows only, the temp dir got deleted before the platform closed and released its lock file. Fixed the order |
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
Re-review follow-up: one reproduced blocking issue remains.
mind signing your commits and force push? |
group('company', 'free-company') after group('company', 'paid-company',
groupProperties: {'plan': 'pro'}) kept the paid company's properties, so the
next /flags request evaluated free-company with plan = pro. A new key for a
group type now clears the flag properties stored for that type before the
new ones are applied, as posthog-js does.
8fdd633 to
266a0f5
Compare
…rm-support # Conflicts: # posthog_flutter/pubspec.yaml
|
@marandaneto done, all commits are signed and force pushed. The group properties issue is fixed too, and main is merged in |
…rm-support # Conflicts: # posthog_flutter/pubspec.yaml
thanks, i am attending nextappcon this week but @PostHog/team-client-libraries can have a final look and merge/release if all good |
|
will verify this on windows tomorrow, otherwise this is looking good to me 👍 |
just to update - still working on this |
|
I integrated this SDK into a separate Flutter application using a local path dependency on PR head Windows smoke test passed. Auto-registration, capture, identity persistence, offline queue delivery, feature flags, and error capture worked. The PR’s unit tests also completed: 1,206 passed, 14 skipped. Two non-blocking follow-ups:
One environment-specific observation: |
dustinbyrne
left a comment
dustinbyrne
left a comment
There was a problem hiding this comment.
looks like everything's been addressed and i'd consider the findings from the windows smoke test to be non-blocking (we can always follow up with small fixes, improvements to test coverage, etc.)
thank you again @SnowLukin!!
This branch has not been deployed
4 participants
Motivation and context
Adds Windows and Linux support to the Flutter SDK. Related to #51.
Applications use the existing
Posthog().setup(config)API. Flutter registers the plugin automatically; no separate package or platform-specific setup is needed. The implementation ports PostHog core to Dart insideposthog_flutter.path_providerimplementations with a separate directory per project.Existing public APIs remain compatible. The only public API addition is
PosthogFlutterDart.registerWith(), used by Flutter's plugin registrant.Session replay, surveys, structured logs, push notifications and native crash capture are outside this implementation.
How did you test it?
I manually tested this on Windows and am already using it in an application.
The Windows CI job also tests version-resource extraction from the built example executable under a Unicode path. The current CI run is awaiting approval to run.
Checklist
Agent context
Autonomy: Human-driven (agent-assisted)
DRI: @SnowLukin
Used Codex, GitHub CLI and Flutter tools for implementation, testing and review. Kept desktop support within the existing package and API.