Conversation
WalkthroughAdds session tracking: new SessionService manages sessions via app lifecycle, storage, and EventService; ClixConfig gains sessionTimeoutMs; Clix creates, wires, and starts SessionService after notifications initialize; NotificationService forwards tapped message IDs to SessionService; package bumped to 0.0.4 and changelog updated. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 236ddd03f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/src/services/session_service.dart`:
- Around line 47-54: didChangeAppLifecycleState currently calls the async
helpers _onResumed() and _onPaused() fire-and-forget, which can produce
unhandled Future errors (e.g., from _updateLastActivity()); fix by ensuring the
Futures are observed: either make didChangeAppLifecycleState async and await the
calls, or wrap each call with explicit error handling (e.g.,
_onPaused().catchError(...) or a try/catch inside _onPaused/_onResumed that logs
and swallows/report errors). Update references to _onResumed, _onPaused and
_updateLastActivity so storage write failures are caught and logged instead of
becoming unhandled Future exceptions.
🧹 Nitpick comments (3)
lib/src/services/session_service.dart (3)
32-45: Duplicate session-resume logic betweenstart()and_onResumed().Both methods contain the same "check last activity → continue or start new session" pattern. Extract a shared helper to reduce duplication and keep the logic consistent.
♻️ Proposed refactor
Future<void> start() async { WidgetsBinding.instance.addObserver(this); - - final lastActivity = await _storageService.get<int>(_lastActivityKey); - if (lastActivity != null) { - final elapsed = DateTime.now().millisecondsSinceEpoch - lastActivity; - if (elapsed <= _effectiveTimeoutMs) { - await _updateLastActivity(); - ClixLogger.debug('Continuing existing session'); - return; - } - } - await _startNewSession(); + await _resumeOrStartSession(); } Future<void> _onResumed() async { // Small delay to allow notification tap handlers to set pendingMessageId await Future.delayed(const Duration(milliseconds: 100)); + await _resumeOrStartSession(); + } + Future<void> _resumeOrStartSession() async { final lastActivity = await _storageService.get<int>(_lastActivityKey); if (lastActivity != null) { final elapsed = DateTime.now().millisecondsSinceEpoch - lastActivity; if (elapsed <= _effectiveTimeoutMs) { await _updateLastActivity(); + ClixLogger.debug('Continuing existing session'); return; } } await _startNewSession(); }Also applies to: 60-73
60-62: The 100 ms artificial delay is fragile.The comment explains the delay allows notification tap handlers to set
pendingMessageId, but 100 ms is an arbitrary race-condition workaround with no guarantee. On slow devices or under load, the tap handler may not have executed yet; on fast paths, it adds unnecessary latency. Consider documenting this tradeoff more explicitly or exploring a more deterministic coordination mechanism in the future.
1-99: NoremoveObserver/ teardown method.
addObserver(this)is called instart()but there is no correspondingremoveObserverpath. If the SDK is ever re-initialized (e.g., during testing or hot-restart scenarios), the old observer remains registered.♻️ Proposed addition
+ void dispose() { + WidgetsBinding.instance.removeObserver(this); + }
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/src/services/session_service.dart`:
- Around line 14-29: SessionService registers itself as a WidgetsBindingObserver
in start() but never removes itself; add a public dispose() method on
SessionService that calls WidgetsBinding.instance.removeObserver(this) (and any
other cleanup like cancelling timers or clearing _pendingMessageId if needed)
and ensure callers tear down the service by invoking dispose(); reference the
SessionService class and its start() method to locate where addObserver(this) is
called and add the corresponding removeObserver(this) call in the new dispose()
method.
🧹 Nitpick comments (2)
lib/src/services/session_service.dart (2)
31-44: Duplicate session-check logic betweenstart()and_onResumed().Lines 34–42 and 63–71 contain identical session-continuation logic. Extract a shared helper to reduce duplication and keep the two paths consistent.
♻️ Proposed refactor
+ Future<bool> _shouldContinueSession() async { + final lastActivity = await _storageService.get<int>(_lastActivityKey); + if (lastActivity != null) { + final elapsed = DateTime.now().millisecondsSinceEpoch - lastActivity; + if (elapsed <= _effectiveTimeoutMs) { + await _updateLastActivity(); + return true; + } + } + return false; + } + Future<void> start() async { WidgetsBinding.instance.addObserver(this); - - final lastActivity = await _storageService.get<int>(_lastActivityKey); - if (lastActivity != null) { - final elapsed = DateTime.now().millisecondsSinceEpoch - lastActivity; - if (elapsed <= _effectiveTimeoutMs) { - await _updateLastActivity(); - ClixLogger.debug('Continuing existing session'); - return; - } + if (await _shouldContinueSession()) { + ClixLogger.debug('Continuing existing session'); + return; } await _startNewSession(); }Apply the same extraction to
_onResumed().Also applies to: 59-72
61-61: Hardcoded 100ms delay is a fragile race-condition workaround.The comment explains the intent, but a fixed delay doesn't guarantee the notification tap handler has completed — it could be too short on slow devices or unnecessarily long otherwise. Consider a more deterministic approach, such as having
NotificationServiceset the pending message ID before the lifecycle event fires, or using aCompleter/signal that_onResumedcan await.
_handleInitialMessage() was called without await in _setupMessageHandlers(), causing a race where sessionService.start() could fire SESSION_START before the initial notification set pendingMessageId on cold-start push tap. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
didChangeAppLifecycleState is synchronous but calls async _onResumed() and _onPaused(). Wrap both in try-catch to prevent unhandled Future errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prevents memory leak and stale callbacks when SessionService is torn down. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/src/services/session_service.dart`:
- Around line 31-45: Wrap the SDK operations inside start() in a try/catch that
catches ClixError (and optionally any other errors you want to handle) to
prevent exceptions from bubbling out during initialization; specifically,
surround calls to _storageService.get<int>(_lastActivityKey),
_updateLastActivity(), and _startNewSession() with the try/catch, and in the
catch block log the error via ClixLogger.error (including the error details) and
abort/return gracefully so startup doesn't throw unhandled ClixError exceptions.
Prevent errors from bubbling out during SDK initialization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
SessionServicetracksSESSION_STARTevents usingWidgetsBindingObserverfor lifecycle detection (resumed/paused)sessionTimeoutMs(default 30s, minimum 5s enforced)pendingMessageIdthroughNotificationServicesessionTimeoutMstoClixConfigChanges
lib/src/services/session_service.dart—SessionServicewithWidgetsBindingObserver,SessionEventenumlib/src/core/clix.dart— Initialize and startSessionServiceafterNotificationServicelib/src/core/clix_config.dart/.g.dart— AddsessionTimeoutMsfield (default 30000)lib/src/services/notification_service.dart— SetpendingMessageIdon push tap for session attributionSpec
Summary by CodeRabbit