Skip to content

Bundle Manrope, fix the offload cap, and prune superseded rows - #168

Merged
abdulsaheel merged 5 commits into
mainfrom
feat/optimizations
Jul 29, 2026
Merged

Bundle Manrope, fix the offload cap, and prune superseded rows#168
abdulsaheel merged 5 commits into
mainfrom
feat/optimizations

Conversation

@abdulsaheel

@abdulsaheel abdulsaheel commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

User description

Four independent changes.

Manrope is bundled. google_fonts fetched it from fonts.gstatic.com on
first launch — before the consent screen, and contrary to PRIVACY.md, which
says the only thing sent automatically is Firebase. It is now bundled the way
Barlow Condensed already was, and the dependency is gone. Verified in the
compiled bundle: FontManifest lists Manrope with all 5 weights. A test fails
if the import, the dependency, or a font asset comes back.

PRIVACY.md names the tile provider. It said routes are never sent
anywhere, without mentioning that drawing a map downloads tiles from CARTO,
which tells CARTO which area is on screen. Also corrects the pubspec comment
that still credited tile.openstreetmap.org.

The auto-continue cap no longer stops a productive offload. It counted
every round, so one background wake drained about six rounds regardless of
backlog. It now counts only consecutive rounds that banked nothing; a round
that persisted records and advanced the trim token resets it. A 10-minute
ceiling bounds the run, which is the limit that actually applies on a
background wake.

Storage. idx_decoded_rr_ts(rr_ts_ms) was a strict prefix of the
(rr_ts_ms, beat_index) unique index, so it served no query the wider index
could not while costing a second b-tree write per beat. A test asserts the
planner still uses an index, with no temp b-tree. sleep_session_candidates
and wake_day_features are keyed (day_id, algo_version), so every
kAlgoVersion bump added a generation and nothing removed the old one; two
generations are now kept, so a rollback to the previous build still finds rows.

CI guards the sibling pins. A local flutter pub get with
pubspec_overrides.yaml present rewrites the tracked lock to
path: ../analytics and drops the resolved-ref. It happened while preparing
this branch — a flutter build re-triggered it after the lock was cleaned —
so the guard is here because it was needed. It fails on a path source, and on
lock/pubspec ref drift, which is what let a release cite an analytics change
its pinned SHA did not contain.

No kAlgoVersion bump: nothing here changes an analytics output.
Tests: 1051 pass. flutter analyze clean.


PR Type

Bug fix, Enhancement, Tests


Description

  • Bundle Manrope font locally, removing google_fonts runtime HTTP fetch before consent screen

  • Fix auto-continue offload cap: only unproductive rounds spend the budget; add 10-minute wall-clock ceiling

  • Drop redundant idx_decoded_rr_ts index; prune superseded per-day intermediate table generations

  • Add CI guard to fail when pubspec.lock resolves sibling packages from local paths instead of pinned SHAs


Diagram Walkthrough

flowchart LR
  A["google_fonts runtime fetch\n(fonts.gstatic.com)"]
  B["Bundled Manrope\n(assets/fonts/Manrope/)"]
  A -- "replaced by" --> B

  C["Auto-continue cap\n(counts every round)"]
  D["Auto-continue cap\n(counts only unproductive rounds\n+ 10-min wall-clock ceiling)"]
  C -- "fixed to" --> D

  E["idx_decoded_rr_ts\n(redundant prefix index)"]
  F["Dropped; unique index\n(rr_ts_ms, beat_index) serves all scans"]
  E -- "removed" --> F

  G["sleep_session_candidates /\nwake_day_features\n(all algo_version generations kept)"]
  H["pruneSupersededIntermediates()\nkeeps 2 newest generations"]
  G -- "pruned by" --> H

  I["pubspec.lock with\npath: ../analytics"]
  J["CI guard fails on\npath source or SHA drift"]
  I -- "caught by" --> J
Loading

File Walkthrough

Relevant files
Bug fix
3 files
theme.dart
Replace all GoogleFonts.manrope calls with bundled local font
+44/-21 
ble_engine.dart
Track wall-clock start; reset unproductive count on productive round
+18/-4   
sync_policy.dart
Add time ceiling; rename count to unproductive-only semantics
+22/-6   
Dependencies
1 files
pubspec.yaml
Remove google_fonts dep; declare bundled Manrope font assets
+31/-14 
Enhancement
2 files
db.dart
Drop redundant rr index; add pruneSupersededIntermediates method
+43/-3   
derivation_engine.dart
Call pruneSupersededIntermediates during maintenance prune pass
+7/-0     
Tests
3 files
db_storage_hygiene_test.dart
Tests for dropped index and intermediate generation pruning
+89/-0   
no_runtime_font_fetch_test.dart
Tests asserting no google_fonts import and all families bundled
+43/-0   
sync_policy_test.dart
Add tests for time ceiling and productive-round cap bypass
+16/-2   
Configuration changes
1 files
test.yml
Add CI step to guard sibling lock pins against local path drift
+37/-0   
Documentation
2 files
PRIVACY.md
Disclose CARTO map tile fetches and IP exposure                   
+7/-0     
OFL.txt
Add SIL Open Font License for bundled Manrope family         
+93/-0   

Summary by CodeRabbit

  • Privacy

    • Clarified that displaying route maps retrieves map tiles from CARTO and shares only the user’s IP address; routes and health data are not uploaded.
  • Improvements

    • Improved automatic data backfill continuation with limits based on unproductive rounds and elapsed time.
    • Reduced storage buildup by cleaning up superseded intermediate data.
    • Improved database query indexing for decoded data.
  • Fonts

    • Bundled Manrope and Barlow Condensed fonts locally, preventing runtime font downloads.
  • Quality

    • Added automated checks for database storage hygiene, bundled fonts, synchronization behavior, and dependency consistency.

google_fonts fetched Manrope from fonts.gstatic.com on first launch, before
the consent screen and contrary to PRIVACY.md. The family is now bundled the
same way Barlow Condensed already was, and the dependency is gone. A test
fails if either regresses.

PRIVACY.md said routes are never sent anywhere but did not mention that
drawing a map downloads tiles from CARTO, which reveals the area on screen.
Documented, along with the stale pubspec comment that still credited
tile.openstreetmap.org.
The cap counted every auto-continue round, so one background wake drained
about six rounds no matter how far behind the strap was. It now counts only
consecutive rounds that banked nothing; a round that persisted records and
advanced the trim token resets it. A 10-minute ceiling bounds the run, which
is the limit that actually applies on a background wake.
idx_decoded_rr_ts(rr_ts_ms) was a strict prefix of the (rr_ts_ms, beat_index)
unique index, so it served no query the wider index could not, and cost a
second b-tree write per beat on the hottest path. A test asserts the planner
still uses an index, with no temp b-tree, for the rr_ts_ms range scan.

sleep_session_candidates and wake_day_features are keyed
(day_id, algo_version), so every kAlgoVersion bump added a generation and
nothing removed the old one. Two generations are kept, so a rollback to the
previous build still finds rows to read.
A local `flutter pub get` with pubspec_overrides.yaml present rewrites the
tracked lock to `path: ../analytics` and drops the resolved-ref, so the lock
no longer records which sibling commit was tested. Any local build or test
run triggers it, and it has reached main.

The guard runs before `pub get` and fails on a path source, or when the lock's
resolved-ref does not match the ref pinned in pubspec.yaml — the drift that
let a release cite an analytics change its pinned SHA did not contain.
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@abdulsaheel, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a90fc3a9-09a4-4e01-bb61-600d1de1d168

📥 Commits

Reviewing files that changed from the base of the PR and between 7cd5d11 and 4cf736e.

📒 Files selected for processing (6)
  • .github/workflows/test.yml
  • PRIVACY.md
  • lib/ble/ble_engine.dart
  • lib/sync/sync_policy.dart
  • pubspec.yaml
  • test/sync_policy_test.dart

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 4cf736e)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Migration

DROP INDEX IF EXISTS idx_decoded_rr_ts is placed inside the schema creator (_createDecodedRr or equivalent), which runs for new databases. Existing databases that already have idx_decoded_rr_ts will never execute this drop because the creator is not re-run on upgrade — only onUpgrade and _repairOpenSchema are. Per AGENTS.md §3.11, _repairOpenSchema re-runs creators so same-version merged builds self-heal, but a user upgrading from a prior version needs an explicit if (oldV < N) ladder entry to drop the redundant index. Without it, existing installs keep the extra b-tree indefinitely and the test that asserts the index is gone will pass only on a fresh database.

// idx_decoded_rr_ts(rr_ts_ms) was a strict prefix of the unique index
// above, so SQLite could already serve every rr_ts_ms lookup and ordering
// from it. The narrower index only added a second b-tree to maintain on
// the hottest write path in the app.
await db.execute('DROP INDEX IF EXISTS idx_decoded_rr_ts');
SQL Injection Risk

pruneSupersededIntermediates interpolates table directly into a rawQuery string and a db.delete call. Although table is sourced from a hardcoded const list in this method, the method is static and public, so a future caller could pass an arbitrary string. More concretely, the rawQuery with the interpolated table name is not parameterizable in SQLite, but the pattern establishes a precedent that is easy to copy incorrectly. The immediate risk is low given the const list, but the delete call's where clause is correctly parameterized while the table name is not, which is inconsistent and worth noting.

for (final table in const [
  'sleep_session_candidates',
  'wake_day_features',
]) {
  final versions = (await db.rawQuery(
    'SELECT DISTINCT algo_version FROM $table ORDER BY algo_version DESC',
  ))
      .map((r) => r['algo_version'] as int)
      .toList();
  if (versions.length <= keepVersions) continue;
  final cutoff = versions[keepVersions - 1];
  deleted += await db.delete(
    table,
    where: 'algo_version < ?',
    whereArgs: [cutoff],
  );
}
Unproductive Count Off-by-One

In AutoContinueRun.observe, a productive round resets _unproductive to 0 before BackfillContinuation.shouldAutoContinue is consulted. However, continued increments _unproductive only when !productive. This means a productive round that calls observe(productive: true) then continued(productive: true, now: ...) correctly keeps the streak at 0. But if observe is called with productive: false and then continued is called with productive: true (which can happen if the productive variable is computed once and reused), the streak is not reset by observe (since productive is false) but is also not incremented by continued (since productive is true). The productive flag passed to observe and continued must be the same value for the logic to be coherent; the call site in ble_engine.dart does use the same productive variable for both, so this is safe today, but the API allows inconsistent calls silently.

void observe({required bool productive}) {
  if (productive) _unproductive = 0;
}

/// Call when the gate said continue.
void continued({required bool productive, required double now}) {
  _startedAt ??= now;
  if (!productive) _unproductive++;
}

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 4cf736e

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Make intermediate pruning atomic across both tables

The cutoff is set to versions[keepVersions - 1], which is the oldest version in the
keep-set, and then rows with algo_version < cutoff are deleted. This retains the
cutoff version itself, so with keepVersions = 2 and versions [50, 49, 48], the
cutoff is 49 and only 48 is deleted — correct. However, the condition should be
algo_version < cutoff to exclude the cutoff version from deletion, which is what the
code does. The real bug is that the test expects deleted == 4 (two days × v48, in
both tables), which matches this logic. But if keepVersions = 2 and versions are
[50, 49, 48], versions[keepVersions - 1] is versions[1] = 49, so rows with
algo_version < 49 (i.e., v48) are deleted — 2 days × 2 tables = 4 rows. This is
correct. No bug here on the pruning boundary itself. However, the pruning runs
outside a transaction across two tables, so a crash between the two db.delete calls
leaves the tables in an inconsistent state (one pruned, one not). Wrap both deletes
in a single transaction to make the prune atomic.

