WEBDEV-8974: Migrate the radio player search handler into elements - #85
Open
jbuckner wants to merge 2 commits into
Open
WEBDEV-8974: Migrate the radio player search handler into elements#85jbuckner wants to merge 2 commits into
jbuckner wants to merge 2 commits into
Conversation
Move the search subsystem out of @internetarchive/radio-player ahead of the
player itself. Pure logic, no DOM, so it reviews on its own rather than
riding along in the player's PR.
Brings over SearchHandler, TranscriptIndex, the search helper and models,
and both backends.
Bugs fixed on the way through:
- An empty query hung the browser. The scan loop is
`while ((i = haystack.indexOf(query, i + 1)) !== -1)`, and indexOf with an
empty needle clamps to the string length and never returns -1, so the loop
has no way to end. Confirmed in node: the index sticks at 32 for a 32
character haystack.
- The transcript index measured each entry's span with displayText but put
rawText into the merged string. Those differ for a music entry, whose
displayText is a placeholder standing in for usually-empty rawText, so it
claimed a span two dozen characters long that overlapped the entries after
it. Measured with rawText now.
- The merged string was trimmed after the spans had already been measured
against the untrimmed version. A transcript opening on music starts with
an empty entry, so the trim slid every span one character left and search
quietly returned nothing for the first entry. Nothing is adjusted after
measuring now.
- A match ending on an entry's last character got the wrong end time. The
span's end is exclusive, so looking it up landed on the space between
entries, found nothing, and fell back to the entry the match started in.
- Overlapping matches from a backend built backwards spans, and substring
reverses those silently, so text came back duplicated. Only the first of
an overlapping set is taken.
- Entries with no text of their own vanished from the transcript while a
search was running, taking their time codes with them, so the view lost
its music rows.
- The full text backend built its tags into a regular expression without
escaping. The default {{{ and }}} survive only because an invalid
quantifier is treated literally, and a tag holding a bracket would throw.
Range is TextRange here, since Range is a DOM global and this is re-exported
from the package root. TranscriptEntryRange and SearchResult are interfaces;
TextRange stays a class for its length getter. The response payload types
are declared rather than any.
Claude-Session: https://claude.ai/code/session_01U6iNyGicKJywdrWJMTyFin
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This was referenced Aug 27, 2026
…igrate-search-handler * WEBDEV-8968-migrate-transcript-view: WEBDEV-8972: Run CI on every PR, not just ones based on main (#90)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## WEBDEV-8968-migrate-transcript-view #85 +/- ##
=======================================================================
+ Coverage 81.58% 82.76% +1.17%
=======================================================================
Files 46 53 +7
Lines 1423 1555 +132
Branches 319 336 +17
=======================================================================
+ Hits 1161 1287 +126
- Misses 171 173 +2
- Partials 91 95 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 participants
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.
WEBDEV-8974. Stacked on #84, part of WEBDEV-8962.
Split out of the player's ticket because it's pure logic with no DOM in it, and putting the two together made a ~3,000 line PR.
src/elements/ia-radio-player/has no element in it yet — that's the next and last PR, which lands beside this.Seven bugs came over with it, all fixed and pinned by tests that fail without the fix:
while ((i = haystack.indexOf(query, i + 1)) !== -1), andindexOfwith an empty needle clamps to the string length and never returns-1, so the loop can't end. Confirmed in node — the index sticks at 32 for a 32-character haystack.displayText.lengthwhilerawTextwas what went into the merged string. For a music entry those differ ([Transcript unavailable]against a usually-emptyrawText), so it claimed a 24-character span overlapping the entries after it.substringreverses those silently, so text came back duplicated. Only the first of an overlapping set is taken now.RegExpunescaped. The default{{{/}}}survive only because JS treats an invalid quantifier as a literal; a tag containing a bracket would throw.RangeisTextRangehere — it's re-exported from the package root and shadowing the DOM'sRangeis a trap.TranscriptEntryRangeandSearchResultare interfaces;TextRangestays a class for itslengthgetter. The response payload types are declared rather thanany.CI shows only
deploy-previewhere because the PR isn't based onmain(WEBDEV-8972). Verified locally: build, full suite 380 passing, eslint and prettier clean, 95% coverage on this subsystem.