Repository navigation
Conversation
recentDayResults applied its LIMIT after joining and sorting every day's served row, payload_json (~100 KB) included, so the 30-day read behind refreshComputeFreshness went from ~60 ms at 30 days of history to 250-600 ms at 730. That runs on every derive and on every Home load while a night is still building: the growth soak (OpenStrap#609) measured getToday at 22 -> 303 ms. Pick the newest N served versions from the covering index first, then fetch only those rows. The served-version subquery is factored out of _servedDayJoin, which expands to the same SQL as before, so the ceiling stays in one place. recentDayResultsMeta takes the same join. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideThis PR improves Home-path database performance by selecting the newest N served day IDs through the covering index before joining large result payloads, preserving version-ceiling semantics and query results while avoiding scans and sorts over all historical rows. A regression test verifies equivalence for full and metadata reads and checks that performance remains nearly flat as history grows. Sequence diagram for optimized recent day database readsequenceDiagram
participant HomePath
participant LocalDb
participant SQLite
participant DayResult
HomePath->>LocalDb: recentDayResults(limit)
LocalDb->>SQLite: _servedNewestDaysJoin(limit)
SQLite->>DayResult: Select MAX(algo_version) <= _servedAlgoCeiling
DayResult-->>SQLite: Newest N day_id/version pairs
SQLite->>DayResult: Join selected rows via _servedOn
DayResult-->>SQLite: N result rows with payloads
SQLite-->>LocalDb: Rows ordered by day_id DESC
LocalDb-->>HomePath: Newest N day results
Flow diagram for newest-N served day readsflowchart LR
A[recentDayResults or recentDayResultsMeta] --> B[_servedNewestDaysJoin]
B --> C[_servedVersions]
C --> D[Covering index: newest day_id values]
D --> E[ORDER BY day_id DESC LIMIT N]
E --> F[_servedOn joins selected versions]
F --> G[Fetch N result rows and payloads]
G --> H[Return newest N days]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters: 1
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main performance change: reading the newest N days without sorting all historical data. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| 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 functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 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.
Comment @coderabbitai help to get the list of available commands.
![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 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="test/db_serve_version_and_reads_test.dart" line_range="298" />
<code_context>
+
+ expect(key(await LocalDb.recentDayResults(30)), key(await oracle(30)));
+ expect(key(await LocalDb.recentDayResultsMeta(30)), key(await oracle(30)));
+ expect(long, lessThanOrEqualTo(short * 3 + 30),
+ reason: 'recentDayResults(30): $short ms at 30 days of history, '
+ '$long ms at 600');
</code_context>
<issue_to_address>
**Timing assertion flakes on slow runners**
When the test runs on a loaded or slow machine and query latency varies between the short- and long-history measurements, the test `the newest-N reads stay flat as history grows` compares wall-clock medians with `long <= short * 3 + 30` at line 298. On a loaded or slow runner, the 600-day measurement exceeds that threshold even when the query is correct, so the test fails for machine load rather than a regression.
Move the timing check to a controlled benchmark, or avoid failing the test based on wall-clock latency.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: test/db_serve_version_and_reads_test.dart:298
An absolute bound (600-day read <= 3x the 30-day read + 30 ms) can fail on a slow or loaded runner. Time the old full-history query back to back on the same 600 days instead, so load slows both sides alike: 3x faster or better (measured ~10x with the change, 1x without). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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
1 participant
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.
Why
Home gets slower the longer someone has used the app. The growth soak (#609) measured
HomeData.loadat 22 ms with 30 days of history and 303 ms with 730, nearly all of it ingetToday.The cause is
LocalDb.recentDayResults. It applied itsLIMITafter joining and sorting every day's served row,payload_json(~100 KB a day) included, so reading the last 30 days meant fetching and sorting all of history. It runs insiderefreshComputeFreshness, which happens on every derive and on everygetTodaywhile a night is still building, and in_latestBundle/crossdaycallers.Per call, measured with a real 103 KB bundle:
recentDayResults(30)refreshComputeFreshnessrecentDayResultsMeta(30)What changed
_servedDayJoininto_servedVersions._servedDayJoinexpands to exactly the same SQL as before, so every other reader and the coach views are unchanged, and the ceiling (F-31) stays in one place._servedNewestDaysJoinappliesORDER BY day_id DESC LIMIT ?inside that subquery. The newest N days are picked from the covering index first, and only those N rows are fetched.recentDayResultsandrecentDayResultsMetause it. The other readers read all of history by design, so they are untouched.Tests
db_serve_version_and_reads_test, new test "the newest-N reads stay flat as history grows":recentDayResultsandrecentDayResultsMeta.main: 14 → 132 ms, fails.The existing F-31 tests (a future version is never served, an older one still is) pass unchanged.
flutter analyzeshows no issues, and the DB / repository / coach / readiness tests pass (484).Not in this PR
sleepWindowRowshas the same shape (1 → 131 ms at 730 days) but filters on the served row before itsLIMIT, so this rewrite doesn't fit it. It isn't on the Home path.🤖 Generated with Claude Code
Summary by Sourcery
Keep recent day-result reads fast by limiting eligible days before retrieving their full records.
Enhancements:
Tests:
Summary by CodeRabbit