Revert webpack support #560
Conversation
This reverts commit 9c42e0d.
WalkthroughReplaces dynamic service-worker-based worklet/worker loading with Vite worker URL imports across audio and video paths, removes the loadAudioWorklet hack, updates package manifests, and expands watch/audio API to include latency, captions, and speaking while initializing the render worklet via AudioContext.addModule. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as App
participant Audio as Audio (watch)
participant AC as AudioContext
participant RW as RenderWorklet (module)
participant AWN as AudioWorkletNode
App->>Audio: new Audio(stream, props{ latency, captions, speaking })
Audio->>AC: create AudioContext
Note over Audio,AC: Vite-only import of render worklet URL
Audio->>AC: audioWorklet.addModule(RenderWorklet URL)
AC-->>Audio: module loaded
Audio->>AC: new AudioWorkletNode("render")
Audio->>AWN: postMessage({ type:"init", latency })
par Subsystems
Audio->>Audio: this.captions = new Captions(...)
Audio->>Audio: this.speaking = new Speaking(...)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @kixelated's task —— View job Code Review - Revert Webpack SupportTodo List:
Review SummaryI've reviewed this revert PR that removes webpack support functionality. The changes look clean and well-executed. Here are my findings: ✅ No Issues FoundChanges Analysis:
Security & Code Quality:
The revert appears complete and doesn't leave any webpack-specific code artifacts behind. The change from the complex service worker workaround back to direct Vite worker imports actually improves code maintainability. 👍 This revert is safe to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
js/hang/src/publish/video/detection.ts (2)
55-56: Minor: name the worker and add an error handler for easier debugging.Apply within these lines:
-const worker = new Worker(WorkerUrl, { type: "module" }); +const worker = new Worker(WorkerUrl, { type: "module", name: "detection" });Add right after creation (outside this hunk):
worker.addEventListener("error", (e) => console.error("detection worker error:", e.error ?? e.message) );
71-84: Avoid unhandled rejections on teardown by racing detect with cancellation.Current flow schedules
process()afterready(), but if the effect cleans up whileapi.detect()awaits, terminating the worker can throw. Wrap each detect in a cancel race and guard rescheduling:effect.spawn(async (cancel) => { const ready = await Promise.race([api.ready(), cancel]); if (!ready) return; const process = async (): Promise<void> => { const frame = this.video.frame.peek(); if (!frame) return; try { const cloned = frame.clone(); const res = (await Promise.race([ api.detect(Comlink.transfer(cloned, [cloned]), this.#threshold), cancel, ])) as Catalog.DetectionObjects | void; if (!res) return; // canceled this.objects.set(res); this.#track.writeJson(res); timeout = setTimeout(process, this.#interval); } catch (err) { // Swallow errors if shutting down if (await Promise.race([Promise.resolve(true), cancel])) return; console.error("detection detect error:", err); } }; process(); });js/hang/src/publish/audio/captions.ts (1)
96-96: Harden worklet loading with error handling.
addModule()can reject (network error, bad URL, blocked context). Catch and surface a user-friendly message; keep state consistent.- await ctx.audioWorklet.addModule(CaptureWorklet); + try { + await ctx.audioWorklet.addModule(CaptureWorklet); + } catch (err) { + console.error("failed to load capture worklet:", err); + this.text.set(undefined); + return; + }js/hang/src/publish/audio/index.ts (2)
135-135: WrapaddModulein try/catch to avoid silent failures.- await context.audioWorklet.addModule(CaptureWorklet); + try { + await context.audioWorklet.addModule(CaptureWorklet); + } catch (err) { + console.error("failed to load capture worklet:", err); + return; + }
139-140: GuardchannelCountfor Firefox/unspecified values.
settings.channelCountcan be undefined; passingundefinedintoAudioWorkletNodeOptions.channelCountand later arithmetic (bitrate) risks errors/NaN. Default to 1 when absent.- channelCount: settings.channelCount, + channelCount: settings.channelCount ?? 1,And later in config:
- numberOfChannels: u53(settings.channelCount), - bitrate: u53(settings.channelCount * 32_000), + numberOfChannels: u53((settings.channelCount ?? 1)), + bitrate: u53(((settings.channelCount ?? 1) * 32_000)),js/hang/src/publish/audio/speaking.ts (1)
87-87: Add error handling around worklet registration.- await ctx.audioWorklet.addModule(CaptureWorklet); + try { + await ctx.audioWorklet.addModule(CaptureWorklet); + } catch (err) { + console.error("failed to load capture worklet:", err); + return; + }js/hang/src/watch/audio/index.ts (6)
10-12: Fix prop docs: Speaking/Captions take props, not booleans.The comments suggest boolean flags, but the types are
CaptionsPropsandSpeakingProps. Adjust the docs to avoid misuse.- // Enable to download the speaking track. (boolean) + // Enable to download the speaking track.Also applies to: 23-25
17-19: Align latency prop semantics with usage (ms) and wire it into AudioContext.Comment says “latency hint for AudioContext” but code fixes it to
"interactive"and only passeslatencyto the worklet. Either update docs or actually use it. Suggest using numericlatencyHint(seconds) derived fromthis.latency(ms).- // The latency hint to use for the AudioContext. - latency?: DOMHighResTimeStamp; + // Target end-to-end buffer latency in milliseconds (used by the render worklet and as AudioContext latencyHint). + latency?: DOMHighResTimeStamp; @@ - const context = new AudioContext({ - latencyHint: "interactive", + const context = new AudioContext({ + latencyHint: this.latency / 1000, // seconds sampleRate, });Also applies to: 89-92
86-88: Comment is stale relative to code path.We don't create an AudioContext when disabled. Update the comment to avoid confusion.
- // NOTE: We still create an AudioContext even when muted. - // This way we can process the audio for visualizations. + // NOTE: We create an AudioContext only when enabled. + // Consumers can connect the node to speakers/analyzers as needed.
89-95: Expose the actual sampleRate via the signal.
#sampleRateis never set; exposecontext.sampleRateso readers don’t seeundefined.const context = new AudioContext({ latencyHint: this.latency / 1000, // seconds sampleRate, }); effect.cleanup(() => context.close()); + effect.set(this.#sampleRate, context.sampleRate);
95-103: Specify AudioWorkletNode channel config to match the stream.Without explicit options, the node defaults to stereo which can cause unwanted mixing when
channelCount !== 2. Also clear theonmessagehandler on cleanup.- // Create the worklet node - const worklet = new AudioWorkletNode(context, "render"); - effect.cleanup(() => worklet.disconnect()); + // Create the worklet node with explicit channel configuration + const worklet = new AudioWorkletNode(context, "render", { + numberOfInputs: 0, + numberOfOutputs: 1, + outputChannelCount: [channelCount], + channelCount, + channelCountMode: "explicit", + channelInterpretation: "discrete", + }); + effect.cleanup(() => { + worklet.port.onmessage = null; + worklet.disconnect(); + });Optional: if you want processing to run even when nothing else connects, attach a silent sink:
const sink = context.createGain(); sink.gain.value = 0; worklet.connect(sink).connect(context.destination); effect.cleanup(() => sink.disconnect());
149-167: Add minimal backpressure to avoid unbounded decode queue if the context isn’t running.If the node isn’t connected or the context is suspended,
decodeQueueSizecan grow. Add a soft cap.- decoder.decode(chunk); + decoder.decode(chunk); + // Soft backpressure: yield if we get too far ahead. + if (decoder.decodeQueueSize > 24) { + await new Promise((r) => setTimeout(r, 0)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
js/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
js/hang/package.json(1 hunks)js/hang/src/publish/audio/captions.ts(2 hunks)js/hang/src/publish/audio/index.ts(2 hunks)js/hang/src/publish/audio/speaking.ts(2 hunks)js/hang/src/publish/video/detection.ts(2 hunks)js/hang/src/util/hacks.ts(0 hunks)js/hang/src/watch/audio/index.ts(3 hunks)js/package.json(0 hunks)
💤 Files with no reviewable changes (2)
- js/package.json
- js/hang/src/util/hacks.ts
🧰 Additional context used
📓 Path-based instructions (2)
{rs,js}/hang/**
📄 CodeRabbit inference engine (CLAUDE.md)
All media logic belongs in the hang layer (rs/hang and js/hang)
Files:
js/hang/src/publish/video/detection.tsjs/hang/src/publish/audio/captions.tsjs/hang/src/publish/audio/index.tsjs/hang/src/publish/audio/speaking.tsjs/hang/src/watch/audio/index.tsjs/hang/package.json
js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place JS/TS packages under the js/ directory
Files:
js/hang/src/publish/video/detection.tsjs/hang/src/publish/audio/captions.tsjs/hang/src/publish/audio/index.tsjs/hang/src/publish/audio/speaking.tsjs/hang/src/watch/audio/index.tsjs/hang/package.json
🧠 Learnings (1)
📚 Learning: 2025-08-13T21:30:35.001Z
Learnt from: CR
PR: kixelated/moq#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-13T21:30:35.001Z
Learning: Applies to js/pnpm-workspace.yaml : Use pnpm workspaces for JS/TS; manage package layout via js/pnpm-workspace.yaml
Applied to files:
js/hang/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (8)
js/hang/src/publish/video/detection.ts (1)
7-8: Ambient Module Declaration Exists – No Further Action NeededThe project already includes an ambient declaration for
*?worker&urland the tsconfig covers it, so TypeScript will resolve these imports without errors.• File js/hang/src/vite-env.d.ts declares:
// Add support for worklet imports declare module "*?worker&url" { const workerUrl: string; export default workerUrl; }• js/hang/tsconfig.json extends the base and includes
"src", ensuring the above.d.tsis picked up.You can safely ignore the original comment—no additional types or tsconfig changes are required.
js/hang/src/publish/audio/captions.ts (1)
7-7: Vite-only import: verify TS support for?worker&url.Same concern as the video detection worker URL. Ensure
vite/clienttypes or an ambient module for*?worker&urlexist to keeptsc --noEmitgreen.Use the same script shared in detection.ts to verify.
Reference: Vite docs on worker URL query. (main.vitejs.dev)js/hang/package.json (3)
41-42: Good: move toworkspace:^for local packages.This aligns with pnpm/Yarn modern workspace resolution and the repo’s workspace setup.
45-46: Confirmed availability of @huggingface/transformers@^3.7.2
Rannpm view @huggingface/transformers versionandnpm view @huggingface/transformers versions --json | jq '.[-5:]', which returned version 3.7.2 as the latest release. Your lockfile should resolve to this version without issues.
43-43: Dependency range^4.1.3for Zod is validVerification shows that Zod v4.1.4 is published on npm, so the existing range resolves correctly:
npm view zod version→ 4.1.4npm pack zod@^4.1.3succeeds, pulling v4.1.4No change is strictly required; you can safely keep
"zod": "^4.1.3".
If you’d like to pin to the exact latest minor, you can optionally bump to^4.1.4, for example:• js/hang/package.json (line 43)
- "zod": "^4.1.3", + "zod": "^4.1.4", // optional bump to include only subsequent patch releasesjs/hang/src/publish/audio/index.ts (1)
14-17: Vite-only import for worklet: ensure repo-wide TS support.As above, confirm
?worker&urltyping. Also consider adding a short comment in code linking to Vite worker docs, since this is intentionally Vite-specific.Reuse the typing check script. Reference: Vite worker URL query docs. (main.vitejs.dev)
js/hang/src/publish/audio/speaking.ts (1)
6-6: Vite-only worklet import: verify TS typing for?worker&url.Same action item as other files to keep
tschappy.Use the typing verification script.
Reference: Vite docs for worker URL imports. (main.vitejs.dev)js/hang/src/watch/audio/index.ts (1)
27-29: All worklet imports are consistent and no legacy loader remains
Every*.tsfile underjs/hang/src/watchandjs/hang/src/publishuses the?worker&urlpattern, and there are no strayloadAudioWorkletreferences. Ready to merge.
feels bad man
Summary by CodeRabbit
New Features
Refactor
Chores