Skip to content

fix(shots): read back shots imported from de1app - #786

Merged
tadelv merged 4 commits into
mainfrom
fix/shot-history-unreadable-imported-profile
Sep 10, 2026
Merged

tadelv merged 4 commits into
mainfrom
fix/shot-history-unreadable-imported-profile

Conversation

@allofmeng

@allofmeng allofmeng commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

What changed, and why?

  • de1app .shot files record the shot, not the profile that produced it, so TclShotParser (tcl_shot_parser.dart:48) and the no-profile branch of ShotV2JsonParser (shot_v2_json_parser.dart:80) store a placeholder profile with an empty steps array. Profile.fromJson (profile.dart:52) 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.
  • DriftStorageService mapped rows with a bare rows.map(ShotMapper.fromRow), so the single unreadable row aborted the whole page — one 2021 import hid a history of otherwise fine shots.
  • ShotMapper.fromRow now reads the stored workflow with Workflow.fromRecordedJson, which parses its profile with Profile.fromRecordedJson. A recorded profile is history, not something to brew from.
  • Workflow.fromJson and Profile.fromJson stay strict, backing the profile library, the profile and workflow REST handlers, the stored current workflow, and DE1 upload.
  • Reading was only half of it (review feedback). 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 used the strict ShotRecord.fromJson, so a recovered de1app shot read back fine but threw again on any annotation edit or re-import. ShotRecord.fromRecordedJson mirrors the same split and is used on those two stored-shot paths; ShotRecord.fromJson is unchanged and stays strict everywhere else.
  • ShotMapper.fromRows skips 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.
  • Two more strict paths, found by review (b7fd471). ShotExportSection.importJson — the restore side of the app's own /backup archive endpoint, distinct from ShotImporter — still used strict ShotRecord.fromJson, so a recovered de1app shot could be exported but rejected on restore; switched to fromRecordedJson. Separately, 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 unreadable row came back short even with more rows waiting, truncating the backup. The pageShots wiring is now a standalone pageShotsForExport (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:

[streamline.js] [WARN] Could not load shot history source: Error: HTTP error! status: 500

nohisotry_log.txt, 132 identical failures:

19:53:27 SEVERE ShotsHandler - Error getting paginated shots
### ArgumentError: Invalid argument(s): Profile must have a non-empty "steps" array
#0  new Profile.fromJson (profile.dart:52)
#1  new Workflow.fromJson (workflow.dart:68)
#2  ShotMapper.fromRow (shot_mapper.dart:12)
#9  DriftStorageService.getShotsPaginated (drift_storage_service.dart:95)
#10 ShotsHandler._getShots (shots_handler.dart:107)

The timeline pins the trigger to a de1app import:

19:38-19:52  GET /api/v1/shots  -> 200   (repeatedly, history works)
19:53:16     SafFolderCopier - Picked directory: de1plus ... Copied 6 files
19:53:27     GET /api/v1/shots  -> 500   (and every call after)

The poison row is named by the one single-shot failure: GET /api/v1/shots/de1app-1626149813 -> 500, while de1app-1785894912, -1785895243 and -1785895651 still return 200. Unix 1626149813 is July 2021, an old TCL-format .shot file; 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 analyzeNo issues found!
  • flutter test — 3868 passed, 8 failed, out of 3877 (the new suite added 2 tests). The 8 are test/webui_support/webui_token_injection_test.dart failing with SocketException: 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 clean origin/main with none of this branch's changes, and are unrelated to shots.
  • Regression suite test/database/shot_history_imported_profile_test.dart (11 tests), against an in-memory Drift database (the update tests drive the real ShotsHandler):
    • a stored profile with no steps reads back (reproduces de1app-1626149813);
    • a stored profile with no title reads back;
    • one step-less shot does not hide the rest of the history (the actual issue: the page returns all three shots instead of throwing);
    • PUT /api/v1/shots/de1app-1626149813 with 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);
    • the same for a title-less imported shot, which reads back as Unknown profile;
    • a row that cannot be mapped at all is skipped, not fatal;
    • getShot still surfaces the failure for a single unreadable id;
    • an unreadable row does not truncate the rest of a paged backup — drives the real pageShotsForExport (via import 'package:reaprime/main.dart' as app;) against a real Drift DB with pageSize: 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.fromJson still rejects a step-less profile while Workflow.fromRecordedJson accepts it;
    • Profile.fromJson still rejects an empty steps array and an empty title.
  • test/data_export/shot_export_section_test.dart gained "imports a recorded shot with a step-less imported profile (gh#784)", driving ShotExportSection.importJson directly with a de1app-style step-less-profile record — exercises the /backup restore endpoint specifically, since ShotImporter is a separate code path.
  • test/webserver/workflow_handler_test.dart caught a first attempt that made Workflow.fromJson lenient everywhere: PUT /api/v1/workflow with 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 through Workflow.fromRecordedJson, that suite is green (47 tests), and the new suite pins the boundary.
  • No hardware testing; this is a storage read path exercised entirely by the Drift tests.

Impact

Note any user-visible behavior, compatibility, migration, API/spec, documentation, or security impact. Write None if there is none.

  • Users who imported a de1app folder get their shot history back. GET /api/v1/shots returns 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.
  • A recorded profile with no title reads back as Unknown profile, and missing tank_temperature / target_volume_count_start read back as 0. These only apply to profiles embedded in stored shots.
  • No behavior change for the profile library or any write boundary: with requireExecutable: true every guard still fires before the new fallbacks, so Profile.fromJson is byte-for-byte the parser it was. PUT /api/v1/workflow still 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.
  • The app's /backup archive round-trips a recovered de1app shot: export via ShotExportSection.exportJson no longer truncates around an unreadable row, and ShotExportSection.importJson can restore a step-less-profile shot from that archive. ShotImporter (a separate, non-/backup code path) already handled this from the prior commit.
  • Recovered de1app shots are editable: annotations, notes and metadata can be written through PUT /api/v1/shots/<id> instead of returning 500.
  • No API, spec, schema, or migration changes. No REST or WebSocket surface was touched, so assets/api/rest_v1.yml and assets/api/websocket_v1.yml are unchanged.
  • doc/Profiles.md documents the strict/lenient split and when each parser applies. doc/AI_STORAGE_NOTES.md records 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 why pageShotsForExport re-queries past a dropped row instead of trusting the mapped page length.
  • The importers still store steps: []. That is the honest representation of a .shot file, and it now round-trips.

Contributor Responsibility

AI-assisted development is allowed. The submitter remains responsible for the submitted work.

  • I have reviewed and understand all changes in this PR and take responsibility for their correctness, security, behavior, licensing, and provenance, including any AI-assisted or AI-generated work.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KYQbrfYh9W4xTy2tsEya3m

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
tadelv self-requested a review September 4, 2026 11:49

@tadelv tadelv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread lib/src/services/database/mappers/shot_mapper.dart
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 tadelv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread lib/src/util/shot_importer.dart
Comment thread lib/main.dart Outdated
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 tadelv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread lib/main.dart Outdated
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

@tadelv tadelv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at f1f3d4b. The final AGENTS.md cleanup is fixed, the previous functional blockers remain addressed with regression coverage, and current PR checks are green. No remaining merge-blocking issues found.

@tadelv
tadelv merged commit 552d726 into main Sep 10, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] shot history can not be rendered in streamline.js

2 participants