focus-on-state-transition #1447
Conversation
📝 WalkthroughWalkthroughIntroduced a private emit(handle, event) helper in plugins/listener/src/fsm.rs that conditionally shows the Windows main window for specific session state events before emitting them. Replaced direct event emissions throughout the FSM with this helper, and changed several call sites to ignore the returned tauri::Result. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant FSM as FSM
participant Helper as emit(...)
participant UI as HyprWindow::Main (Windows)
participant Tauri as Tauri Handle
participant Bus as Event Bus
User->>FSM: Trigger state/data change
FSM->>Helper: emit(handle, SessionEvent)
alt Event is RunningActive/RunningPaused/Inactive and OS is Windows
Helper->>UI: show()
note right of UI: Only on Windows
end
Helper->>Tauri: event.emit(handle)
Tauri->>Bus: Dispatch SessionEvent
note over FSM,Helper: Many call sites ignore Result<br/>(errors swallowed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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
🧹 Nitpick comments (2)
plugins/listener/src/fsm.rs (2)
615-631: GeneralizeemitoverRuntimeand reduce log noise
- Avoid locking the helper to
Wry; make it generic like other fns in this file.- The info log will fire on every state transition in prod; consider
debugto keep noise down.-fn emit(handle: &tauri::AppHandle<tauri::Wry>, event: &SessionEvent) -> tauri::Result<()> { +fn emit<R: tauri::Runtime>(handle: &tauri::AppHandle<R>, event: &SessionEvent) -> tauri::Result<()> { @@ - } else { - tracing::info!("show_main_window_when_state_transition"); - } + } else { + tracing::debug!("show_main_window_when_state_transition"); + }Additionally, naming this helper
emitcan be confused withEvent::emit; consider a more explicit name in a follow-up (e.g.,emit_with_state_ui).
338-341: Clarify log label to match the new helper pathThis error now originates from the centralized
emithelper, not a generic "broadcast." Rename for clearer observability.- tracing::error!("broadcast_error: {:?}", e); + tracing::error!("emit_error: {:?}", e);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugins/listener/src/fsm.rs(6 hunks)
🧰 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/listener/src/fsm.rs
⏰ 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 (4)
plugins/listener/src/fsm.rs (4)
495-500: SwallowingPartialWordsemit errors—confirm intent
let _ = emit(...)drops failures silently. If the UI relies on these events, silent drops can desync state. If intentional (to avoid noisy logs), consider adding a rate-limited metric/log in a follow-up.
532-537: SwallowingFinalWordsemit errors—confirm intentSame as above. Final results are higher value; losing the event without any signal can hinder debuggability. Please confirm this behavior is desired.
696-704: Muted-state events also ignore emit failures—confirmThese events affect UX controls. Dropping errors here may hide issues when toggling mute from the UI. Intentional?
830-834: State-transition emits are ignored—verify impactState events drive window behavior and client state. Ignoring
emitfailures here could leave the frontend out of sync after transitions. Was this trade-off intentional, or should we at least log failures like the amplitude path?
No description provided.