Repository navigation
feat: Implement Model Lifecycle Standardization - #4
Conversation
This commit implements a comprehensive model lifecycle management system
with proper logging, download progress tracking, and user feedback.
Phase 1: Logging Infrastructure
- Add src-pyloid/services/logger.py with domain-based logging
- Implement hybrid log format: [timestamp] [LEVEL] [domain] message | {json}
- Configure 100MB log rotation with 1 backup file
- Valid domains: model, audio, hotkey, settings, database, clipboard, window
- Add 18 tests in test_logger.py
- Update all existing services to use domain-based loggers
Phase 2: Model Manager
- Add src-pyloid/services/model_manager.py for download/cache management
- Implement CancelToken for cancellation support
- Implement DownloadProgress with percent, speed, ETA tracking
- Add MODEL_REPOS mapping for non-standard repos (turbo model)
- Use huggingface_hub.snapshot_download() with progress callback
- Run downloads in daemon threads for proper cancellation
- Add 16 tests in test_model_manager.py
- Add RPC methods: get_model_info, start_model_download, cancel_model_download
- Update server.py with download progress event callbacks
- Add frontend API methods and TypeScript types
Phase 3: Onboarding Download Step
- Add src/components/ModelDownloadProgress.tsx component
- Shows download progress with progress bar, speed, ETA
- Handles completed, error, cancelled states with retry option
- Update Onboarding.tsx with new download step (Step 4)
- Auto-start download, auto-advance on completion
- Cancel returns to model selection
Phase 4: Settings Modal
- Add src/components/ModelDownloadModal.tsx
- Wraps ModelDownloadProgress in shadcn Dialog
- Update SettingsTab.tsx to check model cache on change
- Show download modal if model not cached
- Cancel reverts to previous model selection
Phase 5: Startup and Recovery
- Add src/components/ModelRecoveryModal.tsx
- Update App.tsx to check model cache on startup
- Show recovery modal if model missing after onboarding
- Block UI until model is downloaded
Additional Fixes:
- Add open_external_url RPC method for opening URLs in system browser
- Update Sidebar.tsx to use API for GitHub/Issues links
Technical Details:
- Downloads use huggingface_hub.snapshot_download() with custom tqdm class
- Progress updates throttled to 10/second to avoid UI overhead
- Turbo model uses deepdml/faster-whisper-large-v3-turbo-ct2 repo
- All 91 backend tests pass
- TypeScript compiles without errors
Files Added:
- src-pyloid/services/model_manager.py
- src-pyloid/tests/test_logger.py
- src-pyloid/tests/test_model_manager.py
- src/components/ModelDownloadProgress.tsx
- src/components/ModelDownloadModal.tsx
- src/components/ModelRecoveryModal.tsx
Files Modified:
- src-pyloid/services/logger.py (enhanced)
- src-pyloid/services/*.py (all services use domain loggers)
- src-pyloid/server.py (RPC methods for model management, open_external_url)
- src-pyloid/main.py (download progress callback registration)
- src-pyloid/app_controller.py (domain loggers)
- src/lib/api.ts (model management API, openExternalUrl)
- src/lib/types.ts (ModelInfo, DownloadProgress types)
- src/pages/Onboarding.tsx (download step)
- src/components/SettingsTab.tsx (download modal integration)
- src/components/Sidebar.tsx (external URL handling)
- src/App.tsx (startup model check)
|
Caution Review failedThe pull request is closed. WalkthroughAdds domain-based structured logging and a domain logger service, implements a Model Manager with download/progress/cancellation, wires new RPCs and frontend API calls for model info and downloads, and introduces UI components for downloading/recovering models integrated into onboarding and settings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as Frontend App
participant API as Frontend API
participant Server as RPC Server
participant MM as Model Manager
participant HF as HuggingFace Hub
User->>App: Select or start app (model needed)
App->>API: getModelInfo(modelName)
API->>Server: "get_model_info"
Server->>MM: get_model_info(model_name)
MM->>HF: check cache / repo metadata
HF-->>MM: cached status + size
MM-->>Server: ModelInfo
Server-->>API: ModelInfo
API-->>App: ModelInfo
alt model not cached
App->>App: show ModelDownloadModal/RecoveryModal
User->>App: confirm download
App->>API: startModelDownload(modelName)
API->>Server: "start_model_download"
Server->>MM: download_model(on_progress callback)
MM->>HF: snapshot_download (background thread)
loop progress
MM-->>Server: progress update
Server-->>API: download-progress event
API-->>App: CustomEvent download-progress
App->>App: update UI progress
end
MM-->>Server: download-complete
Server-->>API: download-complete event
API-->>App: CustomEvent download-complete
App->>App: onComplete(true) → close modal / advance
else model cached
App->>App: apply setting immediately
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 |
![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
🧹 Nitpick comments (18)
src-pyloid/services/logger.py (2)
207-237: Thread-safety concern with singleton pattern.The
get_loggerfunction modifies_domain_loggerswithout synchronization. In multi-threaded scenarios, concurrent calls for the same domain could create duplicate loggers.Consider adding a lock for thread-safety:
+_logger_lock = threading.Lock() + def get_logger(domain: str) -> DomainLogger: ... global _domain_loggers # Return cached logger if exists - if domain in _domain_loggers: - return _domain_loggers[domain] + with _logger_lock: + if domain in _domain_loggers: + return _domain_loggers[domain] - # Auto-initialize if not done - if not _initialized: - setup_logging() + # Auto-initialize if not done + if not _initialized: + setup_logging() - # Create underlying Python logger - logger_name = f"VoiceFlow.{domain}" - py_logger = logging.getLogger(logger_name) - py_logger.setLevel(logging.DEBUG) + # Create underlying Python logger + logger_name = f"VoiceFlow.{domain}" + py_logger = logging.getLogger(logger_name) + py_logger.setLevel(logging.DEBUG) - # Create domain logger wrapper - domain_logger = DomainLogger(domain, py_logger) - _domain_loggers[domain] = domain_logger + # Create domain logger wrapper + domain_logger = DomainLogger(domain, py_logger) + _domain_loggers[domain] = domain_logger - return domain_logger + return domain_logger
239-263: Legacy wrappers ignore additional arguments.The legacy functions accept
*argsand**kwargsfor API compatibility but discard them silently. This could mask issues if callers expect format-string arguments to work.As per static analysis hints, this is flagged as unused arguments. Consider either forwarding kwargs to the domain logger or documenting that format-string arguments are not supported in the legacy API.
def debug(msg: str, *args, **kwargs): """Legacy debug function for backward compatibility.""" - get_logger("app").debug(msg) + get_logger("app").debug(msg, **kwargs)src-pyloid/services/model_manager.py (4)
50-62: CancelToken is not thread-safe.The
_cancelledflag is read and written without synchronization. While this may work due to Python's GIL for simple bool assignments, usingthreading.Eventwould be more robust and idiomatic.+import threading + @dataclass class CancelToken: """Token for cancelling long-running operations.""" - _cancelled: bool = False + def __post_init__(self): + self._event = threading.Event() def cancel(self) -> None: """Request cancellation of the operation.""" - self._cancelled = True + self._event.set() def is_cancelled(self) -> bool: """Check if cancellation has been requested.""" - return self._cancelled + return self._event.is_set()
83-150: ProgressTracker class is defined but unused.The
ProgressTrackerclass is defined with full tqdm-compatible interface, but the actual download implementation (lines 332-343) uses a differentDownloadProgressBarclass. This appears to be dead code.Either remove
ProgressTrackeror consolidate it with theDownloadProgressBarimplementation used in_do_download.
439-453: Use a named function instead of lambda for on_progress default.As flagged by static analysis (E731), prefer a named function over lambda assignment.
+def _noop_progress(p: DownloadProgress) -> None: + """No-op progress callback.""" + pass + def ensure_model_ready( ... ): if not self.is_model_cached(model_name): log.info("Model not cached, downloading", model=model_name) # Set up defaults if not provided if on_progress is None: - on_progress = lambda p: None + on_progress = _noop_progress if cancel_token is None: cancel_token = CancelToken()
456-465: Singleton access is not thread-safe.The
get_model_managerfunction has a race condition where multiple threads could create multiple instances.+_model_manager_lock = threading.Lock() _model_manager: Optional[ModelManager] = None def get_model_manager() -> ModelManager: """Get the singleton ModelManager instance.""" global _model_manager - if _model_manager is None: - _model_manager = ModelManager() + if _model_manager is None: + with _model_manager_lock: + if _model_manager is None: + _model_manager = ModelManager() return _model_managersrc/components/ModelDownloadModal.tsx (1)
24-28: Consider removing the redundantonOpenChangehandler.Since
onPointerDownOutsideandonEscapeKeyDownboth prevent closing, theonOpenChangecallback will never fire withisOpen=false. This makes the handler effectively dead code. For consistency,ModelRecoveryModaluses the same blocking pattern withoutonOpenChange.- <Dialog open={open} onOpenChange={(isOpen) => !isOpen && onCancel()}> + <Dialog open={open}>src/components/SettingsTab.tsx (2)
91-112: MissingupdateSettingin useCallback dependency array.
handleModelChangecallsupdateSettingbut doesn't include it in the dependency array. This could lead to stale closure issues ifupdateSettingwere to change (though unlikely since it's defined in component scope withsettingsdependency).const handleModelChange = useCallback( async (newModel: string) => { if (!settings) return; try { const modelInfo = await api.getModelInfo(newModel); if (modelInfo.cached) { // Model is cached, just update settings updateSetting("model", newModel); } else { // Model needs download - show modal setPendingModel(newModel); setDownloadModalOpen(true); } } catch (err) { console.error("Failed to get model info:", err); toast.error("Failed to check model status"); } }, - [settings] + [settings, updateSetting] );
115-125: MissingupdateSettingdependency here as well.Same issue as
handleModelChange-updateSettingis called but not in the dependency array.const handleDownloadComplete = useCallback( (success: boolean) => { if (success && pendingModel) { // Download succeeded, update settings updateSetting("model", pendingModel); } setDownloadModalOpen(false); setPendingModel(null); }, - [pendingModel] + [pendingModel, updateSetting] );src-pyloid/tests/test_model_manager.py (1)
233-239: Cache path assertion may be too restrictive.The assertion
assert ".cache" in str(cache_path) or "huggingface" in str(cache_path)could fail ifHF_HOMEis set to a custom path like/data/models. Perget_cache_path()inmodel_manager.py, whenHF_HOMEis set, it returnsPath(HF_HOME) / "hub".Consider a more robust assertion:
def test_get_cache_path_returns_path(self, model_manager): """get_cache_path returns the huggingface cache directory.""" cache_path = model_manager.get_cache_path() assert isinstance(cache_path, Path) - # Should be in user's home directory - assert ".cache" in str(cache_path) or "huggingface" in str(cache_path) + # Path should end with "hub" (the HF cache structure) + assert cache_path.name == "hub"src-pyloid/tests/test_logger.py (1)
156-156: Rename ambiguous variableltoline.Single-letter
lis easily confused with1orI. This also resolves the Ruff E741 hint.- lines = [l for l in content.strip().split('\n') if 'Simple message' in l] + lines = [line for line in content.strip().split('\n') if 'Simple message' in line]src/pages/Onboarding.tsx (1)
83-89: Dependency array missingstartDownload.The
useEffectreferencesstartDownloadbut doesn't include it in the dependency array. This is flagged by the ESLint exhaustive-deps rule. SincestartDownloadis stable (doesn't depend on state that changes), this works, but consider wrappingstartDownloadinuseCallbackor adding// eslint-disable-next-line react-hooks/exhaustive-depswith a comment explaining why.src-pyloid/server.py (2)
17-19: Type annotations should indicate optionality.The global variables can be
None, but the annotations don't reflect this.+from typing import Optional + # Active download state -_active_download_token: CancelToken = None -_download_thread: threading.Thread = None +_active_download_token: Optional[CancelToken] = None +_download_thread: Optional[threading.Thread] = None
233-236: Potential race condition when cancelling previous download.When a new download is requested, the previous download is cancelled but the code doesn't wait for the old
_download_threadto terminate. This could lead to interleaved progress events from both downloads if the old thread hasn't yet exited when the new one starts.Consider joining the old thread with a short timeout before starting the new download:
# Cancel any existing download if _active_download_token and not _active_download_token.is_cancelled(): model_log.info("Cancelling previous download for new request") _active_download_token.cancel() + # Wait briefly for the old thread to recognize cancellation + if _download_thread and _download_thread.is_alive(): + _download_thread.join(timeout=1.0)src/components/ModelDownloadProgress.tsx (2)
83-89: Missing dependency in useEffect.
startDownloadis called inside the effect but not listed in the dependency array. This can lead to stale closures ifstartDownloadcaptures state. Consider wrappingstartDownloadinuseCallbackwith appropriate dependencies, or add a suppression comment explaining the intentional omission.+ const startDownload = useCallback(async () => { + try { + setState("downloading"); + setError(null); + + const result = await api.startModelDownload(modelName); + + if (result.alreadyCached) { + setState("completed"); + onComplete(true); + } + } catch (err) { + setState("error"); + setError(err instanceof Error ? err.message : "Failed to start download"); + onComplete(false); + } + }, [modelName, onComplete]); + // Auto-start download useEffect(() => { if (autoStart && !hasStarted.current && state === "idle") { hasStarted.current = true; startDownload(); } - }, [autoStart, state]); + }, [autoStart, state, startDownload]);Note: This requires removing the standalone
startDownloadfunction definition at lines 91-108.
119-125:handleRetryimmediately callsstartDownloadafter resetting state.After setting
setState("idle"), React's state update is asynchronous, butstartDownloadis called synchronously after. This works becausestartDownloadsets state to"downloading"immediately, but thehasStarted.current = falsereset followed by immediatestartDownload()call is slightly confusing.This is functional but could be clearer. Consider letting the auto-start effect trigger the download after state resets to idle, or document why this direct call is safe.
src-pyloid/app_controller.py (2)
257-290: Test recording method works correctly.The logging covers all key points in the test flow. The exception handling returns appropriate error dictionaries to the caller.
The static analyzer suggests moving Line 286's success log into an else block after the exception handler (TRY300). The current structure works but could be slightly more explicit:
try: settings = self.settings_service.get_settings() text = self.transcription_service.transcribe( audio, language=settings.language, ) - model_log.info("Test transcription complete", text_length=len(text) if text else 0) - return {"success": True, "transcript": text or ""} except Exception as e: model_log.error("Test transcription error", error=str(e)) return {"success": False, "error": str(e), "transcript": ""} + else: + model_log.info("Test transcription complete", text_length=len(text) if text else 0) + return {"success": True, "transcript": text or ""}
306-311: Reset logic works but could be more encapsulated.The logging is appropriate. However, Line 311 directly accesses the private
_settingsattribute to clear the cache.Consider adding a cache-clearing method to
SettingsServiceto avoid direct access to private members:def reset_all_data(self): """Reset all data and return to fresh state.""" window_log.info("Resetting all user data") self.db.reset_all_data() - # Reset settings service cache - self.settings_service._settings = None + self.settings_service.clear_cache()Then add to
SettingsService:def clear_cache(self): """Clear the cached settings.""" self._settings = None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
src-pyloid/app_controller.py(8 hunks)src-pyloid/main.py(11 hunks)src-pyloid/server.py(4 hunks)src-pyloid/services/audio.py(4 hunks)src-pyloid/services/clipboard.py(3 hunks)src-pyloid/services/database.py(3 hunks)src-pyloid/services/hotkey.py(5 hunks)src-pyloid/services/logger.py(1 hunks)src-pyloid/services/model_manager.py(1 hunks)src-pyloid/services/settings.py(2 hunks)src-pyloid/services/transcription.py(4 hunks)src-pyloid/tests/test_logger.py(1 hunks)src-pyloid/tests/test_model_manager.py(1 hunks)src/App.tsx(5 hunks)src/components/ModelDownloadModal.tsx(1 hunks)src/components/ModelDownloadProgress.tsx(1 hunks)src/components/ModelRecoveryModal.tsx(1 hunks)src/components/SettingsTab.tsx(6 hunks)src/components/Sidebar.tsx(2 hunks)src/lib/api.ts(3 hunks)src/lib/types.ts(1 hunks)src/pages/Onboarding.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
src/components/ModelDownloadModal.tsx (2)
src/components/ui/dialog.tsx (5)
Dialog(131-131)DialogContent(133-133)DialogHeader(136-136)DialogTitle(139-139)DialogDescription(134-134)src/components/ModelDownloadProgress.tsx (1)
ModelDownloadProgress(36-254)
src-pyloid/services/database.py (1)
src-pyloid/services/logger.py (5)
get_logger(207-236)debug(104-106)debug(240-242)info(108-110)info(245-247)
src/components/ModelDownloadProgress.tsx (2)
src/lib/types.ts (2)
DownloadProgress(46-53)DownloadComplete(55-61)src/lib/api.ts (1)
api(4-89)
src/lib/types.ts (1)
src-pyloid/services/model_manager.py (2)
ModelInfo(65-69)DownloadProgress(73-80)
src-pyloid/tests/test_model_manager.py (1)
src-pyloid/services/model_manager.py (9)
CancelToken(51-61)is_cancelled(59-61)cancel(55-57)ModelInfo(65-69)DownloadProgress(73-80)is_model_cached(188-223)get_model_info(225-242)download_model(244-266)get_cache_path(168-176)
src-pyloid/services/hotkey.py (1)
src-pyloid/services/logger.py (5)
get_logger(207-236)info(108-110)info(245-247)debug(104-106)debug(240-242)
src-pyloid/services/settings.py (3)
src-pyloid/services/logger.py (3)
get_logger(207-236)info(108-110)info(245-247)src-pyloid/tests/test_settings.py (1)
db(9-13)src-pyloid/services/database.py (1)
set_setting(61-69)
src/lib/api.ts (2)
src-pyloid/services/model_manager.py (1)
ModelInfo(65-69)src/lib/types.ts (1)
ModelInfo(40-44)
src-pyloid/services/transcription.py (1)
src-pyloid/services/logger.py (3)
get_logger(207-236)debug(104-106)debug(240-242)
src-pyloid/services/audio.py (1)
src-pyloid/services/logger.py (7)
get_logger(207-236)info(108-110)info(245-247)warning(112-114)warning(250-252)debug(104-106)debug(240-242)
src/components/Sidebar.tsx (1)
src/lib/api.ts (1)
api(4-89)
src/components/SettingsTab.tsx (2)
src/lib/api.ts (1)
api(4-89)src/components/ModelDownloadModal.tsx (1)
ModelDownloadModal(17-47)
src-pyloid/tests/test_logger.py (1)
src-pyloid/services/logger.py (12)
reset_logging(177-204)get_logger(207-236)setup_logging(126-174)info(108-110)info(245-247)error(116-118)error(255-257)debug(104-106)debug(240-242)warning(112-114)warning(250-252)get_default_log_path(38-40)
src/pages/Onboarding.tsx (1)
src/components/ModelDownloadProgress.tsx (1)
ModelDownloadProgress(36-254)
src-pyloid/server.py (1)
src-pyloid/services/model_manager.py (6)
CancelToken(51-61)DownloadProgress(73-80)is_cancelled(59-61)cancel(55-57)is_model_cached(188-223)download_model(244-266)
src-pyloid/main.py (2)
src-pyloid/server.py (1)
register_download_progress_callback(22-25)src-pyloid/services/logger.py (8)
setup_logging(126-174)get_logger(207-236)info(108-110)info(245-247)error(116-118)error(255-257)debug(104-106)debug(240-242)
src-pyloid/services/model_manager.py (5)
src-pyloid/services/logger.py (5)
get_logger(207-236)error(116-118)error(255-257)info(108-110)info(245-247)src-pyloid/server.py (1)
on_progress(257-266)src-pyloid/tests/test_model_manager.py (2)
on_progress(160-161)on_progress(252-253)src-pyloid/app_controller.py (1)
load_model(76-88)src-pyloid/services/transcription.py (1)
load_model(23-42)
🪛 Ruff (0.14.8)
src-pyloid/tests/test_model_manager.py
183-183: Unused lambda argument: p
(ARG005)
195-195: Unused lambda argument: p
(ARG005)
207-207: Unused lambda argument: p
(ARG005)
256-256: Unused function argument: cancel_token
(ARG001)
src-pyloid/services/audio.py
33-33: Unused method argument: frames
(ARG002)
33-33: Unused method argument: time
(ARG002)
src-pyloid/tests/test_logger.py
29-29: Unused function argument: reset_logger_state
(ARG001)
46-47: try-except-pass detected, consider logging the exception
(S110)
46-46: Do not catch blind exception: Exception
(BLE001)
156-156: Ambiguous variable name: l
(E741)
src-pyloid/services/logger.py
240-240: Unused function argument: args
(ARG001)
240-240: Unused function argument: kwargs
(ARG001)
245-245: Unused function argument: args
(ARG001)
245-245: Unused function argument: kwargs
(ARG001)
250-250: Unused function argument: args
(ARG001)
250-250: Unused function argument: kwargs
(ARG001)
255-255: Unused function argument: args
(ARG001)
255-255: Unused function argument: kwargs
(ARG001)
260-260: Unused function argument: args
(ARG001)
260-260: Unused function argument: kwargs
(ARG001)
src-pyloid/app_controller.py
83-83: Do not catch blind exception: Exception
(BLE001)
174-174: Do not catch blind exception: Exception
(BLE001)
287-287: Consider moving this statement to an else block
(TRY300)
288-288: Do not catch blind exception: Exception
(BLE001)
297-297: Starting a process without a shell
(S606)
298-298: Do not catch blind exception: Exception
(BLE001)
src-pyloid/server.py
281-281: Do not catch blind exception: Exception
(BLE001)
src-pyloid/main.py
83-83: Do not catch blind exception: Exception
(BLE001)
84-84: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
111-111: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
174-174: Do not catch blind exception: Exception
(BLE001)
175-175: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
184-184: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
210-210: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
240-240: Do not catch blind exception: Exception
(BLE001)
241-241: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
257-257: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
350-350: Do not catch blind exception: Exception
(BLE001)
351-351: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src-pyloid/services/model_manager.py
96-96: Unused method argument: kwargs
(ARG002)
109-109: Avoid specifying long messages outside the exception class
(TRY003)
138-138: Do not catch blind exception: Exception
(BLE001)
219-219: Consider moving this statement to an else block
(TRY300)
221-221: Do not catch blind exception: Exception
(BLE001)
328-328: Do not catch blind exception: Exception
(BLE001)
353-353: Do not catch blind exception: Exception
(BLE001)
400-400: Avoid specifying long messages outside the exception class
(TRY003)
444-444: Do not assign a lambda expression, use a def
Rewrite on_progress as a def
(E731)
444-444: Unused lambda argument: p
(ARG005)
451-451: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (53)
src-pyloid/services/logger.py (3)
43-78: LGTM! Well-structured hybrid formatter.The
HybridFormattercorrectly handles timestamp formatting, level name mapping (WARNING → WARN), domain extraction from logger name, and optional structured JSON data appending.
126-175: LGTM! Robust logging setup with proper rotation.The
setup_loggingfunction correctly initializes rotating file handlers with 100MB max size, creates the log directory if needed, and configures both file and console handlers with the hybrid formatter.
177-205: LGTM! Good test isolation support.The
reset_loggingfunction properly closes handlers, clears the root logger, removes all domain loggers, and resets global state for clean test isolation.src/components/Sidebar.tsx (1)
107-122: LGTM! Clean migration to internal URL handling.Replacing anchor tags with buttons that call
api.openExternalUrl()provides consistent external URL handling through the backend. The styling and accessibility are preserved.Consider adding error handling to surface failures to the user:
onClick={() => api.openExternalUrl(GITHUB_REPO_URL).catch(console.error)}src-pyloid/services/audio.py (2)
6-8: LGTM! Proper domain-based logging setup.The module correctly imports
get_loggerand creates a domain-specific logger for "audio". This aligns with the project-wide logging standardization.
24-35: LGTM! Good use of structured logging.The logging calls appropriately use structured data via kwargs (
device_id=,status=) which will be formatted as JSON in the hybrid log format. This makes logs more searchable and parseable.Also applies to: 64-74
src-pyloid/services/settings.py (2)
4-6: LGTM! Domain-based logger correctly initialized.The settings module properly imports and initializes its domain-specific logger, consistent with the project-wide logging standardization.
73-98: LGTM! Good observability for settings changes.The
updateddictionary pattern cleanly tracks which settings changed, and the structured logging with**updatedprovides excellent visibility into configuration changes. This will be valuable for debugging user-reported issues.src-pyloid/services/transcription.py (4)
5-8: LGTM!The import of
get_loggerandMODEL_REPOS, along with the module-level logger initialization using the "model" domain, correctly follows the new domain-based logging pattern established in this PR.
11-13: LGTM!The helper function provides a clean abstraction for model ID lookup with a safe fallback to the original
model_namewhen no mapping exists inMODEL_REPOS.
31-36: LGTM!Using
model_idforWhisperModelwhile preserving the user-facingmodel_namein_current_model_namecorrectly separates the HuggingFace repo ID from the display/selection name.
74-74: LGTM!Structured logging with keyword arguments follows the
DomainLoggerpattern and provides useful debugging context for audio processing and transcription results.Also applies to: 89-89
src/components/ModelRecoveryModal.tsx (1)
1-60: LGTM!The modal correctly implements a blocking recovery flow:
- Prevents dismissal via outside click and Escape key, ensuring the user must complete the download
- Uses
autoStart={true}for immediate download initiation- Gates the
onCompletecallback on success, leaving the retry UI visible on failureThe component is well-structured with proper accessibility considerations through
DialogHeader.src-pyloid/services/database.py (3)
6-8: LGTM!The import and logger initialization correctly follow the domain-based logging pattern with the "database" domain.
173-173: LGTM!Using
**resultto spread the stats dictionary as structured log data is a clean approach for debugging statistics retrieval.
189-189: LGTM!Using
log.infofor the reset operation is appropriate since it's a significant action that warrants visibility in logs.src-pyloid/services/hotkey.py (3)
4-6: LGTM!The import and logger initialization correctly follow the domain-based logging pattern with the "hotkey" domain.
33-33: LGTM!Log levels are appropriately chosen:
infofor user-facing activation/deactivation events,debugfor internal force deactivation.Also applies to: 62-62, 68-68
80-80: LGTM!The structured logging with keyword arguments (
hotkey=,key=,ctrl_pressed=,win_pressed=) provides useful debugging context for hotkey registration and key release handling.Also applies to: 99-99
src/components/ModelDownloadModal.tsx (2)
30-35: LGTM!Using
sr-onlyfor theDialogHeaderprovides accessibility for screen readers while keeping the visual design focused on the progress UI.
36-43: LGTM!The
ModelDownloadProgressintegration withautoStart={true}and proper callback wiring correctly delegates download control to the child component.src-pyloid/services/clipboard.py (1)
4-6: LGTM! Domain-based logging migration looks correct.The switch from direct
debug/infoimports toget_logger("clipboard")aligns with the PR's domain-based logging strategy. The structured logging with keyword arguments (e.g.,text_length=len(text)) follows the hybrid format pattern.src/components/SettingsTab.tsx (1)
437-444: Modal integration looks correct.The conditional rendering guards against rendering with an empty
modelName, and the handlers are properly wired to manage the download lifecycle.src-pyloid/tests/test_model_manager.py (1)
1-15: Comprehensive test coverage for the Model Manager API.The test suite covers the key components:
CancelToken,ModelInfo,DownloadProgress,MODEL_SIZES, andModelManagerbehaviors including caching, progress callbacks, and cancellation. Good separation between unit and integration tests.src/lib/types.ts (2)
40-44: Ensure snake_case to camelCase conversion is handled.Python uses
size_byteswhile TypeScript expectssizeBytes. Verify that the RPC serialization layer handles this conversion, otherwisesizeByteswill beundefinedon the frontend.
46-53: No action required. The field naming difference between Python'smodel_nameand TypeScript'smodelis intentionally handled by the serialization layer in the backend (src-pyloid/server.pyline 260), which mapsprogress.model_nameto the"model"key when sending download progress events to the frontend.Likely an incorrect or invalid review comment.
src/App.tsx (2)
45-74: Model recovery flow looks well-structured.The nested try-catch appropriately handles model cache check failures gracefully, allowing the app to continue (letting the backend handle model loading errors if they occur later). The flow correctly sets
currentModelbefore checking cache status.
114-118: Modal rendering pattern is correct.The
ModelRecoveryModalis always rendered but visibility is controlled by theopenprop. SincecurrentModelis set beforemodelMissingcan become true, the modal will always have a valid model name when open.src/lib/api.ts (2)
53-55: LGTM!The
openExternalUrlmethod follows the established pattern and correctly delegates to the backend RPC.
77-88: LGTM!The model management API methods are well-structured:
- Correct parameter naming convention (camelCase to snake_case for RPC)
- Return types align with
ModelInfoand backend response shapes fromsrc/lib/types.ts- Consistent with existing API patterns in this file
src-pyloid/tests/test_logger.py (3)
28-47: Good fixture design for test isolation.The dependency on
reset_logger_stateensures proper cleanup ordering, which is important for file handle management on Windows. Thetry-except-passpattern is acceptable here for best-effort cleanup.Consider adding a brief inline comment explaining that
reset_logger_stateis used for ordering rather than direct usage to silence the static analysis hint (ARG001).
84-96: LGTM!Comprehensive validation that all expected domains are present and functional. The explicit assertion on
VALID_DOMAINSensures the contract is maintained.
213-229: Good rotation test with small threshold.Using
max_bytes=1000for testing rotation is a pragmatic approach to verify the rotation mechanics without writing 100MB.src/pages/Onboarding.tsx (3)
537-545: Consider handling the failure case inhandleDownloadComplete.When
successisfalse, the function silently does nothing. The user remains on the download step with no way to proceed or go back (since navigation is hidden). TheModelDownloadProgresscomponent shows an error state with a retry button, but if the user doesn't retry, they're stuck.Consider adding an
elsebranch to handle failure, or ensure the download component's error UI is sufficient for recovery.
645-653: LGTM!Proper integration of the download step with auto-start and callback handlers. The component will manage its own lifecycle and communicate completion/cancellation back to the onboarding flow.
670-671: Good UX decision to hide navigation during download.Hiding the navigation prevents users from accidentally navigating away during an active download, which could leave the download in an inconsistent state. The download component provides its own cancel mechanism.
Also applies to: 742-782
src-pyloid/server.py (3)
152-158: LGTM!The
open_external_urlmethod is straightforward. The inline import ofwebbrowseris acceptable since it's a standard library module and keeps the module-level imports focused on the core functionality.
268-288: Exception handling is appropriate for resilience.The broad
Exceptioncatch (line 281) is acceptable here since this is a background thread that must not crash silently. The error is logged and the completion event is sent to the frontend with error details, ensuring the UI can display the failure.
212-221: LGTM!Proper conversion from Python snake_case (
size_bytes) to JavaScript camelCase (sizeBytes) in the response, maintaining API consistency with the frontend types.src/components/ModelDownloadProgress.tsx (3)
15-32: LGTM!Well-implemented utility functions with proper edge case handling:
formatByteshandles zero bytesformatEtahandles zero/negative/infinite values gracefully
47-70: LGTM!Event handlers are properly memoized with
useCallbackand have correct dependency arrays. The state machine transitions are handled cleanly.
127-186: LGTM!Clear and user-friendly UI states for completed, error, and cancelled scenarios. Each state provides appropriate feedback and recovery actions.
src-pyloid/app_controller.py (5)
12-17: LGTM!Clean domain-based logger initialization. The separation into model, audio, and window domains provides clear observability boundaries for different lifecycle concerns.
76-88: LGTM!The model loading logic correctly uses structured logging with domain context and ensures
_model_loadingstate is reset via the finally block. The broad exception handling is appropriate here since errors are logged, forwarded to the UI, and state is properly cleaned up.
103-126: LGTM!Hotkey handlers use appropriate log levels and structured context. The debug level for disabled popup is correct, and the warning for empty audio properly flags an unexpected condition.
129-182: LGTM!Transcription flow has comprehensive logging throughout the lifecycle with appropriate structured context. Exception handling correctly logs errors, notifies the UI, and ensures the completion callback always fires to reset UI state.
292-299: LGTM!The method correctly logs the action with path context. The static analyzer flags
os.startfileas a security concern (S606), but this is a false positive sincefolder_pathis derived from an internal database path (self.db.db_path.parent) with no user input—there's no command injection risk.src-pyloid/main.py (6)
6-12: LGTM!Clean migration to domain-based logging. Calling
setup_logging()first ensures logging is configured before any log messages, and using the "window" domain aligns with the UI lifecycle concerns in this file.
68-85: LGTM!Screen detection logic correctly uses structured logging. The static analyzer suggests
logging.exception(TRY400), but this doesn't apply toDomainLogger, which uses a custom structured logging pattern with explicit error context via kwargs.
87-241: LGTM!Popup lifecycle management has comprehensive logging throughout initialization, resize, event handling, and teardown. The broad exception handling is appropriate for UI lifecycle methods, and all errors are logged with structured context. Static analyzer suggestions (TRY400) don't apply to the custom
DomainLogger.
260-262: LGTM!Clean adapter to forward download progress from the model manager to the frontend. The function signature matches the callback interface defined in
server.py.
266-268: LGTM!Callback registration correctly wires up the download progress event flow from the model manager through to the main window UI.
282-364: LGTM!Startup sequence is well-instrumented with structured logging. The conditional initialization based on
onboarding_completeis clearly logged, making it easy to trace the application's entry state.
| # Start download in daemon thread | ||
| thread = threading.Thread(target=download_thread, daemon=True) | ||
| thread.start() | ||
|
|
||
| # Wait for completion while checking for cancellation | ||
| while thread.is_alive(): | ||
| if cancel_token.is_cancelled(): | ||
| log.info("Model download cancelled by user", model=model_name) | ||
| # Thread is daemon, so it will be abandoned when we return | ||
| return False | ||
| thread.join(timeout=0.1) # Check every 100ms |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.py" | grep -E "(model_manager|download)" | head -20Repository: infiniV/VoiceFlow
Length of output: 140
🏁 Script executed:
cd src-pyloid/services && grep -n "def download_thread" model_manager.pyRepository: infiniV/VoiceFlow
Length of output: 95
🏁 Script executed:
cd src-pyloid/services && head -400 model_manager.py | tail -100Repository: infiniV/VoiceFlow
Length of output: 3735
🏁 Script executed:
grep -B 20 -A 30 "Start download in daemon thread" src-pyloid/services/model_manager.pyRepository: infiniV/VoiceFlow
Length of output: 1898
🏁 Script executed:
grep -B 50 "def download_thread" src-pyloid/services/model_manager.py | head -70Repository: infiniV/VoiceFlow
Length of output: 2141
🏁 Script executed:
grep -A 5 "snapshot_download" src-pyloid/services/model_manager.py | head -20Repository: infiniV/VoiceFlow
Length of output: 758
🏁 Script executed:
sed -n '300,360p' src-pyloid/services/model_manager.pyRepository: infiniV/VoiceFlow
Length of output: 2445
🏁 Script executed:
grep -B 15 "def download_model" src-pyloid/services/model_manager.py | head -25Repository: infiniV/VoiceFlow
Length of output: 456
🏁 Script executed:
sed -n '250,320p' src-pyloid/services/model_manager.pyRepository: infiniV/VoiceFlow
Length of output: 2604
Daemon thread abandonment on cancellation may waste resources.
When download is cancelled, the daemon thread continues running snapshot_download() in the background with no way to stop it. The main thread simply returns without signaling cancellation to the thread, which keeps consuming bandwidth and writing to disk until the process exits or the download completes.
While huggingface_hub does handle partial files with resumable download support, the abandoned thread pattern is problematic for:
- Bandwidth waste during long downloads
- Delayed process termination
- Inconsistent application state
Consider passing a cancellation mechanism (flag or threading.Event) to the download thread so it can gracefully exit and clean up resources when cancel_token.is_cancelled() is detected.
🤖 Prompt for AI Agents
In src-pyloid/services/model_manager.py around lines 356-366, the download
thread is started as a daemon and abandoned on cancellation; instead, modify the
code so the main thread signals a cancellation event to the download thread and
waits for it to stop. Concretely: create a threading.Event (e.g., cancel_event)
and pass it into download_thread (and into any lower-level download helper);
inside download_thread/snapshot_download wrapper periodically check
cancel_event.is_set() and abort/cleanup the streaming download if set; on
cancel_token.is_cancelled() call cancel_event.set() then join the thread (with
timeout loop) and return only after the thread has terminated or a short
timeout, removing the daemon flag so the worker can exit cleanly and resources
(partial files, network streams) are cleaned up.
- Fix logger.py exception method to capture traceback by using logger.exception() which automatically sets exc_info=True - Replace fragile byte update heuristic (n > 100) with explicit unit="B" check in model_manager.py download progress tracking
Logging Infrastructure
Model Manager
Onboarding Download Step
Settings Modal
Startup and Recovery
Additional Fixes:
Technical Details:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.