You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Count notification retention in calendar days, not 336 hours - #687
FiredKeyStore._cutoffLabel() and NotificationIds._prune each computed the 14-day retention cutoff as dayLabelOf(DateTime.now().subtract(const Duration(days: 14))). That is 336 hours, not 14 calendar days: across a DST change one of those days is 23 or 25 h, so in the hour either side of local midnight the cutoff lands one label early or late. AGENTS.md §3.7 calls this out ("Day-length arithmetic must not assume 86400 s"), and day_label.dart already has dayLabelBefore for exactly this.
Impact is small, to be upfront: twice a year, dated fired-key / slot bookkeeping is kept a day longer or shorter. The dedupe only needs today's keys. The other reason to fix it is that the same cutoff was written out twice and could drift.
What changes
FiredKeyStore.cutoffLabel([DateTime? now]) (was private _cutoffLabel()): dayLabelBefore(todayLabel(now), retentionDays)!. Both of its callers (the DB prune and the prefs-mirror prune) use it.
NotificationIds._prune uses FiredKeyStore.cutoffLabel() instead of its own copy (NotificationIds.retentionDays is already defined as FiredKeyStore.retentionDays). Its now-unused day_label.dart import is dropped.
Tests
New test/notify_retention_dst_test.dart moves the process timezone to America/New_York (libc setenv/tzset, as day_window_dst_test does) and checks the cutoff at 00:30 the day after the 23 h day (2026-02-23, the old code gave 2026-02-22), at 23:30 the day after the 25 h day (2026-10-19, old: 2026-10-20), and on an ordinary day. A first test asserts the zone really has DST, so the others can't pass vacuously. With the old formula, the two DST cases fail.
All 9 test files that touch FiredKeyStore / NotificationIds: 86 passed.
flutter analyze: no issues.
Full suite not re-run for this change: it only touches the two prune paths, all of whose tests are above.
FiredKeyStore and NotificationIds each computed the retention cutoff as
dayLabelOf(now.subtract(Duration(days: 14))). That is 14 x 24 h: across
a DST change one day is 23 or 25 h, so in the hour either side of local
midnight the cutoff landed one label early or late (AGENTS.md: day
arithmetic must not assume 86400 s).
One shared FiredKeyStore.cutoffLabel([now]) now does it with the
existing calendar helpers, dayLabelBefore(todayLabel(now), 14), and
NotificationIds uses it instead of its own copy. The effect was only a
day more or less of notification bookkeeping kept twice a year, but the
two copies could also have drifted apart.
No actionable comments were generated in the recent review.
Walkthrough
Notification retention pruning now uses a shared calendar-day cutoff helper. The helper accepts an optional date, and the database, SharedPreferences, and notification ID pruning paths use it.
FiredKeyStore.cutoffLabel([DateTime? now]) calculates the cutoff using calendar-day labels. Database and SharedPreferences pruning use the helper. Notification ID pruning also uses it and removes its day_label.dart import.
OpenStrap/edge#645: Changes pruning of expired SharedPreferences fired-key flags, another part of fired-key retention.
OpenStrap/edge#642: Uses calendar-day arithmetic for a separate retention cutoff.
Suggested reviewers:abdulsaheel
Merge Risk: Minimal · up to b380e
Notification retention now counts calendar days. No merge-blocking issue was identified.
Pre-merge checks: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly describes the main change: notification retention now uses calendar days instead of a fixed 336-hour period.
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.)
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Reviewing files that changed from the base of the PR and between 69341a6 and b380eb7.
Files ignored due to path filters: 1
test/notify_retention_dst_test.dart is excluded by !test/**
Files selected for processing: 2
lib/notify/fired_keys.dart
lib/notify/notification_ids.dart
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
Expose a shared calendar-day cutoff helper so notification retention no longer depends on elapsed 24-hour periods.
lib/notify/fired_keys.dart L179-184
Uses of the API
Use the shared cutoff for fired-key database and preferences pruning, and for notification-slot pruning, removing the duplicated date-label dependency.
Verify the shared cutoff stays at 14 calendar days across DST transitions and on an ordinary day.
test/notify_retention_dst_test.dart L1-64
Tips and commands
Interacting with Sourcery
Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment @sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment @sourcery-ai review to trigger a new review!
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments### Comment 1
<locationpath="test/notify_retention_dst_test.dart"line_range="30" />
<code_context>
+ lib.lookupFunction<_UnsetenvNative, _UnsetenvDart>('unsetenv')(key);+ } else {
+ final value = tz.toNativeUtf8();+ lib.lookupFunction<_SetenvNative, _SetenvDart>('setenv')(key, value, 1);+ calloc.free(value);+ }
</code_context>
<issue_to_address>
**Windows test setup fails**
When the Flutter test suite runs on Windows, `_setProcessTz` looks up POSIX `setenv`/`tzset` symbols that Windows does not provide, so `setUpAll` throws before assertions run and the test file fails.
Use a Windows-compatible way to set and restore the process time zone, or skip this setup on Windows.
Also at `test/notify_retention_dst_test.dart:42`.
</issue_to_address>
Sourcery assessment
Needs a human reviewer. 1 finding to address first, and the new calendar-day cutoff is used to prune both fired-key and notification-slot claims. If it removes a claim too soon, a later event can send a duplicate notification; reverting does not restore claims already deleted.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Medium · Windows test setup fails
When the Flutter test suite runs on Windows, _setProcessTz looks up POSIX setenv/tzset symbols that Windows does not provide, so setUpAll throws before assertions run and the test file fails.
Use a Windows-compatible way to set and restore the process time zone, or skip this setup on Windows.
Also at test/notify_retention_dst_test.dart:42.
Prompt for AI agents
In `test/notify_retention_dst_test.dart` at line 30:
**Windows test setup fails**
When the Flutter test suite runs on Windows, `_setProcessTz` looks up POSIX `setenv`/`tzset` symbols that Windows does not provide, so `setUpAll` throws before assertions run and the test file fails.
Use a Windows-compatible way to set and restore the process time zone, or skip this setup on Windows.
Also at `test/notify_retention_dst_test.dart:42`.
This branch has not been deployed
No deployments
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
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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Why
FiredKeyStore._cutoffLabel()andNotificationIds._pruneeach computed the 14-day retention cutoff asdayLabelOf(DateTime.now().subtract(const Duration(days: 14))). That is 336 hours, not 14 calendar days: across a DST change one of those days is 23 or 25 h, so in the hour either side of local midnight the cutoff lands one label early or late. AGENTS.md §3.7 calls this out ("Day-length arithmetic must not assume 86400 s"), andday_label.dartalready hasdayLabelBeforefor exactly this.Impact is small, to be upfront: twice a year, dated fired-key / slot bookkeeping is kept a day longer or shorter. The dedupe only needs today's keys. The other reason to fix it is that the same cutoff was written out twice and could drift.
What changes
FiredKeyStore.cutoffLabel([DateTime? now])(was private_cutoffLabel()):dayLabelBefore(todayLabel(now), retentionDays)!. Both of its callers (the DB prune and the prefs-mirror prune) use it.NotificationIds._pruneusesFiredKeyStore.cutoffLabel()instead of its own copy (NotificationIds.retentionDaysis already defined asFiredKeyStore.retentionDays). Its now-unusedday_label.dartimport is dropped.Tests
test/notify_retention_dst_test.dartmoves the process timezone to America/New_York (libcsetenv/tzset, asday_window_dst_testdoes) and checks the cutoff at 00:30 the day after the 23 h day (2026-02-23, the old code gave2026-02-22), at 23:30 the day after the 25 h day (2026-10-19, old:2026-10-20), and on an ordinary day. A first test asserts the zone really has DST, so the others can't pass vacuously. With the old formula, the two DST cases fail.FiredKeyStore/NotificationIds: 86 passed.flutter analyze: no issues.🤖 Generated with Claude Code
Summary by Sourcery
Use calendar-day retention cutoffs consistently across notification bookkeeping and verify them across daylight-saving transitions.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit