refactor(models): consolidate model catalog and deepen download flow - #33
Merged
Conversation
- settings: Settings dataclass is the single schema (DB serialization, defaults, camelCase RPC aliases all derived); fixes broken prependSpace toggle and adds drift-guard tests - recordings: RecordingsRepository extracted per domain glossary; job status protocol helpers; per-job transcribe overrides on _Job; enqueue/status race fixed; LoopbackDiscovery seam over the three platform backends - ptt: DictationPipeline extracted from AppController (release-to-paste flow, now unit-tested); stale model-reload snapshot fix - main: UiEventBridge replaces the two parallel backend->UI signal pathways; Hyprland integration moved to services/hyprland.py - frontend: useBackendEvent hook (6 subscriptions deduped), Onboarding split into pages/onboarding/ steps, useAudioSourcePreview extracted - docs: backfill ADRs 0001-0003; add lint/typecheck scripts Backend suite: 271 -> 332 passing; tsc and eslint clean (baseline warnings only)
- ci.yml: lint + typecheck + version check + backend tests on every PR and push to main (first CI test coverage for this repo) - scripts/version.mjs: bump/check for the five version files; wired as pnpm run version:bump / version:check; pnpm run check runs the full gate - release.yml: fail fast when the tag doesn't match package.json; verify the Linux artifact's audio-library layout (bundled libasound/libportaudio gone, PyAV av.libs copy intact) — regression guard for the v1.3x-v1.5.0 'no mics in release build' bug - test_clipboard.py: skip real-clipboard integration tests when headless - CLAUDE.md: document new commands + Meeting Mode overview
Parallel job running pnpm run build (Vite + PyInstaller) with an ldd smoke check on every PR — catches spec/bundling drift before release tags. No artifact upload; free runners (public repo).
- add services/model_catalog.py as the single source of truth for model names, sizes, and HF repo ids; drop the duplicated copies in model_manager, settings, and transcription - get_model_info now returns repoId; frontend derives HuggingFace URLs and picker metadata from it instead of hardcoded maps (removes the drifted 6-entry MODEL_META in SettingsTab) - ModelManager owns a single-flight download session via start_download/cancel_download/get_download_status, replacing the mutable module globals in server.py - add useModelDownload hook owning the download state machine; it re-attaches to an in-flight download via get_download_status - consolidate model loading into TranscriptionService and remove the unused ModelManager.load_model/ensure_model_ready cpu-only path - wrap DialogOverlay in forwardRef
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR expands VoiceFlow's Meeting Mode (long-form recording) architecture by introducing persistent recording layers, centralizing model management, refactoring event delivery, and decomposing onboarding UI into separate modules. It adds CI/release validation, settings RPC schema mapping, and comprehensive testing infrastructure. ChangesMeeting Mode implementation and platform refactor
Sequence DiagramssequenceDiagram
participant Backend as Backend<br/>(Hotkey Thread)
participant Pipeline as DictationPipeline
participant Transcription as TranscriptionService
participant Clipboard as ClipboardService
participant DB as DatabaseService
participant Bridge as UiEventBridge
participant UI as Frontend<br/>(Qt Main)
Backend->>Pipeline: run(audio)
Pipeline->>Pipeline: wait_for_model()
Pipeline->>Transcription: transcribe(audio, language)
Transcription-->>Pipeline: text
Pipeline->>Clipboard: write(text)
Pipeline->>DB: create history entry & audio attachment
Pipeline-->>Backend: pasted text
Backend->>Bridge: emit_event("ptt-transcription-complete", text)
Bridge-->>UI: on main thread via queued signal
UI->>UI: update popup with transcribed text
sequenceDiagram
participant Frontend as Frontend<br/>(useModelDownload)
participant API as Backend RPC
participant Manager as ModelManager
participant Worker as Download Thread
participant Bridge as UiEventBridge
Frontend->>API: startModelDownload(modelName)
API->>Manager: start_download(modelName, emit)
alt Model already cached
Manager-->>API: emit download-complete (alreadyCached=True)
API-->>Frontend: {alreadyCached: true}
else Download needed
Manager->>Worker: spawn daemon download thread
Worker->>Worker: download_model()
Worker->>Manager: emit download-progress (progress dict)
Manager->>Bridge: emit_event("download-progress", progress)
Bridge-->>Frontend: CustomEvent with progress
Frontend->>Frontend: update state/progress
Worker->>Manager: emit download-complete
Manager->>Bridge: emit_event("download-complete", {cancelled: false})
Bridge-->>Frontend: CustomEvent
Frontend->>Frontend: state = completed
end
Frontend->>API: getDownloadStatus()
API->>Manager: get_download_status()
Manager-->>API: {active: false/true, ...progress}
API-->>Frontend: DownloadStatus
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
infiniV
marked this pull request as ready for review
June 11, 2026 12:47
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
.github/workflows/ci.yml (1)
1-11: ⚡ Quick winConsider security hardening for CI workflow.
The workflow has several security hygiene gaps flagged by static analysis:
No
permissionsblock — defaults to broad permissions onpull_requestfrom forks. Adding explicit least-privilege permissions is recommended:permissions: contents: readUnpinned action references — using
@v4tags is common but pinning to commit SHAs prevents supply-chain attacks if an action is compromised. For a private/low-risk repo this is acceptable; for public repos consider pinning.
persist-credentials: falseonactions/checkout— prevents the token from persisting in the runner's git config.These are defense-in-depth measures. Given this is a draft PR and the CI is functional, this can be addressed in a follow-up.
🔒 Example hardening for the test job
name: CI on: pull_request: push: branches: [main] +permissions: + contents: read + concurrency: group: ci-${{ github.ref }} cancel-in-progress: true jobs: test: runs-on: ubuntu-22.04 timeout-minutes: 20 env: QT_QPA_PLATFORM: offscreen steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + persist-credentials: falseAlso applies to: 20-20, 33-33, 38-38, 47-47, 83-83, 95-95, 100-100, 109-109
🤖 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 @.github/workflows/ci.yml around lines 1 - 11, Add CI security hardening: insert an explicit least-privilege permissions block (e.g., permissions: contents: read) at the top of the workflow, pin third‑party actions (replace tags like `@v4` with specific commit SHAs) for critical steps such as the actions/checkout usage, and ensure actions/checkout has persist-credentials: false in its step configuration; apply these changes consistently to every job and every uses: of third‑party actions within this workflow.Source: Linters/SAST tools
src-pyloid/services/recording/repository.py (1)
163-215: 💤 Low valueConsider simplifying the dynamic SET clause construction.
The
sets.insert(1, ...)/params.insert(-1, ...)pattern works but is subtle and order-dependent. A clearer approach builds the clause additively:♻️ Suggested simplification
def update_transcript_status( self, recording_id: int, status: str, progress: Optional[float] = None, error: Optional[str] = None, ) -> None: - sets = ["transcript_status = ?", "updated_at = ?"] - params: list = [status, datetime.now().isoformat()] - if progress is not None: - sets.insert(1, "transcript_progress = ?") - params.insert(1, progress) - if error is not None: - sets.insert(-1, "transcript_error = ?") - params.insert(-1, error) - params.append(recording_id) + sets = ["transcript_status = ?"] + params: list = [status] + if progress is not None: + sets.append("transcript_progress = ?") + params.append(progress) + if error is not None: + sets.append("transcript_error = ?") + params.append(error) + sets.append("updated_at = ?") + params.append(datetime.now().isoformat()) + params.append(recording_id) conn = self._get_connection() ...🤖 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 `@src-pyloid/services/recording/repository.py` around lines 163 - 215, The dynamic SET construction in update_transcript_status and update_summary_status is fragile due to index-based inserts; instead build sets and params additively in order: initialize sets = ["transcript_status = ?", "updated_at = ?"] (or "summary_status = ?" for update_summary_status) and params = [status, datetime.now().isoformat()], then if progress is not None do sets.append("transcript_progress = ?") / "summary_progress = ?" and params.append(progress), then if error is not None append the appropriate "..._error = ?" and params.append(error), and (for update_summary_status) if provider is not None append "summary_provider = ?" and params.append(provider); finally append recording_id to params and execute the UPDATE with ", ".join(sets). This removes subtle index manipulation and preserves clear, predictable ordering of placeholders and parameters for conn.execute.src-pyloid/services/recording/controller.py (1)
561-572: 💤 Low valueRedundant audio file deletion.
delete_recordingin the controller (lines 563-570) deletes the audio file, thenself.repo.delete_recording(recording_id)(line 571) callsRecordingsRepository.delete_recordingwhich also deletes the audio file viaself._db._delete_audio_file. The second deletion silently fails because the file is already gone.Either remove the inline deletion here and let the repository handle it, or have the repository skip file deletion (repository becomes pure DB layer).
♻️ Option: let repository handle file deletion
def delete_recording(self, recording_id: int) -> dict: - row = self.repo.get_recording(recording_id) - if row and row.get("audio_relpath"): - audio_path = (self.data_root / row["audio_relpath"]).resolve() - try: - audio_path.relative_to(self.data_root.resolve()) - if audio_path.exists(): - audio_path.unlink() - except (ValueError, OSError): - pass self.repo.delete_recording(recording_id) return {"ok": True}🤖 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 `@src-pyloid/services/recording/controller.py` around lines 561 - 572, The controller currently duplicates audio deletion: remove the inline filesystem deletion in Controller.delete_recording (the block that resolves audio_path, checks relative_to, exists(), and unlinks) and rely on self.repo.delete_recording to perform the file removal; update Controller.delete_recording to simply fetch the row (if needed) and call self.repo.delete_recording(recording_id) returning {"ok": True}, leaving actual file removal to RecordingsRepository.delete_recording (which calls _delete_audio_file).src-pyloid/services/dictation.py (1)
21-21: ⚡ Quick winUse domain-based logging per coding guidelines.
The file imports bare
info,warning,errorhelpers instead of using the domain-basedget_logger(domain)pattern. Per coding guidelines, logging should useget_logger("dictation")for structured logging with domain prefixes.-from services.logger import info, warning, error +from services.logger import get_logger + +log = get_logger("dictation")Then replace calls like
info(...)→log.info(...),warning(...)→log.warning(...),error(...)→log.error(...).🤖 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 `@src-pyloid/services/dictation.py` at line 21, The module currently imports bare helpers from services.logger (from services.logger import info, warning, error) instead of using domain-based logging; change to obtain a domain logger via get_logger("dictation") (e.g., log = get_logger("dictation")) and then update all usages: replace info(...) with log.info(...), warning(...) with log.warning(...), and error(...) with log.error(...), ensuring you import get_logger from services.logger and remove the old direct imports.Source: Coding guidelines
src-pyloid/tests/test_dictation.py (1)
48-57: 💤 Low valueMinor: unused
transcriptionvariable (per Ruff RUF059).The static analysis flagged that
transcriptionis unpacked but never used. You can prefix it with underscore to signal intent:- pipeline, transcription, clipboard = make_pipeline(db) + pipeline, _transcription, clipboard = make_pipeline(db)🤖 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 `@src-pyloid/tests/test_dictation.py` around lines 48 - 57, The test_happy_path_pastes_and_saves_history unpacks a second value into transcription that is never used; change the unpacking from "pipeline, transcription, clipboard = make_pipeline(db)" to use an underscore-prefixed name (e.g., "pipeline, _transcription, clipboard = make_pipeline(db)") or simply "_" (e.g., "pipeline, _, clipboard = make_pipeline(db)") so the unused variable intent is explicit and Ruff RUF059 is satisfied.Source: Linters/SAST tools
src/lib/models.ts (1)
1-1: ⚡ Quick winUse the
@/alias for localsrcimports in new TS modules.Line [1] should use the project alias instead of a relative
./path to match the repository import contract.As per coding guidelines,
src/**/*.tsimports fromsrc/should use the@/path alias.Proposed change
-import { MODEL_OPTIONS } from "./constants"; +import { MODEL_OPTIONS } from "`@/lib/constants`";🤖 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 `@src/lib/models.ts` at line 1, Update the import in models.ts to use the project alias instead of a relative path: replace the local import of MODEL_OPTIONS (import { MODEL_OPTIONS } from "./constants";) with the aliased import using "`@/constants`" so the module follows the repository's src/ alias convention.Source: Coding guidelines
🤖 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 `@CLAUDE.md`:
- Line 83: Update the documented event names emitted by MeetingsController to
match the backend contract used in tests: replace any occurrences of
"meetings.transcribe-progress" and "meetings.summarize-progress" with
"recording-transcribe-progress" and "recording-summarize-progress" (this applies
to the description referencing controller.py / MeetingsController and the
duplicate mentions at lines ~172-173); ensure the CLAUDE.md text that describes
emitted events and their emitter in main.py uses the new "recording-..." event
names consistently.
In `@src-pyloid/services/dictation.py`:
- Around line 128-152: The duration_ms calculation is wrong for multi-channel
audio because len(audio_int16) counts samples×channels; compute frames =
len(audio_int16) // self.audio_service.CHANNELS (or len(...) / CHANNELS) and
then calculate duration_ms = int((frames /
float(self.audio_service.SAMPLE_RATE)) * 1000). Update the logic around
audio_int16 and duration_ms (references: audio_int16,
self.audio_service.CHANNELS, self.audio_service.SAMPLE_RATE, duration_ms) to use
frames per channel (handle non-divisible lengths defensively) before converting
to milliseconds and keep the rest of the WAV writing unchanged.
In `@src-pyloid/services/hyprland.py`:
- Around line 69-70: The print-based warning in hyprctl result handling should
be replaced with the domain logger: import and call get_logger with the hyprland
domain (e.g., get_logger("hyprland")) and replace the print(...) in the code
that handles the subprocess result (the block around the rule rejection
handling) with logger.warning(f"hyprctl rejected rule {rule!r}:
{result.stderr.strip() or result.stdout.strip()}"); remove the flush=True and
the print call so structured logging (and configured rotation) is used instead.
In `@src-pyloid/services/model_catalog.py`:
- Around line 67-69: The get_repo_id function currently fabricates a HuggingFace
repo ID for unknown model names which hides catalog drift; change get_repo_id to
validate model_name against the MODEL_REPOS mapping and raise a clear exception
(e.g., KeyError or custom ValueError) when model_name is not present instead of
returning a synthesized f"Systran/faster-whisper-{model_name}" string; update
any callers to handle or surface this error so missing catalog entries are
caught early.
In `@src/components/meetings/useAudioSourcePreview.ts`:
- Around line 22-25: When switching to live mode (inside useAudioSourcePreview
where isLive triggers activeKey.current = "" and return) and inside the clear()
implementation, also stop the backend preview RPC/stream so the capture device
is released; add a call to the existing preview-stop mechanism (e.g. invoke the
preview stop RPC or cancel the active preview stream) just before or immediately
after clearing activeKey.current. Update both the isLive branch and the clear()
path to call the same stopPreview/stopPreviewStream helper so the preview RPC is
terminated and recordingsStart will not be blocked.
In `@src/hooks/useBackendEvent.ts`:
- Around line 17-19: The listener unconditionally casts Event to CustomEvent and
dereferences detail; add a runtime guard inside listener to ensure the incoming
event is actually a CustomEvent before calling handlerRef.current. Specifically,
in the listener function check e.g. event instanceof CustomEvent (or typeof
(event as any).detail !== "undefined") and that handlerRef.current is defined,
then call handlerRef.current((event as CustomEvent<T>).detail); otherwise ignore
or log a warning to avoid passing malformed events to downstream handlers.
Ensure you update the listener symbol in useBackendEvent to include this guard.
In `@src/hooks/useModelDownload.ts`:
- Around line 73-89: In useModelDownload's start callback, when
api.startModelDownload returns result.alreadyCached and you
setState("completed"), also invoke onComplete?.(true) so consumers are notified
the model is ready; update the start callback (the start function inside
useModelDownload) to call onComplete?.(true) in the branch that handles
result.alreadyCached (mirroring the event-driven completion path) while keeping
the same dependency strategy for the hook.
In `@src/pages/onboarding/StepAudio.tsx`:
- Around line 55-69: In handleDeviceChange move the optimistic UI update into
the success path: do not call setMicrophone(backendId) before awaiting
api.updateSettings(...) and api.startTestRecording(); instead call
setMicrophone(backendId) and setIsListening(true) only after those calls
succeed, and in the catch blocks ensure you setIsListening(false) (and
optionally revert microphone state) so the UI cannot show a selected/listening
device when the backend switch failed; reference handleDeviceChange,
setMicrophone, api.updateSettings, api.startTestRecording, setIsListening.
In `@src/pages/onboarding/StepFinal.tsx`:
- Around line 16-22: StepFinal currently hardcodes "Ctrl + Win" in the
onboarding UI; replace that with a dynamic shortcut renderer that returns the
correct platform-specific keys. Add a helper (e.g., getShortcutForPlatform) used
by the StepFinal component to detect the runtime platform (use process.platform
if running in Electron/backend-exposed env or navigator.platform/userAgent
fallback in browser) and map to the appropriate sequences for Windows, macOS,
and Linux (Wayland/X11) / PTT flows. Replace the two hardcoded <kbd> blocks with
a renderShortcutKeys function that iterates the returned key array and renders
matching <kbd> elements and separators so the UI shows the real
configured/default shortcut per platform. Ensure the helper and renderer are
named (getShortcutForPlatform, renderShortcutKeys, StepFinal) so reviewers can
find and test the change.
In `@src/pages/onboarding/StepModel.tsx`:
- Around line 73-84: The language dropdown currently calls setLanguage directly,
allowing users to pick unsupported languages after selecting an English-only
model; update the Select onValueChange handler (or the option rendering) to
consult isEnglishOnlyModel(modelId) and prevent setting anything other than "en"
(e.g., ignore non-"en" values or force "en"), or alternatively disable non-"en"
Select options when isEnglishOnlyModel(modelId) is true so the dropdown cannot
select unsupported languages; adjust the setter usage around setLanguage and the
Select onValueChange to enforce this constraint.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 1-11: Add CI security hardening: insert an explicit
least-privilege permissions block (e.g., permissions: contents: read) at the top
of the workflow, pin third‑party actions (replace tags like `@v4` with specific
commit SHAs) for critical steps such as the actions/checkout usage, and ensure
actions/checkout has persist-credentials: false in its step configuration; apply
these changes consistently to every job and every uses: of third‑party actions
within this workflow.
In `@src-pyloid/services/dictation.py`:
- Line 21: The module currently imports bare helpers from services.logger (from
services.logger import info, warning, error) instead of using domain-based
logging; change to obtain a domain logger via get_logger("dictation") (e.g., log
= get_logger("dictation")) and then update all usages: replace info(...) with
log.info(...), warning(...) with log.warning(...), and error(...) with
log.error(...), ensuring you import get_logger from services.logger and remove
the old direct imports.
In `@src-pyloid/services/recording/controller.py`:
- Around line 561-572: The controller currently duplicates audio deletion:
remove the inline filesystem deletion in Controller.delete_recording (the block
that resolves audio_path, checks relative_to, exists(), and unlinks) and rely on
self.repo.delete_recording to perform the file removal; update
Controller.delete_recording to simply fetch the row (if needed) and call
self.repo.delete_recording(recording_id) returning {"ok": True}, leaving actual
file removal to RecordingsRepository.delete_recording (which calls
_delete_audio_file).
In `@src-pyloid/services/recording/repository.py`:
- Around line 163-215: The dynamic SET construction in update_transcript_status
and update_summary_status is fragile due to index-based inserts; instead build
sets and params additively in order: initialize sets = ["transcript_status = ?",
"updated_at = ?"] (or "summary_status = ?" for update_summary_status) and params
= [status, datetime.now().isoformat()], then if progress is not None do
sets.append("transcript_progress = ?") / "summary_progress = ?" and
params.append(progress), then if error is not None append the appropriate
"..._error = ?" and params.append(error), and (for update_summary_status) if
provider is not None append "summary_provider = ?" and params.append(provider);
finally append recording_id to params and execute the UPDATE with ",
".join(sets). This removes subtle index manipulation and preserves clear,
predictable ordering of placeholders and parameters for conn.execute.
In `@src-pyloid/tests/test_dictation.py`:
- Around line 48-57: The test_happy_path_pastes_and_saves_history unpacks a
second value into transcription that is never used; change the unpacking from
"pipeline, transcription, clipboard = make_pipeline(db)" to use an
underscore-prefixed name (e.g., "pipeline, _transcription, clipboard =
make_pipeline(db)") or simply "_" (e.g., "pipeline, _, clipboard =
make_pipeline(db)") so the unused variable intent is explicit and Ruff RUF059 is
satisfied.
In `@src/lib/models.ts`:
- Line 1: Update the import in models.ts to use the project alias instead of a
relative path: replace the local import of MODEL_OPTIONS (import { MODEL_OPTIONS
} from "./constants";) with the aliased import using "`@/constants`" so the module
follows the repository's src/ alias convention.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d51c87f5-e97a-44b8-b818-b000c7773154
📒 Files selected for processing (51)
.github/workflows/ci.yml.github/workflows/release.ymlCLAUDE.mddocs/adr/0001-stereo-channel-layout-for-recordings.mddocs/adr/0002-recording-vs-meeting-naming.mddocs/adr/0003-meeting-mode-isolation.mdpackage.jsonscripts/version.mjssrc-pyloid/app_controller.pysrc-pyloid/main.pysrc-pyloid/server.pysrc-pyloid/services/database.pysrc-pyloid/services/dictation.pysrc-pyloid/services/hyprland.pysrc-pyloid/services/model_catalog.pysrc-pyloid/services/model_manager.pysrc-pyloid/services/recording/controller.pysrc-pyloid/services/recording/loopback.pysrc-pyloid/services/recording/recovery.pysrc-pyloid/services/recording/repository.pysrc-pyloid/services/settings.pysrc-pyloid/services/transcription.pysrc-pyloid/services/ui_event_bridge.pysrc-pyloid/tests/test_audio.pysrc-pyloid/tests/test_clipboard.pysrc-pyloid/tests/test_dictation.pysrc-pyloid/tests/test_meeting_jobs.pysrc-pyloid/tests/test_model_manager.pysrc-pyloid/tests/test_recordings_repository.pysrc-pyloid/tests/test_settings.pysrc-pyloid/tests/test_settings_rpc.pysrc-pyloid/tests/test_ui_event_bridge.pysrc/components/ModelDownloadProgress.tsxsrc/components/SettingsTab.tsxsrc/components/meetings/MeetingRecorderContext.tsxsrc/components/meetings/MeetingRecorderPage.tsxsrc/components/meetings/useAudioSourcePreview.tssrc/components/ui/dialog.tsxsrc/hooks/useBackendEvent.tssrc/hooks/useModelDownload.tssrc/lib/api.tssrc/lib/models.tssrc/lib/types.tssrc/pages/Onboarding.tsxsrc/pages/Popup.tsxsrc/pages/onboarding/StepAudio.tsxsrc/pages/onboarding/StepFinal.tsxsrc/pages/onboarding/StepHardware.tsxsrc/pages/onboarding/StepModel.tsxsrc/pages/onboarding/StepTheme.tsxsrc/pages/onboarding/StepWelcome.tsx
Comment on lines
+128
to
+152
| # Normalize audio into flat int16 PCM | ||
| audio_array = np.asarray(audio) | ||
| if audio_array.ndim > 1: | ||
| audio_array = audio_array.reshape(-1) | ||
|
|
||
| if np.issubdtype(audio_array.dtype, np.floating): | ||
| audio_clipped = np.clip(audio_array, -1.0, 1.0) | ||
| audio_int16 = (audio_clipped * 32767).astype(np.int16) | ||
| elif audio_array.dtype == np.int16: | ||
| audio_int16 = audio_array | ||
| else: | ||
| # Fallback: clip to int16 range | ||
| audio_clipped = np.clip(audio_array, -32768, 32767) | ||
| audio_int16 = audio_clipped.astype(np.int16) | ||
|
|
||
| with wave.open(str(tmp_path), "wb") as wf: | ||
| wf.setnchannels(self.audio_service.CHANNELS) | ||
| wf.setsampwidth(2) # 16-bit PCM | ||
| wf.setframerate(self.audio_service.SAMPLE_RATE) | ||
| wf.writeframes(audio_int16.tobytes()) | ||
|
|
||
| tmp_path.replace(output_path) | ||
|
|
||
| duration_ms = int((len(audio_int16) / float(self.audio_service.SAMPLE_RATE)) * 1000) | ||
| size_bytes = output_path.stat().st_size |
There was a problem hiding this comment.
Duration calculation may be incorrect for multi-channel audio.
If audio_service.CHANNELS > 1, the duration calculation at Line 151 will be wrong. After reshape(-1), len(audio_int16) equals samples × channels, but the formula divides only by SAMPLE_RATE:
duration_ms = int((len(audio_int16) / float(self.audio_service.SAMPLE_RATE)) * 1000)For stereo audio, this would report 2× the actual duration. Consider:
- duration_ms = int((len(audio_int16) / float(self.audio_service.SAMPLE_RATE)) * 1000)
+ frame_count = len(audio_int16) // self.audio_service.CHANNELS
+ duration_ms = int((frame_count / float(self.audio_service.SAMPLE_RATE)) * 1000)This is likely safe if PTT always uses mono, but the defensive fix prevents a latent bug.
🤖 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 `@src-pyloid/services/dictation.py` around lines 128 - 152, The duration_ms
calculation is wrong for multi-channel audio because len(audio_int16) counts
samples×channels; compute frames = len(audio_int16) //
self.audio_service.CHANNELS (or len(...) / CHANNELS) and then calculate
duration_ms = int((frames / float(self.audio_service.SAMPLE_RATE)) * 1000).
Update the logic around audio_int16 and duration_ms (references: audio_int16,
self.audio_service.CHANNELS, self.audio_service.SAMPLE_RATE, duration_ms) to use
frames per channel (handle non-divisible lengths defensively) before converting
to milliseconds and keep the rest of the WAV writing unchanged.
Comment on lines
+69
to
+70
| print(f"[WARN] hyprctl rejected rule {rule!r}: {result.stderr.strip() or result.stdout.strip()}", | ||
| flush=True) |
There was a problem hiding this comment.
Replace stdout print() warnings with domain logger calls.
Line 69 writes warnings via print(...), which bypasses structured/domain logging and log-rotation behavior required for Python services.
Suggested fix
def setup_popup_window_rules() -> None:
@@
- import subprocess
+ import subprocess
+ from services.logger import get_logger
from services.process_env import system_env
+ log = get_logger("window")
@@
- if result.returncode != 0:
- print(f"[WARN] hyprctl rejected rule {rule!r}: {result.stderr.strip() or result.stdout.strip()}",
- flush=True)
+ if result.returncode != 0:
+ log.warning(
+ "hyprctl rejected windowrulev2",
+ rule=rule,
+ error=(result.stderr.strip() or result.stdout.strip()),
+ )As per coding guidelines, **/*.py: “Use domain-based logging via get_logger(domain) for structured logging … with 100MB log rotation.”
🤖 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 `@src-pyloid/services/hyprland.py` around lines 69 - 70, The print-based
warning in hyprctl result handling should be replaced with the domain logger:
import and call get_logger with the hyprland domain (e.g.,
get_logger("hyprland")) and replace the print(...) in the code that handles the
subprocess result (the block around the rule rejection handling) with
logger.warning(f"hyprctl rejected rule {rule!r}: {result.stderr.strip() or
result.stdout.strip()}"); remove the flush=True and the print call so structured
logging (and configured rotation) is used instead.
Source: Coding guidelines
Comment on lines
+67
to
+69
| def get_repo_id(model_name: str) -> str: | ||
| """Get the HuggingFace repo ID for a model name.""" | ||
| return MODEL_REPOS.get(model_name, f"Systran/faster-whisper-{model_name}") |
There was a problem hiding this comment.
Fail fast for unknown model names instead of synthesizing repo IDs.
Line 69 silently fabricates a repo ID for unknown keys, which can hide catalog drift and trigger download attempts for non-existent repositories.
Suggested fix
def get_repo_id(model_name: str) -> str:
"""Get the HuggingFace repo ID for a model name."""
- return MODEL_REPOS.get(model_name, f"Systran/faster-whisper-{model_name}")
+ try:
+ return MODEL_REPOS[model_name]
+ except KeyError as exc:
+ raise ValueError(f"Unknown model: {model_name}") from exc🤖 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 `@src-pyloid/services/model_catalog.py` around lines 67 - 69, The get_repo_id
function currently fabricates a HuggingFace repo ID for unknown model names
which hides catalog drift; change get_repo_id to validate model_name against the
MODEL_REPOS mapping and raise a clear exception (e.g., KeyError or custom
ValueError) when model_name is not present instead of returning a synthesized
f"Systran/faster-whisper-{model_name}" string; update any callers to handle or
surface this error so missing catalog entries are caught early.
- useModelDownload: call onComplete(true) when model already cached - StepAudio: commit mic UI state only after backend switch succeeds - StepModel: lock language to 'en' while an English-only model is selected - useAudioSourcePreview: stop backend preview stream on live mode / clear() - useBackendEvent: guard against non-CustomEvent dispatches - StepFinal: show platform-correct Super-key label (Win/⌘/Super) - CLAUDE.md: fix documented meeting event names to recording-*-progress
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft — opened to exercise the new CI (ci.yml test + build jobs). Do not merge yet.
Summary
Architecture deepening of the model download/management flow.
services/model_catalog.pyis the single source of truth for model names, sizes, and HF repo ids (drops duplicated copies in model_manager, settings, transcription).get_model_inforeturnsrepoId; frontend derives HuggingFace URLs + picker metadata from it (removes the drifted 6-entryMODEL_META).ModelManagerowns a single-flight download session (start_download/cancel_download/get_download_status), replacing mutable globals inserver.py.useModelDownloadhook owns the download state machine and re-attaches to in-flight downloads.TranscriptionService; removed the unused cpu-onlyModelManager.load_model/ensure_model_ready.DialogOverlaywrapped inforwardRef.Verification
pnpm run check: 336 passed, 5 skipped; typecheck clean; lint 0 errors.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation