fix(viewers): report a legacy capture directory, and inline the timeline assets - #22
Merged
Merged
Conversation
…ommand `generate_benchmark_html` wrapped `load_real_capture_data` in `except (FileNotFoundError, ValueError, KeyError)` to sniff which of two formats `data_path` holds. `LegacyCaptureError` is a subclass of `FileNotFoundError`, so the clause caught it too and fell through to `load_benchmark_data`, which reads a recording directory as a benchmark result directory with no results in it. The user pointed `openadapt-viewer benchmark --data DIR` at a pre-2026-07-17 capture, read `Generated: ...`, and opened a viewer holding zero tasks. The conversion command `recording_db` raises was in the exception nobody saw. `LegacyCaptureError` is re-raised ahead of the broad clause. The inheritance stays: the absence of a readable recording.db is a FileNotFoundError, and the CLI's top-level handler and the scanner's per-directory handler both report it correctly through that base class without an extra except clause. Its docstring now states the obligation this defect broke -- a caller catching FileNotFoundError to mean "try another format" must re-raise this class first. `_legacy_capture` moves from tests/test_scanner_recording_db.py to tests/capture_examples.py, so the two test modules that need a legacy directory build the same one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every generated capture page carried:
<link rel="stylesheet" href="src/openadapt_viewer/styles/episode_timeline.css">
<script src="src/openadapt_viewer/components/episode_timeline.js"></script>
Both resolve against the directory the output HTML lands in, so they loaded
only when that file happened to sit at the root of a source checkout. The two
files ship inside the installed package, so anyone who ran
`pip install openadapt-viewer` got two 404s and an episode timeline that was
unstyled and did nothing.
They are now read with `importlib.resources` and inlined, which is how the
page already carries core.css and how PageBuilder already carries its
JavaScript. `importlib.resources` rather than a path built from `__file__`
because the lookup has to answer for an installed distribution, which is the
case that was broken. The script keeps its place in `<head>` without `defer`:
it defines the `EpisodeTimeline` class the page constructs during Alpine's
`init`.
An asset that cannot be read prints a warning and is omitted, rather than
failing the whole page or degrading in silence.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
📸 Generated Screenshots PreviewScreenshots have been generated. Download the artifacts to preview them. Generated Files:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects in generator code, in one PR because they sit in the same corner of the repo and both are small.
1. A legacy capture directory generated an empty viewer
Point
openadapt-viewer benchmark --data DIRat a pre-2026-07-17 capture (acapture.dbplusepisodes.json, norecording.db) and it printedGenerated: ...and wrote a viewer holding zero tasks. No error, no hint.generate_benchmark_htmlworks out which of two formatsdata_pathholds by trying the capture loader and catching(FileNotFoundError, ValueError, KeyError).LegacyCaptureErroris a subclass ofFileNotFoundError, so that clause caught it and fell through toload_benchmark_data, which reads a recording directory as a benchmark result directory with no results in it. The migration command thatrecording_dbputs in the exception message never got out.It now re-raises ahead of the broad clause.
Before:
After:
Why
LegacyCaptureErrorstill subclassesFileNotFoundErrorThat inheritance is what made the defect possible, so I looked at whether to drop it. I kept it, because every other place in the repo that catches
FileNotFoundErrorwants precisely what it gives:cli.py:295, the benchmark command's handler. It prints the message and exits 1. That's the "After" output above, and it reads correctly today only because of the base class.scanner.py, whose per-directory clause isexcept (OSError, sqlite3.Error, ValueError). A bad directory skips, the scan carries on.scripts/generate_readme_screenshots.py:120, which turns it into aScreenshotGenerationErrornaming the capture.tests/test_scanner_recording_db.pyassertpytest.raises(FileNotFoundError, match="migrate_legacy_capture.py").Narrowing the base class fixes one call site and breaks four. The thing that was actually wrong is using a broad exception clause as a format test. The class docstring now says so: a caller catching
FileNotFoundErrorto mean "try the other format" has to re-raise this one first.2. Generated pages pointed at paths inside a source checkout
generate_capture_htmlwrote this into every page:Both resolve against wherever the output HTML lands, so they only ever loaded when that file happened to sit at the root of a checkout. Both files ship inside the installed package. For anyone who ran
pip install openadapt-viewer, they were two guaranteed 404s, and the episode timeline came out unstyled and inert.They're now read with
importlib.resourcesand inlined, which is how the same page already carriescore.cssand howPageBuilderalready carries its JavaScript.importlib.resourcesrather than a path built from__file__, because the lookup has to answer for an installed distribution, and that is the case that was broken. The script keeps its position in<head>with nodefer: it defines theEpisodeTimelineclass the page constructs inside Alpine'sinit.An asset that cannot be read prints a warning and is left out, rather than failing the page or degrading in silence.
Tests
Both regression tests were written first and watched fail on
main:A companion test covers the fallback that stays: a benchmark result directory still loads through it.
_legacy_capturemoved out oftests/test_scanner_recording_db.pyintotests/capture_examples.py, so both modules that need a legacy directory build the same one.Verified against an installed wheel
The reproduce case for defect 2 needs an install, not a checkout, so I built the wheel and ran the generator from an empty directory:
The only remaining external reference in that page is the Alpine.js CDN tag, which predates this change.
ruff check .passes over the whole repo. Full suite: 193 passed, 45 skipped, with the openadapt-capture example recordings checked out.