Add Turkish target language and fix macOS whisper runtime#32
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe change adds Turkish as a translation target across the UI, localized mappings, documentation, and smoke tests. It also validates whisper-cpp runtime libraries during installation, improves macOS missing-library diagnostics, hardens archive extraction, and reports invalid translation JSON with a descriptive error. ChangesTurkish language support
Whisper runtime handling
Translation response validation
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main.js`:
- Around line 2152-2159: Update the looksLikeDyldMissingLib detector to require
a genuine macOS loader signature: retain the explicit “dyld: library not loaded”
check, and only accept “image not found” when the stderr also references a
dylib. Remove the broad standalone “no such file” and “.dylib” conditions while
preserving the process.platform === 'darwin' guard.
In `@scripts/postinstall.js`:
- Around line 30-40: Update hasWhisperRuntimeLibraries to require the Windows
runtime DLL dependencies in addition to whisper-cli.exe before returning true.
Validate the selected build’s required whisper.dll and ggml*.dll files, and when
the cpu/ fallback directory exists, validate the corresponding DLL set there as
well; return false if any required dependency is missing.
- Around line 484-492: Update the post-install success path in main() to re-run
hasWhisperRuntimeLibraries() before every successful return, rather than
validating only WHISPER_CLI. If the runtime libraries are still missing after
installation or extraction, report the installation as incomplete and avoid
logging success; retain the existing successful return only when the
completeness predicate passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a322648-e122-4596-a800-6810b128553e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (5)
README.mdindex.htmlmain.jsrenderer.jsscripts/postinstall.js
Validate the installed whisper runtime by launching the CLI, narrow macOS dyld detection, complete Turkish provider and documentation coverage, and credit the contributor across localized READMEs.
Blue-B
left a comment
Blue-B
left a comment
There was a problem hiding this comment.
Verified locally: the full check and Electron E2E pass, the packaged Linux runtime launches and completes a real extraction, and the macOS dylib packaging layout was inspected. Follow-up review fixes and contributor attribution are included. Ready to merge.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/postinstall.js (1)
662-731: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRun the runtime probe on the CPU fallback before reporting success.
The fallback copy step can still leavewhisper-cli.exeunusable if a DLL is missing or incompatible, but this path currently logs success unconditionally. ReusehasWhisperRuntimeLibraries(CPU_CLI, CPU_DIR)here and warn if the CPU bundle can’t load.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/postinstall.js` around lines 662 - 731, The CPU fallback installation in the download flow reports success without validating the copied executable and libraries. After copying files in the CPU fallback block, call hasWhisperRuntimeLibraries with CPU_CLI and CPU_DIR; only report the bundle as successfully installed when it passes, otherwise emit a warning that the CPU fallback cannot load.
🧹 Nitpick comments (2)
scripts/smoke-test.js (1)
110-113: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the original parse error is preserved.
This test only checks the new message prefix, so it would pass even if
{ cause: error }were removed. Also assert that the thrown error’scauseis the originalSyntaxError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/smoke-test.js` around lines 110 - 113, Update the parseContextAwareJson assertion in the smoke test to capture the thrown error and verify its cause is the original SyntaxError, while retaining the existing message-prefix check.scripts/postinstall.js (1)
192-210: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffGood use of execFileSync argument arrays — but no checksum verification before extraction.
Switching to
execFileSyncwith explicit argument arrays (instead of a shell string) is exactly right and avoids shell-injection risk for tar/unzip/PowerShell.However, the archives downloaded from GitHub releases are extracted here with no hash/checksum verification against the fetched release metadata. Since this pipeline downloads and then executes third-party binaries, verifying a published checksum (or at minimum pinning/validating asset digests) before extraction would meaningfully harden the install chain.
As per path instructions for
scripts/**: "Watch for unpinned downloads, missing checksum verification, and shell injection in spawn/exec calls."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/postinstall.js` around lines 192 - 210, Update the download-and-extract flow used by extractZip to verify each GitHub release archive against its published checksum or validated asset digest before any extraction occurs. Reuse the fetched release metadata and existing archive path handling, reject missing or mismatched verification data with a clear error, and only invoke the tar, PowerShell, or unzip branches after validation succeeds.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/smoke-test.js`:
- Around line 79-87: Update runWhisperRuntimeProbe to invoke a real whisper-cli
binary rather than process.execPath, using an available test fixture or resolved
installed executable and its containing runtime directory. Keep the missing-cli
assertion, and ensure the positive assertion exercises the binary so
hasWhisperRuntimeLibraries validates actual Whisper runtime loading.
---
Outside diff comments:
In `@scripts/postinstall.js`:
- Around line 662-731: The CPU fallback installation in the download flow
reports success without validating the copied executable and libraries. After
copying files in the CPU fallback block, call hasWhisperRuntimeLibraries with
CPU_CLI and CPU_DIR; only report the bundle as successfully installed when it
passes, otherwise emit a warning that the CPU fallback cannot load.
---
Nitpick comments:
In `@scripts/postinstall.js`:
- Around line 192-210: Update the download-and-extract flow used by extractZip
to verify each GitHub release archive against its published checksum or
validated asset digest before any extraction occurs. Reuse the fetched release
metadata and existing archive path handling, reject missing or mismatched
verification data with a clear error, and only invoke the tar, PowerShell, or
unzip branches after validation succeeds.
In `@scripts/smoke-test.js`:
- Around line 110-113: Update the parseContextAwareJson assertion in the smoke
test to capture the thrown error and verify its cause is the original
SyntaxError, while retaining the existing message-prefix check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c03c0a4b-e935-476a-9bd6-227992c7b466
📒 Files selected for processing (10)
README.mddocs/README.ja.mddocs/README.ko.mddocs/README.pl.mddocs/README.zh.mdmain.jsscripts/e2e-smoke.jsscripts/postinstall.jsscripts/smoke-test.jstranslator-enhanced.js
🚧 Files skipped from review as they are similar to previous changes (1)
- main.js
| function runWhisperRuntimeProbe() { | ||
| const runtimeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'wst-runtime-probe-')); | ||
| try { | ||
| assert.strictEqual(hasWhisperRuntimeLibraries(path.join(runtimeDir, 'missing-cli'), runtimeDir), false); | ||
| assert.strictEqual(hasWhisperRuntimeLibraries(process.execPath, path.dirname(process.execPath)), true); | ||
| } finally { | ||
| fs.rmSync(runtimeDir, { recursive: true, force: true }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure relevant to the smoke test
git ls-files scripts/smoke-test.js
wc -l scripts/smoke-test.js
# Show the relevant sections with line numbers
sed -n '1,220p' scripts/smoke-test.jsRepository: Blue-B/WhisperSubTranslate
Length of output: 5801
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the helper and any related tests/usages
rg -n "hasWhisperRuntimeLibraries|runWhisperRuntimeProbe|whisper-cli|libwhisper|libggml" scripts/smoke-test.js .Repository: Blue-B/WhisperSubTranslate
Length of output: 7818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the runtime-library probe helper and nearby logic
sed -n '1,120p' scripts/postinstall.js
printf '\n---\n'
sed -n '120,220p' scripts/postinstall.js
printf '\n---\n'
sed -n '220,420p' scripts/postinstall.jsRepository: Blue-B/WhisperSubTranslate
Length of output: 13679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any smoke-test documentation or comments that describe the intended probe
rg -n "runWhisperRuntimeProbe|hasWhisperRuntimeLibraries|Smoke tests passed|whisper runtime" scripts docs README.md main.js package.jsonRepository: Blue-B/WhisperSubTranslate
Length of output: 1092
Probe a real whisper-cli binary here. process.execPath only runs Node/Electron --help, so this check can pass without loading Whisper’s runtime libraries and doesn’t validate the installed runtime.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/smoke-test.js` around lines 79 - 87, Update runWhisperRuntimeProbe to
invoke a real whisper-cli binary rather than process.execPath, using an
available test fixture or resolved installed executable and its containing
runtime directory. Keep the missing-cli assertion, and ensure the positive
assertion exercises the binary so hasWhisperRuntimeLibraries validates actual
Whisper runtime loading.
|
Thank you, @AtillaTahak, for this contribution. Turkish translation support and the macOS runtime fix are now merged, and I have added you to the Contributors section. I appreciate the careful cross-platform work! |
Thank you so much for the review and for merging the PR! I'm glad I could contribute to the project. I really appreciate being added to the Contributors section as well. Looking forward to contributing more in the future. 🚀 |
2 participants
Adds Turkish as a selectable translation target and fixes macOS whisper runtime packaging so libwhisper/libggml dylibs are copied correctly.
Summary by CodeRabbit
tr) as a selectable target language, including consistent display labels across the UI.