Pin the link-priority decision point so #200 cannot come back - #211
Merged
abdulsaheel merged 2 commits intoAug 9, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a centralized BLE link-priority mapper, shared Dart source-scanning utilities, runtime mapping tests, and structural tests for the priority decision flow. ChangesBLE connection-priority validation
Estimated code review effort: 3 (Moderate) | ~15 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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_engine.dart`:
- Around line 3221-3225: The _setOffloadActive transition currently launches
_applyConnectionPriority without waiting, allowing historical INIT and
SEND_HISTORICAL_DATA commands to precede Android’s high-priority update. Make
the transition or centralized priority application awaitable, then await it
before each initial INIT and _startHistoricalRefresh SEND_HISTORICAL_DATA
command; retain non-blocking behavior for non-flood paths and add regression
coverage for both triggers.
- Around line 3248-3253: Update connectionPriorityFor and its callers so
standardHrFallback is not treated as proof that the live flood is inactive;
derive priority from the actual armed stream set, or explicitly disable R10/R11,
IMU, and optical streams before applying fallback. Add a regression test
covering fallback latched during an already active full-live session, including
relevant raw decode and export/session trigger paths.
- Around line 3244-3262: Update _applyConnectionPriority to enqueue requests
through a session-scoped serialized chain so concurrent callers execute
sequentially. Recompute connectionPriorityFor when each queued operation starts,
then request that priority and update _appliedPriority only if the captured
session remains the current connected session; preserve retry behavior on
failures and prevent torn-down sessions from restoring the cache.
In `@test/connection_priority_structural_test.dart`:
- Around line 57-117: Strengthen the structural tests around
requestConnectionPriority and connectionPriorityFor: require exactly one
connectionPriorityFor policy declaration specifically in
lib/ble/ble_engine.dart, and require that the sole requestConnectionPriority
call in that engine uses a connectionPriorityRequest obtained from
connectionPriorityFor rather than another selector or hard-coded value. Add
regression coverage for calls moved outside the engine, duplicate policy
declarations, and bypassing the policy without a literal.
In `@test/connection_priority_test.dart`:
- Around line 52-180: Add controlled-device lifecycle regression tests around
the priority-application flow, rather than only testing connectionPriorityFor.
Verify each history trigger waits for high-priority completion, rapid state
changes leave the final request as the effective priority, teardown cannot write
into the next session’s cache, and standardHrFallback does not reduce priority
while high-rate streams remain enabled.
🪄 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: 9a953459-bfe5-4388-a18a-1bed53a62e02
📒 Files selected for processing (3)
lib/ble/ble_engine.darttest/connection_priority_structural_test.darttest/connection_priority_test.dart
Comment on lines
+3248
to
+3253
| final want = connectionPriorityFor( | ||
| offloadActive: _offloadActive, | ||
| liveEnabled: _liveEnabled, | ||
| liveHrOnly: _liveHrOnly, | ||
| standardHrFallback: state.standardHrFallback, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not treat standardHrFallback as proof that the flood is off.
Line 1481 can set state.standardHrFallback while _liveEnabled remains true, _liveHrOnly remains false, and the previously enabled R10/R11, IMU, and optical streams remain armed. The next keep-alive selects balanced from this latch even though the full live flood is still active.
Track the actual armed stream set for the policy, or disable the high-rate streams before applying fallback priority. Add a regression test that latches fallback during an already active full-live session.
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/ble/ble_engine.dart` around lines 3248 - 3253, Update
connectionPriorityFor and its callers so standardHrFallback is not treated as
proof that the live flood is inactive; derive priority from the actual armed
stream set, or explicitly disable R10/R11, IMU, and optical streams before
applying fallback. Add a regression test covering fallback latched during an
already active full-live session, including relevant raw decode and
export/session trigger paths.
Source: Coding guidelines
Comment on lines
+52
to
+180
| void main() { | ||
| group('connectionPriorityFor', () { | ||
| test('a history offload takes the fast interval', () { | ||
| expect( | ||
| connectionPriorityFor( | ||
| offloadActive: true, | ||
| liveEnabled: false, | ||
| liveHrOnly: false, | ||
| standardHrFallback: false, | ||
| ), | ||
| ConnectionPriority.high, | ||
| ); | ||
| }); | ||
|
|
||
| test('an offload outranks the HR-only downgrade (headless drain)', () { | ||
| // A background/headless drain connects with live in HR-only mode. The | ||
| // offload is the whole point of that wake — throttling it to lowPower | ||
| // would stretch the drain across the OS's short background window. | ||
| expect( | ||
| connectionPriorityFor( | ||
| offloadActive: true, | ||
| liveEnabled: true, | ||
| liveHrOnly: true, | ||
| standardHrFallback: false, | ||
| ), | ||
| ConnectionPriority.high, | ||
| ); | ||
| }); | ||
|
|
||
| test('an offload outranks the marginal-radio fallback', () { | ||
| // The fallback throttles the LIVE flood, not the history drain. A weak | ||
| // radio still has to get the backlog off the band. | ||
| expect( | ||
| connectionPriorityFor( | ||
| offloadActive: true, | ||
| liveEnabled: true, | ||
| liveHrOnly: false, | ||
| standardHrFallback: true, | ||
| ), | ||
| ConnectionPriority.high, | ||
| ); | ||
| }); | ||
|
|
||
| test('the full live flood takes the fast interval with no offload', () { | ||
| // A workout streams R10/R11 + 100 Hz IMU + optical with no history | ||
| // drain running. Dropping to balanced here is what would starve it into | ||
| // tripping MarginalRadioDetector -> the sticky standardHrFallback latch, | ||
| // which silently zeroes live step counting for the rest of the process. | ||
| expect( | ||
| connectionPriorityFor( | ||
| offloadActive: false, | ||
| liveEnabled: true, | ||
| liveHrOnly: false, | ||
| standardHrFallback: false, | ||
| ), | ||
| ConnectionPriority.high, | ||
| ); | ||
| }); | ||
|
|
||
| test('the backgrounded HR-only link drops to lowPower', () { | ||
| expect( | ||
| connectionPriorityFor( | ||
| offloadActive: false, | ||
| liveEnabled: true, | ||
| liveHrOnly: true, | ||
| standardHrFallback: false, | ||
| ), | ||
| ConnectionPriority.lowPower, | ||
| ); | ||
| }); | ||
|
|
||
| test('the marginal-radio fallback does not get the fast interval', () { | ||
| // THE DEFECT THIS PINS: enableLiveStreams() sets _liveHrOnly = false and | ||
| // then returns early under the fallback, so the flood toggles are never | ||
| // sent. Flags alone say "full live"; the wire carries HR only. | ||
| expect( | ||
| connectionPriorityFor( | ||
| offloadActive: false, | ||
| liveEnabled: true, | ||
| liveHrOnly: false, | ||
| standardHrFallback: true, | ||
| ), | ||
| ConnectionPriority.balanced, | ||
| ); | ||
| }); | ||
|
|
||
| test('the background downgrade outranks the fallback', () { | ||
| // Both latched: nothing is consuming live data, so take the deeper | ||
| // saving rather than the foreground-safe one. | ||
| expect( | ||
| connectionPriorityFor( | ||
| offloadActive: false, | ||
| liveEnabled: true, | ||
| liveHrOnly: true, | ||
| standardHrFallback: true, | ||
| ), | ||
| ConnectionPriority.lowPower, | ||
| ); | ||
| }); | ||
|
|
||
| test('an idle link with nothing armed sits at balanced', () { | ||
| // Live streams off entirely (spot-check cleanup restores them to OFF | ||
| // when they were off before). Balanced rather than lowPower: a | ||
| // foreground app can re-arm the flood at any moment. | ||
| expect( | ||
| connectionPriorityFor( | ||
| offloadActive: false, | ||
| liveEnabled: false, | ||
| liveHrOnly: false, | ||
| standardHrFallback: false, | ||
| ), | ||
| ConnectionPriority.balanced, | ||
| ); | ||
| }); | ||
|
|
||
| test('liveHrOnly is ignored when no live stream is armed', () { | ||
| // disableLiveStreams() clears _liveEnabled and _liveHrOnly together, but | ||
| // the policy must be total rather than relying on that pairing holding. | ||
| expect( | ||
| connectionPriorityFor( | ||
| offloadActive: false, | ||
| liveEnabled: false, | ||
| liveHrOnly: true, | ||
| standardHrFallback: true, | ||
| ), | ||
| ConnectionPriority.balanced, | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add lifecycle regression tests for priority application.
These tests call only connectionPriorityFor. They cannot detect request ordering, stale-session cache writes, or a fallback latch during an active full-live stream.
Add controlled device tests that verify: high priority completes before each history trigger, the final request wins after rapid state changes, teardown cannot update the next session cache, and fallback does not lower priority until the high-rate streams are disabled.
As per coding guidelines, “Behavior changes, especially regressions involving readiness, abstention, idempotence, synchronization, migrations, and lifecycle safety, must include regression tests.”
🤖 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 `@test/connection_priority_test.dart` around lines 52 - 180, Add
controlled-device lifecycle regression tests around the priority-application
flow, rather than only testing connectionPriorityFor. Verify each history
trigger waits for high-priority completion, rapid state changes leave the final
request as the effective priority, teardown cannot write into the next session’s
cache, and standardHrFallback does not reduce priority while high-rate streams
remain enabled.
Source: Coding guidelines
Collaborator
|
@svssathvik7 can you fix the merge conflict |
OpenStrap#209 fixed issue OpenStrap#200 by giving the connection interval a real policy (`desiredLinkPriority`) and a single serialized applier. What is not pinned is that the engine keeps going through them. The original bug was not a wrong policy — it was no policy at all: a literal `requestConnectionPriority(ConnectionPriority.high)` sat in the connect path, ran once, and nothing stepped it back down. A refactor of that path can re-add exactly that line and every behavioural test here stays green, because `desiredLinkPriority` would still return the right answer to a caller that no longer exists. There is no BLE fake to assert against, so this greps instead, like no_debug_only_apis_test: * exactly one real requestConnectionPriority call in lib/, in the engine, inside _applyLinkPriority, taking its target from desiredLinkPriority — so no request can bypass the policy; * every ConnectionPriority literal inside that mapping switch; * exactly one desiredLinkPriority declaration, in sync_policy.dart, since the literal check is per-file and a second copy would satisfy it while the engine stopped being the single decision point. Comments AND string literals are stripped before matching. Both matter: this file names the offending API in prose, and the engine's own failure path logs `requestConnectionPriority(...) failed`, so a naive text count reports two calls where there is one. Mutation-tested rather than assumed: re-adding the original literal request trips the call-site and literal checks; a duplicate policy declaration trips the last one.
svssathvik7
force-pushed
the
fix/ble-connection-priority-strap-drain
branch
from
August 9, 2026 04:17
67d7c9a to
f373684
Compare
svssathvik7
changed the title
Contributor
Author
|
@abdulsaheel Conflict resolved, but not the way a conflict fix usually goes — flagging it explicitly rather than burying it in a merge. The conflict was not textual drift. #209 and this PR were two complete implementations of the same fix in the same region of #209's implementation is better than the one I had here, on every axis I was unsure about:
So I dropped mine entirely and reset onto CodeRabbit's review here was accurate — three of its five findings were real bugs in my implementation, and #209 already avoids all three. Its fourth was that my guard counted raw text occurrences; the version in this PR strips comments and string literals, matches call syntax rather than the bare name, locates bodies by brace depth, and requires the sole call to take its argument from If you would rather not carry another grep-style test, close it — no argument from me. The value is narrow and specific: #200 shipped because a call site drifted away from a policy that still looked correct in isolation, and nothing in the suite could see that. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/link_priority_structural_test.dart`:
- Around line 41-49: The _codeLines helper currently splits source before
removing multiline constructs and ignores block comments, allowing tokens inside
them to affect structural checks. Replace it with a stateful scanner that
removes triple-quoted strings and /* ... */ comments while preserving newlines
and existing code-line behavior, then add regression cases covering multiline
strings and block comments containing the targeted priority tokens.
- Around line 121-155: Strengthen the structural tests around _applyLinkPriority
so they verify the requestConnectionPriority argument is the value produced by
the switch (want) mapping, with want assigned from desiredLinkPriority. Add a
negative regression case that rejects passing ConnectionPriority.high directly
to the request, while preserving lifecycle-safety coverage for the test behavior
change.
🪄 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: c7e93d60-799b-4aa7-a177-22e50fc467b1
📒 Files selected for processing (1)
test/link_priority_structural_test.dart
Review of the guard as it stood. Both findings are mutation-proven, not
argued: each mutation below was applied to a clean tree and the suite run.
1. THE MAPPING SWITCH WAS THE ONE UNGUARDED STEP.
`_applyLinkPriority` was treated as a trusted region — test 3 exempted every
`ConnectionPriority` literal inside it, and test 2 only asked that
`desiredLinkPriority` appear somewhere in the body. So the last hop before the
radio, LinkPriority -> ConnectionPriority, had no coverage at all. Mutating
all three arms to `.high`:
LinkPriority.balanced => ConnectionPriority.high,
LinkPriority.lowPower => ConnectionPriority.high,
is issue OpenStrap#200 exactly — the link pinned at ~11.25 ms overnight — and it passed
13/13. `desiredLinkPriority` still returned the right LinkPriority; nothing
checked what it was translated into. Nothing in test/ referenced
`ConnectionPriority` outside the structural file itself.
A grep cannot close this: it cannot tell a correct arm from a wrong one. So
the switch is lifted to `BleEngine.connectionPriorityFor`, @VisibleForTesting,
and `link_priority_policy_test.dart` asserts it arm by arm, plus that no two
arms collapse onto one radio priority. The structural test now pins the whole
chain instead of its ends — the request argument must be literally
`connectionPriorityFor(want)`, and that `want` must be assigned from
`desiredLinkPriority` — and adds a check that the mapper stays
@VisibleForTesting, since without it a refactor can make the arm-by-arm test
vanish silently.
2. `_codeLines` STRIPPED `//` BEFORE STRING LITERALS, SO IT FAILED OPEN.
`line.indexOf('//')` truncated at the `//` inside a string, discarding
everything after it on that line. A real bypass call placed after a URL
literal was invisible and the guard reported one call site, all green. It is
live in the tree today: lib/ui/kit/route_map.dart:59 loses five brace pairs
to this, and survives only because they happen to balance. It also missed
`/* */` entirely (87 such lines in lib/, and a block comment naming the API
produced two spurious failures), and its `'''` patterns ran inside a per-line
`.map`, where `\n` can never match — dead code against the ~80 multi-line
strings in lib/.
Replaced with a stateful scanner in test/support/dart_source.dart that also
tracks `${...}` interpolation, so `'${m['k']}'` does not end the string early.
Line count and column widths are preserved, so reported positions stay true.
`no_debug_only_apis_test.dart` now uses it too rather than keeping a second,
differently-wrong copy. 12 regression cases in dart_source_test.dart, one per
way the old helper got it wrong.
Verified by mutation — 8 cases, each on a clean tree:
all three mapping arms to .high fails (was: passed)
bypass call behind a '//' in string fails (was: passed)
literal passed to the request fails
original literal request at connect fails
second desiredLinkPriority decl fails
second connectionPriorityFor decl fails
mapper loses @VisibleForTesting fails
block comment naming the API passes (was: 2 false failures)
flutter analyze lib test clean. Full suite +1501, 0 failures.
Production change is the extraction only; no behaviour change, no
kAlgoVersion bump.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What this adds
link_priority_policy_test.dartpins the stepping rule. Nothing pins that the engine still goes through it.Issue #200 was not a wrong policy — it was no policy at all: a literal
sat in the connect path, ran once, and nothing ever stepped it back down. A future refactor of that path can re-add exactly that line, and every behavioural test in the repo stays green, because
desiredLinkPrioritystill returns the right answer — to a caller that no longer exists.There is no BLE fake here to assert against, so this greps, in the same spirit as
no_debug_only_apis_test.dart(and for the same reason: the failure is invisible to any test that runs the code).The four assertions
requestConnectionPrioritycall inlib/, and it is in the engine._applyLinkPriorityand takes its target fromdesiredLinkPriority— so a request cannot bypass the policy viaConnectionPriority.values[...]or any other selector.ConnectionPriority.<value>literal lives inside that method's mapping switch.desiredLinkPrioritydeclaration, insync/sync_policy.dart.(4) is load-bearing because (3) is scoped per-file: a second copy of the policy elsewhere in
lib/would carry its own literals and quietly satisfy (3) while the engine stopped being the single decision point.Comments and string literals are stripped before matching. Both matter — this file names the offending API in prose, and the engine's own failure path logs
requestConnectionPriority(...) failed, so a naive text count reports two calls where there is one. Method bodies are located by brace depth rather than indentation, so reformatting won't silently vacate the check.Verification
Mutation-tested rather than assumed:
desiredLinkPrioritydeclaration in anotherlib/fileOn the CI-pinned Flutter 3.41.6:
flutter analyze lib testclean; full suite+1484 ~2, 0 failures (baremainis+1480 ~2).Test-only change. No production code, no
kAlgoVersionbump.One unrelated observation while running the suite:
workout_reliability_test.dart— "a queued job stays parked for the session, then runs on release" failed once under full-suite concurrency (Expected: <1> Actual: <0>) and passed 3/3 in isolation and in two subsequent full runs. Looks like a timing flake, not something this touches. Flagging in case it is already known.Summary by CodeRabbit
Improvements
Tests