refactor: split SherpaOnnxManager + extract magic numbers (P0-2, P1-4)

[ZhaoChaoqun]

Summary

• Split SherpaOnnxManager.swift (803 lines → ~200 lines orchestrator) into 3 focused components:
  • ModelPathResolver.swift — resolves local model paths, checks file existence
  • ModelDownloader.swift — handles downloading from ModelScope/GitHub with source detection and fallback
  • ModelExtractionService.swift — handles tar.bz2 extraction via /usr/bin/tar
  • SherpaOnnxManager.swift — thin orchestrator that delegates to the above, preserving existing public API
• Extract magic numbers as named constants across 5 files:
  • ChineseSpellingCorrector.swift — logitDiffThreshold (5.0), softmaxProbThreshold (0.9), maxCorrectionRatio (0.2), ortIntraOpThreads (2), maxSequenceLength (512)
  • DualEngineASR.swift — flushSilenceSampleCount (1600)
  • QwenASRRecognizer.swift — streamChunkSeconds (2.0), streamRollbackTokens (5), streamUnfixedChunks (0), streamMaxNewTokens (32), defaultOfflineSegmentSeconds (20.0)
  • ASREngine.swift — flushSilenceSampleCount (1600)
  • SherpaOnnxOnlineRecognizer.swift — sampleRate (16000), featureDim (80), numThreads (1), maxActivePaths (4), rule1/2/3 endpoint detection thresholds

Test plan

• xcodebuild -scheme Typeless -configuration Debug builds successfully
• Manual smoke test: verify ASR recording and model download still work correctly
• Verify no behavior changes (pure refactor — all public APIs preserved)

🤖 Generated with Claude Code

[ZhaoChaoqun]

Code Review: PR #15 — SherpaOnnxManager Refactor + Magic Numbers

Overall Assessment: ✅ Approve with minor suggestions

This is a clean structural refactor. The 803-line god class is decomposed into well-scoped components with clear single responsibilities. All public APIs on SherpaOnnxManager are preserved as thin delegate wrappers. The magic number extraction is thorough with descriptive names and inline comments explaining "why." Well done.

---

1. Behavior Preservation ✅

Carefully compared old vs new code line-by-line:

• Path resolution: All getXxxPath() / isXxxDownloaded() methods are exact copies — same file names, same fallback order (FP32 → INT8), same directory structure. ✅
• Download flows: downloadModel, downloadPunctuationModel, downloadITNFst, downloadCSCModel — all preserve the "already downloaded? → early return" guard, the source detection → primary/fallback pattern, and the progress/completion callbacks. ✅
• Fallback logic: didCompleteWithError → retry with fallback source → fallbackSource = nil to prevent infinite loops. Preserved. ✅
• modelsDirectory: Both ModelPathResolver and the old SherpaOnnxManager use identical initialization logic (applicationSupportDirectory + "Nano Typeless/models"). ✅

2. Download/Progress/Fallback Flows ✅ (with one note)

All three download patterns are preserved:

• ASR models: URLSessionDownloadDelegate with progress callbacks via didWriteData — preserved in ModelDownloader. ✅
• Punctuation model: URLSession.shared.downloadTask with completion handler + fallback recursion — preserved. ✅
• ITN/CSC models: DispatchSource timer-based progress polling — preserved, including the 0.3s interval now using Self.progressPollInterval. ✅

3. project.pbxproj ✅

All 3 new files properly registered:

• PBXBuildFile section: ✅
• PBXFileReference section: ✅
• PBXGroup (Sources group): ✅ — placed adjacent to SherpaOnnxManager.swift, good grouping.
• Build phase (PBXSourcesBuildPhase): ✅

4. Magic Number Constants ✅

All extractions are correct. Names are descriptive and comments explain the derivations:

File	Constants	Quality
ChineseSpellingCorrector	ortIntraOpThreads, maxSequenceLength, logitDiffThreshold, softmaxProbThreshold, maxCorrectionRatio	✅ Clear names, comments explain the ML semantics
DualEngineASR	flushSilenceSampleCount	✅ Comment explains 0.1s × 16kHz = 1600
ASREngine (QwenASREngine)	flushSilenceSampleCount	✅ Same constant, same comment — correctly duplicated in the separate class
QwenASRRecognizer	streamChunkSeconds, streamRollbackTokens, streamUnfixedChunks, streamMaxNewTokens, defaultOfflineSegmentSeconds	✅ All with explanatory comments
SherpaOnnxOnlineRecognizer	sampleRate, featureDim, numThreads, maxActivePaths, rule1/2/3 endpoint thresholds	✅ Clear naming, comments explain each rule's purpose

5. SherpaOnnxManager as Orchestrator ✅

The refactored class is a proper thin orchestrator:

• Owns 3 collaborators (pathResolver, downloader, extractionService)
• Delegates all path queries to ModelPathResolver
• Delegates all downloads to ModelDownloader
• Wires extraction via closure injection in init()
• Retains static config constants (URLs, folder names) — correct, these are shared config
• Still inherits NSObject (needed for historical reasons but no longer for URLSessionDownloadDelegate) — harmless

---

Issues Found

🟡 Medium: Dead code & confusing closure in didFinishDownloadingTo (ModelDownloader.swift L465-484)

let destDir = (extractionHandler != nil) ? location : location  // location is temp file
// ...
let result = handler(location, URL(fileURLWithPath: ""))

Two problems here:

1. Line 468: destDir is assigned identically in both branches (location), making the ternary a no-op. The variable destDir is then never used. This is dead code.
2. Line 473: The second argument URL(fileURLWithPath: "") is passed to the extractionHandler, but the handler closure in SherpaOnnxManager.init() ignores it ({ sourceURL, _ in ... }). This works correctly because the closure captures self.pathResolver.modelsDirectory directly, but it's confusing and fragile — a future developer might think the second parameter matters.

This doesn't cause a bug (behavior matches original code which also used self.modelsDirectory directly), but it should be cleaned up.

🟡 Medium: Access control relaxation (ModelDownloader methods)

In the original SherpaOnnxManager, these methods were private:

• startDownload(...)
• selectFastestSource(...) / selectFastestPunctSource()
• formatBytes(...)
• cleanup()

In ModelDownloader, they are all internal (default). While this doesn't matter for a single-target app, it's good practice to mark methods that shouldn't be called externally as private. Specifically, startDownload, formatBytes, and cleanup are implementation details that only the class itself and its URLSessionDownloadDelegate extension should use.

🟢 Minor: modelsDirectory is let (public-in-module) on ModelPathResolver

ModelPathResolver.modelsDirectory is declared as let (internal access). It's accessed by SherpaOnnxManager for download destination paths, which is fine. But consider whether it should be a computed var or at least documented that it's safe to expose since it's immutable.

🟢 Minor: Duplicate flushSilenceSampleCount in ASREngine + DualEngineASR

Both QwenASREngine and DualEngineASR define private static let flushSilenceSampleCount = 1600 independently. This is acceptable since they're separate classes with separate semantics, but if this value ever needs to change, it would need updating in two places. Consider whether a shared constant makes sense, or leave as-is if intentional.

---

Summary

The refactoring is well-executed. The decomposition boundaries are logical, the delegation pattern is clean, and all existing behavior is preserved. The magic number extraction is thorough with good naming and documentation. The two medium issues (dead code in didFinishDownloadingTo and access control) are non-blocking but should ideally be addressed before merge.

Verdict: Approve — the issues above are minor and don't affect correctness.

[claude]

Review Summary

Clean refactor — the split of SherpaOnnxManager into ModelPathResolver, ModelDownloader, and ModelExtractionService is well-structured. Magic number extraction is straightforward and correct.

Two issues found in ModelDownloader.swift, both related to the extractionHandler indirection introduced during the split:

1. didFinishDownloadingTo passes a bogus destination URL (line 473) — see inline comment
2. extractionHandler ignores its destDir parameter — the handler injected by SherpaOnnxManager always extracts to pathResolver.modelsDirectory, but downloadPunctFromSource passes its destDir argument as the second parameter. This happens to work today because the caller always passes pathResolver.modelsDirectory, but the API contract is misleading.

[claude]

Bug: handler(location, URL(fileURLWithPath: "")) passes an empty-path URL as the destination directory. The handler injected by SherpaOnnxManager.init ignores this parameter (uses _), so extraction happens to go to the right place. But this is fragile — the extractionHandler signature promises a (sourceURL, destDir) -> Bool contract that isn't honored here.

Also, line 468 has a dead assignment (let destDir = ... is never read).

Consider either:

• Removing the second parameter from extractionHandler since the destination is always captured in the closure, or
• Passing the actual modelsDirectory here (requires the downloader to know about it, or have it injected)