delete the things that were only ever measuring themselves - #263
delete the things that were only ever measuring themselves#263abdulsaheel wants to merge 7 commits into
Conversation
overrides rewrote day_result and nothing told the screens, so an edit only showed up after a restart. also dropped reanalyzeDays and the _bumpInsightsRevision alias — no callers.
importFromDbFile already sniffs and inflates, and unlike gzip.decoder it checks the trailer — the hand-rolled block would restore a truncated backup short and call it a success.
the notification-relay list decodes every app icon at whatever the launcher shipped — up to 512 px — to paint a 32 pt row. cacheWidth only: these are third-party icons and not all of them are square, so pinning both dimensions would squash them. same reasoning as _IconChoice in settings.dart.
BurstStats.onHistoricalData took the record hex and ran the whole thing back through hexToBytes just to reach inner[1] — the revision — which the ingest path had already read as recType two hundred lines earlier. one throwaway buffer per record, on every record of every offload: roughly 6 MB of garbage for a full gen4 backfill. pass the revision, drop the Sample the function never looked at.
restLeft was a field behind setState, so each of the ninety ticks between sets rebuilt _LiveStrengthState and with it a fresh LiveShell — header, transport, footer, body — in the file that added LiveTick to stop exactly this. a session with twenty sets does it well over a thousand times. it's a ValueNotifier now, with only the rest ring listening. setState stays at the zero crossing, where the footer and the body branch genuinely change. the yoga hold timer had the same wrapper for nothing — that body already rebuilds at 1 Hz through the shell clock, so the setState just bought a second rebuild of the same subtree. deleted. test pumps a set, ticks a second, and checks the LiveShell instance is the same one while the countdown moved. fails on the old code.
13 sites each ran a full-table COUNT(*) over every table just to feed one 'raw=' in the session-start log. counts() stays, two tests use it.
spo2 is refused permanently, so the logger was printing compile-time constants — 14 passes over the sleep arrays on the calling isolate, unconditionally, for every derived day. sleepSpo2Red/Ir went with it: deriveDayBundle never read them, they were only serialized and copied across the isolate boundary to be ignored. substrate keeps its raw channels, and the refusal metric is untouched.
|
@coderabbitai review stacked on #261 so it targets that branch rather than main — review it anyway please. everything here is meant to be output-identical, so anything that looks like it changes a number is a real finding. |
|
Warning Review limit reached
Next review available in: 51 minutes Limit details: You’ve used all 2 included reviews currently available. 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?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (7)
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 |
|
🧠 Learnings used
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
1 participant
User description
Stacked on #261. Nothing here changes a derived number — there is no
kAlgoVersionbump, and if one becomes necessary the change is wrong. This is "faster and shorter", nothing else.deletions
~90 lines of SpO2 diagnostics ran on the calling isolate, per derived day, in release. SpO2 is refused permanently, not parked — the pipeline says so in its own words and
odiis aconst Metric.absent, so most of what the logger printed was a compile-time constant. It did ~14 full passes over ~28,800-sample arrays plus twoHashSets and a growableList<double>, for every day including backfill. Deleting it orphanedDayBundleInput.sleepSpo2Red/sleepSpo2Ir, whichderiveDayBundlenever read — they were serialized and copied across the isolate boundary purely to be ignored. 8 call sites fixed.The refusal metric, its
tier,inputs_usedand note are untouched. This deletes a logger, never an abstention path.dbCountscost 13 full-tableCOUNT(*)pairs and fed one log line. With the writes gone the field would have been written never and read once, printing a permanentraw=0— a field that exists only to log a lie — so it went too.LocalDb.counts()stays; two tests use it. Every enclosing block survived: five of those sites sit insideif (…) { notifyListeners(); }-shaped blocks, and deleting the block rather than the statement would have re-introduced the staleness bug #261 just fixed.importEdgeBackuphand-inflated gzip thatimportFromDbFilealready inflates. The duplicate was also the unsafe one: Dart'sgzip.decoderreturns partial output on a truncated.db.gzwithout raising, so a half-synced backup restored short and reported success — on the one path where the original is already gone. The downstream inflate reads the CRC32/ISIZE trailer off the file.the one that is a bug wearing a perf costume
_reanalyzeForOverridenever bumpedinsightsRevision. Every sleep override, nap edit and phone-steps toggle rewritesday_resultand noRevisionReloadscreen noticed. One line.hot paths
onHistoricalDatare-parsed its own hex back into bytes to read one byte the caller already had. It takesrevisionnow; the unusedSample?went with it. Semantics are exact — the caller already computesrecType = inner.length > 1 ? inner[1] : -1, so the newrevision < 0guard is the oldinner.length < 2guard. ~6 MB of garbage per gen4 offload (not the 18 MB originally claimed; v20/v21/v26 never reach this path).The rest countdown rebuilt the entire workout shell once a second, in the file that introduced
LiveTickto stop exactly this — ~1,200 full-subtree rebuilds in a strength session.restLeftis aValueNotifier; only the body branch is wrapped, the footer is untouched, andsetStatestays at the zero crossing where the shape genuinely changes.The test asserts the
LiveShellinstance is identical across a tick, not just that the text moved — a text-only assertion passes on the broken version too. Verified it fails pre-change.App icons decoded at source resolution.
cacheWidthonly; the source is a third-party launcher icon and need not be square.DayBundleInput.fromJsonunboxed every array the substrate deliberately packed.dblsreturns aFloat64List— copied, never aliased, so the synchronous test path cannot hand two repos a shared mutable array. Theints/strsfast paths were skipped on purpose: Smis and Strings box nothing per element, and astrsfast path would hand out aconst []where a growable list goes out today.Worth a reviewer's eye:
Float64Listis fixed-length where.toList()was growable. Nothing mutates these and all sixderiveDayBundletest callers exercise the typed lists throughtoJson/fromJson, so if something downstream ever starts growing a caller-owned list it throws rather than corrupts — loud, not silent.housekeeping
One commit (
drop the spo2 diagnostic logger…) also contains theFloat64Listchange; the message only describes the first. The no-amend rule caught it after the fact. Diff is correct, message is incomplete.DerivationEngine.runDaysnow has zero production callers — deletingreanalyzeDaystook the last one, and onlyderive_result_protection_test.dartcalls it. Left alone deliberately; that is a decision, not a reflex delete.2795 tests pass, 422 golden skips, analyze clean.
PR Type
Bug fix, Enhancement
Description
Remove
sleepSpo2Red/sleepSpo2Irfields and ~90-line SpO2 diagnostic logger that ran per-day on the calling isolate, burning CPU on permanently-refused dataEliminate
dbCountsfield and all 13LocalDb.counts()calls; fix sleep-override/nap re-derive not refreshing screens by callingbumpInsights()insteadStop double-inflating gzip backups in
importEdgeBackup;importFromDbFilealready handles it with trailer validationConvert strength-workout rest countdown from
setStatefield toValueNotifier, preventing fullLiveShellrebuilds on every tick; fix yoga hold timer similarlyDiagram Walkthrough
File Walkthrough
3 files
Pass revision int to BurstStats, drop hex re-parseDrop sleepSpo2Red/Ir fields; optimize dbls() with Float64ListDecode notification app icons at display size only3 files
Remove SpO2 diagnostic logger and spo2Red/spo2Ir inputsRemove dbCounts, fix override re-derive screen refresh, dropdouble-gzipRest countdown to ValueNotifier; fix yoga hold timer rebuild7 files
Update onHistoricalRecord calls to pass revision intRemove sleepSpo2Red/Ir from DayBundleInput test fixtureRemove spo2 arrays from pipeline test fixturesRemove sleepSpo2Red/Ir from HR ceiling zones test fixtureRemove sleepSpo2Red/Ir from resting HR test fixtureRemove sleepSpo2Red/Ir from strain/RHR test fixtureAdd test: rest countdown does not rebuild LiveShell1 files
Remove sleepSpo2Red/Ir from derive probe tool