refactor: reuse over reinvention (PR 2/3) - #168
Merged
Conversation
EtienneLescot
force-pushed
the
chore/ponytail-1-dead-code
branch
from
July 26, 2026 18:23
371925e to
8c9b832
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
clamp(value, min, max) was defined privately in 16 files across the renderer, the electron main process and the native glue — byte-identical every time. src/utils/math.ts now owns it plus clamp01; electron already imports from src/, so one home serves both sides. Left alone deliberately: the domain-specific clamps (clampPlaybackSpeed, clampVirtualTime, clampFocusToStage, clampDeltaMs, clampSpanAgainstNeighbours) are real functions, not copies. useTimeline.ts redeclared the same three NaN-guards inside nine separate actions. They are floors, not clamps, so they are hoisted once as finiteSec/finiteMs/finiteFraction rather than folded into clamp. Also drops easeOutExpo, smoothStep and easeInOutCubic from mathUtils — exported, no caller. tsc clean; 94 files / 1108 tests pass.
uuid had five call sites across two files, all v4. crypto.randomUUID() is already used ten times elsewhere in the same tree — renderer runs in a secure context, main runs Node 22. Drops the dependency. findVcVarsAll() was duplicated byte-for-byte (modulo comments) between build-windows-wgc-helper.mjs and build-windows-compositor-addon.mjs, as was the promisified spawn. Both move to scripts/msvcEnv.mjs. The two runInVsEnv bodies genuinely differ (SDK lib compat shims vs cargo) and stay put. The two scripts drop from 197/187 lines to 99/93. Kept deliberately: `motion`. Its spring() is a real integrator (keyframes, velocity, stiffness, damping, restDelta, generator protocol) driving cursor smoothing; hand-rolling one to shed a dependency is writing code, not deleting it. tsc clean; 94 files / 1108 tests pass.
m:ss.t was hand-rolled in Modals (formatTc), V4Timeline (fmt) and operations (formatSec); the auto-hours variant existed twice, byte- identical, as MediaStage's formatTimecode and virtual-preview's formatSeconds. All five now come from timeline/format.ts as formatSec and formatSeconds, with formatMs delegating to formatSec. Left alone deliberately, because they are different formats rather than copies: ExportDialog's formatHms (hh:mm:ss, padded hours, no tenths) and timeUtils' formatTimePadded (mm:ss). formatSize was duplicated between MediaStage and LeftPanel; it moves to utils/formatBytes.ts. NOT converted to Intl.NumberFormat as the review suggested — `unit: "byte"` formats in decimal units and localises the unit name, so a French locale renders 1 GiB as "1,1 Mdo": wrong magnitude, unreadable label. The binary ladder stays. Adds format.test.ts covering the cases the six copies disagreed on (negatives, NaN, Infinity, the hour boundary). tsc clean; 95 files / 1113 tests pass.
sceneDescription hand-rolled a linear-gradient parse (strip parens, flat
comma split, first-token per stop) next to a comment admitting "presets
never contain nested parens". exporter/gradientParser's parseCssGradient
already does this and is strictly better: it tokenises rgba()/hsl() stops
that the flat split would have shredded, and resolveLinearGradientAngle
handles the "to bottom right" keyword directions the regex ignored. Same
180-degree default. All 73 sceneDescription tests pass unchanged.
editorSettings' nextLegacy listed 16 `if (patch.x !== undefined)` lines.
Note a plain {...base, ...patch} is NOT equivalent — it writes undefined
over values the caller left alone — so the keys are filtered first. Added
a test for exactly that; it fails against the naive spread.
tsc clean; 95 files / 1114 tests pass.
…dients
Three of the copies folded into the shared helpers were not byte-identical,
and the differences were the interesting part.
`clamp` — blurEffects and video-editor/types both guarded non-finite input;
the shared version did not, so `clamp(NaN, …)` returned NaN instead of a
floor. That reached the exporter: clampFocusToDepth feeds clampFocusToScale,
which frameRenderer and wgsl/evaluate use to build the zoom transform. The
guard now lives in the shared `clamp` (non-finite -> min), which is
blurEffects' old contract exactly and hardens the other 14 call sites for
free. Focus wants a midpoint rather than a floor — an unknown focus means
"centre" — so it keeps a two-line local, same convention as useTimeline's
`finiteFraction`.
`parseWallpaper` — parseCssGradient is anchored on a trailing ")", so it
rejects strings the `startsWith("linear-gradient(")` branch accepts; a
trailing space was enough. Returning null fell through to the image arm and
handed the native scene a CSS string as a file path. The branch is total
again: no parse means angle 180 and no stops, and the Rust side already
falls back to the layout background on empty stops.
Tests: the NaN cases at the shared definition and at both callers, plus the
rgba()/keyword-direction wins the swap was made for — untested until now —
and the reject path.
- `nextLegacy`'s cast was `Partial<LegacyShape>`, which accepts any key. Pick it down to the patch's own keys so an EditorSettingsPatch field with no legacy counterpart fails to compile, as the 16 `if`s used to (verified: a bogus key now errors TS2344). - build-windows-compositor-addon's `run()` picked up ROOT as its default cwd in the msvcEnv extraction; it was poc-d3d/, which is where cargo has to run for .cargo/config.toml's FFMPEG_DIR to apply. Only call site passes cwd explicitly so nothing broke, but the next one would have. - chat-service now imports randomUUID from node:crypto like codex-session next to it, rather than reaching for the global in main-process code. - Dead JSDoc for the deleted firstTokenOf, stacked on parseWallpaper's own and still claiming the presets never nest parens. Plus two import placements.
EtienneLescot
force-pushed
the
chore/ponytail-2-stdlib
branch
from
July 26, 2026 19:12
c5f501e to
af79e27
Compare
EtienneLescot
added a commit
that referenced
this pull request
Jul 27, 2026
…nto installers Two ways this release could ship broken, and one obligation it was missing. CI never ran on this branch. ci.yml triggered on [main, feat/ai-edition] only, so PRs #167, #168 and #169 — 30k+ lines of deletion and refactor — merged into release/1.8.0 with lint, typecheck, tests, build and the PR-title check all skipped, and CodeRabbit reporting "reviews are disabled for this base branch". Added release/** to both triggers. The installers had no speech-to-text. electron/native/bin/ is gitignored and build-whisper-stt.yml publishes the binaries as artifacts of its own run; nothing carried them into build.yml. The model is downloaded at runtime, the binary is not, so resolveBinaryPath() would find nothing and transcription and captions would fail with a "build it via scripts/build-whisper-stt.sh" message shown to end users. scripts/stage-whisper-stt.sh now fetches them in all four build legs and fails the build if they are missing — a red build beats a silent one. Artifact retention 30 -> 90 days, since a release build now depends on it. Added THIRD-PARTY-NOTICES.md and shipped it (with LICENSE) into resources/. We redistribute FFmpeg's LGPL shared libraries and whisper.cpp; fetch-ffmpeg.mjs went to real lengths to keep the build LGPL rather than GPL, but nothing carried the attribution or the source offer into the installer. Also stops shipping the static ffmpeg.exe on Windows ("!win32-*/ffmpeg.exe"): the compositor addon dlopens the av*.dll set, but no runtime code spawns the exe — it is there for bench-export.mjs. It was adding a large binary and an LGPL obligation to every installer for nothing. Its licence-gate comment also still pointed at electron/media/ffmpegCapabilities.ts, deleted with the web export path. tsconfig.test.json makes the untypechecked half of the repo visible: tsconfig.json includes only src + electron and excludes **/*.test.ts, so no test file has ever been typechecked. It runs as an advisory CI job — 82 pre-existing errors, so gating on it today would just be red.
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.
Second of three from the over-engineering review. Stacked on #167 — review that one first; this PR's base is its branch, so the diff here is only the refactor work.
Small net (−117 lines) by design: this is deduplication, not deletion. The win is one definition instead of sixteen.
Verification
tsc --noEmitvitest --runTests went up (1108 → 1114): three of these refactors touch logic subtle enough to deserve a pinned case.
What changed
clamp× 16 → 1. Byte-identical private copies across the renderer, electron main and the native glue. Nowsrc/utils/math.ts— electron already imports fromsrc/, so one home serves both. The domain-specific ones (clampPlaybackSpeed,clampVirtualTime,clampFocusToStage,clampDeltaMs) are real functions and stay.useTimeline.tsalone redeclared the same three NaN-guards in nine separate actions; those are floors, not clamps, so they're hoisted asfiniteSec/finiteMs/finiteFractionrather than folded in. Also dropseaseOutExpo/smoothStep/easeInOutCubic— exported, no caller.Six timecode formatters → two.
m:ss.twas hand-rolled inModals(formatTc),V4Timeline(fmt) andoperations(formatSec); the auto-hours variant existed twice byte-identical asMediaStage.formatTimecodeandvirtual-preview.formatSeconds. All now come fromtimeline/format.ts. Newformat.test.tscovers the cases the six copies actually disagreed on — negatives, NaN, Infinity, the hour boundary.uuid→crypto.randomUUID(). Five call sites, allv4;crypto.randomUUID()was already used ten times in the same tree. Dependency dropped.One MSVC bootstrap.
findVcVarsAll()was duplicated byte-for-byte between the two Windows build scripts, as was the promisifiedspawn. Both move toscripts/msvcEnv.mjs; the tworunInVsEnvbodies genuinely differ (SDK lib compat shims vs cargo) and stay put. The scripts drop from 197/187 lines to 99/93.sceneDescriptionreusesparseCssGradient. It hand-rolled a linear-gradient parse next to a comment admitting "presets never contain nested parens". The shared parser is strictly better: it tokenisesrgba()/hsl()stops the flat split would have shredded, andresolveLinearGradientAnglehandles theto bottom rightkeyword directions the regex ignored. Same 180° default. All 73sceneDescriptiontests pass unchanged.Three review findings I did not apply
Each was in the original report; each turns out to be a bad trade on inspection.
formatSize→Intl.NumberFormat.unit: "byte"formats in decimal units (1 kB = 1000 B, not 1024) and localises the unit name — on a French locale 1 GiB renders as1,1 Mdo. Wrong magnitude, unreadable label. The binary ladder stays; the two copies are merged intoutils/formatBytes.tsinstead.captionBackgroundCss→ 8-digit hex. Its output flows throughcues.ts:257into the native scene JSON, so changingrgba()to#rrggbbaarisks the Rust-side parser for ~10 lines. Not worth it.motion'sspring(). A real integrator (keyframes, velocity, stiffness, damping, restDelta, generator protocol) driving cursor smoothing. Hand-rolling one to shed a dependency is writing code, not deleting it.A note on
editorSettingsnextLegacylisted 16if (patch.x !== undefined)lines. A plain{...base, ...patch}is not equivalent — it writesundefinedover values the caller left alone — so the keys are filtered first. I added a test for exactly that case and confirmed it fails against the naive spread before keeping the guarded version.Moved to PR 3
ModalShell→@radix-ui/react-dialog(8 call sites) and theLangButton→Popoverswap. Both replace hand-rolled focus/Escape/outside-click handling, which is a real runtime behaviour change to live dialogs rather than a like-for-like dedup. They belong with the other behaviour-changing work, where they can be verified against the running app.