workout export fix, luminance dedupe, remove stray local test files - #312
workout export fix, luminance dedupe, remove stray local test files#312abdulsaheel wants to merge 1 commit into
Conversation
…dupe luminance, drop leaked payload dumps - app_state.dart: a workout row still 'live' past the resume ceiling gets finalized locally with a reconcile-time end_ts, but that end_ts is made up (we don't actually know when it ended) — it was still getting pushed to Apple Health/Health Connect as a real workout. Stopped the export for that branch, added a regression test. - theme.dart: P.luminance() was reimplementing Color.computeLuminance(), same WCAG formula. Just call the built-in. - removed payload.json/payload_july10.json/payload_null.json — real dated HRV/sleep output that shouldn't have been committed. Added payload*.json to .gitignore so it can't happen again.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Reviewer's GuideThe PR fixes Health export correctness by keeping stale orphan reconciliation local, consolidates luminance logic on Flutter’s built-in API, and cleans up untracked local payload artifacts with a corresponding ignore rule. Sequence diagram for stale workout reconciliation without Health exportsequenceDiagram
participant AppState
participant LocalDb
participant HealthExporter
AppState->>LocalDb: putSession(row with status done and end_ts)
Note over AppState,HealthExporter: Fabricated reconciliation end_ts is never exported
AppState-->>AppState: _log(stale live-session row finalized)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="lib/state/app_state.dart" line_range="5451-5452" />
<code_context>
final reconciledEndTs = nowMs ~/ 1000;
await LocalDb.putSession({
...row,
'status': 'done',
'end_ts': row['end_ts'] ?? reconciledEndTs,
});
- final id = row['id'] as String?;
</code_context>
<issue_to_address>
**issue (broader_impact):** The stale row is marked `done` and given a valid-looking `end_ts`, so removing the immediate `exportWorkoutId` call does not prevent a later full health export from writing it. The health export path skips only rows whose status is `live` and accepts any completed row with `end_ts > start_ts`, causing the fabricated workout to reach Apple Health/Health Connect on a subsequent export.
**Triggers:** When a later full health export processes the reconciled session.
**Suggested fix:** Persist an explicit non-exportable/reconciled marker and make both immediate and full health export paths skip it, or leave the row out of the exportable completed-session set.
</issue_to_address>
### Comment 2
<location path="test/app_state_regressions_test.dart" line_range="226-227" />
<code_context>
+ addTearDown(app.dispose);
+ await app.debugReconcileOrphanedLiveWorkout();
+ // exportWorkoutId is fired unawaited from the reconcile; give it a
+ // chance to run before asserting nothing came through.
+ await Future<void>.delayed(const Duration(milliseconds: 20));
+
+ expect(calls, isEmpty,
</code_context>
<issue_to_address>
**issue (testing):** The regression test waits a fixed 20 ms for the removed unawaited export, but the export chain includes asynchronous preferences, database, configuration, deletion, and platform calls. A reintroduced export can therefore still be running when `calls` is asserted, allowing the test to pass without observing the platform call.
**Triggers:** When the export path takes longer than 20 ms on a loaded or slower test run.
**Suggested fix:** Use an explicit completion signal or polling loop with a bounded timeout, and assert after the export future's observable work has completed rather than relying on a fixed sleep.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and if this decision is wrong, stale workouts will remain marked done locally but never reach Apple Health or Health Connect, and reverting will not automatically re-export them. The omission is bounded and can be repaired by explicitly reprocessing the affected sessions; the change does not create or publish fabricated health data.
Blocking findings: lib/state/app_state.dart:5452, test/app_state_regressions_test.dart:227
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 'status': 'done', | ||
| 'end_ts': row['end_ts'] ?? reconciledEndTs, |
There was a problem hiding this comment.
issue (broader_impact): The stale row is marked done and given a valid-looking end_ts, so removing the immediate exportWorkoutId call does not prevent a later full health export from writing it. The health export path skips only rows whose status is live and accepts any completed row with end_ts > start_ts, causing the fabricated workout to reach Apple Health/Health Connect on a subsequent export.
Triggers: When a later full health export processes the reconciled session.
Suggested fix: Persist an explicit non-exportable/reconciled marker and make both immediate and full health export paths skip it, or leave the row out of the exportable completed-session set.
| // chance to run before asserting nothing came through. | ||
| await Future<void>.delayed(const Duration(milliseconds: 20)); |
There was a problem hiding this comment.
issue (testing): The regression test waits a fixed 20 ms for the removed unawaited export, but the export chain includes asynchronous preferences, database, configuration, deletion, and platform calls. A reintroduced export can therefore still be running when calls is asserted, allowing the test to pass without observing the platform call.
Triggers: When the export path takes longer than 20 ms on a loaded or slower test run.
Suggested fix: Use an explicit completion signal or polling loop with a bounded timeout, and assert after the export future's observable work has completed rather than relying on a fixed sleep.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|
squashed into #314 for one clean review — closing this round. |
1 participant
User description
another audit pass fixes:
tests: flutter test on touched files, flutter analyze clean.
Summary by Sourcery
Prevent fabricated workout durations from reaching Health exports while deduplicating luminance calculations and cleaning up stray local files.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
PR Type
Bug fix, Tests, Enhancement
Description
Stop exporting stale workouts to Health.
Replace custom luminance with built-in Flutter method.
Add test for stale workout export prevention.
Remove leaked local JSON payload files.
Diagram Walkthrough
File Walkthrough
1 files
Stop exporting stale orphaned workouts to Health.1 files
Use built-in Flutter luminance calculation for contrast.1 files
Add regression test for stale workout export prevention.1 files
Remove public luminance helper from UI documentation.3 files
Remove stray local test payload JSON file.Remove stray local test payload JSON file.Remove stray local test payload JSON file.