Skip to content

fix(audio): guard evaluateFinishedRecording against null and non-object parameters - #1840

Closed
hsusul wants to merge 1 commit into
OpenWhispr:mainfrom
hsusul:fix/1839-recording-validation-null-safety
Closed

fix(audio): guard evaluateFinishedRecording against null and non-object parameters#1840
hsusul wants to merge 1 commit into
OpenWhispr:mainfrom
hsusul:fix/1839-recording-validation-null-safety

Conversation

@hsusul

@hsusul hsusul commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Fixes #1839

Problem

In src/helpers/recordingValidation.js:
evaluateFinishedRecording({ blobSize, receivedAudioData } = {}) threw TypeError: Cannot destructure property 'blobSize' of 'null' as it is null when passed null.

Solution

  • Handled null and non-object parameters safely using params && typeof params === "object" ? params : {}.
  • Added unit tests in test/helpers/recordingValidation.test.js.

Verification

  • node --test test/helpers/recordingValidation.test.js (passes, 10/10 tests)
  • npm run typecheck (passes, 0 errors)
  • npm run lint (passes, 0 errors)
  • npm run i18n:check (passes)
  • npm run build:renderer (passes)
  • git diff --check (clean)

@Chadpiha Chadpiha self-assigned this Aug 25, 2026
@Chadpiha

Copy link
Copy Markdown
Collaborator

Thanks for the careful work here, @hsusul — the tests are well written and the verification checklist (typecheck, lint, i18n, build) is genuinely appreciated.

I'm going to close this one, along with #1839, for a reason that's about our conventions rather than your code: evaluateFinishedRecording has exactly one caller (audioManager.js in finalizeBatchRecording), and it always passes a freshly built object literal with a Blob that's guaranteed non-null a few lines earlier — so null can't reach the helper through any real path. Our house rule (per CLAUDE.md) is to validate at real boundaries (IPC, external APIs, persisted data) and trust internal callers past that point.

There's also a subtle reason we prefer the current behavior: if a future refactor ever did pass null here, we'd want the loud crash at the bug site — a swallowed error here would surface as a silent empty transcription, which is a failure mode we've worked hard to eliminate (see #871, #1435).

Thanks again, and please keep the contributions coming! 🙏

@Chadpiha

Copy link
Copy Markdown
Collaborator

One general note, since you've clearly got an eye for edge cases: our convention (CLAUDE.md) is to validate exactly once, at real trust boundaries — IPC payloads arriving from the renderer, external API responses not already mediated by a typed SDK, persisted data an older app version could have written in a different shape, user-editable files — and to trust internal callers past that point, so that impossible states fail loudly instead of being silently coerced. Hardening PRs at those boundaries are very welcome! The strongest ones start from an observable app behavior (a crash log, a repro via real user actions) rather than a direct helper call. Would love to see more contributions in that direction — thanks for the energy you're putting into OpenWhispr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

evaluateFinishedRecording throws TypeError when called with null

2 participants