fix(shots): read back shots imported from de1app - #786
Conversation
de1app .shot files record the shot, not the profile that produced it, so TclShotParser and the no-profile branch of ShotV2JsonParser store a placeholder profile with an empty steps array. Profile.fromJson refuses to read that back, so importing a de1app folder made every later GET /api/v1/shots throw ArgumentError and return 500, and streamline.js rendered no history at all. Because DriftStorageService mapped rows with a bare rows.map(...), the single unreadable row aborted the whole page, hiding a history of otherwise fine shots behind one 2021 import. Read a stored shot's workflow with Workflow.fromRecordedJson, which parses its profile leniently. A recorded profile is history, not something to brew from. Workflow.fromJson and Profile.fromJson stay strict for the profile library, the profile and workflow REST handlers, the stored current workflow, and DE1 upload, so PUT /api/v1/workflow still rejects a step-less profile with 400 (issue #338). Skip and log a row that cannot be mapped, so future corruption costs its own shot instead of the entire list. Single-shot reads still surface the error, because there the failing row is the answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYDtpEj71im9nmUmWkhveZ
tadelv
left a comment
There was a problem hiding this comment.
One required fix: the new recorded-workflow read path makes imported step-less shots visible, but the existing shot update path still reparses those same records through the strict executable workflow parser. That leaves the recovered shots uneditable. Please preserve strictness at executable/write boundaries while making stored-shot updates round-trip recorded workflows, and add a regression test for updating an imported step-less shot.
Reading a stored shot leniently was not enough. PUT /api/v1/shots/<id> merges the patch into existingShot.toJson() and reparses the result, and ShotImporter reads back shots from a backup export; both went through the strict ShotRecord.fromJson, so a recovered de1app shot with steps: [] read back fine but threw again on any annotation edit or re-import. Add ShotRecord.fromRecordedJson, mirroring the existing Workflow and Profile split, and use it on those two stored-shot read paths. ShotRecord.fromJson stays strict everywhere else, so no executable profile or workflow boundary is weakened. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L5WcW9yBsD9DMBy838wsGQ
tadelv
left a comment
There was a problem hiding this comment.
Two required fixes remain in the backup path. The recorded-shot parser split is sound and the previous PUT /api/v1/shots/<id> blocker is fixed, but the current backup restore path still reparses shots through the strict parser, and the new skip-on-map export callback can truncate a paged backup. Please address both and add regression coverage for backup export/import.
Two reviewer-flagged gaps remained in the de1app step-less-profile fix: ShotExportSection.importJson (the restore side of the app's /backup archive endpoint) still used strict ShotRecord.fromJson, so a recovered de1app shot could be exported but rejected on restore. Switch it to fromRecordedJson, mirroring ShotImporter and the shot update path. ShotExportSection.exportJson treats a page shorter than pageSize as end-of-stream, but ShotMapper.fromRows silently drops unmappable rows, so a raw page containing one came back short even with more rows left, truncating the backup. Extract the pageShots wiring into a standalone pageShotsForExport (lib/main.dart), which re-queries past a dropped row using the raw row's cursor until it fills the page or the table is genuinely exhausted. Regression tests: a step-less-profile import through ShotExportSection, and a paged backup export around an unreadable row via the real pageShotsForExport (not a duplicated test-only closure). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KYQbrfYh9W4xTy2tsEya3m
tadelv
left a comment
There was a problem hiding this comment.
The two substantive blockers from the previous round are fixed: /backup restore now uses recorded-shot parsing, and backup export now preserves page semantics across skipped rows with regression coverage. CI on b7fd471 is green. One required repository-guidance cleanup remains in lib/main.dart; otherwise this looks ready.
AGENTS.md keeps rationale for new/rewritten code in doc/AI_*_NOTES.md, not inline doc comments. The removed block duplicated what doc/AI_STORAGE_NOTES.md already records. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KYQbrfYh9W4xTy2tsEya3m
Summary
What changed, and why?
.shotfiles record the shot, not the profile that produced it, soTclShotParser(tcl_shot_parser.dart:48) and the no-profile branch ofShotV2JsonParser(shot_v2_json_parser.dart:80) store a placeholder profile with an emptystepsarray.Profile.fromJson(profile.dart:52) refuses to read that back, so importing a de1app folder made every laterGET /api/v1/shotsthrowArgumentErrorand return 500, and streamline.js rendered no history at all.DriftStorageServicemapped rows with a barerows.map(ShotMapper.fromRow), so the single unreadable row aborted the whole page — one 2021 import hid a history of otherwise fine shots.ShotMapper.fromRownow reads the stored workflow withWorkflow.fromRecordedJson, which parses its profile withProfile.fromRecordedJson. A recorded profile is history, not something to brew from.Workflow.fromJsonandProfile.fromJsonstay strict, backing the profile library, the profile and workflow REST handlers, the stored current workflow, and DE1 upload.PUT /api/v1/shots/<id>merges the patch intoexistingShot.toJson()and reparses the result, andShotImporterreads back shots from a backup export; both used the strictShotRecord.fromJson, so a recovered de1app shot read back fine but threw again on any annotation edit or re-import.ShotRecord.fromRecordedJsonmirrors the same split and is used on those two stored-shot paths;ShotRecord.fromJsonis unchanged and stays strict everywhere else.ShotMapper.fromRowsskips and logs a row it cannot map, so future corruption costs its own shot instead of the entire list. Single-shot reads (getShot,getLatestShot) still surface the error, because there the failing row is the answer.ShotExportSection.importJson— the restore side of the app's own/backuparchive endpoint, distinct fromShotImporter— still used strictShotRecord.fromJson, so a recovered de1app shot could be exported but rejected on restore; switched tofromRecordedJson. Separately,ShotExportSection.exportJsontreats a page shorter thanpageSizeas end-of-stream, butShotMapper.fromRowssilently drops unmappable rows, so a raw page containing one unreadable row came back short even with more rows waiting, truncating the backup. ThepageShotswiring is now a standalonepageShotsForExport(lib/main.dart) that re-queries past a dropped row — advancing the cursor from the raw row, not the last successfully mapped shot — until it either fills the requested page or the table is genuinely exhausted.The evidence, from the logs attached to the issue.
webview_console.log:nohisotry_log.txt, 132 identical failures:The timeline pins the trigger to a de1app import:
The poison row is named by the one single-shot failure:
GET /api/v1/shots/de1app-1626149813-> 500, whilede1app-1785894912,-1785895243and-1785895651still return 200. Unix1626149813is July 2021, an old TCL-format.shotfile; the 2026 ones came through the v2 JSON parser's real-profile branch.Linked Issue
Fixes #784
Verification
How did you verify the change? Include relevant tests and any manual or hardware testing.
flutter analyze—No issues found!flutter test— 3868 passed, 8 failed, out of 3877 (the new suite added 2 tests). The 8 aretest/webui_support/webui_token_injection_test.dartfailing withSocketException: Failed to create server socket (OS Error: Address already in use, errno = 48), address = 0.0.0.0, port = 3000, caused by a Decaid instance running on the dev machine holding port 3000. They fail identically on cleanorigin/mainwith none of this branch's changes, and are unrelated to shots.test/database/shot_history_imported_profile_test.dart(11 tests), against an in-memory Drift database (the update tests drive the realShotsHandler):de1app-1626149813);PUT /api/v1/shots/de1app-1626149813with an annotation patch on a step-less shot returns 200, persists the note, and leaves the profile step-less (both update tests return 500 on the previous commit);Unknown profile;getShotstill surfaces the failure for a single unreadable id;pageShotsForExport(viaimport 'package:reaprime/main.dart' as app;) against a real Drift DB withpageSize: 2, an unmappable row placed inside what would otherwise be the first page, and asserts all three good shots are exported. Confirmed this fails (truncates to one shot) against the pre-fix callback;Workflow.fromJsonstill rejects a step-less profile whileWorkflow.fromRecordedJsonaccepts it;Profile.fromJsonstill rejects an empty steps array and an empty title.test/data_export/shot_export_section_test.dartgained "imports a recorded shot with a step-less imported profile (gh#784)", drivingShotExportSection.importJsondirectly with a de1app-style step-less-profile record — exercises the/backuprestore endpoint specifically, sinceShotImporteris a separate code path.test/webserver/workflow_handler_test.dartcaught a first attempt that madeWorkflow.fromJsonlenient everywhere:PUT /api/v1/workflowwith an empty profile title returned 200 instead of 400, breaking issue [Bug]PUT /api/v1/workflow hangs forever when profile JSON fails enum parse #338. Leniency is now reached only throughWorkflow.fromRecordedJson, that suite is green (47 tests), and the new suite pins the boundary.Impact
Note any user-visible behavior, compatibility, migration, API/spec, documentation, or security impact. Write
Noneif there is none.GET /api/v1/shotsreturns 200 instead of 500, and streamline.js renders the list again. This recovers existing databases with no migration — the offending rows become readable rather than needing repair or deletion.Unknown profile, and missingtank_temperature/target_volume_count_startread back as0. These only apply to profiles embedded in stored shots.requireExecutable: trueevery guard still fires before the new fallbacks, soProfile.fromJsonis byte-for-byte the parser it was.PUT /api/v1/workflowstill returns 400 for a step-less profile (issue [Bug]PUT /api/v1/workflow hangs forever when profile JSON fails enum parse #338), verified by the existing suite./backuparchive round-trips a recovered de1app shot: export viaShotExportSection.exportJsonno longer truncates around an unreadable row, andShotExportSection.importJsoncan restore a step-less-profile shot from that archive.ShotImporter(a separate, non-/backupcode path) already handled this from the prior commit.PUT /api/v1/shots/<id>instead of returning 500.assets/api/rest_v1.ymlandassets/api/websocket_v1.ymlare unchanged.doc/Profiles.mddocuments the strict/lenient split and when each parser applies.doc/AI_STORAGE_NOTES.mdrecords why de1app imports carry a step-less profile, why one bad row used to hide the whole history, the single-shot-read exception, why every stored-shot JSON read path (row mapper, shot update, backup import/export) has to be lenient together, and whypageShotsForExportre-queries past a dropped row instead of trusting the mapped page length.steps: []. That is the honest representation of a.shotfile, and it now round-trips.Contributor Responsibility
AI-assisted development is allowed. The submitter remains responsible for the submitted work.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KYQbrfYh9W4xTy2tsEya3m