import a .noopbak, and stop losing the AI key on a locked relaunch - #213
Conversation
a .noopbak is a zip around noop's own sqlite database, and on iOS it is the only export noop offers — so pointing people at the raw sensor CSV left every iOS migrant with no way in. the backup carries the same 1 Hz channels the band does, so it re-derives at full fidelity instead of importing noop's scores. the key was written with the keychain's default whenUnlocked accessibility. this app is relaunched in the background constantly and often while the phone is locked, when that item cannot be read — the empty read was cached as "no key", so by the time the app was opened the key had silently gone. the shell's bottom chrome is 106pt against the 120pt of padding screens reserved for it, and the live-workout banner stacks above the pill, so the last card of every tab sat under it while a workout was running.
|
Warning Review limit reached
Next review available in: 10 minutes 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?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds BYOK keychain recovery, shared NOOP ingestion with ChangesBYOK keychain recovery
NOOP backup import
Responsive bottom spacing
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant NoopImporter
participant NoopBackupImporter
participant NoopIngest
participant LocalDb
User->>NoopImporter: select .noopbak backup
NoopImporter->>NoopBackupImporter: importDatabase(path)
NoopBackupImporter->>NoopIngest: offer paged sensor samples
NoopIngest->>LocalDb: addLiveCoverage for uncovered steps
NoopIngest-->>NoopBackupImporter: finalize derived days
NoopBackupImporter-->>NoopImporter: NoopImportResult
Possibly related PRs
Suggested labels: 🚥 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: 8
🤖 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/coach/coach_config.dart`:
- Around line 133-149: Update the API-key persistence flow in the enclosing
configuration method so _key and _keyUnreadable are changed only after the
secure _secure.write or _secure.delete operation succeeds; retain the previous
cached state when either operation throws. Define the behavior for a successful
keychain operation followed by a failed prefs.setBool(_kKeyPresent, ...) call,
and add regression tests covering failed writes, failed deletes, and
marker-write failures.
- Around line 98-105: Preserve retry eligibility in the markerless legacy-key
path: when read returns empty while expectKey is false, update the state used by
app.dart’s resume retry so the key is retried after unlock instead of marking
keyUnreadable false. Add a regression test covering a markerless legacy key
during a locked relaunch and verifying successful recovery on resume.
In `@lib/import/noop_backup_import.dart`:
- Around line 251-260: Update the timestamp-bound handling in the span
aggregation loop to clamp each MIN/MAX result independently to the plausible
Unix-seconds range instead of skipping the table when either bound is invalid.
Preserve valid portions of the table’s span, then merge the clamped bounds into
lo and hi so isolated corrupt timestamps cannot make _span return null.
In `@lib/import/noop_ingest.dart`:
- Around line 231-238: Update the gravity carry logic in the surrounding ingest
loop so fax, fay, and faz are updated independently: apply each non-null source
axis, including s.ay and s.az when s.ax is absent, while retaining the previous
value for null axes before assigning ax[i], ay[i], and az[i].
- Around line 199-205: Update the CSV ingestion flow around decideRow, offer,
and finish to track dates accepted as buffer rows but never promoted to
_curDate, then derive those pending older dates in finish before finalizeImport.
Preserve normal high-water-date processing and ensure each pending date is
passed through _deriveAndPrune so its samples are not silently discarded.
- Around line 144-160: Update finish() to process the final date when it has
buffered seconds or RR data, using the existing _secs, _rrTs, and _rrMs
collections in the guard. Ensure RR-only dates still derive imported days and
preserve the existing step-counter flushing behavior.
- Line 92: Move the canonical day-label implementation into data/day_label.dart,
exposing dayLabelOf or an epoch-seconds wrapper there, and update the ingest
code around localDateLabel to import and use that helper instead of the
compute/substrate.dart implementation.
In `@test/noop_backup_import_test.dart`:
- Around line 188-197: Update the cleanup assertions around
NoopImporter.importFile to capture the set of openstrap_noopbak_ directories in
Directory.systemTemp before the import, then assert the post-import matching set
is unchanged. Remove the unrelated tmp directory count and compare directory
identities rather than machine-global emptiness, preserving detection of
directories created by this import.
🪄 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: ef8544b4-c53f-4f7d-bdf9-e910cdfff10d
📒 Files selected for processing (16)
lib/app.dartlib/coach/coach_config.dartlib/import/import_container.dartlib/import/noop_backup_import.dartlib/import/noop_import.dartlib/import/noop_ingest.dartlib/ui/coach/ai_coach_screen.dartlib/ui/design/app_scaffold.dartlib/ui/import/import_screen.dartlib/ui/screens/metric_screen.dartlib/ui/today/today_screen.dartlib/ui/workouts/workouts_screen.darttest/bottom_gutter_test.darttest/coach_config_key_test.darttest/import_container_test.darttest/noop_backup_import_test.dart
| if (apiKey != null) { | ||
| final k = apiKey.trim(); | ||
| _key = k.isEmpty ? null : k; | ||
| _keyUnreadable = false; | ||
| if (k.isEmpty) { | ||
| await _secure.delete(key: _kKey); | ||
| await _secure.delete(key: _kKey, iOptions: _apple, mOptions: _macos); | ||
| await prefs.setBool(_kKeyPresent, false); | ||
| } else { | ||
| await _secure.write(key: _kKey, value: k); | ||
| await _secure.write( | ||
| key: _kKey, | ||
| value: k, | ||
| iOptions: _apple, | ||
| mOptions: _macos, | ||
| ); | ||
| // Written AFTER the keychain succeeds: a marker claiming a key that was | ||
| // never stored would leave the app permanently reporting "unreadable". | ||
| await prefs.setBool(_kKeyPresent, true); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not update the cached key before secure storage succeeds.
Lines 135-136 change _key and _keyUnreadable before write or delete. If secure storage throws, memory no longer matches persisted storage. A failed write can make the app use an unpersisted replacement key. A failed delete can hide a still-stored key until restart.
Keep the previous cached state until the keychain operation succeeds. Define and test the partial-failure behavior when the keychain succeeds but the marker write fails.
As per coding guidelines, “Behavior changes, especially regressions involving ... 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 `@lib/coach/coach_config.dart` around lines 133 - 149, Update the API-key
persistence flow in the enclosing configuration method so _key and
_keyUnreadable are changed only after the secure _secure.write or _secure.delete
operation succeeds; retain the previous cached state when either operation
throws. Define the behavior for a successful keychain operation followed by a
failed prefs.setBool(_kKeyPresent, ...) call, and add regression tests covering
failed writes, failed deletes, and marker-write failures.
Source: Coding guidelines
PR Reviewer Guide 🔍(Review updated until commit dc1cabe)Here are some key observations to aid the review process:
|
the legacy case was still broken: an install predating the marker has no marker, so a locked relaunch concluded "no key" and never retried — exactly the bug this was meant to fix. the marker is three-state now, and only a foreground read settles it, which is also the only way out of a marker that outlived its item. the settings screen seeded its field from a key it could not read and saving then deleted the real one. the prior-evening buffer was retained across any gap, so after ~400 days the day walk never reached the target date and the day went missing from an import that reported success. paging could stall outright on a REAL timestamp column, and a partial 260MB extraction leaked when the write failed.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/coach/coach_config.dart (1)
126-145: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize key-state mutations.
load()checks_generationbefore its migration and marker awaits. A concurrentsave()can write a new key, then an older migration can write the oldreadvalue back to secure storage. A trusted empty load can also write afalsemarker aftersave()writestrue.Use one critical section for migration writes, deletes, normal writes, and marker updates. Recheck the generation inside that critical section. Add regression tests that interleave migration or trusted-empty loading with
save()and verify that secure storage and_kKeyPresentmatch the newest save.As per coding guidelines, “When adding or changing a capability, cover every call path” and “Behavior changes, especially regressions involving ... lifecycle safety, must include regression tests.”
Also applies to: 196-219
🤖 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/coach/coach_config.dart` around lines 126 - 145, Serialize all key-state mutations in the shared critical section used by the coach configuration flow, including migration writes, deletes, normal writes, and _kKeyPresent updates. In load(), recheck _generation after acquiring the critical section and before applying migration or trusted-empty results, so an older load cannot overwrite a newer save; preserve the newest save in both secure storage and the marker. Add regression tests covering interleaved migration and trusted-empty load operations with save(), verifying secure storage and _kKeyPresent reflect the latest save across every affected call path.Source: Coding guidelines
test/noop_backup_import_test.dart (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
strandedDates.
strandedDatesis new onNoopImportResult, new state inNoopIngest, and drives new user-facing text inlib/ui/import/import_screen.dartat Lines 115-122. No test in this cohort covers it.This file cannot cover it:
_importwalks local days in ascending order, so the backup path never strands a date. Only the CSV path reaches that branch. Add a test that feedsNoopIngestan out-of-order date sequence and asserts the date appears instrandedDates, and that a date which later derives does not.I can generate that test. Do you want me to open an issue to track it?
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/noop_backup_import_test.dart` at line 23, Add a regression test covering the CSV ingestion path in NoopIngest: feed it an out-of-order date sequence, assert the stranded date is included in NoopImportResult.strandedDates, and assert a date that later derives is excluded. Keep the existing backup-path test unchanged, since _import cannot produce stranded dates.Source: Coding guidelines
lib/import/noop_import.dart (1)
63-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
NoopImportResult's optional parameters named. The root cause is one positional signature carrying four trailing arguments, three of which areint.stepsis declared beforelateRowsbut constructed after it, so a transposition compiles silently and reports wrong counts to the user.
lib/import/noop_import.dart#L63-L80: change the optional positional parameter list[this.lateRows = 0, this.steps = 0, this.strandedDates = const {}]to named parameters.lib/import/noop_backup_import.dart#L176-L177: update this call site to passlateRows:,steps:, andstrandedDates:by name.🤖 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/import/noop_import.dart` around lines 63 - 80, Change NoopImportResult’s constructor to use named optional parameters for lateRows, steps, and strandedDates instead of positional parameters; update lib/import/noop_import.dart lines 63-80 accordingly. At lib/import/noop_backup_import.dart lines 176-177, pass all three arguments by their names, preserving their existing values and eliminating positional transposition risk.
🤖 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/import/import_container.dart`:
- Around line 257-260: Update the catch handler around the import operation to
delete tempDir directly instead of constructing ResolvedNoopDatabase solely for
disposal. Make cleanup best-effort by preventing any delete failure from
replacing the original exception, then preserve the existing rethrow so the
ImportFormatException remains the propagated error.
- Around line 221-227: Update the extraction flow around db.writeContent(sink)
to enforce _kMaxUncompressedBytes while bytes are streamed, rather than relying
only on the declared db.size and post-write comparison. Bound the sink or
decoded stream so writes beyond the archive member’s declared size or maximum
allowed size immediately throw ImportFormatException, and retain validation that
the decoded output cannot be shorter than db.size.
In `@lib/import/noop_backup_import.dart`:
- Around line 297-300: Update _import to compute each table’s column set once
before the day loop, then pass those cached sets into every _read call; remove
the per-invocation _columnNames probe from _read while preserving existing
table-specific import behavior.
- Around line 248-262: Update the no-progress fallback around the cursor
advancement logic to prevent rows sharing lastTs beyond the page limit from
being silently skipped. Prefer increasing the page size and re-reading the
remaining rows at that timestamp, or otherwise record the truncation explicitly
with a counter and comment; do not leave cursor = lastTs as an untracked loss.
Preserve the existing handling for rows already emitted by the fallback loop.
In `@lib/import/noop_ingest.dart`:
- Around line 141-145: Update the RR-value guard in the ingestion method around
`if (!(ms > 0))` to reject all non-finite values, including positive and
negative infinity, while continuing to reject zero and negatives. Use the
language’s finite-value check and preserve the existing early-return behavior
for invalid intervals.
In `@lib/ui/coach/coach_settings_screen.dart`:
- Around line 109-111: Update the confirmation text in the blindClear branch of
the Coach settings screen so it neutrally states that the API key was not
changed, covering both existing-key and no-key cases; leave the non-blindClear
message unchanged.
In `@lib/ui/import/import_screen.dart`:
- Around line 115-122: Add a nullable _warning state alongside _result and
_error, and assign the stranded-date message to _warning instead of appending it
to _result in the stranded import handling. Clear _warning wherever _result and
_error are reset, including _run and the “Import another file” action. Render
_warning in a warning-styled card between the success result card and error
text, preserving partial-success behavior without using _error.
In `@test/noop_backup_import_test.dart`:
- Around line 377-378: Update the row-count assertion in the relevant importer
test to require the expected count explicitly rather than only checking
lessThanOrEqualTo(n), so both duplicated and dropped rows fail the test. If the
expected count intentionally differs from n, assert that exact value and
document the reason in the test.
- Around line 258-263: Replace the NaN row in the backup-import test with a
direct unit test invoking NoopIngest.rr using double.nan, and assert the guard’s
expected behavior for that input. Keep the test focused on rr rather than
NoopBackupImporter._read, since SQLite NULL conversion skips the row before
ingestion.
---
Outside diff comments:
In `@lib/coach/coach_config.dart`:
- Around line 126-145: Serialize all key-state mutations in the shared critical
section used by the coach configuration flow, including migration writes,
deletes, normal writes, and _kKeyPresent updates. In load(), recheck _generation
after acquiring the critical section and before applying migration or
trusted-empty results, so an older load cannot overwrite a newer save; preserve
the newest save in both secure storage and the marker. Add regression tests
covering interleaved migration and trusted-empty load operations with save(),
verifying secure storage and _kKeyPresent reflect the latest save across every
affected call path.
In `@lib/import/noop_import.dart`:
- Around line 63-80: Change NoopImportResult’s constructor to use named optional
parameters for lateRows, steps, and strandedDates instead of positional
parameters; update lib/import/noop_import.dart lines 63-80 accordingly. At
lib/import/noop_backup_import.dart lines 176-177, pass all three arguments by
their names, preserving their existing values and eliminating positional
transposition risk.
In `@test/noop_backup_import_test.dart`:
- Line 23: Add a regression test covering the CSV ingestion path in NoopIngest:
feed it an out-of-order date sequence, assert the stranded date is included in
NoopImportResult.strandedDates, and assert a date that later derives is
excluded. Keep the existing backup-path test unchanged, since _import cannot
produce stranded dates.
🪄 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: ac08fdc3-d054-431c-bdf2-0a2f47fdd9ca
📒 Files selected for processing (12)
lib/app.dartlib/coach/coach_config.dartlib/import/import_container.dartlib/import/noop_backup_import.dartlib/import/noop_import.dartlib/import/noop_ingest.dartlib/state/app_state.dartlib/ui/coach/ai_coach_screen.dartlib/ui/coach/coach_settings_screen.dartlib/ui/import/import_screen.darttest/coach_config_key_test.darttest/noop_backup_import_test.dart
| if (db.size > _kMaxUncompressedBytes) { | ||
| throw ImportFormatException( | ||
| '“${p.basename(path)}” unpacks to ' | ||
| '${(db.size / (1024 * 1024 * 1024)).toStringAsFixed(1)} GB, which is ' | ||
| 'not something we can import.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
archive Dart package ArchiveFile writeContent declared size vs decoded output size
💡 Result:
In the Dart archive package, the ArchiveFile class represents a file within an archive, where the declared size (accessible via the size property) refers to the expected uncompressed size of the file [1][2]. When you use the writeContent method, the package writes the actual, decoded (uncompressed) content to the provided OutputStream [1][3]. There is no inherent conflict between these two values in typical operation; rather, the size property serves as metadata indicating the total length of the decompressed data [2]. The writeContent method is designed to stream this data efficiently, often decompressing it on-the-fly to the output destination without requiring the entire decompressed file to be stored in memory [3][4]. Key considerations include: 1. Metadata vs. Payload: The size property is a field on the ArchiveFile object that stores the uncompressed size [2]. The writeContent method performs the actual extraction or writing of the file's payload [1]. 2. Memory Management: The writeContent method accepts an optional freeMemory parameter (defaulting to true), which, when enabled, allows the package to release memory used for caching the decompressed data once the write operation is complete, thereby optimizing memory usage [1][5][6]. 3. Streaming: In production usage, particularly when extracting to disk, it is common to use an OutputFileStream or similar approach to ensure that the decompressed output is written directly to the target storage rather than being buffered entirely in RAM [3][7]. If you encounter a mismatch between the expected size and the output size, it typically indicates that the archive's metadata may be incorrect or that the decompression process was interrupted. The writeContent method will generally write the full amount of data available in the compressed source [1].
Citations:
- 1: https://pub.dev/documentation/archive/latest/archive/ArchiveFile-class.html
- 2: https://chromium.googlesource.com/external/github.com/brendan-duncan/archive/+/refs/heads/input_stream/lib/src/archive_file.dart
- 3: https://pub.dev/documentation/archive/latest/
- 4: https://github.com/brendan-duncan/archive
- 5: https://pub.dev/packages/archive/versions/4.0.8/changelog
- 6: https://chromium.googlesource.com/external/github.com/brendan-duncan/archive/+/refs/tags/3.5.1/CHANGELOG.md
- 7: https://pub.dev/documentation/archive/latest/archive_io/extractArchiveToDisk.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pubspec deps/archive version =="
if [ -f pubspec.yaml ]; then
rg -n "archive:|archive:" pubspec.yaml -A2 -B2 || true
fi
if [ -f pubspec.lock ]; then
rg -n "archive:|packages:" pubspec.lock -A10 -B2 || true
fi
echo "== import_container outline =="
ast-grep outline lib/import/import_container.dart --view expanded || true
echo "== relevant import_container lines =="
sed -n '200,270p' lib/import/import_container.dart
echo "== writeContent/decode usages =="
rg -n "writeContent|ZipDecoder|decode|_kMaxUncompressedBytes|truncate|written" lib/import/import_container.dartRepository: OpenStrap/edge
Length of output: 5250
🌐 Web query:
dart archive package 4.0.9 archive_file.dart writeContent source size limit written bytes
💡 Result:
The dart archive package does not impose a hard size limit on individual files during the writeContent operation in version 4.0.9 [1][2]. The writeContent method, defined as void writeContent(OutputStream output, {bool freeMemory = true}), is specifically designed to handle file data by decompressing it and streaming the result directly to an OutputStream (such as an OutputFileStream) [1][2]. This architecture allows the package to process very large files—even those larger than available system memory—by decompressing and writing them to the output target without requiring the entire uncompressed file to reside in memory at once [2][3]. In version 4.0.9, improvements were made to this process to ensure that memory is released after the data has been written, further reducing the memory footprint during extraction [4]. If you encounter memory issues while writing files, it is generally recommended to ensure you are using streaming approaches (like InputFileStream and OutputFileStream) and to close the output streams properly after the operation is complete [2][5][6]. There is no "written bytes" limit imposed by the archive package itself; limits would instead be defined by the underlying file system, the available disk space, or platform-specific constraints [2].
Citations:
- 1: https://pub.dev/documentation/archive/latest/archive/ArchiveFile-class.html
- 2: https://github.com/brendan-duncan/archive/blob/main/doc/migrating_3_to_4.md
- 3: https://pub.dev/packages/archive
- 4: https://pub.dev/packages/archive/changelog
- 5: https://chromium.googlesource.com/external/github.com/brendan-duncan/archive/+/f01d6a340ffe24e0ef46fa682d1b6bcc7b7aef13
- 6: Out of Memory on iphone6P brendan-duncan/archive#295
🌐 Web query:
pub.dev packages archive 4.0.9 source ArchiveFile writeContent size property
💡 Result:
In version 4.0.9 of the Dart archive package, the ArchiveFile class does not possess a specific writeContent size property [1]. Instead, writeContent is a method used to stream the file's contents to an output destination [1][2]. The method is defined as follows: void writeContent(OutputStream output, {bool freeMemory = true}) This method writes the contents of the archive file to the provided OutputStream [1][2]. If the freeMemory parameter is set to true (which is the default), any memory used to store decompressed data is released after the write operation is completed [1][2]. To manage output buffer sizes during file extraction in native applications, developers typically use the OutputFileStream class, which allows for specifying a buffer size, rather than setting a property on ArchiveFile itself [3][4]. For example, the internal implementation of extractArchiveToDisk uses the file size to determine an appropriate buffer size for the output stream [3]: bufferSize??= OutputFileStream.kDefaultBufferSize; final fileSize = file.size; final fileBufferSize = fileSize < bufferSize? fileSize: bufferSize; final output = OutputFileStream(filePath, bufferSize: fileBufferSize); file.writeContent(output);
Citations:
- 1: https://pub.dev/documentation/archive/latest/archive/ArchiveFile-class.html
- 2: https://github.com/brendan-duncan/archive/blob/main/doc/migrating_3_to_4.md
- 3: https://pub.dev/documentation/archive/latest/archive_io/extractArchiveToDisk.html
- 4: https://pub.dev/documentation/archive/latest/archive_io/extractFileToDisk.html
🌐 Web query:
github brendan-duncan archive archive_file.dart size final source code
💡 Result:
In the brendan-duncan/archive Dart library, the archive_file.dart file contains the ArchiveFile class, which represents a file within an archive [1][2]. The file includes a property named size, defined as an int, which represents the uncompressed size of the file [1]. Regarding the specific source code and its size, please note the following: - In recent versions of the archive library (notably since version 4.0.0), the library structure was reorganized. The original lib/src/archive_file.dart file was removed and replaced by a new implementation located at lib/src/archive/archive_file.dart [3][4]. - The "size" of the source code file itself (in bytes or lines) is not a static constant; it has changed frequently across different versions of the library due to ongoing development, refactoring, and feature additions [3][4][5][6][7]. - As of current public documentation, the ArchiveFile class continues to define the int size property to track the uncompressed size of the archive entry [2]. For the most up-to-date source code, you can view the current implementation directly in the GitHub repository under lib/src/archive/archive_file.dart [8].
Citations:
- 1: https://chromium.googlesource.com/external/github.com/brendan-duncan/archive/+/refs/heads/input_stream/lib/src/archive_file.dart
- 2: https://pub.dev/documentation/archive/latest/archive/ArchiveFile-class.html
- 3: brendan-duncan/archive@3.6.1...v4.0.9
- 4: brendan-duncan/archive@3.6.1...4.0.2
- 5: brendan-duncan/archive@2.0.13...3.3.8
- 6: brendan-duncan/archive@3.4.9...3.4.10
- 7: brendan-duncan/archive@3.2.2...3.3.8
- 8: https://github.com/brendan-duncan/archive
🌐 Web query:
github brendan-duncan archive 4.0.9 lib/src/zip/zip_file_decoder.dart read file declare size
💡 Result:
In the archive package version 4.0.9 by brendan-duncan, the size of a file within a ZIP archive is managed through the ZipFile and ArchiveFile classes [1][2]. When the ZipDecoder processes a ZIP file, it parses header information into ZipFile objects [1][3]. The ZipFile class contains an uncompressedSize property [1], which is typically read from the ZIP file's central directory or local file header during the decoding process [4]. This size is then used to populate the size property of the resulting ArchiveFile object [2]. Specifically regarding file size declaration: - The ArchiveFile class defines a size property (an int) which represents the size of the file in bytes [2]. According to the package documentation, this property is set when decoding an archive [2]. - The ZipFile class also explicitly tracks uncompressedSize and compressedSize [1]. - Issues have been noted where the uncompressedSize in a ZipFile might be reported as 0 if the ZIP structure is irregular or uses specific ZIP64 headers that are not being parsed as expected in certain scenarios [5]. If you are inspecting lib/src/zip/zip_file_decoder.dart (or the equivalent logic in ZipDecoder or ZipFile classes in version 4.0.9), you will find that the size is extracted from the binary data using methods such as input.readUint32() (or readUint64() for ZIP64) to read the uncompressed size from the ZIP file headers [4][1].
Citations:
- 1: https://pub.dev/documentation/archive/latest/archive/ZipFile-class.html
- 2: https://pub.dev/documentation/archive/latest/archive/ArchiveFile-class.html
- 3: https://pub.dev/documentation/archive/latest/archive/ZipDecoder-class.html
- 4: brendan-duncan/archive@0d17b27
- 5: The size of ArchiveFile in some zip files is 0 brendan-duncan/archive#396
🌐 Web query:
"archive" "uncompressedSize" "writeContent" "OutputStream" package:archive
💡 Result:
In the context of the archive package (typically referring to the Dart archive library), uncompressedSize is a property of an archive file entry (such as ZipFile), and writeContent is a method used to write the content of that entry to an OutputStream [1][2][3]. The archive package in Dart provides encoders and decoders for various formats like zip and tar [3]. In this library: - uncompressedSize: This property indicates the size of the file content in bytes when it is not compressed [1][2]. - writeContent(OutputStream outputStream): This method is used to write the decompressed data of an entry directly to a provided OutputStream, which is useful for efficient extraction without storing the entire file in memory [3][4][5]. While the terms uncompressedSize, writeContent, and OutputStream are also common in Java archiving libraries (such as Apache Commons Compress or the standard java.util.zip package), their usage varies [6][7][8][9]: - In Apache Commons Compress, uncompressedSize is often a property managed on ArchiveEntry objects, and writeContent is not a standard method name for all stream types, though similar functionality exists for writing data to streams [6][9]. - In Java's standard ZipOutputStream, you typically call putNextEntry followed by write to output content, with size information sometimes required beforehand depending on the compression method used [8][10]. The specific combination of these terms most directly identifies the archive package in the Dart ecosystem, where entity.writeContent(outputStream) is the standard pattern for writing entry content [3][5].
Citations:
- 1: https://pub.dev/documentation/archive/latest/archive/ZipFileHeader-class.html
- 2: https://pub.dev/documentation/archive/latest/archive/ZipFile-class.html
- 3: https://pub.dev/packages/archive
- 4: https://pub.dev/documentation/archive/latest/
- 5: https://pub.dev/documentation/archive/latest/index.html
- 6: https://github.com/apache/commons-compress/blob/master/src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveOutputStream.java
- 7: https://commons.apache.org/proper/commons-compress/apidocs/org/apache/commons/compress/compressors/snappy/SnappyCompressorOutputStream.html
- 8: https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/util/zip/ZipOutputStream.html
- 9: https://docs.glngn.com/latest/api/org.apache.commons.commons-compress/org/apache/commons/compress/archivers/zip/ZipArchiveOutputStream.html
- 10: https://github.com/openjdk/jdk/blob/6cc4bb1169f34bc091cad3e2deec37cd5585e8d5/src/java.base/share/classes/java/util/zip/ZipOutputStream.java
🌐 Web query:
brendan-duncan archive archive 4.0.9 _uncompressedSize writeContent _input length
💡 Result:
In the context of the Dart archive package (maintained by Brendan Duncan), version 4.0.9 is a release [1][2] that focuses on improving File I/O and reducing memory usage [3]. Regarding the specific terms mentioned: writeContent: This is a method on the ArchiveFile class used to write the contents of an archived file to a given output stream [4][5]. It is specifically designed to allow decompression of file data directly to disk without storing the entire decompressed content in memory, which helps prevent Out of Memory errors when handling large files [5][6]. The method signature is typically writeContent(OutputStream output, {bool freeMemory = true}) [4]. _uncompressedSize: While not a public API property typically exposed for direct manipulation in user code, this represents the internal size of the file after decompression [7]. In the archive package, logic often tracks the size of data to manage stream reading and memory allocation [3][8]. _input length: This generally refers to the length of the input data being processed from an InputStream, which is critical for the package to correctly decode compressed formats like ZIP or TAR without reading the entire archive into memory [5][9]. The library's 4.0 architecture specifically addresses the need for efficient file processing by transitioning away from a memory-only design to one that prioritizes File I/O [3]. Developers are encouraged to use InputFileStream and OutputFileStream in conjunction with writeContent to process archives safely [5][9].
Citations:
- 1: https://pub.dev/packages/archive/changelog
- 2: brendan-duncan/archive@v4.0.8...v4.0.9
- 3: https://chromium.googlesource.com/external/github.com/brendan-duncan/archive/+/f01d6a340ffe24e0ef46fa682d1b6bcc7b7aef13
- 4: https://pub.dev/documentation/archive/latest/archive/ArchiveFile-class.html
- 5: https://pub.dev/documentation/archive/latest/
- 6: Out of Memory on iphone6P brendan-duncan/archive#295
- 7: https://www.dotnetframework.org/default.aspx/Net/Net/3@5@50727@3053/DEVDIV/depot/DevDiv/releases/Orcas/SP/wpf/src/Base/MS/Internal/IO/Zip/ZipIOCentralDirectoryFileHeader@cs/1/ZipIOCentralDirectoryFileHeader@cs
- 8: brendan-duncan/archive@3.1.8...3.3.8
- 9: https://github.com/brendan-duncan/archive
🌐 Web query:
"archived" "writeContent" "output.write" "_input.read" "library archive"
💡 Result:
The terms "archive", "writeContent", "output.write", and "_input.read" do not refer to a single, unified API or standard library function. Instead, they appear to relate to two distinct domains: the libarchive C library (often used in system tools) and general I/O stream patterns in programming languages like Java [1][2][3][4]. libarchive (C Library) libarchive is a C library used for reading and writing streaming archives like tar and cpio [5]. Its architecture involves: - archive_write_data: This is the primary function used to write data content into an archive [2][6][5]. - Callback System: To write an archive, you must register callback functions for opening, writing, closing, and freeing the archive [7]. These callbacks are invoked by the library when it needs to interact with the underlying output medium [7]. - Internal naming: Internally, functions like archive_write_output are used to manage the writing process, and the library relies on client-provided callbacks to handle the actual byte-level operations [1]. Java/General I/O Streams In Java and similar environments, the patterns you described are common idioms for data transfer: - _input.read / output.write: These represent standard methods (e.g., InputStream.read and OutputStream.write) used to move data from a source (input) to a destination (output) [3][8][9]. - Pattern: The most efficient way to transfer data is to use a buffer (e.g., byte[] buffer = new byte) and loop through the data, reading from the input and writing to the output until the source is exhausted [3][4]. - Modern Convenience: In modern Java (9+), this pattern is often simplified to input.transferTo(output) [3]. R "archive" Package There is an R package named "archive" which provides bindings to libarchive [10][11]. It uses functions like archive_write to return connection objects, allowing you to write files into archives using R's native connection-based I/O functions [12][10][13]. If you are encountering these specific names in a codebase, they are likely either custom wrapper methods, parts of a legacy internal library, or specific variable names within an implementation of the streaming I/O pattern described above [1][4].
Citations:
- 1: https://github.com/libarchive/libarchive/blob/v3.0.0a/libarchive/archive_write.c
- 2: https://man.archlinux.org/man/archive_write.3.en
- 3: https://stackoverflow.com/questions/43157/easy-way-to-write-contents-of-a-java-inputstream-to-an-outputstream
- 4: https://codingtechroom.com/question/-read-inputstream-write-outputstream-java
- 5: https://linux.die.net/man/3/libarchive
- 6: https://man.archlinux.org/man/archive_write.3.en.raw
- 7: https://man.archlinux.org/man/archive_write_open.3.en.raw
- 8: https://www.engineering.upenn.edu/~cis1xx/resources/java/fileIO/introToFileIO.html
- 9: https://www.w3schools.com/java/java_fileoutputstream.asp
- 10: https://archive.r-lib.org/index.html
- 11: https://cran.r-project.org/web/packages/archive/
- 12: https://archive.r-lib.org/reference/index.html
- 13: https://cran.r-project.org/web/packages/archive/archive.pdf
🌐 Web query:
site:chromium.googlesource.com "archive_4.0.9" "writeContent" "uncompressedSize"
💡 Result:
In the Dart 'archive' library (version 4.0.9 and other 4.0.x versions), the 'writeContent' method is used on an 'ArchiveFile' entity to decompress and write file contents directly to an output stream, such as an 'OutputFileStream' [1][2][3]. This approach is designed to minimize memory usage by writing data directly to disk without storing the full decompressed content in memory [1][2][4]. While 'writeContent' performs the action of decompressing and writing, 'uncompressedSize' is a property typically found in archive metadata structures (such as 'ZipFileHeader') that stores the size of the data before compression [5]. In the context of the library's 4.0 redesign, managing memory efficiently during file operations like 'writeContent' became a primary goal [4]. When extracting archives to disk, the library uses these properties to handle file streams effectively [1][2][3]. Example usage pattern for 'writeContent': final outputStream = OutputFileStream('out/${file.name}'); file.writeContent(outputStream); outputStream.closeSync; [1][3] The 'archive' library was updated in the 4.0 series to ensure that 'writeContent' releases memory after the data has been written, further reducing overall memory footprint [6].
Citations:
- 1: https://chromium.googlesource.com/external/github.com/brendan-duncan/archive/+/refs/heads/main
- 2: https://chromium.googlesource.com/external/github.com/brendan-duncan/archive/+/refs/heads/4.0.backup
- 3: https://chromium.googlesource.com/external/github.com/brendan-duncan/archive/+/refs/tags/4.0.2
- 4: https://chromium.googlesource.com/external/github.com/brendan-duncan/archive/+/f01d6a340ffe24e0ef46fa682d1b6bcc7b7aef13
- 5: https://chromium.googlesource.com/external/github.com/brendan-duncan/archive/+/refs/heads/input_stream/lib/src/zip/zip_file_header.dart
- 6: https://chromium.googlesource.com/external/github.com/brendan-duncan/archive/+/cfa3723f46107b68d7d5de6331e37d842fa48106/CHANGELOG.md
🌐 Web query:
"archive" "zip" "4.0.9" "local file header" "uncompressedSize"
💡 Result:
In the ZIP file format specification, the uncompressed size field in the Local File Header is a 4-byte field located at offset 22 [1][2]. Regarding the handling of this field: 1. Standard Usage: In a typical ZIP archive, this field stores the size of the uncompressed data [1][2]. 2. Streaming (Bit 3): If bit 3 of the general purpose bit flag is set, the uncompressed size (along with the CRC-32 and compressed size) may be set to zero in the Local File Header [3][1]. In this case, the correct value is provided in the Data Descriptor that follows the compressed data [3][1]. 3. ZIP64 Format: If the uncompressed size exceeds the capacity of the 4-byte field (i.e., it is 0xFFFFFFFF), the value is instead stored in a ZIP64 extended information extra field [3][2]. In the context of the Dart 'archive' library (version 4.0.9), the ZipFileHeader class implements this specification by maintaining an uncompressedSize property [4][5]. The library parses this from the header but also supports updating the uncompressedSize from the ZIP64 extended information extra field if it is present [5].
Citations:
- 1: https://pkware.cachefly.net/webdocs/APPNOTE/APPNOTE-6.3.9.TXT
- 2: https://en.wikipedia.org/wiki/ZIP_(file_format)
- 3: https://github.com/zlib-ng/minizip-ng/blob/develop/doc/zip/appnote.iz.txt
- 4: https://pub.dev/documentation/archive/latest/archive/ZipFileHeader-class.html
- 5: https://chromium.googlesource.com/external/github.com/brendan-duncan/archive/+/refs/heads/input_stream/lib/src/zip/zip_file_header.dart
🌐 Web query:
"archive_4_0_9" "void writeContent" "LibraryFileZip"
💡 Result:
The method writeContent is part of the Dart archive package (specifically used for handling archive entities like ArchiveFile) [1][2]. It is used to decompress or write the content of an archive file entity directly to a specified output stream (such as an OutputFileStream), which allows for processing large files without storing the entire decompressed data in memory [1][2]. In the context of the archive package, version 4.0.9 is a stable release [3][4]. Documentation and examples for the archive package demonstrate the use of writeContent as follows [1][2]: final outputStream = OutputFileStream('out/${file.name}'); // The writeContent method will decompress the file content directly to disk without // storing the decompressed data in memory. entity.writeContent(outputStream); outputStream.closeSync; This functionality is specific to the Dart archive package by Brendan Duncan, not to be confused with libzip (a C library) [5][6] or zip.js [7].
Citations:
- 1: https://github.com/brendan-duncan/archive/blob/main/README.md
- 2: https://pub.dev/packages/archive
- 3: https://pub.dev/packages/archive/changelog
- 4: https://pub.dev/packages/archive/versions
- 5: https://libzip.org/documentation/libzip/
- 6: https://libzip.org/documentation/
- 7: https://gildas-lormeau.github.io/zip.js/api/classes/ZipWriter.html
🌐 Web query:
"Dart archive" "writeContent" "output.write" "_input" "_uncompressedSize" archive_file.dart
💡 Result:
The term "writeContent" refers to a method within the ArchiveFile class of the Dart archive package [1]. This method is designed to write the contents of a file from an archive to a specified OutputStream [1][2]. Method Signature: void writeContent(OutputStream output, {bool freeMemory = true}) Functionality and Usage: 1. Destination: It writes the decompressed file content directly to the provided OutputStream [3]. This is particularly useful for streaming data to disk using an OutputFileStream, which allows for memory-efficient extraction of files from archives without loading the entire uncompressed data into memory [3][4]. 2. Memory Management: The optional freeMemory parameter (defaulting to true) determines whether the internal cached decompressed data should be cleared from memory after the write operation is complete [1]. 3. Context: While the method name writeContent does not take _input or _uncompressedSize as arguments, these internal variables exist within the context of the archive package's stream handling [5]. For example, the Inflate class can use an InputStream (_input) and an optional uncompressedSize parameter to manage memory allocation during decompression [5]. When extracting files manually, you typically iterate through an Archive object and call writeContent on each ArchiveFile, passing an OutputFileStream as the argument [3][2]. Example snippet: final outputStream = OutputFileStream('out/${file.name}'); file.writeContent(outputStream); outputStream.closeSync; [3][4]
Citations:
- 1: https://pub.dev/documentation/archive/latest/archive/ArchiveFile-class.html
- 2: https://pub.dev/documentation/archive/latest/archive_io/extractArchiveToDisk.html
- 3: https://pub.dev/packages/archive
- 4: https://pub.dev/documentation/archive/latest/
- 5: https://pub.dev/documentation/archive/latest/archive/Inflate-class.html
🌐 Web query:
"archive_file.dart" "void writeContent" "archive" "dart"
💡 Result:
In the Dart archive package, writeContent is a method of the ArchiveFile class used to write the contents of a file to an OutputStream [1]. Method Signature: void writeContent(OutputStream output, {bool freeMemory = true}) Details: - Purpose: It writes the contents of the ArchiveFile to the specified output stream [1]. - Decompression: If the file data is compressed, this method handles the decompression automatically before writing to the output stream [1]. - Memory Management: The freeMemory parameter, when set to true (default), ensures that any cached decompressed data in memory is cleared after the write operation is completed, which helps in minimizing memory usage [1]. - Usage: This method is commonly used when processing archives (like Zip or Tar) to write file contents directly to disk using an OutputFileStream [2][3]. For example: // Example of using writeContent with an OutputFileStream final outputStream = OutputFileStream('path/to/destination/file'); archiveFile.writeContent(outputStream); outputStream.closeSync; By using OutputFileStream and writeContent together, you can decompress and extract file content directly to disk without needing to hold the entire file in memory [2][3]. In professional or production code, it is recommended to use the provided utility functions like extractArchiveToDisk or extractFileToDisk from the archive_io library, which manage these stream operations and cleanup tasks for you [4][5][6].
Citations:
- 1: https://pub.dev/documentation/archive/latest/archive/ArchiveFile-class.html
- 2: https://pub.dev/documentation/archive/latest/
- 3: https://github.com/brendan-duncan/archive
- 4: https://pub.dev/documentation/archive/latest/archive_io/extractArchiveToDisk.html
- 5: https://pub.dev/documentation/archive/latest/archive_io/extractFileToDisk.html
- 6: https://pub.dev/documentation/archive/latest/index.html
🌐 Web query:
"source:archive 4.0.9" "writeContent" "while" "output.write" ArchiveFile archive/dart
💡 Result:
In the Dart archive package (version 4.0.9 and later), the writeContent method of an ArchiveFile object is designed to write the contents of a file directly to an OutputStream without necessarily storing the entire decompressed data in memory [1][2]. When using this method, you typically provide an OutputFileStream (or another implementation of OutputStream) as the argument [3][4]. This approach is efficient for memory management, as it allows the archive to decompress and write data to disk incrementally [3][5]. A common implementation pattern looks like this: final outputStream = OutputFileStream('path/to/output/file'); // The writeContent method decompresses and writes data directly to the stream. archiveFile.writeContent(outputStream); outputStream.closeSync; // Ensure the stream is closed after writing. As noted in the package documentation, ArchiveFile.writeContent will release its internal memory after the data has been written if the freeMemory parameter (which defaults to true) is used [1][2]. This behavior helps reduce overall memory usage when processing archives [2].
Citations:
- 1: https://pub.dev/documentation/archive/latest/archive/ArchiveFile-class.html
- 2: https://pub.dev/packages/archive/changelog
- 3: https://pub.dev/packages/archive
- 4: https://pub.dev/documentation/archive/latest/archive/OutputFileStream-class.html
- 5: https://github.com/brendan-duncan/archive/blob/main/README.md
🌐 Web query:
"pub.dev packages archive versions 4.0.9 source api lib/src/archive_file.dart"
💡 Result:
In the archive package, the file path for ArchiveFile changed significantly during the transition to version 4.0. Prior to version 4.0, ArchiveFile was located at lib/src/archive_file.dart [1]. Starting with version 4.0, it was moved to lib/src/archive/archive_file.dart [1][2]. For archive version 4.0.9, the source code for ArchiveFile is located at lib/src/archive/archive_file.dart [1][2]. You can access the official API documentation for the ArchiveFile class on pub.dev [3]. If you are looking to view the source code directly, it is hosted on GitHub under the brendan-duncan/archive repository [4].
Citations:
- 1: brendan-duncan/archive@3.6.1...v4.0.9
- 2: https://chromium.googlesource.com/external/github.com/brendan-duncan/archive/+/refs/heads/4.0.backup/lib/archive.dart
- 3: https://pub.dev/documentation/archive/latest/archive/ArchiveFile-class.html
- 4: https://github.com/brendan-duncan/archive
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read vendored/cached package source from Dart's pub cache if present.
for c in "$DART_PUB_CACHE" "$HOME/.pub-cache" /usr/local/share/pub-cache; do
if [ -n "${c:-}" ] && [ -d "$c/hosted/pub.dev/archive-4.0.9/lib/src/archive/archive_file.dart" ]; then
echo "== cache file =="
echo "$c/hosted/pub.dev/archive-4.0.9/lib/src/archive/archive_file.dart"
wc -l "$c/hosted/pub.dev/archive-4.0.9/lib/src/archive/archive_file.dart"
rg -n "class ArchiveFile|void writeContent|uncompressedSize|_uncompressedSize|OutputStream output|while|output.write" "$c/hosted/pub.dev/archive-4.0.9/lib/src/archive/archive_file.dart"
sed -n '1,220p' "$c/hosted/pub.dev/archive-4.0.9/lib/src/archive/archive_file.dart" | nl -ba
break
fi
done
echo "== git index pub cache location candidates =="
for d in "$HOME/.pub-cache/hosted/pub.dev/archive-4.0.9" "$HOME/.dart_tool/pub-cache/hosted/pub.dev/archive-4.0.9"; do
[ -d "$d" ] && echo "$d" && find "$d" -path '*archive_file.dart' -maxdepth 4 -print
doneRepository: OpenStrap/edge
Length of output: 199
Bound the extracted file size against the archive member size.
db.size is decoded from ZIP metadata, and db.writeContent(sink) streams the decoded result without exposing that size to the import code before the local file grows. The only check afterward compares written < db.size, so a member whose real content exceeds its declared size can still write past _kMaxUncompressedBytes. Use a bounded stream/check that rejects overruns and rejects under-runs.
🤖 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/import/import_container.dart` around lines 221 - 227, Update the
extraction flow around db.writeContent(sink) to enforce _kMaxUncompressedBytes
while bytes are streamed, rather than relying only on the declared db.size and
post-write comparison. Bound the sink or decoded stream so writes beyond the
archive member’s declared size or maximum allowed size immediately throw
ImportFormatException, and retain validation that the decoded output cannot be
shorter than db.size.
PR Code Suggestions ✨Latest suggestions up to dc1cabe
Previous suggestionsSuggestions
|
|
Persistent review updated to latest commit ba95c13 |
keying the page cursor on a truncated second looked equivalent to keying it on the column's own value and is not: against a REAL column `ts > 3.0` does not exclude `3.2`, so my guard against the resulting loop stopped the read after two pages and dropped the rest of the day. the cursor is the last row actually emitted now, and a timestamp that fills a whole page is drained rather than stepped past. infinity poisons a day's HRV exactly as NaN does, and only NaN was rejected. the out-of-order warning rendered inside the green success card, next to a tick. the column probe ran once per table per day when the schema cannot change at all.
PR Code Suggestions ✨Explore these optional code suggestions:
|
There was a problem hiding this comment.
Pull request overview
This pull request extends OpenStrap Edge’s import and UI plumbing to (1) ingest NOOP iOS .noopbak backups through the same 1 Hz derivation pipeline as NOOP CSV imports, (2) prevent BYOK AI keys from “disappearing” after locked/background relaunches by changing keychain accessibility and adding an “unreadable” state with retry, and (3) fix scroll bottom padding so content clears the shell’s floating chrome (including the live-workout banner).
Changes:
- Add
.noopbak(zipped SQLite) import support and unify NOOP CSV/DB ingestion via a sharedNoopIngeststreaming pipeline. - Fix BYOK AI key persistence/readability across locked relaunches, and adjust UI to retry when the key is temporarily unreadable.
- Replace hard-coded bottom padding with a measured bottom gutter based on
MediaQuery.padding.bottom, including tests.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/import/noop_import.dart | Routes NOOP imports to either CSV streaming or .noopbak DB ingestion; returns richer import result (late/stranded). |
| lib/import/noop_ingest.dart | New shared streaming ingest that buffers a rolling window, banks step coverage, and derives day-by-day. |
| lib/import/noop_backup_import.dart | New SQLite reader that pages NOOP backup tables by day and feeds NoopIngest. |
| lib/import/import_container.dart | Adds .noopbak database extraction/validation (partial extraction detection) and database sniffing for NOOP. |
| lib/state/app_state.dart | Stores last NOOP import result so UI can surface stranded/out-of-order days. |
| lib/ui/import/import_screen.dart | Updates NOOP import copy and surfaces “stranded dates” warning after import. |
| lib/coach/coach_config.dart | Changes keychain accessibility to first_unlock, adds marker + unreadable/undetermined states, and prevents clobbering on failed reads. |
| lib/app.dart | Refreshes unreadable/undetermined AI key on resume (foreground). |
| lib/ui/coach/ai_coach_screen.dart | Shows a retry notice instead of setup wall when the key is unreadable. |
| lib/ui/coach/coach_settings_screen.dart | Prevents “blind clear” of a key when the stored key couldn’t be read; surfaces save failures. |
| lib/ui/design/app_scaffold.dart | Adds dsBottomGutter() and uses it to compute list bottom padding (incl. bottomBar overlay). |
| lib/ui/screens/metric_screen.dart | Switches ListView bottom padding from hard-coded constant to dsBottomGutter(). |
| lib/ui/today/today_screen.dart | Switches ListView bottom padding from hard-coded constant to dsBottomGutter(). |
| lib/ui/workouts/workouts_screen.dart | Switches ListView bottom padding from hard-coded constant to dsBottomGutter(). |
| test/noop_backup_import_test.dart | Adds end-to-end and edge-case tests for .noopbak import (paging boundaries, corrupt ts, NaN RR, gaps). |
| test/coach_config_key_test.dart | Adds tests for key persistence across locked relaunches and failure modes. |
| test/bottom_gutter_test.dart | Adds widget tests ensuring gutter matches shell chrome and banner height. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| var cursor = from - 1; | ||
| while (true) { | ||
| final rows = await src.query( | ||
| table, | ||
| columns: cols, | ||
| where: 'ts > ? AND ts < ?', | ||
| whereArgs: [cursor, to], | ||
| orderBy: 'ts', | ||
| limit: kNoopBackupPageRows, | ||
| ); | ||
| if (rows.isEmpty) return; | ||
| final full = rows.length == kNoopBackupPageRows; | ||
| final lastTs = _int(rows.last['ts']); |
|
Persistent review updated to latest commit 7c214cf |
PR Code Suggestions ✨Explore these optional code suggestions:
|
`ts > from - 1` is only the half-open window we want for integral timestamps. against a fractional one the last fraction of a second before midnight belongs to both days, and the map-keyed channels absorb that but rr appends — so the night got a duplicate beat and its rmssd was wrong. the first page is bounded inclusively now. both non-finite-rr tests were vacuous: every derived metric lives inside payload_json, so sweeping the row's typed columns for a non-finite double finds nothing and passes however broken the guard is. they assert on the beat count the pipeline actually used, and both fail when the guard is removed. the table names existed twice, and mistyping one silently dropped a whole channel from every import. one definition now. the out-of-order warning outlived the card that dismissed it.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/import/noop_backup_import.dart (1)
120-139: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate readable schemas before computing the timestamp span.
_spanqueriesMIN(ts)before the column probe. A drifted optional table withouttsthrows a raw SQL error instead of being skipped. AhrSampletable withoutbpmpasses the table-name check, then HR is skipped while other channels can still make the import appear usable.
lib/import/noop_backup_import.dart#L120-L139: probe columns before_span; exclude incomplete optional tables from the span; reject incompletehrSampleschemas withImportFormatException.test/noop_backup_import_test.dart#L464-L488: add regressions for an optional table withouttsand anhrSampletable withoutbpm.As per coding guidelines, “Behavior changes, especially regressions involving … migrations … 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 `@lib/import/noop_backup_import.dart` around lines 120 - 139, In lib/import/noop_backup_import.dart:120-139, move the column probe before _span, exclude optional tables lacking ts from span computation, and throw ImportFormatException when hrSample lacks bpm; preserve importing other valid channels. In test/noop_backup_import_test.dart:464-488, add regression coverage for an optional table without ts and an hrSample table without bpm.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.
Outside diff comments:
In `@lib/import/noop_backup_import.dart`:
- Around line 120-139: In lib/import/noop_backup_import.dart:120-139, move the
column probe before _span, exclude optional tables lacking ts from span
computation, and throw ImportFormatException when hrSample lacks bpm; preserve
importing other valid channels. In test/noop_backup_import_test.dart:464-488,
add regression coverage for an optional table without ts and an hrSample table
without bpm.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6c00b20e-b0aa-42d6-93fa-ac87323653ce
📒 Files selected for processing (7)
lib/import/import_container.dartlib/import/noop_backup_import.dartlib/import/noop_ingest.dartlib/ui/coach/coach_settings_screen.dartlib/ui/import/import_screen.darttest/noop_backup_import_test.darttest/noop_schema_drift_test.dart
|
Persistent review updated to latest commit 8060dd5 |
|
@CodeRabbit review |
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
the span selects MIN(ts)/MAX(ts), and it ran before the column probe — so a table drifted far enough to have no ts at all threw a raw SQL error out of the span rather than being skipped by the probe that exists for it. an hrSample carrying no bpm passed the table-name check and was then skipped by every read, leaving the other channels to carry the import to a plausible day count with no heart rate anywhere in it.
|
Persistent review updated to latest commit 196e0bc |
PR Code Suggestions ✨Explore these optional code suggestions:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/import/noop_backup_import.dart (1)
299-307: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a deterministic tie-breaker for offset paging on identical timestamps.
At
lib/import/noop_backup_import.dart:304-306, both the bounded page and the drain query order only byts, while the drain query usesOFFSET. Identical rows can come out in different orders between those two queries, sooffset: drainedcan skip rows or re-emit them. Apply the same stable order to both queries. Userowidonly when supported NOOP tables are notWITHOUT ROWID; otherwise add another immutable column, such asrrMs, torrIntervalorders.Proposed fix when rowid is supported
- orderBy: 'ts', + orderBy: 'ts, rowid', ... - orderBy: 'ts', + orderBy: 'ts, rowid',🤖 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/import/noop_backup_import.dart` around lines 299 - 307, Update the query ordering in the NOOP import paging flow so both the bounded page query and the drain query use an identical deterministic tie-breaker after ts. Use rowid for supported NOOP tables that are not WITHOUT ROWID; otherwise order by another immutable column such as rrMs, consistently in both queries, preserving the existing offset behavior.
🤖 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.
Outside diff comments:
In `@lib/import/noop_backup_import.dart`:
- Around line 299-307: Update the query ordering in the NOOP import paging flow
so both the bounded page query and the drain query use an identical
deterministic tie-breaker after ts. Use rowid for supported NOOP tables that are
not WITHOUT ROWID; otherwise order by another immutable column such as rrMs,
consistently in both queries, preserving the existing offset behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d934db4b-d029-4b00-8d98-84bc7301caf2
📒 Files selected for processing (2)
lib/import/noop_backup_import.darttest/noop_backup_import_test.dart
the drain pages an equal-timestamp group with offset, and ordering by ts alone is not a total order once a second holds several beats — the two queries were free to hand that group back in different orders, which skips and repeats rows. rowid gives a stable one where the table has it, probed rather than assumed.
|
Persistent review updated to latest commit dc1cabe |
User description
Importing a
.noopbak(#160, #199). A.noopbakis a zip aroundnoop-backup.sqlite, noop's own database, and on iOS it is the only export noop offers — the raw sensor CSV is Android-only, so pointing people at it left every iOS migrant with no way across. The backup holds the same 1 Hz channels the band does (heart rate, RR, gravity, skin temp, and the cumulative step counter, ~1 M rows each on a fortnight), so it goes through the substrate and re-derives day by day rather than importing noop's own sleep stages and daily scores — a second set of those would only contradict the ones computed here. On a real 13-day backup that is 14 days and 4.6 M rows in about two minutes, with step totals within a percent of what noop itself recorded. The CSV and the database now share one ingest, so the rolling two-day window, the step banking and the out-of-order handling exist once.Two things that file taught me:
rrIntervalis keyed on (deviceId, ts, rrMs), so one second can hold several beats and paging on the timestamp alone drops whatever falls past a page edge; and a backup that stops unpacking halfway — a phone out of space — still opens as a perfectly valid database, so a fraction of someone's history would import as if it were all of it. Both are handled, and the emptyspo2Sample/respSampletables and the deviceId that differs between the sample tables andsleepSessionare pinned by tests, since a filter on that id silently drops every sleep session.Reading a
.noopbak's database is what the last import PR left undone.The AI key disappearing after the phone sleeps. Two reports of a key that works for a few minutes and is gone after a sleep/wake, with the app asking for it to be set up again. It was stored with the keychain's default
whenUnlockedaccessibility, and this app gets relaunched in the background constantly — a background task, or the BLE restore central waking on a link drop — routinely while the phone is locked, which is exactly when such an item cannot be read. That empty read was then cached as "no key". It is stored asfirst_unlocknow, an existing key is rewritten once to carry that, a read that throws no longer erases what is already held, and a key that is known to exist but could not be read this time says so and retries instead of showing the setup wall.The last card of every tab, during a workout. Screens reserved a flat 120pt for the floating nav pill. The pill and the home indicator come to 106pt on a current iPhone, and the live-workout banner stacks above the pill inside the same bar — so once a workout is running the reservation is short and the last card sits under the chrome. It comes from what the shell actually reports now, banner included.
PR Type
Enhancement, Bug fix
Description
Add support for importing
.noopbakSQLite backups (the only export option on iOS) through the existing 1 Hz pipeline.Fix AI key disappearing after background relaunches on locked devices by changing keychain accessibility to
first_unlock.Handle temporarily unreadable AI keys gracefully with a retry prompt instead of asking users to re-enter them.
Fix bottom gutter calculation to dynamically clear the shell's floating chrome and live-workout banners, preventing buried content.
Diagram Walkthrough
File Walkthrough
5 files
Refactor to route `.noopbak` files and extract ingest logicExtract common 1 Hz data ingestion logic for NOOP sourcesAdd logic to read and page through NOOP's SQLite databaseAdd support for extracting SQLite databases from `.noopbak` archivesAdd UI state and retry prompt for temporarily unreadable AI keys4 files
Add tests for `.noopbak` import and step bankingAdd tests for AI key persistence across locked device relaunchesAdd tests for dynamic bottom gutter calculationAdd tests for resolving.noopbakfiles and handling partialextractions
2 files
Change keychain accessibility tofirst_unlockand handle unreadablekeys
Refresh unreadable AI key when the app returns to the foreground5 files
Summary by CodeRabbit
New Features
.noopbakbackups alongside CSV files and ZIP archives.Bug Fixes