Conversation
📝 WalkthroughWalkthroughThis PR removes the paused meeting state and all pause/resume logic across desktop UI, utils store, plugins, and routes. It simplifies recording to a two-state model (inactive/running_active), collapses Stop UI, adjusts gating conditions, updates event handling, and streamlines enhance mutation inputs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Desktop UI (Listen/Controls)
participant Store as ongoing-session store
participant Tauri as Listener Plugin (Tauri)
participant FSM as FSM
User->>UI: Click Start
UI->>Store: start()
Store->>Tauri: start_session
Tauri->>FSM: StateEvent::Start
FSM-->>Tauri: SessionEvent::RunningActive
Tauri-->>Store: sessionEvent(running_active)
Store-->>UI: status = running_active
User->>UI: Click Stop
UI->>Store: stop()
Store->>Tauri: stop_session
Tauri->>FSM: StateEvent::Stop
FSM-->>Tauri: SessionEvent::Inactive
Tauri-->>Store: sessionEvent(inactive)
Store-->>UI: status = inactive
note over UI,Store: No Pause/Resume path
sequenceDiagram
autonumber
participant Notif as Notification Plugin
participant Tauri as Listener Plugin
participant FSM as FSM
Note over Notif,Tauri: MicStopped detection
Notif->>Tauri: stop_session()
Tauri->>FSM: StateEvent::Stop
FSM-->>Tauri: SessionEvent::Inactive
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/utils/src/stores/ongoing-session.ts (1)
128-135: Prevent listener leaks: unlisten on start (if any) and before stop.Without calling the previous
sessionEventUnlisten, repeated start/stop cycles accumulate listeners, causing duplicate updates and memory growth.Apply these minimal changes:
@@ start: (sessionId: string) => { - console.log("start", sessionId); set((state) => mutate(state, (draft) => { draft.sessionId = sessionId; draft.loading = true; }) ); + // Avoid duplicate updates across sessions + get().sessionEventUnlisten?.(); @@ - listenerCommands.stopSession().then(() => { + // Ensure we detach the event handler before stopping + const { sessionEventUnlisten } = get(); + sessionEventUnlisten?.(); + listenerCommands.stopSession().then(() => { set(initialState);Also applies to: 148-176
apps/desktop/src/routes/app.control.tsx (1)
199-211: Type error:onClickunion isn’t safely callable with an argument.A union of function types with different params can’t be invoked with a single call signature. This will fail TS checking.
Apply:
-}: { - onClick?: ((e: React.MouseEvent<HTMLButtonElement>) => void) | (() => void); +}: { + onClick?: (e?: React.MouseEvent<HTMLButtonElement>) => void;Optionally, keep the call as-is:
- onClick?.(e); + onClick?.(e);
🧹 Nitpick comments (9)
crates/llama/Cargo.toml (1)
21-22: Tag 0.1.121 confirmed; update header commentTag 0.1.121 exists repository-wide in utilityai/llama-cpp-rs for both llama-cpp-2 and llama-cpp-sys-2. Update the header comment on line 6 of crates/llama/Cargo.toml to reference tag 0.1.121 instead of the old branch name.
crates/whisper-local/src/model.rs (1)
352-357: Test-only: prefer tracing over println to keep CI quiet.Switching to
tracing::info!avoids noisy stdout unless a subscriber is set; keeps local timing visible when desired.Suggested tweak:
- let start = std::time::Instant::now(); + let start = std::time::Instant::now(); let segments = whisper.transcribe(&audio).unwrap(); - let duration = start.elapsed(); - println!("segments: {:#?}", segments); - println!("time: {:?}", duration); + let duration = start.elapsed(); + tracing::info!(segments = segments.len(), ?duration, "whisper_test_transcribe_done");packages/utils/src/stores/ongoing-session.ts (2)
73-74: Drop noisy console log in production path.Keep logs minimal per guidelines; the store already reflects state for debugging.
Apply:
- console.log("start", sessionId);
136-147: Optional: rely on the emitted “running_active” event to set status to avoid races.You already transition on the event above; setting status here too can double-fire and briefly flip UI if ordering changes.
Apply:
- listenerCommands.startSession(sessionId).then(() => { - set({ status: "running_active", loading: false }); - }).catch((error) => { + listenerCommands.startSession(sessionId).catch((error) => { console.error(error); set(initialState);apps/desktop/src/components/toast/ota.tsx (1)
130-132: Make notification gating react to status changes immediatelyNice simplification. To avoid waiting for the next 3‑min refetch when a meeting ends, re-run the effect on status changes and optionally stop polling while active.
Apply:
- }, [checkForUpdate.data]); + }, [checkForUpdate.data, ongoingSession.status]);Optionally:
const checkForUpdate = useQuery({ queryKey: ["check-for-update"], queryFn: async () => { if (process.env.NODE_ENV === "production") { return check(); } return null; }, + enabled: ongoingSession.status !== "running_active", refetchInterval: 1000 * 60 * 3, refetchIntervalInBackground: true, });apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
282-313: Unify onStop signature and remove dead form/config code in StopButtonStop no longer uses templates or save-audio UI. Drop the unused schema, form, and config queries; make onStop parameterless to reflect the new API. This trims network calls and imports.
Apply:
-const stopButtonSchema = z.object({ - saveAudio: z.boolean(), - selectedTemplate: z.string(), -}); - -type StopButtonFormData = z.infer<typeof stopButtonSchema>; - -function StopButton({ onStop }: { onStop: (templateId: string | null) => void }) { - const defaultTemplateQuery = useQuery({ - queryKey: ["config"], - queryFn: () => dbCommands.getConfig().then((config) => config.general.selected_template_id), - refetchOnWindowFocus: true, - }); - - const saveRecordingsQuery = useQuery({ - queryKey: ["config", "save_recordings"], - queryFn: () => dbCommands.getConfig().then((config) => config.general.save_recordings), - refetchOnWindowFocus: true, - }); - - const form = useForm<StopButtonFormData>({ - resolver: zodResolver(stopButtonSchema), - defaultValues: { - saveAudio: false, // Default fallback - selectedTemplate: "auto", - }, - }); - - useEffect(() => { - if (defaultTemplateQuery.data) { - form.setValue("selectedTemplate", defaultTemplateQuery.data); - } - }, [defaultTemplateQuery.data, form]); - - useEffect(() => { - if (saveRecordingsQuery.data !== undefined) { - form.setValue("saveAudio", saveRecordingsQuery.data ?? false); - } - }, [saveRecordingsQuery.data, form]); - - return ( - <Button - variant="destructive" - className="w-full flex-1 justify-center text-xs" - onClick={() => onStop(null)} - > - <StopCircleIcon - color="white" - className="w-4 h-4" - /> - <Trans>Stop</Trans> - </Button> - ); -} +function StopButton({ onStop }: { onStop: () => void }) { + return ( + <Button variant="destructive" className="w-full flex-1 justify-center text-xs" onClick={onStop}> + <StopCircleIcon color="white" className="w-4 h-4" /> + <Trans>Stop</Trans> + </Button> + ); +}And since onStop is now parameterless, no change needed at Line 312 () and Line 270 (onStop={handleStopSession}) remains correct.
Follow-up cleanups (outside the edited hunk):
- Remove now-unused imports:
-import { zodResolver } from "@hookform/resolvers/zod"; -import { commands as dbCommands } from "@hypr/plugin-db"; -import { useForm } from "react-hook-form"; -import { z } from "zod";
- Drop the unused store selector property in WhenActive:
- const ongoingSessionStore = useOngoingSession((s) => ({ - stop: s.stop, - setAutoEnhanceTemplate: s.setAutoEnhanceTemplate, - })); + const ongoingSessionStore = useOngoingSession((s) => ({ + stop: s.stop, + }));Also applies to: 324-369
apps/desktop/src/routes/app.control.tsx (2)
455-457: Avoidas any; use a type guard forcurrentState.This narrows correctly without a cast.
Apply:
- if (["running_active", "inactive"].includes(currentState)) { - setRecordingStatus(currentState as any); - } + if (currentState === "running_active" || currentState === "inactive") { + setRecordingStatus(currentState); + }
469-472: Same here: removeas anyon event payload type.Use an explicit guard.
Apply:
- if (["inactive", "running_active"].includes(payload.type)) { - setRecordingStatus(payload.type as any); - setRecordingLoading(false); - } + if (payload.type === "inactive" || payload.type === "running_active") { + setRecordingStatus(payload.type); + setRecordingLoading(false); + }plugins/listener/src/fsm.rs (1)
171-183: Remove deadsession_state_txwiring or reintroduce its initialization.The watch sender is never initialized now; the on_transition send is a no-op. Either remove it or rewire if still needed.
Apply:
@@ pub struct Session { @@ - session_state_tx: Option<tokio::sync::watch::Sender<State>>, @@ } @@ impl Session { pub fn new(app: tauri::AppHandle) -> Self { @@ - session_state_tx: None, @@ } } @@ fn on_transition(&mut self, source: &State, target: &State) { @@ - if let Some(tx) = &self.session_state_tx { - let _ = tx.send(target.clone()); - } }Also applies to: 189-202, 783-786
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
Cargo.lockis excluded by!**/*.lockapps/desktop/src/locales/en/messages.pois excluded by!**/*.poapps/desktop/src/locales/ko/messages.pois excluded by!**/*.poplugins/listener/js/bindings.gen.tsis excluded by!**/*.gen.tsplugins/listener/permissions/autogenerated/commands/pause_session.tomlis excluded by!plugins/**/permissions/**plugins/listener/permissions/autogenerated/commands/resume_session.tomlis excluded by!plugins/**/permissions/**plugins/listener/permissions/autogenerated/reference.mdis excluded by!plugins/**/permissions/**plugins/listener/permissions/default.tomlis excluded by!plugins/**/permissions/**plugins/listener/permissions/schemas/schema.jsonis excluded by!plugins/**/permissions/**
📒 Files selected for processing (17)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx(6 hunks)apps/desktop/src/components/left-sidebar/index.tsx(1 hunks)apps/desktop/src/components/toast/ota.tsx(1 hunks)apps/desktop/src/routes/app.control.tsx(4 hunks)apps/desktop/src/routes/video.tsx(0 hunks)crates/llama/Cargo.toml(1 hunks)crates/whisper-local/Cargo.toml(1 hunks)crates/whisper-local/src/model.rs(1 hunks)packages/utils/src/stores/ongoing-session.ts(1 hunks)plugins/listener/build.rs(0 hunks)plugins/listener/src/commands.rs(0 hunks)plugins/listener/src/error.rs(0 hunks)plugins/listener/src/events.rs(0 hunks)plugins/listener/src/ext.rs(0 hunks)plugins/listener/src/fsm.rs(1 hunks)plugins/listener/src/lib.rs(0 hunks)plugins/notification/src/handler.rs(1 hunks)
💤 Files with no reviewable changes (7)
- plugins/listener/src/events.rs
- plugins/listener/build.rs
- plugins/listener/src/lib.rs
- plugins/listener/src/error.rs
- plugins/listener/src/ext.rs
- apps/desktop/src/routes/video.tsx
- plugins/listener/src/commands.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
plugins/notification/src/handler.rsapps/desktop/src/components/toast/ota.tsxapps/desktop/src/components/left-sidebar/index.tsxcrates/whisper-local/src/model.rsapps/desktop/src/routes/app.control.tsxpackages/utils/src/stores/ongoing-session.tsapps/desktop/src/components/editor-area/note-header/listen-button.tsxplugins/listener/src/fsm.rs
🧬 Code graph analysis (1)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
packages/ui/src/components/ui/button.tsx (1)
Button(37-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci (macos, macos-14)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (9)
crates/whisper-local/Cargo.toml (1)
26-26: Apply doc tweak: update reference comment to CodebergFeatures “raw-api” and “tracing_backend” are confirmed present at rev 5f054c9; no further changes needed.
-# https://github.com/tazz4843/whisper-rs/blob/e3d67d5/Cargo.toml +# Upstream migrated to Codeberg: https://codeberg.org/tazz4843/whisper-rsplugins/notification/src/handler.rs (1)
122-125: Replace pause with stop on MicStopped — aligns with removal of paused state.Behavior matches the PR objective and simplifies control flow. Please just verify
stop_session()is idempotent when the session is already inactive, since this path can fire late.If helpful, confirm via a quick manual run or unit test that multiple
stop_session()calls are no-ops and don’t emit error toasts.apps/desktop/src/components/left-sidebar/index.tsx (1)
37-41: Approve meeting-running check updateNo lingering
running_pausedreferences found in the codebase; using only"running_active"forisMeetingRunningmatches the PR goal and simplifies gating for OngoingSession.apps/desktop/src/components/editor-area/note-header/listen-button.tsx (3)
63-75: Good: disable STT model polling while recordingThe enabled guard tied to "running_active" avoids unnecessary polling during an active session.
82-87: Consent reminder condition — LGTMOnly shows on first words-less start of the active session for this note; matches intent.
245-252: Stop behavior is clean and side-effect minimalStopping, closing the popover, and dismissing the consent toast when there’s no transcript looks correct.
apps/desktop/src/routes/app.control.tsx (2)
439-445: State model simplification looks good.Union narrowed to "inactive" | "running_active" and derived isRecording logic is consistent.
493-497: Approve start/stop toggle implementation
Search confirms nopauseSession,resumeSession, orrunning_pausedreferences remain.plugins/listener/src/fsm.rs (1)
541-541: No paused-state symbols remain in listener FSM
Verified thatplugins/listener/src/fsm.rscontains no occurrences ofRunningPaused,Pause,Resume, orrunning_paused.
yujonglee
force-pushed
the
remove-pause
branch
from
320f443 to
c1ca843
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src/components/editor-area/index.tsx (1)
621-644: Fix type/shape mismatch: remove triggerType from auto-enhance pathuseAutoEnhance still types and passes triggerType, but enhance.mutate no longer accepts it. This will cause TS type errors and pass unused data at runtime.
Apply this diff:
- enhanceMutate: (params: { triggerType: "auto"; templateId?: string | null }) => void; + enhanceMutate: (params: { templateId?: string | null }) => void; @@ - enhanceMutate({ - triggerType: "auto", - templateId: autoEnhanceTemplate, - }); + enhanceMutate({ + templateId: autoEnhanceTemplate, + });packages/utils/src/stores/ongoing-session.ts (2)
145-173: Unsubscribe from sessionEvent on stop to prevent leaks/duplicate updatesThe unlisten function is stored but never invoked on stop, so listeners persist across sessions.
Apply this diff:
stop: () => { - const { sessionId } = get(); + const { sessionId, sessionEventUnlisten } = get(); set((state) => mutate(state, (draft) => { draft.loading = true; }) ); listenerCommands.stopSession().then(() => { + // Detach listener to avoid leaks and duplicate events + sessionEventUnlisten?.(); set(initialState); // We need refresh since session in store is now stale. // setTimeout is needed because of debounce. setTimeout(() => { if (sessionId) { const sessionStore = sessionsStore.getState().sessions[sessionId]; sessionStore.getState().refresh(); } }, 1500); }).catch((error) => {
88-98: Guard against multiple subscriptions when start is called repeatedlyEnsure any previous listener is removed before registering a new one.
Apply this diff:
- listenerEvents.sessionEvent.listen(({ payload }) => { + // Ensure single active subscription + get().sessionEventUnlisten?.(); + listenerEvents.sessionEvent.listen(({ payload }) => { if (payload.type === "audioAmplitude") {plugins/listener/src/fsm.rs (1)
529-535: End-of-stream path still leaves session active — call Stop directly here.The TODO reflects a real bug: sending on stop_tx doesn’t transition the FSM. Mirror the timeout path to ensure we enter Inactive when the listen stream ends.
Apply this diff:
- // TODO: this not work - session still on ACTIVE - if stop_tx.send(()).await.is_err() { - tracing::warn!("failed_to_send_stop_signal"); - } + // Directly stop the FSM to ensure transition to Inactive on stream end. + if let Some(state) = app.try_state::<crate::SharedState>() { + let mut guard = state.lock().await; + guard.fsm.handle(&crate::fsm::StateEvent::Stop).await; + } break;
🧹 Nitpick comments (2)
plugins/listener/src/fsm.rs (2)
245-246: Optional: drop stop_tx/stop_rx and use direct FSM Stop everywhere.With pause removed, the mpsc indirection is unnecessary. Removing it simplifies teardown and avoids double-Stop calls.
Apply this diff:
- let (stop_tx, mut stop_rx) = tokio::sync::mpsc::channel::<()>(1) + // stop channel no longer needed; Stop is invoked directly on the FSM- let stop_tx = stop_tx.clone();- let app_handle = self.app.clone(); - tasks.spawn(async move { - if stop_rx.recv().await.is_some() { - if let Some(state) = app_handle.try_state::<crate::SharedState>() { - let mut guard = state.lock().await; - guard.fsm.handle(&crate::fsm::StateEvent::Stop).await; - } - } - });Also applies to: 449-449, 548-556
180-180: Optional: remove unused session_state_tx plumbing.It’s never initialized (always None) and only read in on_transition. Cleaning it up reduces dead code after the pause/state-channel removals.
Apply this diff:
- session_state_tx: Option<tokio::sync::watch::Sender<State>>,- session_state_tx: None,- if let Some(tx) = &self.session_state_tx { - let _ = tx.send(target.clone()); - }Also applies to: 199-199, 783-785
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
apps/desktop/src/locales/en/messages.pois excluded by!**/*.poapps/desktop/src/locales/ko/messages.pois excluded by!**/*.poplugins/listener/js/bindings.gen.tsis excluded by!**/*.gen.tsplugins/listener/permissions/autogenerated/commands/pause_session.tomlis excluded by!plugins/**/permissions/**plugins/listener/permissions/autogenerated/commands/resume_session.tomlis excluded by!plugins/**/permissions/**plugins/listener/permissions/autogenerated/reference.mdis excluded by!plugins/**/permissions/**plugins/listener/permissions/default.tomlis excluded by!plugins/**/permissions/**plugins/listener/permissions/schemas/schema.jsonis excluded by!plugins/**/permissions/**
📒 Files selected for processing (15)
apps/desktop/src/components/editor-area/index.tsx(2 hunks)apps/desktop/src/components/editor-area/note-header/listen-button.tsx(6 hunks)apps/desktop/src/components/left-sidebar/index.tsx(1 hunks)apps/desktop/src/components/toast/ota.tsx(1 hunks)apps/desktop/src/routes/app.control.tsx(4 hunks)apps/desktop/src/routes/video.tsx(0 hunks)packages/utils/src/stores/ongoing-session.ts(1 hunks)plugins/listener/build.rs(0 hunks)plugins/listener/src/commands.rs(0 hunks)plugins/listener/src/error.rs(0 hunks)plugins/listener/src/events.rs(0 hunks)plugins/listener/src/ext.rs(0 hunks)plugins/listener/src/fsm.rs(1 hunks)plugins/listener/src/lib.rs(0 hunks)plugins/notification/src/handler.rs(1 hunks)
💤 Files with no reviewable changes (7)
- plugins/listener/src/events.rs
- plugins/listener/src/lib.rs
- apps/desktop/src/routes/video.tsx
- plugins/listener/build.rs
- plugins/listener/src/commands.rs
- plugins/listener/src/ext.rs
- plugins/listener/src/error.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/components/left-sidebar/index.tsx
- apps/desktop/src/components/editor-area/note-header/listen-button.tsx
- apps/desktop/src/components/toast/ota.tsx
- apps/desktop/src/routes/app.control.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
plugins/notification/src/handler.rsplugins/listener/src/fsm.rspackages/utils/src/stores/ongoing-session.tsapps/desktop/src/components/editor-area/index.tsx
🧬 Code graph analysis (1)
plugins/notification/src/handler.rs (1)
plugins/listener/src/fsm.rs (1)
app_handle(551-551)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci (macos, macos-14)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (6)
plugins/notification/src/handler.rs (1)
122-125: Confirm stop-on-mic-stop behaviorSwitching from pause to stop means every MicStopped event fully ends the session. Please confirm this aligns with the new FSM semantics and won’t prematurely terminate sessions on brief mic dropouts.
apps/desktop/src/components/editor-area/index.tsx (3)
200-203: LGTM: template-driven enhance call simplifiedPassing only { templateId } (null for “auto”) matches the new mutate signature.
205-207: LGTM: manual enhance defaultsCalling enhance.mutate({}) now correctly defers to the selected template in config.
405-412: LGTM: mutate variables narrowedmutationFn now only accepts { templateId? }, which matches the new callers.
packages/utils/src/stores/ongoing-session.ts (1)
11-11: LGTM: two-state session statusUnion trimmed to "inactive" | "running_active" is consistent with the PR objective.
plugins/listener/src/fsm.rs (1)
538-542: Timeout now triggers Stop directly — aligns with the two-state model.This keeps behavior consistent post-pause removal.
1 participant
No description provided.