lib/data/db.dart [4564-4569]

-final cutoff = versions[keepVersions - 1];
-deleted += await db.delete(
-  table,
-  where: 'algo_version < ?',
-  whereArgs: [cutoff],
-);
+var deleted = 0;
+await db.transaction((txn) async {
+  for (final table in const [
+    'sleep_session_candidates',
+    'wake_day_features',
+  ]) {
+    final versions = (await txn.rawQuery(
+      'SELECT DISTINCT algo_version FROM $table ORDER BY algo_version DESC',
+    ))
+        .map((r) => r['algo_version'] as int)
+        .toList();
+    if (versions.length <= keepVersions) continue;
+    final cutoff = versions[keepVersions - 1];
+    deleted += await txn.delete(
+      table,
+      where: 'algo_version < ?',
+      whereArgs: [cutoff],
+    );
+  }
+});
+return deleted;
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that pruning two tables in separate db.delete calls outside a transaction could leave them in an inconsistent state on crash. However, this is a recomputable intermediate table (not a durable ledger), so inconsistency is low-risk and self-healing on next derivation. The improvement is valid but of moderate importance.

Low
Possible issue
Fix off-by-one in unproductive streak before gate consult

observe only resets _unproductive on a productive round but never increments it on
an unproductive one. The increment happens inside continued, which is called only
when the gate says to continue. This means a round that is unproductive but where
the gate says stop never increments the streak — which is fine for the stop case.
However, a round that is unproductive and the gate says continue increments in
continued, so the streak is correct for the continue path. The issue is that observe
is called before the gate, and continued is called after — so the streak seen by the
gate during shouldAutoContinue is always one behind: the current round's
unproductive contribution is not yet counted when the gate is consulted. This means
the cap is effectively maxAutoContinues + 1 unproductive rounds, not
maxAutoContinues. Move the unproductive increment into observe so the gate sees the
up-to-date streak.

lib/sync/sync_policy.dart [257-259]

 void observe({required bool productive}) {
-  if (productive) _unproductive = 0;
+  if (productive) {
+    _unproductive = 0;
+  } else {
+    _unproductive++;
+  }
 }
 
+/// Call when the gate said continue.
+void continued({required bool productive, required double now}) {
+  _startedAt ??= now;
+}
+
Suggestion importance[1-10]: 3

__

Why: The suggestion claims the gate sees a streak one behind, but the PR's design is intentional: observe clears the streak for productive rounds before the gate, and continued increments for unproductive rounds after the gate. Moving the increment into observe would change the semantics so an unproductive round that causes the gate to stop would still increment the streak, which contradicts the PR's explicit design comment about ordering. The suggested change conflicts with the PR's intended behavior.

Low

Previous suggestions

Suggestions up to commit 4cf736e
CategorySuggestion                                                                                                                                    Impact
General
Capture wall clock once to avoid elapsed drift

_wallSecs() is called twice — once for elapsed and once inside continued — with no
guarantee the two calls return the same value. If the clock advances between the two
calls, _startedAt is set to a slightly later timestamp than the one used to compute
elapsed, causing the elapsed measurement to drift. Capture _wallSecs() once before
the gate and reuse it for both calls.

lib/ble/ble_engine.dart [2554-2566]

