chore: port additional safe stale-PR updates#171
Conversation
Reviewer's GuideIntroduces a cached speech statistics summary in AudioSegmentProcessor to avoid recomputing aggregates, adds a regression test for proactive long-segment splitting, and switches font loading from Google Fonts CDN to self-hosted @fontsource packages with a tightened CSP. Sequence diagram for getStats speech summary caching in AudioSegmentProcessorsequenceDiagram
participant App
participant AudioSegmentProcessor
participant ProcessorState
App->>AudioSegmentProcessor: getStats()
AudioSegmentProcessor->>ProcessorState: read speechStats
alt cached speech summary exists
AudioSegmentProcessor->>ProcessorState: read cachedSpeechSummary
AudioSegmentProcessor-->>App: StatsSummary with speech from cache
else no cached summary
AudioSegmentProcessor->>ProcessorState: compute averages from speechStats
AudioSegmentProcessor->>ProcessorState: set cachedSpeechSummary
AudioSegmentProcessor-->>App: StatsSummary with newly computed speech
end
App->>AudioSegmentProcessor: processAudioData(chunk, timestamp, energy)
AudioSegmentProcessor->>ProcessorState: update speechStats
AudioSegmentProcessor->>ProcessorState: set cachedSpeechSummary to null
Class diagram for updated AudioSegmentProcessor state and cachingclassDiagram
class AudioSegmentProcessor {
-ProcessorState state
-ProcessorOptions options
+processAudioData(chunk : Float32Array, timestamp : number, energy : number) ProcessedSegment[]
+getStateInfo() ProcessorState
+getStats() StatsSummary
-average(values : number[]) number
}
class ProcessorState {
+inSpeech : boolean
+speechStartTime : number
+speechEnergies : number[]
+silenceEnergies : number[]
+speechStats : SegmentStats[]
+silenceStats : SegmentStats[]
+cachedSpeechSummary : StatsSummary
+currentStats : CurrentStats
+noiseFloor : number
+recentEnergies : number[]
}
class SegmentStats {
+duration : number
+avgEnergy : number
+energyIntegral : number
}
class StatsSummary {
+avgDuration : number
+avgEnergy : number
+avgEnergyIntegral : number
}
class CurrentStats {
+silence : StatsSummary
+speech : StatsSummary
}
AudioSegmentProcessor --> ProcessorState
ProcessorState --> SegmentStats
ProcessorState --> StatsSummary : cachedSpeechSummary
ProcessorState --> CurrentStats
CurrentStats --> StatsSummary
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new proactive split test hardcodes
firstSegment.durationto be>= maxDuration, which is a bit brittle given processing quantization; consider asserting the duration is close tomaxDurationwithin a small tolerance instead of requiring it to exceed it. - You’ve added caching for
speechStatssummaries but not forsilenceStats; if the same hot path patterns exist there, you might want to either cache both for symmetry or explicitly document why only speech is cached.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new proactive split test hardcodes `firstSegment.duration` to be `>= maxDuration`, which is a bit brittle given processing quantization; consider asserting the duration is close to `maxDuration` within a small tolerance instead of requiring it to exceed it.
- You’ve added caching for `speechStats` summaries but not for `silenceStats`; if the same hot path patterns exist there, you might want to either cache both for symmetry or explicitly document why only speech is cached.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Applied the valid test-brittleness feedback in the latest push. Applied:
Not applied in this PR:
Validation run:
|
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
src/index.tsx
Outdated
| import '@fontsource/plus-jakarta-sans/latin-600.css'; | ||
| import '@fontsource/jetbrains-mono/latin-400.css'; | ||
| import '@fontsource/jetbrains-mono/latin-500.css'; | ||
| import '@fontsource/material-symbols-outlined/latin-400.css'; |
There was a problem hiding this comment.
WARNING: @fontsource/material-symbols-outlined variable font — only weight-400 loaded
The original Google Fonts URL loaded Material Symbols as a variable font with both the weight axis (wght@100..700) and the fill axis (FILL@0..1):
https://fonts.googleapis.com/css2?family=Material+Symbols+Outlined:wght,FILL@100..700,0..1
The @fontsource/material-symbols-outlined package ships the font as a variable font, but latin-400.css only registers the wght=400 static slice. The FILL axis (used to toggle filled vs. outlined icon style) and the full weight range will be unavailable unless you import the variable font CSS instead:
| import '@fontsource/material-symbols-outlined/latin-400.css'; | |
| import '@fontsource-variable/material-symbols-outlined'; |
Note: the variable-font package is @fontsource-variable/material-symbols-outlined (separate npm package). If icons currently render but look wrong (all filled or all outlined regardless of CSS), this is the cause. Verify icon rendering in the browser after this change.
There was a problem hiding this comment.
Applied in 90f1eef. Switched from @fontsource/material-symbols-outlined/latin-400.css to @fontsource-variable/material-symbols-outlined/fill.css and updated the CSS family to Material Symbols Outlined Variable. This preserves variable-axis icon rendering while keeping payload smaller than full.css.
package.json
Outdated
| "vitest": "^4.0.18" | ||
| }, | ||
| "dependencies": { | ||
| "@fontsource/crimson-pro": "^5.2.8", |
There was a problem hiding this comment.
SUGGESTION: @fontsource packages belong in devDependencies, not dependencies
Vite bundles font CSS and WOFF2 files at build time — the @fontsource/* packages are build-time assets, not runtime dependencies. Moving them to devDependencies keeps the production dependency graph clean and avoids unnecessary installs in CI/CD environments that skip dev deps.
This is low-risk for a browser-only app (no server-side node_modules consumption), but it's the conventional placement for bundled assets.
There was a problem hiding this comment.
Acknowledged. I did not move these font packages to devDependencies in this PR. Keeping them in dependencies is intentional for this app branch to avoid dependency-class churn while we are doing targeted stale-PR ports; it does not affect bundle/runtime behavior. If you want, I can open a separate cleanup PR to standardize dependency placement across the repo.
There was a problem hiding this comment.
Applied in ab4465d. Moved all @fontsource packages from dependencies to devDependencies in this PR and revalidated build/tests.
|
|
||
| if (this.state.speechStats.length > this.options.maxHistoryLength) { | ||
| this.state.speechStats.shift(); | ||
| this.state.cachedSpeechSummary = null; |
There was a problem hiding this comment.
SUGGESTION: Redundant cachedSpeechSummary = null assignment
The cache was already set to null at line 268 (immediately after the push). By the time execution reaches line 272, cachedSpeechSummary is already null, so this second assignment is a no-op.
Consider removing it to reduce noise:
| this.state.cachedSpeechSummary = null; | |
| this.state.speechStats.shift(); |
Alternatively, if you want to keep it as a defensive belt-and-suspenders guard, add a comment explaining the intent.
There was a problem hiding this comment.
Applied in 90f1eef. Removed the redundant second cachedSpeechSummary = null inside the shift() block.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Other Observations (not in diff)Issues found in unchanged code or previously discussed that cannot receive new inline comments:
What ChangedThis PR migrates web fonts from Google Fonts CDN to self-hosted Risk assessment: Low. The font migration is a straightforward build-time substitution with no runtime logic changes. The CSP tightening is a security improvement. The stats cache is a minor optimization in a non-hot path with correct invalidation logic. The new test covers a previously untested code path. Files Reviewed (5 files)
|
src/index.tsx
Outdated
| import '@fontsource/plus-jakarta-sans/latin-600.css'; | ||
| import '@fontsource/jetbrains-mono/latin-400.css'; | ||
| import '@fontsource/jetbrains-mono/latin-500.css'; | ||
| import '@fontsource-variable/material-symbols-outlined/fill.css'; |
There was a problem hiding this comment.
WARNING: fill.css from @fontsource-variable/material-symbols-outlined only bundles the FILL axis subset of the variable font. The CSS in index.css uses font-variation-settings: 'FILL' 0, 'wght' 400, 'GRAD' 0, 'opsz' 24, but the wght/GRAD/opsz axes will be unavailable with this subset — those settings will silently have no effect.
Consider importing the full variable font instead:
| import '@fontsource-variable/material-symbols-outlined/fill.css'; | |
| import '@fontsource-variable/material-symbols-outlined'; |
Or, if only FILL variation is needed and weight/grade/optical-size are not required, document that intent here.
There was a problem hiding this comment.
Applied in f3a516e. Switched the import to @fontsource-variable/material-symbols-outlined (full variable set) so the existing font-variation-settings axes (FILL, wght, GRAD, opsz) can take effect consistently.
| } | ||
|
|
||
| if (this.state.speechStats.length > 0) { | ||
| if (this.state.cachedSpeechSummary) { |
There was a problem hiding this comment.
SUGGESTION: silenceStats is still recomputed via three .map() + average() calls on every updateStats() invocation (lines 464–470), while speechStats now benefits from the new cache. For consistency and to fully realise the hot-path savings, consider adding a cachedSilenceSummary field with the same invalidation pattern (clear it when a new entry is pushed to silenceStats).
There was a problem hiding this comment.
Not applying in this PR. This branch is a targeted stale-PR port plus low-risk review fixes; adding silence-summary caching changes runtime behavior and should be evaluated separately with profiling evidence. I can open a dedicated perf follow-up PR for that.
📝 WalkthroughWalkthroughThe PR migrates font loading from external CDN (Google Fonts) to local npm packages ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
| const sampleRate = 16000; | ||
| const maxDuration = 0.5; | ||
|
|
||
| const processor = new AudioSegmentProcessor({ |
There was a problem hiding this comment.
SUGGESTION: The test constructor does not suppress the default logger: console.log, so every processAudioData call will emit multiple log lines during the test run. Add logger: () => {} to keep test output clean.
| const processor = new AudioSegmentProcessor({ | |
| const processor = new AudioSegmentProcessor({ | |
| sampleRate, | |
| maxSegmentDuration: maxDuration, | |
| energyThreshold: 0.01, | |
| minSpeechDuration: 0.1, | |
| snrThreshold: 0, | |
| minSnrThreshold: 0, | |
| logger: () => {} | |
| }); |
There was a problem hiding this comment.
Applied in 4306afe. Added logger: () => {} for the proactive-split test constructor.
| expect(emittedSegments.length).toBeGreaterThan(0); | ||
|
|
||
| const firstSegment = emittedSegments[0]; | ||
| const splitTolerance = 0.15; |
There was a problem hiding this comment.
SUGGESTION: splitTolerance = 0.15 is 30% of maxDuration = 0.5, which is loose enough to mask an off-by-one chunk error. Since chunks are 100 ms each, a tolerance of one chunk duration (0.1) would be tighter and still accommodate quantization:
| const splitTolerance = 0.15; | |
| const splitTolerance = 0.1; // one chunk boundary |
There was a problem hiding this comment.
Applied in 4306afe. Tightened split tolerance from
There was a problem hiding this comment.
Follow-up: in commit 4306afe this test now uses splitTolerance = 0.1 (one 100ms chunk boundary).
| avgEnergy: this.average(this.state.speechStats.map(s => s.avgEnergy)), | ||
| avgEnergyIntegral: this.average(this.state.speechStats.map(s => s.energyIntegral)) | ||
| }; | ||
| this.state.cachedSpeechSummary = stats.speech; |
There was a problem hiding this comment.
SUGGESTION: cachedSpeechSummary aliases stats.speech, which is also stored in currentStats.speech
After this line, both this.state.cachedSpeechSummary and this.state.currentStats.speech (set at line 483) point to the same object. getStats() returns a defensive copy so external callers are safe, but any internal code that reads this.state.currentStats.speech directly and mutates it would silently corrupt the cache.
Consider storing a copy to make the cache independent:
| this.state.cachedSpeechSummary = stats.speech; | |
| this.state.cachedSpeechSummary = { ...stats.speech }; |
| expect(emittedSegments.length).toBeGreaterThan(0); | ||
|
|
||
| const firstSegment = emittedSegments[0]; | ||
| const splitTolerance = 0.1; |
There was a problem hiding this comment.
SUGGESTION: splitTolerance = 0.1 is 20% of maxDuration = 0.5 — still a wide tolerance
With 100 ms chunks and maxDuration = 0.5 s, the split should fire at the chunk boundary at or just after 0.5 s, so the first segment duration should be exactly 0.5 s (or at most 0.6 s if the split fires one chunk late). A tolerance of ±0.1 s allows durations from 0.4 s to 0.6 s, which would pass even if the split fires a full chunk early. Consider tightening to 0.05 (one chunk boundary) to catch off-by-one regressions in the split logic:
| const splitTolerance = 0.1; | |
| const splitTolerance = 0.05; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/audio/AudioSegmentProcessor.ts (1)
194-211: Note: proactively split segments are not recorded inspeechStats.When a segment is split at
maxSegmentDuration, aProcessedSegmentis emitted but no entry is pushed tospeechStats. This means the cached speech summary (and overall VAD statistics) won't reflect these segments. This appears to be pre-existing behavior rather than a regression from the caching change, but worth being aware of if statistics accuracy matters downstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/audio/AudioSegmentProcessor.ts` around lines 194 - 211, When a long segment is proactively split in AudioSegmentProcessor (the block checking this.state.inSpeech && this.state.speechStartTime and comparing currentSpeechDuration to this.options.maxSegmentDuration), the created segment is emitted via createSegment but not recorded in the speechStats cache; update the same path that handles normal segment completion to also update speechStats when you create and push the split segment (e.g., after const segment = this.createSegment(...) and before starting a new segment with this.startSpeech(...)), ensuring you add the same ProcessedSegment summary/metrics object (or call the existing helper that records stats) so speechStats remains consistent with emitted segments and VAD statistics stay accurate.src/index.css (1)
31-32: Stylelint flags missing generic font family — safe to suppress for icon fonts.Stylelint reports
font-family-no-missing-generic-family-keywordon this line. Since Material Symbols is an icon font (no text rendering fallback makes sense), you can silence it with an inline disable comment.🔧 Suggested suppression
.material-symbols-outlined { - font-family: 'Material Symbols Outlined Variable' !important; + /* stylelint-disable-next-line font-family-no-missing-generic-family-keyword */ + font-family: 'Material Symbols Outlined Variable' !important;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.css` around lines 31 - 32, Stylelint is flagging .material-symbols-outlined's font-family for missing a generic-family keyword; silence it by adding an inline stylelint disable for the rule font-family-no-missing-generic-family-keyword either immediately above the .material-symbols-outlined selector (use /* stylelint-disable-next-line font-family-no-missing-generic-family-keyword */) or on the same line as the font-family declaration (use /* stylelint-disable-line font-family-no-missing-generic-family-keyword */) so the Material Symbols Outlined icon font can remain as 'Material Symbols Outlined Variable' without adding a generic fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/index.css`:
- Around line 31-32: Stylelint is flagging .material-symbols-outlined's
font-family for missing a generic-family keyword; silence it by adding an inline
stylelint disable for the rule font-family-no-missing-generic-family-keyword
either immediately above the .material-symbols-outlined selector (use /*
stylelint-disable-next-line font-family-no-missing-generic-family-keyword */) or
on the same line as the font-family declaration (use /* stylelint-disable-line
font-family-no-missing-generic-family-keyword */) so the Material Symbols
Outlined icon font can remain as 'Material Symbols Outlined Variable' without
adding a generic fallback.
In `@src/lib/audio/AudioSegmentProcessor.ts`:
- Around line 194-211: When a long segment is proactively split in
AudioSegmentProcessor (the block checking this.state.inSpeech &&
this.state.speechStartTime and comparing currentSpeechDuration to
this.options.maxSegmentDuration), the created segment is emitted via
createSegment but not recorded in the speechStats cache; update the same path
that handles normal segment completion to also update speechStats when you
create and push the split segment (e.g., after const segment =
this.createSegment(...) and before starting a new segment with
this.startSpeech(...)), ensuring you add the same ProcessedSegment
summary/metrics object (or call the existing helper that records stats) so
speechStats remains consistent with emitted segments and VAD statistics stay
accurate.
chore: port additional safe stale-PR updates 798a214
2 participants
Summary
Ports additional low-risk, still-valid work from stale open PRs onto current
master.Ported from stale PRs
AudioSegmentProcessorto avoid repeated hot-path array/reduce work inupdateStats.AudioSegmentProcessor.test.ts.Details
src/lib/audio/AudioSegmentProcessor.tscachedSpeechSummarystate cache.src/lib/audio/AudioSegmentProcessor.test.tsmaxSegmentDuration.index.html,src/index.css,src/index.tsx,package.json@fontsource/*packages for existing fonts.fonts.googleapis.comandfonts.gstatic.comallowances.Validation
npm test -- src/lib/audio/AudioSegmentProcessor.test.ts✅npm run build✅Scope boundaries
Summary by Sourcery
Port safe changes from stale PRs to improve audio processing performance, increase test coverage, and self-host fonts while tightening CSP.
Enhancements:
Build:
Tests:
Summary by CodeRabbit
Chores
Refactor
Tests