Removed dead code and consolidated repeated helpers - #53
Merged
Conversation
src/lib/payload.ts and src/lib/annotations.ts have had no importer in src/ since #8 and #41 respectively replaced the flows that used them; only their own unit tests kept them reachable. Deleted both, along with the types in lib/types.ts that existed solely to serve them (the Extracted* union, the annotations document shapes, PayloadMetadata, OutputKind, ImageFormat and PackagingType). TrimMode stays, being what lib/ffmpeg.ts and lib/extract.ts read. Also removed two other exports nothing references at all — format.ts's blobSize and testInjection.ts's testInjectionOrInert with its INERT constant — and moved archives.ts's "A video file in the archive" comment back onto ArchiveVideo, which a reorder had left it detached from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeLYzRaTJ7oFj6fmAYmC8x
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeLYzRaTJ7oFj6fmAYmC8x
|
All four were reachable only from their own unit tests, which went with them: - sanitize.ts's sanitizePath, which was the same algorithm as bidsPath.ts's behAssetPath, the one the delivery path actually calls - sanitize.ts's sanitizeFilename, left behind once every filename the app writes came to be built from BIDS entities instead - blur.ts's hitRegion, a geometric point-in-circle test the blur tool does not use — it hit-tests through the focusable rings in the DOM - the sourceName field on ExtractFrameParams and ExtractOverlayParams, which main.ts filled in but neither function ever read. Kept on ExtractClipParams, which reads it for the source's extension. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeLYzRaTJ7oFj6fmAYmC8x
Five shapes that had been written out more than once, now written once: - lib/etag.ts drove the same 16MB chunk loop, short-read guard and all, three times over. All three hashes are now one pass of `eachChunk`, differing only in which digest the bytes go into. - The NFKD-then-strip-combining-marks fold opening both `sanitizeSegment` and `bidsLabel` is now `foldDiacritics`. - The libx264/faststart argument list is now `X264_MP4_ARGS`, shared by lib/ffmpeg.ts's own encodes and lib/extract.ts's overlay. `-an` stays out of it and is added by the paths that have audio to drop: the overlay's input is a PNG sequence, and the command string is quoted verbatim as that file's `encoding` in its sidecar. - Decoding one sample to read its pixel format is now `decodedPixelFormatAt`, called by lib/videoFormat.ts and by lib/streaming.ts, which passes the frame its own index already names. - The dandiset-listing response shape, its title fallback and the next-page cursor are now declared once in lib/api.ts rather than copied between lib/dandisets.ts and lib/embargoed.ts. lib/upload.ts's own next-page handling is left alone deliberately: it strips the API prefix with `replace` rather than a prefix check, which differs from `nextPagePath` on a foreign URL that happens to contain the API base, so sharing the helper there would not be behavior-neutral. Verified against the full integration suite, including the four expectedOutput specs that byte-compare every emitted sidecar and dataset_description.json. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeLYzRaTJ7oFj6fmAYmC8x
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #53 +/- ##
==========================================
+ Coverage 83.92% 84.03% +0.10%
==========================================
Files 42 40 -2
Lines 1910 1810 -100
Branches 487 453 -34
==========================================
- Hits 1603 1521 -82
+ Misses 307 289 -18
🚀 New features to boost your workflow:
|
Nothing moves out of the file and no signature changes; these are its own repetitions written once: - browseEmpty is now browseNote with the list cleared first, which is all it ever was. - The three status-line setters share applyHintClass, so the classes showsOutcome reads back cannot drift between them. - Every JSON a delivery writes — the extract's sidecar, the companions' and the three dataset_description.json files — goes out through deliverJson rather than four copies of serialize, hash and hand over. - pictureTechnicalFields picks BEP047's image or video keys once, for both the extract and its pose overlay. - The trim track's ruler and the overview's share appendTick. Their at-end thresholds differ (0.96 and 0.97) and are preserved exactly, passed in as RulerStyle.atEnd; whether that difference is deliberate is asked in #54 rather than settled here. - The dozen nested Math.max/Math.min clamps now call timeline.ts's own clamp, which was already there and private. - Two reads of `?test`'s faked counts, fakedDatasetCount and fakedListingCount, replace five spellings of the same null check. - wireSeg reads a button's dataset value once per click instead of twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeLYzRaTJ7oFj6fmAYmC8x
The three synthesizers in lib/testInjection.ts each set up their own canvas and each drew their own frame; both are now mockCanvas() and paintMockFrame(), with the hue step and the caption passed in since those are the only parts that actually differed. Also simplified the two nested ternaries deriving mock_video's and mock_video_long's frame counts — intParam has already dealt with anything unparseable, so only a deliberate `=0` is left to reject — and wrote FROM_EMBER_PATH_PREFIX as the literal it always evaluated to rather than running pad2 over two constants. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeLYzRaTJ7oFj6fmAYmC8x
The rings over the picture, the controls beside them and the bookkeeping keeping the two in step were 275 lines in the middle of main.ts. They are now their own module, reached through createBlurTool(els, host). A pure move: every function keeps its body, its name and its comment, and the order blurChanged does its work in is preserved exactly, since the delivery card reads the areas as it re-derives itself. The areas themselves stay in state.blurRegions rather than moving into the tool — the player draws them, the delivery card decides on them and every extraction carries them, so the tool mutates the one array all of those already see. What it needed from the page around it (whether a video is loaded, whether a delivery is running, whether the human-subjects gate offers the tool at all, and what to retire when the areas change) is the BlurToolHost it is handed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeLYzRaTJ7oFj6fmAYmC8x
The pane was 499 lines in the middle of main.ts and reached the rest of the page through four names: browseSay, refreshBrowse, syncBrowseToAuth, and the browse state itself as a "has this been read yet" check. Those are now say, refresh, syncToAuth and opened on the object createBrowsePane returns. A pure move, function for function — the only body to change is listOwnedEmbargoed's, which asked the archive for the signed-in username and cached it in main.ts's own currentUser. That caching stays where the header avatar and the provenance record can share it, and the pane asks for the answer through host.username instead. What it needs from the page around it is the BrowsePaneHost it is handed: the sign-in state, a config to call the archive under, the player a picked video opens in, and the stage that answers for a video asked for anywhere else. browseFailure stays in main.ts beside stageFailure, since the two are a pair — a refusal is said in whichever of the two places the video was asked for, and never in both. main.ts is 3199 lines, down from 3919 before this pass began. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeLYzRaTJ7oFj6fmAYmC8x
CodyCBakerPhD
marked this pull request as ready for review
August 29, 2026 20:02
CodyCBakerPhD
commented
Aug 29, 2026
Per review. package.json stays at 1.4.2, so the entry under that heading is the author's to write. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeLYzRaTJ7oFj6fmAYmC8x
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
3 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.
A functionality-preserving code quality pass, from a review of the whole project. No behavior changes, no public API changes, no config or runtime changes.
Applied in six groups, one commit each, so any group can be reviewed or reverted on its own.
1 — Dead code.
src/lib/payload.tsandsrc/lib/annotations.tshad no importer insrc/since #8 and #41 replaced the flows that used them; only their own unit tests kept them reachable. Both are gone, with thelib/types.tstypes that existed solely to serve them. Also removedformat.ts'sblobSizeandtestInjection.ts'stestInjectionOrInert/INERT, which nothing referenced at all.2 — Four more unused exports.
sanitizePath(the same algorithm asbidsPath.ts'sbehAssetPath, which is the one actually called),sanitizeFilename,blur.ts'shitRegion, and thesourceNamefield onExtractFrameParams/ExtractOverlayParamsthatmain.tsfilled in but neither function read.3 —
lib/consolidation.etag.tsdrove the same 16MB chunk loop three times, now oneeachChunk. The NFKD accent fold shared bysanitizeSegmentandbidsLabelisfoldDiacritics. The libx264 argument list isX264_MP4_ARGS, shared with the overlay encode. Decoding one sample to read its pixel format isdecodedPixelFormatAt. The dandiset-listing response shape, its title fallback and the next-page cursor are declared once inapi.ts.4 —
main.ts's own repetitions.browseEmptyisbrowseNotewith the list cleared first. The three status setters shareapplyHintClass. Every JSON a delivery writes goes out throughdeliverJson.pictureTechnicalFieldspicks BEP047's image or video keys once. Both rulers shareappendTick. The dozen nestedMath.max/Math.minclamps calltimeline.ts'sclamp, which was already there and private.5 —
testInjection.ts. The three mock synthesizers sharemockCanvasandpaintMockFrame; two nested ternaries and apad2over literals simplified.6 — Decomposition. The blur tool (275 lines) and the EMBER browse pane (499 lines) are now
ui/blurTool.tsandui/browsePane.ts, reached through a host object rather than module globals.main.tsis 3,199 lines, down from 3,919.Deliberately not done
lib/upload.tskeeps its own next-page handling. It strips the API prefix withreplacerather than a prefix check, which differs fromnextPagePathon a foreign URL containing the API base, so sharing the helper there would not be behavior-neutral.at-endthresholds (0.96 and 0.97) are preserved exactly and passed in asRulerStyle.atEnd. Whether that difference is deliberate is asked in The two rulers use different at-end thresholds for the same label alignment #54.selection.spec.ts's ruler gradation count, reproduced on unmodifiedmain(3 failures in 12 runs at--workers=4). Its cause — the mock clip is recorded in real time — is untouched by this PR.Verification
At every group:
npm run typecheck,npm run lint,npm test(563 passing),npm run build,pre-commit run --all-files. Groups 3, 4, 5 and 6 additionally ran the full Playwright suite (76 passing), which includes the fourexpectedOutputspecs that byte-compare every emitted sidecar anddataset_description.jsonagainst committed fixtures.Original prompt
Role: You are a senior software engineer performing a rigorous, functionality-preserving code review of this entire project. Your goal is to identify concrete opportunities to improve code quality, maintainability, and clarity without changing any observable behavior or public API.
Hard Constraints (do not violate)
Do NOT alter functionality. No behavior changes, no bug "fixes" that change outputs, no feature additions.
Do NOT change public/exported APIs, signatures, or contracts that external callers or other modules rely on.
Do NOT modify configuration, environment behavior, or runtime side effects.
Every proposed change must be behavior-neutral and independently verifiable.
If you're uncertain whether something is truly dead or safe to change, flag it as "needs verification" rather than assuming.
Scope of Analysis
Systematically review the whole project for:
Dead / unreachable code
Unused functions, methods, classes, variables, constants, imports, and files
Unreachable branches, redundant conditionals, code after early returns
Commented-out code blocks that should be removed
Feature-flagged or legacy paths that are provably never executed
Duplication & consolidation
Repeated logic that can be extracted into a shared internal helper
Near-identical functions that can be merged or parameterized
Copy-pasted blocks, redundant utility implementations
Internal function/structure cleanup
Overly long functions that can be decomposed (internally, without changing behavior)
Poor naming that reduces readability
Unnecessary indirection or wrapper functions that add no value
Inconsistent patterns for the same operation across the codebase
Minor safe improvements
Redundant type conversions, unnecessary allocations, obvious no-ops
Simplifiable boolean/conditional expressions
Unused dependencies in the manifest/package files
Method (think step by step)
First, map the project structure and identify entry points, public surface, and module boundaries so you can reason about what is reachable and what is internal.
For each candidate issue, trace usages across the whole codebase to confirm it is genuinely dead / duplicated / internal before proposing a change.
Rank findings by confidence (High/Medium/Low that the change is safe) and impact.
Output Format
Produce a structured report:
Summary: high-level overview of code health and top themes.
Findings table/list, each with:
ID & category (dead code / duplication / consolidation / cleanup)
Location (file + line/function)
Description of the issue
Proposed change (concrete, minimal, behavior-neutral)
Confidence it's safe (High/Medium/Low) + reasoning
Verification step (e.g., which tests cover it, or how to confirm no callers)
"Needs Verification" section: items you suspect but cannot prove safe alone.
Recommended order of changes, grouped so each group can be applied and tested independently.
Do not apply any changes yet — deliver the review first for approval. When changes are approved, apply them in small, independently reviewable, behavior-preserving commits.
Sure, one at a time in your recmomended order
if its unused at all, but there are tests, then its basically dead code any should be removed