refactor(BREAKING): Require client to be initialized for getEventByPushNotification()#2357
refactor(BREAKING): Require client to be initialized for getEventByPushNotification()#2357krille-chan wants to merge 1 commit into
Conversation
…shNotification() Before we have (in theory) supported that the client does not need to be initialized to call client.getEventByPushNotification() with the intention to allow multiple isolates. However this has never been used (or tested) in any of our clients (thanks to fcm_shared_isolate) and probably never worked. Also suggesting to not need to initialize might more harm than actually been useful. Initializing a client is now possible without waiting for all rooms or other data to be loaded so can be done in very short time. The current implementation also had the problem that it only loads the token, not the refresh token or refresh token expires in datetime. Instead of duplicating all these logic or abstracting it, I'd find it more clean to remove this "feature" and require to initialize the client before, avoiding confusion, for the sake of simplicity and more intuitive behavior of the SDK.
📝 WalkthroughWalkthroughThe PR tightens the contract for ChangesPush Notification Event Fetching
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/src/client.dart`:
- Line 1858: Correct the spelling in the documentation comment that currently
reads "The client **must** be initialized and looged in or will otherwise" by
replacing "looged" with "logged" so it reads "The client **must** be initialized
and logged in or will otherwise"; update the doc comment in lib/src/client.dart
(the comment block containing that sentence) to fix the typo.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1caddab3-a37b-4a7f-a203-98f03ed402a6
📒 Files selected for processing (1)
lib/src/client.dart
| /// initialized, it will only fetch the necessary parts of the database. This | ||
| /// should make it possible to run this parallel to another client with the | ||
| /// same client name. | ||
| /// The client **must** be initialized and looged in or will otherwise |
There was a problem hiding this comment.
Fix typo in documentation.
Line 1858 contains a spelling error: "looged" should be "logged".
📝 Proposed fix
- /// The client **must** be initialized and looged in or will otherwise
+ /// The client **must** be initialized and logged in or will otherwise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// The client **must** be initialized and looged in or will otherwise | |
| /// The client **must** be initialized and logged in or will otherwise |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/client.dart` at line 1858, Correct the spelling in the documentation
comment that currently reads "The client **must** be initialized and looged in
or will otherwise" by replacing "looged" with "logged" so it reads "The client
**must** be initialized and logged in or will otherwise"; update the doc comment
in lib/src/client.dart (the comment block containing that sentence) to fix the
typo.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2357 +/- ##
==========================================
+ Coverage 59.26% 59.27% +0.01%
==========================================
Files 161 161
Lines 20060 20057 -3
==========================================
+ Hits 11888 11889 +1
+ Misses 8172 8168 -4
Continue to review full report in Codecov by Sentry.
|
1 participant
Before we have (in theory) supported that the client does not need to be initialized to call client.getEventByPushNotification() with the intention to allow multiple isolates. However this has never been used (or tested) in any of our clients (thanks to fcm_shared_isolate) and probably never worked. Also suggesting to not need to initialize might more harm than actually been useful. Initializing a client is now possible without waiting for all rooms or other data to be loaded so can be done in very short time.
The current implementation also had the problem that it only loads the token, not the refresh token or refresh token expires in datetime. Instead of duplicating all these logic or abstracting it, I'd find it more clean to remove this "feature" and require to initialize the client before, avoiding confusion, for the sake of simplicity and more intuitive behavior of the SDK.