Skip to content

fix(media): resolve the 17 review findings from #313#327

Merged
garrity-miepub merged 13 commits into
mieweb:mainfrom
jlocala1:fix/media-review-findings
Jul 25, 2026
Merged

fix(media): resolve the 17 review findings from #313#327
garrity-miepub merged 13 commits into
mieweb:mainfrom
jlocala1:fix/media-review-findings

Conversation

@jlocala1

Copy link
Copy Markdown
Collaborator

Closes #323.

All 17 verified findings from the #313 Copilot review threads (each thread has a confirmation reply; the deferral-to-follow-up was agreed there). Four commits, grouped by root cause:

1. ae6ff78 — silence-newline blindness + filler double-count (findings 3–7, 16). A shared isSilenceType() helper, swept everywhere the code tested only 'silence': stats word/silence counts, splitSilence (newline silences are the longest gaps — the ones most worth splitting — and were the only ones you couldn't), WordEditorModal's flow gate, filler matchCounts pollution, clipboard copy of [10.2s] chips. Also fixes the filler double-count: default fillers were incremented in both the DEFAULT loop and the unconditional custom-word set, so UI counts (and "time saved") were exactly 2×.

2. b5b5471 — edit-state correctness (findings 1, 2, 13). The worst bug of the set: threshold rebuild saved deletions keyed by position in the old silence-inserted array but restored them by non-silence word ordinal — mismatched keyspaces, so changing silence thresholds after deleting words silently moved deletions onto the wrong words. Both phases now key by word ordinal. Plus: hasEdits stuck true after undo-to-baseline (compared against raw transcript.words length instead of the silence-inserted baseline), and the hasEdits initializer missing saved text-only edits (incl. ScriptPanel applies).

3. 1ecd1e7 — refs, perf, a11y, packaging (findings 8, 10, 12, 15, 17, 18, 9, 14). MediaPlayer error now clears on src change (it previously unmounted the media element and never retried a corrected src); external playerRef is a stable delegating proxy instead of a []-deps first-commit snapshot that could hold a stale handle forever; getSpeedAtIndex stops re-sorting already-sorted markers per call; originalIndex doc corrected (it indexes the silence-inserted timeline, not transcript.words — the confusion behind two other threads); segment rows drop the custom Enter/Space handlers on the native-<button> path (double-invocation); ScriptPanel's parseScript whitelists wordType values instead of casting arbitrary strings into the union; 22 missing opacity-modifier classes added to miewebUISafelist (MediaEditor + TranscriptView sweep).

4. c411f24 — regression tests. Six new tests covering the behavioral findings; checked each one against the pre-fix code — all six fail there, then pass with the fixes. (The existing 53 media tests caught none of these — that gap is why every fix ships with a test.)

Verification: pnpm format / pnpm lint / pnpm typecheck (the Lint job's three commands) all exit 0; 59/59 media tests pass (53 existing + 6 new).

jlocala1 and others added 4 commits July 23, 2026 09:48
Adds a shared isSilenceType() helper and sweeps every site that
compared against 'silence' alone — silence-newline entries were
counted as spoken words in stats, unsplittable, given the wrong
editor modal, polluting filler counts, and copied into the clipboard
as [Ns] chips. Also collapses the filler matchCounts/durations
double-count: DEFAULT_FILLER_WORDS entries were incremented once in
a defaults loop and again in the custom-word line, doubling both
occurrence counts and time-saved estimates.

Fixes findings 3-7 and 16 of mieweb#323.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…e, hasEdits init

Three review findings on the edit-state lifecycle:
- setSilenceThresholds saved deletions keyed by position in the old
  silence-inserted array but restored by word ordinal — a keyspace
  mismatch that applied deletions to the WRONG words after changing
  thresholds. Both sides now use the word ordinal.
- undo's isOriginal check compared against raw transcript.words; the
  baseline is the silence-inserted timeline, so with any detected
  silence hasEdits stayed true after undoing everything. Now compares
  against the derived baseline (including text equality).
- hasEdits initialized from deleted/inserted flags only, so saved
  text-only edits read as unedited and suppressed onChange. Now
  compares the saved state against the derived baseline.

Fixes findings 1, 2, and 13 of mieweb#323.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…, ScriptPanel validation

Remaining review findings:
- MediaPlayer: clear error state when src changes (a failed load
  trapped the UI even after the host supplied a good URL).
- MediaEditor: external playerRef is now a stable delegating proxy —
  the [] snapshot captured the first-commit handle and could leave
  hosts holding a stale ref with a null mediaElement.
- getSpeedAtIndex: drop the per-call copy+sort (markers are kept
  sorted at insert; stats calls this per word).
- transcript.ts: correct the originalIndex doc — it indexes the
  silence-inserted timeline, not transcript.words.
- TranscriptView segment rows: native button + custom Enter/Space
  handlers double-invoked; native activation only now.
- ScriptPanel: whitelist wordType values on parse instead of casting
  arbitrary strings into the union.
- tailwind-preset: add the 22 media-stack class variants missing from
  miewebUISafelist (TW3 safelist consumers).

Fixes findings 8, 10, 12, 14, 15, 17, 18 (+9) of mieweb#323.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Six tests encoding the verified failure modes: silence-newline stats,
filler single-counting, splitting newline silences, deletion identity
across threshold rebuilds, undo-to-baseline clearing hasEdits, and
hasEdits initialization on text-only saved edits. The pre-fix code
fails all six.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Copilot AI review requested due to automatic review settings July 23, 2026 14:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses the set of verified follow-up fixes from #313 (tracked in #323) across the media editing stack, focusing on correctness (silence handling + edit state), stability of refs/error recovery, performance, Tailwind safelisting, and adding regressions to lock behavior in.

Changes:

  • Introduces and adopts a shared isSilenceType() helper to correctly handle both silence and silence-newline across editing logic and stats.
  • Fixes edit-state correctness issues (including hasEdits baseline comparisons and deletion preservation across silence-threshold rebuilds) and improves getSpeedAtIndex performance by removing per-call sorts.
  • Adds targeted regression tests for the previously verified findings and expands the Tailwind safelist for media-stack classes.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/tailwind-preset.ts Adds Tailwind safelist entries needed by the media components’ opacity/state variants.
src/hooks/useTranscriptEdits.ts Fixes silence handling, edit-state baseline comparisons, threshold rebuild deletion preservation, and removes per-call marker sorting.
src/hooks/useTranscriptEdits.test.ts Adds regression tests covering the verified findings/fixes.
src/components/TranscriptView/TranscriptView.tsx Uses native <button> click handling for segment rows to avoid redundant activation handling.
src/components/TranscriptView/transcript.ts Adds isSilenceType() helper and corrects EditableWord.originalIndex documentation.
src/components/MediaPlayer/MediaPlayer.tsx Clears sticky error state when src changes to allow retry with corrected URLs.
src/components/MediaEditor/WordEditorModal.tsx Uses isSilenceType() so newline silences are treated as silences in the modal flow.
src/components/MediaEditor/ScriptPanel.tsx Validates wordType values instead of accepting arbitrary strings into the union.
src/components/MediaEditor/MediaEditor.tsx Fixes external playerRef to be a stable delegating proxy and updates silence filtering to include silence-newline.

Comment thread src/hooks/useTranscriptEdits.ts
Undo snapshots are captured against the current silence layout; after
setSilenceThresholds rebuilds the timeline, restoring one resurrects
silence chips that no longer match the active thresholds. Threshold
changes are documented as not undoable — drop the stale history.

Found by Copilot review on mieweb#327; regression test fails pre-fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Copilot AI review requested due to automatic review settings July 23, 2026 14:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Comments suppressed due to low confidence (1)

src/hooks/useTranscriptEdits.ts:150

  • getSpeedAtIndex is exported via src/hooks/index.ts, but it now assumes speedMarkers are sorted and breaks early. If a library consumer passes an unsorted marker array (which previously worked because this function sorted defensively), the effective speed can become incorrect.
  // Markers are kept sorted at insert (toggleSpeedMarker) — scanning without
  // a per-call copy+sort matters because stats calls this once per word.
  let effectiveSpeed = defaultSpeed;
  for (const marker of speedMarkers) {
    if (marker.wordIndex <= wordIndex) {
      effectiveSpeed = marker.speed;
    } else {
      break;
    }
  }

Copilot AI review requested due to automatic review settings July 23, 2026 20:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Copilot AI review requested due to automatic review settings July 23, 2026 21:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread src/hooks/useTranscriptEdits.ts Outdated
Comment thread src/hooks/useTranscriptEdits.ts
getSpeedAtIndex is exported publicly (src/hooks/index.ts). The perf fix
replaced its defensive copy+sort with an early-breaking scan, which
silently returns the wrong speed if a caller passes markers out of order.
Use a single O(n) pass that takes the nearest marker at or before the
index instead: same per-call cost as the scan (no allocation, no sort),
but correct for arbitrary order.

Found by Copilot review on mieweb#327; regression test fails on the previous
implementation.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Copilot AI review requested due to automatic review settings July 23, 2026 21:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/hooks/useTranscriptEdits.test.ts:328

  • This test title references “#327 review”, but the surrounding regression suite is for ui#323; consider updating the reference to avoid confusion when mapping tests to the originating issue/review thread.
  it('drops stale undo snapshots on a threshold rebuild (#327 review)', () => {

Comment thread src/hooks/useTranscriptEdits.test.ts
Copilot AI review requested due to automatic review settings July 24, 2026 01:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Copilot AI review requested due to automatic review settings July 24, 2026 17:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comment thread src/hooks/useTranscriptEdits.ts
…keyspace

setSilenceThresholds() saves deletion state keyed by ordinal among
non-silence words, but counted pasted words too (they keep their source
originalIndex). The rebuild re-inits from the original transcript with
no inserts, so a pasted word shifted every later ordinal and deletions
landed on the wrong words. Save now skips inserted words, matching the
restore keyspace. Regression test follows the review repro: copy A,
paste before C, delete C, change thresholds — C stays deleted.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Copilot AI review requested due to automatic review settings July 24, 2026 23:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/hooks/useTranscriptEdits.test.ts:121

  • The test name references "#327 review", but this PR/issue context is ui#323. This mismatch makes it harder to trace the regression back to the right thread/issue.
  it('is order-independent for external callers (#327 review)', () => {

src/hooks/useTranscriptEdits.test.ts:346

  • This test description references "#327 review", but the suite targets ui#323 regressions. Consider updating the reference to match the PR/issue being closed.
  it('keeps deletions on the right words across a threshold rebuild with pasted words (#327 review)', () => {

Comment thread src/hooks/useTranscriptEdits.test.ts
Copilot AI review requested due to automatic review settings July 24, 2026 23:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

The block collected ui#323 findings first and mieweb#327 review findings
since; the name now says so.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Copilot AI review requested due to automatic review settings July 24, 2026 23:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

@garrity-miepub
garrity-miepub merged commit b8e060f into mieweb:main Jul 25, 2026
8 checks passed
garrity-miepub pushed a commit that referenced this pull request Jul 25, 2026
Undo snapshots are captured against the current silence layout; after
setSilenceThresholds rebuilds the timeline, restoring one resurrects
silence chips that no longer match the active thresholds. Threshold
changes are documented as not undoable — drop the stale history.

Found by Copilot review on #327; regression test fails pre-fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
garrity-miepub pushed a commit that referenced this pull request Jul 25, 2026
getSpeedAtIndex is exported publicly (src/hooks/index.ts). The perf fix
replaced its defensive copy+sort with an early-breaking scan, which
silently returns the wrong speed if a caller passes markers out of order.
Use a single O(n) pass that takes the nearest marker at or before the
index instead: same per-call cost as the scan (no allocation, no sort),
but correct for arbitrary order.

Found by Copilot review on #327; regression test fails on the previous
implementation.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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.

Media stack: follow-up fixes for the 17 review findings on #313

3 participants