steps reset at midnight, and a dot for whether anything is syncing - #216
Conversation
…ening the today tile shows the derived day total plus the live count, and the live count is since the ble connection began. the whole engine is built around never dropping that connection, so it spans midnight — at 00:01 the tile carried the whole of yesterday on top of today and kept climbing. the day boundary is watched from the sample path rather than from the widget: a phone parked on another tab across midnight would otherwise make its first read of the day the first read of any day, and a first read counts in full. sync stays invisible — no spinners, no copy — but invisible and broken look the same, so a 6pt dot next to the wordmark breathes while records are actually landing and is absent otherwise.
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughChangesDaily steps and synchronization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HistoricalSync
participant AppState
participant TodayScreen
participant SyncDot
HistoricalSync->>AppState: ingest landed records
AppState->>AppState: record arrival time
TodayScreen->>AppState: read syncingNow
AppState-->>TodayScreen: rebuild on sync state change
TodayScreen->>SyncDot: set active state
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
PR Reviewer Guide 🔍(Review updated until commit 2d10497)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 2d10497
Previous suggestionsSuggestions up to commit 8631555
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/ble/ble_state.dart`:
- Around line 591-596: Normalize negative session totals to zero in the
session-total handling near the rebasing logic in lib/ble/ble_state.dart:591-596
before updating _base and calculating n, while preserving existing
positive-total behavior. Add or update the test at
test/live_step_day_window_test.dart:55-59 to assert that a zero reading
following -5 returns zero.
In `@lib/state/app_state.dart`:
- Around line 3590-3610: Add a one-shot expiry Timer alongside _lastIngestMs,
re-arm it whenever records are recorded so it fires after _syncActiveWindowMs,
and call notifyListeners from its callback to refresh syncingNow. Cancel and
clear the Timer in dispose() to prevent callbacks after disposal.
- Around line 2757-2760: Move the durable-write arrival timestamp update into
_onDataStored(), ensuring _lastIngestMs is set whenever the write completes even
if _lastRecTs was already advanced. Remove the completed _runSyncBurst-based
arrival inference, preserve listener notification behavior, and add a regression
test covering _onDataStored() advancing _lastRecTs before the sync callback.
In `@test/sync_dot_test.dart`:
- Around line 54-59: Add a unique key to the outer fixed SizedBox rendered by
SyncDot, then update both getSize calls in the “it occupies the same space
either way” test to use find.byKey with that key instead of
find.byType(SyncDot).
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: e1e12315-556b-4161-a905-cb56f9fdd9f7
📒 Files selected for processing (7)
lib/ble/ble_state.dartlib/state/app_state.dartlib/ui/design/design.dartlib/ui/design/sync_dot.dartlib/ui/today/today_screen.darttest/live_step_day_window_test.darttest/sync_dot_test.dart
| if (frontierAfter != null && frontierAfter > (_lastRecTs ?? 0)) { | ||
| _lastRecTs = frontierAfter; | ||
| _lastIngestMs = DateTime.now().millisecondsSinceEpoch; | ||
| notifyListeners(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Record sync arrival at the durable-write callback.
Line 2757 can be false because _onDataStored() may already have advanced _lastRecTs. In that ordering, line 2759 does not run and syncingNow stays false while records land.
Set _lastIngestMs in _onDataStored() when the durable write completes. Do not derive arrival from a completed _runSyncBurst session. Add a regression test for this ordering.
As per coding guidelines, “When adding or changing a capability, cover every call path, including all raw decode paths and all relevant export/session triggers.”
🤖 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/state/app_state.dart` around lines 2757 - 2760, Move the durable-write
arrival timestamp update into _onDataStored(), ensuring _lastIngestMs is set
whenever the write completes even if _lastRecTs was already advanced. Remove the
completed _runSyncBurst-based arrival inference, preserve listener notification
behavior, and add a regression test covering _onDataStored() advancing
_lastRecTs before the sync callback.
Source: Coding guidelines
_onDataStored advances the frontier itself, so by the time the sync burst checked whether the frontier had moved it never had — the indicator was hung off a condition that is false exactly when records land. it marks activity at the durable write instead, which is the one path that sees every commit. the window also decayed on wall-clock time with nothing notifying at the boundary, so a band that went quiet left the dot lit until some unrelated change came along. a one-shot timer closes it, cancelled on dispose. a negative counter reading could seat itself as the day's baseline, and the next ordinary reading would report the difference as steps nobody took.
|
Persistent review updated to latest commit 2d10497 |
1 participant
User description
Two things from TestFlight feedback.
Steps never reset at midnight. The Today tile shows the derived day total plus the live count, and the live count is "since the BLE connection began". This app is built around never dropping that connection, so it spans midnight — at 00:01 the tile carried the whole of yesterday on top of today's zero and kept climbing from there. Reported as steps and calories not resetting, just accumulating.
The rebase happens at the day boundary, and the boundary is watched from the sample path rather than from the widget that displays it: a phone parked on the Sleep tab across midnight would otherwise make its first read of the new day the window's first read of any day, and a first read has to count in full — rebasing there instead throws away a real walk, which the live-coverage tests catch.
I could not reproduce the calories half. That tile reads the derived day value with no live component and the day rolls over on the screen's own refresh, so I think the climbing steps number is what was being described. Worth a second look if it persists.
No way to tell whether a sync is happening. Sync is deliberately invisible here — no spinners, no progress, no copy — and that is still right, because it runs constantly and there is nothing to act on. But invisible and broken look identical, which is what the report is actually about. So: a 6pt dot beside the wordmark that breathes while records are landing and is absent otherwise. No text, nothing to dismiss, and the space is held either way so the title never shifts. It is driven by "records are landing right now", not by "connected" or "a sync was requested", so a quiet dot means a quiet link rather than a broken one.
PR Type
Bug fix, Enhancement, Tests
Description
Steps tile now resets at local midnight instead of accumulating across days
Animated 6pt sync dot beside wordmark shows when band data is actively landing
LiveStepDayWindowclass isolates midnight-rebase logic; 5 unit tests addedSyncDotwidget stops animation when inactive; 4 widget tests addedDiagram Walkthrough
File Walkthrough
2 files
Add LiveStepDayWindow class for midnight step rebaseWire LiveStepDayWindow into liveSteps; add syncingNow getter2 files
New animated SyncDot widget for sync activity indicationAdd SyncDot to title row; include syncingNow in select1 files
Export new sync_dot.dart from design barrel2 files
Unit tests for midnight rebase and reconnect edge casesWidget tests for SyncDot visibility and animation lifecycle