Build timeline annotations through the Annotation dataclass on load

[gbeane]

Reasoning

I looked for a real bug first, and didn't find one I could confirm with high confidence, so I moved down the list to duplication and clarity. The strongest candidate was TimelineAnnotations.load() in src/jabs/project/timeline_annotations.py, which had three related problems in one ~50-line method:

1. Duplicated payload construction. load() hand-built the interval data dict (tag, color, description, identity, display_identity) and wrote annotations._tree[start : end + 1] = data directly — exactly what add_annotation() already does from an Annotation. Two copies of the same payload shape, in the same class, that can drift: adding a field to Annotation/add_annotation would silently not appear on annotations that came in through load(). The Annotation dataclass exists precisely to carry this.

2. The data parameter was rebound inside the loop that iterates it. Line 110 did data = {...} for the per-entry payload while the enclosing loop was for annotation in data:. This is not a live bug — for captures the iterator before the first rebind, so iteration completes correctly — but it is a trap: adding any second use of data after the loop (a length check, a retry pass, a log line) would read the last entry's payload instead of the input list.

3. Wrong docstring type. The parameter was documented as data (dict): The dictionary containing serialized timeline annotations while the signature is list[dict[str, Any]].

Why this is safe: add_annotation() writes self._tree[annotation.start : annotation.end + 1] = {...} with the same five keys and the same values that load() was writing inline, so the resulting interval tree is byte-for-byte identical. Validation, skip behavior, and the id_index_to_display fallback to str(identity_index) are all unchanged. Added test_load_stores_same_payload_as_add_annotation to pin the equivalence directly.

The one intentional behavioral difference is the warning destination: the module's print(..., file=sys.stderr) calls are now logger.warning(...) with lazy formatting, per CLAUDE.md's "Never use print() for operational output". Visibility is preserved in both entry points — the GUI configures logging.basicConfig(level=logging.WARNING) in gui_entrypoint.py, and the CLI has no handler configured, so Python's last-resort handler still emits WARNING and above to stderr.

Other candidates I considered and dropped: the unused frame_mask parameter on Feature._compute_window_feature/_compute_signal_features (removing it cascades into the identity arguments of _window_standard/_window_signal, which are protected methods a downstream feature subclass could plausibly override — more risk than a cleanup warrants); and signal_stats.psd_mean_band documenting freqs as "ignored" when it selects the band (real, but a one-line docstring fix, too thin to stand alone and unrelated to this module).

Change

Logic changes

• src/jabs/project/timeline_annotations.py

  • load() now constructs a cls.Annotation and calls annotations.add_annotation(...) instead of assembling the interval payload and writing to _tree directly.
  • Renamed the loop variable from annotation to entry so the data parameter is no longer shadowed; the per-entry dict is gone entirely.
  • Corrected the data param docstring to list[dict[str, Any]] and noted that start/end are inclusive.
  • Moved the "start and end are inclusive, so the interval ends at end + 1" note onto add_annotation(), which is where the + 1 now lives.
  • Replaced the four print() calls in load() and serialize() with a module-level logger and lazy %s formatting; dropped the now-unused import sys.

• tests/project/test_timeline_annotations.py — four new tests:

  • test_load_stores_same_payload_as_add_annotation — the two construction paths produce identical trees (this is the guard for the refactor).
  • test_load_continues_after_skipping_invalid_entry — a skipped entry doesn't stop later entries loading.
  • test_load_without_display_mapping_stringifies_identity — the str(identity_index) fallback when no mapping function is given.
  • test_load_warns_on_skipped_entry — skips are reported through the module logger.

Mechanical updates

None.

Verification

ruff check . and ruff format . clean. Full root suite: 831 passed, 200 skipped. No files under packages/ were touched.

---

This PR was produced by an automated analysis from Claude Code.

---

Generated by Claude Code

[Copilot]

Pull request overview

Refactors TimelineAnnotations.load() to build timeline intervals via the TimelineAnnotations.Annotation dataclass and the existing add_annotation() path, reducing duplicated payload construction and aligning load-time behavior with runtime additions.

Changes:

• Updated TimelineAnnotations.load() to construct Annotation objects and delegate interval creation to add_annotation().
• Replaced print(..., file=sys.stderr) warnings with module logging (logger.warning) in load() and serialize().
• Added tests to ensure load/add payload equivalence, continued loading after skipped entries, identity display fallback behavior, and warning emission via logging.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/jabs/project/timeline_annotations.py	Refactors load path to reuse Annotation + add_annotation() and switches warnings from print to logging.
tests/project/test_timeline_annotations.py	Adds tests covering payload equivalence, skip/continue behavior, display-identity fallback, and logging warnings.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.