+final now = _wallSecs();
 _autoContinue.observe(productive: productive);
 final cont = BackfillContinuation.shouldAutoContinue(
-  ...
+  stillConnected: _session?.connected == true,
+  strapNewestTs: _sessionNewestUnix,
+  ourFrontierTs: _recordGate.frontierTs,
+  rowsPersistedThisSession: d.recordsThisOffload,
+  lastTrimAdvanced: d.lastTrimAdvanced,
   consecutiveUnproductiveCount: _autoContinue.unproductiveStreak,
-  elapsedSeconds: _autoContinue.elapsed(_wallSecs()),
+  elapsedSeconds: _autoContinue.elapsed(now),
 );
 d.resetOffloadCounters();
 if (cont) {
-  _autoContinue.continued(productive: productive, now: _wallSecs());
+  _autoContinue.continued(productive: productive, now: now);
Suggestion importance[1-10]: 6

__

Why: This is a valid and accurate suggestion — calling _wallSecs() twice can cause a tiny drift between the elapsed check and the _startedAt assignment in continued. Capturing now once is a clean improvement. The improved_code correctly reflects the change and the existing code context.

Low
Make intermediate prune atomic across tables

The cutoff is versions[keepVersions - 1], which is the oldest version to keep. Using
< cutoff is correct only if you want to keep exactly keepVersions entries, but with
keepVersions = 2 and versions [50, 49, 48], cutoff = versions[1] = 49, so
algo_version < 49 deletes only v48 — that is correct. However, the delete runs
outside a transaction, so if the loop iterates over multiple tables and fails
mid-way, one table is pruned and the other is not, leaving the DB in an inconsistent
state. Wrap the loop body in a transaction to make the prune atomic across both
tables.

lib/data/db.dart [4564-4569]

 final cutoff = versions[keepVersions - 1];
-deleted += await db.delete(
-  table,
-  where: 'algo_version < ?',
-  whereArgs: [cutoff],
-);
+deleted += await db.transaction((txn) async {
+  return await txn.delete(
+    table,
+    where: 'algo_version < ?',
+    whereArgs: [cutoff],
+  );
+});
Suggestion importance[1-10]: 4

__

Why: The suggestion is technically valid — wrapping each table's delete in a transaction adds atomicity. However, the tables are independent intermediates and partial pruning is not a correctness issue (the next prune call will clean up the rest). The improvement is marginal and the improved_code wraps only one table's delete rather than both, which doesn't fully solve the stated problem.

Low
Possible issue
Increment unproductive streak in observe, not continued

observe resets the unproductive streak on a productive round but never increments it
on an unproductive one. The increment happens inside continued, which is only called
when the gate says "continue". If the gate refuses (e.g. time ceiling hit) the
streak is never incremented, which is fine — but if a round is unproductive and the
gate does say continue, the streak must be incremented before the next observe call.
Currently an unproductive round that is not followed by continued (e.g. the gate
refuses on the time ceiling) leaves the streak unchanged, which is correct. However,
the productive-reset path in observe is the only place the streak can go down, and
continued is the only place it can go up — so a productive round that is observed
but then the gate refuses will reset the streak to 0 even though no continuation
happened. This means _startedAt stays set (run still active) but the streak is
reset, which could allow more unproductive rounds than intended after a time-ceiling
stop. Call end() inside observe when productive and the run is active but the gate
will not be consulted, or alternatively only reset the streak in observe when the
run is actually active.

lib/sync/sync_policy.dart [257-259]

 void observe({required bool productive}) {
-  if (productive) _unproductive = 0;
+  if (productive) {
+    _unproductive = 0;
+  } else if (_startedAt != null) {
+    // Only count unproductive rounds while a run is active; the increment
+    // happens here so the gate sees the updated streak before deciding.
+    _unproductive++;
+  }
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion identifies a real design tension: observe resets the streak but continued increments it, so a productive round that hits the time ceiling resets the streak while _startedAt remains set. However, the PR's design is intentional — observe is called before the gate and continued only when the gate says yes, so the streak only increments on actual continuations. The suggested change moves the increment into observe, which would double-count with continued and break the existing logic. The improved_code contradicts the PR's carefully documented ordering invariant.

Low
Suggestions up to commit 7cd5d11
CategorySuggestion                                                                                                                                    Impact
Possible issue
Reset timer latch on backfill failure path

_autoContinueStartedAt is only reset in the else branch (when cont is false). If
_triggerBackfill throws an exception, _autoContinueStartedAt is never cleared, so
the next connection inherits a stale start time and the time ceiling fires
immediately, preventing any auto-continue. Per AGENTS.md §4.3, flags must be cleared
on the failure path. The reset should also happen in the connection-reset block, but
at minimum _autoContinueStartedAt needs a try/finally guard around _triggerBackfill
or must be cleared in the catch/finally of the surrounding drain.

lib/ble/ble_engine.dart [2567-2575]

     if (cont) {
       _autoContinueStartedAt ??= _wallSecs();
       // Only unproductive rounds spend the budget — see shouldAutoContinue.
       _autoContinueCount = productive ? 0 : _autoContinueCount + 1;
       _log('[SYNC] auto-continue — more backlog remains '
           '(unproductive streak $_autoContinueCount).');
-      await _triggerBackfill(BackfillTrigger.autoContinue);
+      try {
+        await _triggerBackfill(BackfillTrigger.autoContinue);
+      } catch (_) {
+        _autoContinueStartedAt = null;
+        _autoContinueCount = 0;
+        rethrow;
+      }
     } else {
       _autoContinueStartedAt = null;
-  </code>
Suggestion importance[1-10]: 5

__

Why: The concern about _autoContinueStartedAt not being cleared if _triggerBackfill throws is valid. However, the diff already shows _autoContinueStartedAt = null in the connection-reset block (line 1151), which would clear it on the next connection, limiting the impact to only the current connection's remaining auto-continue attempts.

Low
General
Prune both intermediate tables atomically

versions[keepVersions - 1] is the oldest version to keep, so algo_version < cutoff
correctly excludes it and deletes everything older. However, the intent is to delete
rows strictly older than the keepVersions-th newest version, meaning rows with
algo_version < versions[keepVersions - 1] are deleted while versions[keepVersions -
1] itself is retained. With keepVersions = 2 and versions [50, 49, 48], cutoff = 49
and algo_version < 49 deletes only v48 — correct. But if there are exactly
keepVersions distinct versions the continue guard fires, so this is fine. The real
issue is that the two tables are pruned in separate transactions: if the process is
killed between them, one table is pruned and the other is not, leaving the
intermediates inconsistent across tables for the same day. Wrap both deletes in a
single transaction so they are atomic.

lib/data/db.dart [4558-4570]

     final versions = (await db.rawQuery(
       'SELECT DISTINCT algo_version FROM $table ORDER BY algo_version DESC',
     ))
         .map((r) => r['algo_version'] as int)
         .toList();
     if (versions.length <= keepVersions) continue;
     final cutoff = versions[keepVersions - 1];
-    deleted += await db.delete(
-      table,
-      where: 'algo_version < ?',
-      whereArgs: [cutoff],
-    );
+    // Collect the delete work; execute atomically below.
+    deletePlan[table] = cutoff;
+  }
+  // Execute all deletes in one transaction so both tables are pruned together.
+  await db.transaction((txn) async {
+    for (final entry in deletePlan.entries) {
+      deleted += await txn.delete(
+        entry.key,
+        where: 'algo_version < ?',
+        whereArgs: [entry.value],
+      );
+    }
+  });
Suggestion importance[1-10]: 4

__

Why: The atomicity concern is valid — if the process is killed between the two table deletes, they end up inconsistent. However, since these are recomputable intermediates (not durable ledger data), the inconsistency is benign and will self-correct on the next derivation run. The improved_code also introduces an undeclared deletePlan variable, making it incomplete as shown.

Low

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
test/no_runtime_font_fetch_test.dart (1)

28-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the font families the theme actually uses.

The hardcoded list still passes if a new fontFamily is added to the type scale without a matching flutter.fonts entry. Derive the referenced families from lib/theme (or a shared font-family constant) and assert each is bundled. As per coding guidelines, “Behavior changes, especially regressions … 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/no_runtime_font_fetch_test.dart` around lines 28 - 34, Update the test
“every family the type scale uses is bundled” to derive the expected font
families from the theme definitions in lib/theme or a shared font-family
constant, rather than hardcoding Manrope and Barlow Condensed. Compare every
referenced family against pubspec.yaml’s flutter.fonts entries and retain the
regression assertion that all theme-used families are bundled.

Source: Coding guidelines

🤖 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 @.github/workflows/test.yml:
- Around line 51-57: Update the workflow guard preceding the flutter pub get
step to explicitly fail when pubspec_overrides.yaml exists, before dependency
installation runs. Preserve the existing committed pubspec.lock sibling-commit
pin validation, and ensure either condition blocks the job rather than allowing
Flutter to use local path overrides.

In `@lib/ble/ble_engine.dart`:
- Around line 2553-2575: Reset the productive streak state before calling
BackfillContinuation.shouldAutoContinue in lib/ble/ble_engine.dart lines
2553-2575, so productive offloads are evaluated with a cleared
consecutive-unproductive count; increment only when an allowed continuation is
unproductive, and clear both auto-continuation run-state fields when the run
ends. Add a stateful regression in test/sync_policy_test.dart lines 207-214
covering a capped unproductive streak followed by a productive offload that must
continue.

In `@PRIVACY.md`:
- Around line 72-74: Update the RouteMapView privacy wording in PRIVACY.md to
replace “only while a map is on screen” with language stating that it occurs
while the map is active or open, while preserving the surrounding disclosures.

In `@pubspec.yaml`:
- Around line 90-91: Update the tile-provider note in the pubspec configuration
comment to say v1 uses live CARTO tiles instead of live OSM tiles, keeping it
consistent with the CARTO provider documented nearby.

---

Nitpick comments:
In `@test/no_runtime_font_fetch_test.dart`:
- Around line 28-34: Update the test “every family the type scale uses is
bundled” to derive the expected font families from the theme definitions in
lib/theme or a shared font-family constant, rather than hardcoding Manrope and
Barlow Condensed. Compare every referenced family against pubspec.yaml’s
flutter.fonts entries and retain the regression assertion that all theme-used
families are bundled.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0723da88-1d02-45f6-b267-979c09bcc25d

📥 Commits

Reviewing files that changed from the base of the PR and between d9474d8 and 7cd5d11.

⛔ Files ignored due to path filters (6)
  • assets/fonts/Manrope/Manrope-400.ttf is excluded by !**/*.ttf
  • assets/fonts/Manrope/Manrope-500.ttf is excluded by !**/*.ttf
  • assets/fonts/Manrope/Manrope-600.ttf is excluded by !**/*.ttf
  • assets/fonts/Manrope/Manrope-700.ttf is excluded by !**/*.ttf
  • assets/fonts/Manrope/Manrope-800.ttf is excluded by !**/*.ttf
  • pubspec.lock is excluded by !**/*.lock
📒 Files selected for processing (12)
  • .github/workflows/test.yml
  • PRIVACY.md
  • assets/fonts/Manrope/OFL.txt
  • lib/ble/ble_engine.dart
  • lib/compute/derivation_engine.dart
  • lib/data/db.dart
  • lib/sync/sync_policy.dart
  • lib/theme/theme.dart
  • pubspec.yaml
  • test/db_storage_hygiene_test.dart
  • test/no_runtime_font_fetch_test.dart
  • test/sync_policy_test.dart

Comment thread .github/workflows/test.yml
Comment thread lib/ble/ble_engine.dart Outdated
Comment thread PRIVACY.md Outdated
Comment thread pubspec.yaml Outdated
The streak was reset inside the continue branch, so once it reached the cap
the gate saw the stale count and refused the very round that had just banked
records. Run state moved into a pure AutoContinueRun so the ordering is
testable; a capped streak followed by a productive round now continues, and
ending a run restores the full budget.

The CI pin guard also fails when pubspec_overrides.yaml is present, since
pub get would pick it up and stop testing the pinned SHAs.

PRIVACY.md said tiles are fetched only while a map is on screen; RouteMapView
is also mounted to render the share image. Corrected, along with a pubspec
comment that still said OSM tiles.
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 4cf736e

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 4cf736e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant