Restore exported location history - #173
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
Two gaps, both invisible from inside. **Nothing said the file is agent-owned.** The header names its audience - "rules for agents working on OpenTagViewer" - and rule 10 has one table row obliging you to add a rule when a constraint would cost somebody an afternoon. Neither says the file is maintained by agents, that no human reviews it, or that keeping it current is part of the work rather than a favour. An agent picking up work here had to infer all of that, and both inferences are load-bearing: if nobody is filtering, the agent writing is the only check, and if the next agent starts from this file then anything omitted is an afternoon they pay for. That now sits directly under the title, where it is read before anything it governs, rather than in a rule two hundred lines down. **And nothing warned that the register does not travel.** @parawanderer has not read this file, nor most of docs/, most docstrings, or most commit messages. Agents wrote them. So an agent reading them cannot tell house style from its own predecessors' output, and assumes the former. That showed up this week. A review of #173 was drafted in this file's voice - argued, at length, with the reasoning as the payload - and its reader was a first-time contributor who wanted three line numbers and a fix. Right register, wrong reader. The loop has nobody in it: agents write the docs, a later agent reads them as evidence of what the maintainer wants, and writes more of the same. Nothing in the repository contradicts that, because nothing in the repository is written by the maintainer. Rule 16 states the boundary. The reason it argues at length is the same reason it says not to: the length is for an agent about to undo a constraint, and a person reading a review is not that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for listing what you ran locally, and for the edge case handling in the DAO and parser. CI is approved and green: 656 tests, 0 failures. I checked the report artifact rather than the tick; your 16 new tests ran rather than being skipped. Three findings. None were caught by CI and none could have been. 1. Peak memory during import
Ceiling is about 70,000 rows per tag-year: ~100/day from the Find My network, plus @ubrt's local sightings throttled by
No Do not replace the preload with per-row Fix: intern An index on 2. Crash on rotation, and no progress UI
Duration for ~180k rows:
30-60 s with nothing on screen. Fix, progress: Fix, rotation: disposing in Espresso coverage for the progress UI
Add a seam:
Assert the indicator appears, updates, and is gone on every path including each failure. Two traps from 3. Unexpected exceptions reported as a damaged file
} catch (RuntimeException error) {
// Commons CSV reports some malformed record shapes while its iterator advances.
throw new HistoryImportException(
HistoryImportException.Reason.INVALID_ARCHIVE,
"History CSV structure is invalid",
error);
}The Commons CSV case is real: it throws from inside
} else {
title = R.string.history_import_failed_title;
message = R.string.history_import_failed_message;
}This is the unknown-cause branch. It should reach
Changes:
|
1c718b7 to
402e95d
Compare
|
Addressed the three findings in 402e95d:
The activity now dismisses its dialog before touching a destroyed window. ViewModel ownership across rotation is tracked separately in #175. The refreshed fork workflows are currently waiting for approval. |
GitHub Actions
Error
GitHub Actions
Inactive
|
CI is red, and the run reads as flaky when it is not. Two symptoms, one cause. What the run saysThe suite stopped at 509 of 659 and sat there for 30 minutes until The layout is not the problemYour
What is different
A bare Why the hang follows from itThe dialog layout cannot inflate, so the progress dialog never appears, so Fix the inflation and both go away. Options: inflate through an Activity context in the test, or drop the layout test and rely on the Espresso one which uses the real dialog path. Everything else in
|
CI on this branch ran for 45 minutes and was killed. Two faults, and the one that mattered was not the one that showed up red. **The suite hung in readFailureUsesTheGenericFailureMessage.** The per- test logcats put it beyond doubt: every other test in that class finished by 17:51:24, and that one's logcat was written at 18:21:36, 176 KB of device output, at the moment the job timed out. Opening the document happens in the Activity, outside importArchive, so a file that has gone arrives as a bare IOException rather than a HistoryImportException. historyImportFailed reads that as something this app did not anticipate and opens ErrorReportActivity - which is right for a defect here and wrong for a file the picker handed back. The dialog the test waits for never appeared, and inRoot(isDialog()) against a screen with no dialog is the case Espresso retries internally for seconds at a time. AGENTS.md describes that under Eventually; seven tests once took six and a half minutes to it. READ_FAILED already existed for this and was not being used. Opening is now wrapped in it, so the failure reaches the plain dialog rather than the report page, and the test passes because the behaviour is right. **And the layout test inflated by a route the app never takes.** It built a themed context by hand and called LayoutInflater.from on it, which is the SystemColorsLayoutTest pattern - but nothing there inflates a Material progress indicator, and an AppCompatActivity installs a factory a bare ContextThemeWrapper does not. It threw InflateException while the dialog worked on a device. It now inflates through TestHostActivity's own inflater, with cloneInContext carrying a night configuration for the second render, so both themes are covered without touching global state. Rule 12 makes this exact point about drawables: load them the way the app loads them. The progress dialog itself was never broken. aLongImportShowsAndUpdatesProgressBeforeItsResult passed at 17:51:22. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Correction to my last comment: the hang was not a cascade from the inflation failure.
CI is green on all checks now, so this is going in. @hamza-lzr — from @parawanderer: thanks for taking the time to make this contribution to this project!
|
parawanderer#139 landed while this was in review and both touch `AppDependencies`. Conflict resolved by taking main's version and re-applying this branch's five additions: the two imports, the `historyImporterFactory` field, `historyImporter(context)`, `replaceHistoryImporter` and the line in `reset()`. `OpenTagViewerDatabase` merged cleanly - this branch adds `historyImportDao()` and no schema, so it sits beside parawanderer#139's migrations 6-7, 7-8 and 8-9 rather than competing with them. Database version is 9 and every migration is registered.
GitHub Actions
Failure
parawanderer#139 added `LocationReport.provenance` and said in its own docstring that the column exists because the history is exported - "without it the CSV hands somebody a file where their own phone's positions sit unlabelled among Apple's, and nothing in the file says which is which". The writer was never updated, and this branch's importer builds its rows without the field at all, which is a `NOT NULL` column: Room refuses the insert and takes the whole merge transaction with it. Five instrumented tests fail on it, and none of them could have failed before, because each PR was tested against a main without the other. So the round trip now carries it: - `BeaconLocationReport` gains the field, because the export reads models rather than rows, and the three mappings in `BeaconRepository` carry it in both directions - `HistoryCsvWriter` writes a `provenance` column. An Apple row is a stranger's iPhone estimating a position to within a hundred metres or worse; a `local` row is this phone hearing the tag directly and recording its own position as the tag's. Unlabelled, the second reads as the first - `HistoryImporter` requires the column and refuses a row whose value is neither, rather than storing a third kind of report that nothing downstream has a branch for - `HistoryImportDao.merge` sets it, which is the crash **The column is required rather than optional**, and that is free exactly once: the history export shipped in no release - `util/export/` does not exist in `android-app-v1.0.5` - so there are no archives in the field to stay compatible with. Adding it after 1.1.0 would have meant accepting both shapes forever. The two round-trip tests now carry it, and the first uses `local` on purpose: it is the value a missing column does not fall back to, so it cannot pass against a writer or reader that defaults the field into place.
2 participants
Fixes #102
Summary
beacon_ididentity and exact restore metadata to Android history exportsImport behavior
(beacon_id, timestamp)Verification
./gradlew.bat testDebugUnitTest./gradlew.bat testAll./gradlew.bat testAllOnDevice(0 failures, 22 intentional skips)python scripts/add_strings.py --checkThe full suites above passed before the final rebase. After rebasing onto current
main, string completeness and diff checks pass; a local JVM rerun could not start because this session no longer has an Android SDK path configured.