fixes before beta release #1692
Conversation
Allow any non-empty string in the validator field by changing the regex from "\S+" (one or more non-whitespace) to ".+" (one or more of any character). This relaxes validation to permit values that may include whitespace, which was previously rejected, and ensures legitimate inputs containing spaces are accepted. Restrict shell execute capability to validated sh command Constrain the "shell:allow-execute" capability by replacing the simple string entry with a structured object that only permits an "exec-sh" action. This adds a command "sh -c" with an argument validator (\S+) and sidecar set to false, preventing arbitrary execution and enforcing a basic validation on the provided command. This change was needed to tighten security around shell execution in the desktop tauri capabilities config, ensuring only the intended sh execution pattern is allowed and that command inputs are not empty or purely whitespace. dd
📝 WalkthroughWalkthroughMigrate app data/log paths to Tauri BaseDirectory.Data under Changes
Sequence Diagram(s)sequenceDiagram
participant UI as NoteInput Header
participant Store as Session State
participant Renderer as Renderer
UI->>Store: getSessionMode(sessionId)
Store-->>UI: sessionMode
UI->>UI: isBatchProcessing = (sessionMode == "running_batch")
UI->>UI: isLiveProcessing = (sessionMode == "running_active")
alt Transcript: show progress
Note right of UI: condition: currentTab.type == "transcript"\nand !isLiveProcessing && isBatchProcessing
UI->>Renderer: render progress
end
alt Transcript: show editing controls
Note right of UI: condition: currentTab.type == "transcript"\nand isLiveProcessing && !isBatchProcessing
UI->>Renderer: render editing controls
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)apps/desktop/src/store/tinybase/main.ts (1)
⏰ 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)
🔇 Additional comments (3)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/config/registry.ts (1)
93-105: Add else branch to stop server for consistency.The
current_stt_modelside effect (lines 115-124) stops the server in its else branch, butcurrent_stt_providerlacks this cleanup. If a user switches the provider away from "hyprnote", the local STT server will continue running until the model changes. This asymmetry creates a resource leak.Apply this diff to add the missing else branch:
current_stt_provider: { key: "current_stt_provider", default: undefined, sideEffect: async (_value: string | undefined, getConfig) => { const provider = getConfig("current_stt_provider") as string | undefined; const model = getConfig("current_stt_model") as string | undefined; if ( provider === "hyprnote" && model && model !== "cloud" && (model.startsWith("am-") || model.startsWith("Quantized")) ) { await localSttCommands.startServer(model as SupportedSttModel); + } else { + await localSttCommands.stopServer(null); } }, },apps/desktop/src/store/tinybase/main.ts (1)
162-191: Implement data migration for existing users.The storage path has changed from
BaseDirectory.AppDatatoBaseDirectory.Data/hyprnote, but there's no migration logic to move existing session data. This will result in data loss for existing users.Would you like me to generate a migration function that:
- Checks for existing data in the old AppData location
- Copies it to the new Data/hyprnote location
- Optionally cleans up the old location after successful migration
🧹 Nitpick comments (2)
.vscode/settings.json (1)
2-4: Clarify the rationale for disabling ignore files in VSCode.These settings disable VSCode's handling of
.gitignoreand other ignore files in the explorer and search features. While this can improve visibility of newly added/generated files during development, it may also cause the explorer to display ignored files (e.g.,node_modules, build artifacts), potentially affecting developer experience.Since these settings appear tangential to the main PR objectives (data migration, session state refactoring), please clarify:
- Why these changes are necessary for this PR?
- Should this be a team-wide setting or a local developer preference?
- Are there concerns about explorer/search performance or clutter?
plugins/local-stt/src/server/supervisor.rs (1)
23-24: Verify the increased restart limits are justified and monitored.The code changes are correct. However, increasing
max_restartsfrom 15 to 100 andmax_windowfrom 10 seconds to 3 minutes significantly raises the supervisor's tolerance for failures. While this may address transient STT initialization issues, it also means:
- Up to 100 restart attempts before the supervisor gives up, potentially consuming CPU, memory, and file handles if the actor is persistently failing.
- A 3-minute window delays detection of persistent problems.
Ensure that:
- This change addresses a specific, documented issue (e.g., flaky STT startup during model loading).
- Monitoring/alerting is in place to track restart counts and detect patterns of repeated failures.
- The increased limits align with the resource constraints and expected behavior of the STT actors.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.vscode/settings.json(1 hunks)apps/desktop/src-tauri/capabilities/default.json(2 hunks)apps/desktop/src-tauri/tauri.conf.json(1 hunks)apps/desktop/src/components/main/body/sessions/note-input/header.tsx(1 hunks)apps/desktop/src/components/main/body/sessions/note-input/index.tsx(0 hunks)apps/desktop/src/components/settings/ai/llm/configure.tsx(3 hunks)apps/desktop/src/components/settings/ai/llm/select.tsx(1 hunks)apps/desktop/src/components/settings/ai/shared/index.tsx(0 hunks)apps/desktop/src/components/settings/ai/stt/configure.tsx(2 hunks)apps/desktop/src/components/settings/ai/stt/health.tsx(1 hunks)apps/desktop/src/config/registry.ts(1 hunks)apps/desktop/src/config/use-config.ts(1 hunks)apps/desktop/src/store/tinybase/main.ts(2 hunks)plugins/local-stt/src/server/supervisor.rs(1 hunks)
💤 Files with no reviewable changes (2)
- apps/desktop/src/components/main/body/sessions/note-input/index.tsx
- apps/desktop/src/components/settings/ai/shared/index.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src/components/main/body/sessions/note-input/header.tsx (1)
apps/desktop/src/contexts/listener.tsx (1)
useListener(37-49)
apps/desktop/src/components/settings/ai/stt/configure.tsx (1)
apps/desktop/src/components/settings/ai/shared/index.tsx (1)
FormField(71-122)
apps/desktop/src/components/settings/ai/llm/configure.tsx (1)
apps/desktop/src/components/settings/ai/shared/index.tsx (1)
FormField(71-122)
⏰ 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: fmt
🔇 Additional comments (9)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
113-113: LGTM! Clean formatting improvement.The blank line addition improves readability by separating the variable declarations from the return statement. The broader changes (removal of inline Pro messaging from provider items while retaining it for model selection) align with the PR's UI simplification objectives.
apps/desktop/src/config/registry.ts (1)
122-124: LGTM — Explicit server cleanup added.The else branch properly stops the STT server when the provider/model conditions are no longer met. This ensures the server lifecycle is correctly managed.
apps/desktop/src/components/main/body/sessions/note-input/header.tsx (2)
309-311: Clean refactoring: Deriving activity state from session mode.The migration from an
isInactiveprop to derivingisBatchProcessingandisLiveProcessingdirectly fromsessionModeimproves encapsulation and reduces prop drilling. The logic correctly handles all session mode states, and the approach is consistent withuseEditorTabsat line 374.
318-320: Verify isInactive prop removal across Header component consumers.The conditional logic at lines 318-320 is correct. However, the negation checks are redundant since
sessionModecan only have one value at a time:const showProgress = currentTab.type === "transcript" && isBatchProcessing; const showEditingControls = currentTab.type === "transcript" && isLiveProcessing;The current defensive checks are acceptable and clarify intent.
Critically, verify that all consumers of the
Headercomponent have removed theisInactiveprop from their usage. Automated verification could not confirm this across the codebase.apps/desktop/src/components/settings/ai/stt/configure.tsx (1)
109-149: LGTM - Clean form restructuring.The form logic correctly handles two provider configuration patterns:
- Providers without a default base URL display both base URL and API key fields upfront
- Providers with a default base URL show only the API key field, with base URL accessible in the Advanced section
The auto-submit pattern via onChange listener (lines 74-85) combined with preventDefault (lines 112-114) ensures seamless user experience.
apps/desktop/src/components/settings/ai/llm/configure.tsx (2)
181-181: LGTM - Improved UX for locked providers.The conditional badge text clearly communicates the provider's availability status to users.
120-132: The conditional API Key field rendering is intentional and correct.Verification confirms the
config?.apiKeycondition in LLM configure.tsx is proper design, not a bug. LLM providers explicitly declare their API key requirements in their schema:
- Providers requiring keys (OpenAI, OpenRouter, Anthropic, Custom):
apiKey: true→ field shown- Providers not requiring keys (Hyprnote, LM Studio, Ollama):
apiKey: false→ field hiddenThis differs from STT because all STT providers require API keys, making a condition unnecessary. The LLM implementation correctly respects each provider's configuration needs.
apps/desktop/src/store/tinybase/main.ts (1)
177-184: Path migration is consistent.The file path correctly references the new
hyprnote/sessions/directory structure underBaseDirectory.Data, which aligns with the Tauri capability changes indefault.json.Note: This depends on the directory creation at lines 162-171 being fixed to properly await completion.
apps/desktop/src-tauri/capabilities/default.json (1)
29-63: LGTM! Path permissions correctly scoped to new data directory.The migration from
$APPDATAto$DATA/hyprnote/**/*is consistent across all file system permissions (opener, mkdir, exists, read, write). The scoping to thehyprnotesubdirectory is more restrictive than the previous$APPDATA/*pattern, which improves security.
Replace direct use of dirs::data_dir()/dirs::home_dir() with Tauri's app.path().resolve and BaseDirectory to determine application data/logs paths. This makes path resolution use Tauri's platform-aware mechanisms and allows proper error handling in plugin setup and runtime code.
Changes:
- Replace dirs::data_dir() joins with app.path().resolve("hyprnote/...", BaseDirectory::Data) across multiple plugins (db, db2, listener, local-llm, local-stt, misc).
- Update listener to resolve sessions path and pass it into the RecorderActor.
- Use Tauri BaseDirectory imports where needed and adjust error mapping to return appropriate errors.
- Fallback in local-stt.models_dir() to the old dirs::data_dir() behavior if Tauri resolution fails.
- Add a PathResolver variant to tracing errors to report path resolution failures.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/db/src/ext.rs (1)
36-47: Handle potential UTF-8 conversion failure.Line 42:
.to_str().unwrap()could panic if the resolved path contains invalid UTF-8 sequences. While rare, this should be handled gracefully.fn db_local_path(&self) -> Result<String, crate::Error> { use tauri::path::BaseDirectory; let v = { let dir = self.path().resolve("hyprnote", BaseDirectory::Data)?; std::fs::create_dir_all(&dir)?; - dir.join("db.sqlite").to_str().unwrap().to_string() + dir.join("db.sqlite") + .to_str() + .ok_or_else(|| crate::Error::Io(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "Invalid UTF-8 in database path" + )))? + .to_string() }; tracing::info!(path = %v, "local_db"); Ok(v) }
🧹 Nitpick comments (1)
plugins/tracing/src/ext.rs (1)
9-17: Consider logging directory creation failures.Line 15:
let _ = std::fs::create_dir_all(&logs_dir);silently ignores errors. While this prevents blocking the application on logging setup failures, it could hide permission issues or disk space problems during development or troubleshooting.Consider logging the error while still allowing the function to succeed:
fn logs_dir(&self) -> Result<PathBuf, crate::Error> { use tauri::{path::BaseDirectory, Manager}; let logs_dir = self .path() .resolve("hyprnote", BaseDirectory::Data) .map_err(|e| crate::Error::PathResolver(e.to_string()))?; - let _ = std::fs::create_dir_all(&logs_dir); + if let Err(e) = std::fs::create_dir_all(&logs_dir) { + eprintln!("Failed to create logs directory: {}", e); + } Ok(logs_dir) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
plugins/db/src/ext.rs(1 hunks)plugins/db2/src/ext.rs(1 hunks)plugins/listener/src/actors/controller.rs(1 hunks)plugins/local-llm/src/lib.rs(2 hunks)plugins/local-stt/src/ext.rs(1 hunks)plugins/misc/src/commands.rs(7 hunks)plugins/tracing/src/errors.rs(1 hunks)plugins/tracing/src/ext.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
plugins/tracing/src/ext.rs (1)
plugins/tracing/src/commands.rs (1)
logs_dir(7-9)
plugins/listener/src/actors/controller.rs (2)
plugins/listener/src/ext.rs (4)
state(120-120)state(142-142)state(186-186)state(204-204)plugins/listener/src/actors/source.rs (3)
new(340-347)new(432-440)new(500-506)
plugins/local-llm/src/lib.rs (1)
owhisper/owhisper-config/src/lib.rs (1)
data_dir(52-54)
plugins/local-stt/src/ext.rs (1)
owhisper/owhisper-config/src/lib.rs (1)
data_dir(52-54)
plugins/misc/src/commands.rs (1)
owhisper/owhisper-config/src/lib.rs (1)
data_dir(52-54)
⏰ 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: fmt
🔇 Additional comments (10)
plugins/listener/src/actors/controller.rs (1)
278-294: LGTM!Path resolution correctly uses Tauri's managed data directory with proper error propagation to
ActorProcessingErr.plugins/local-llm/src/lib.rs (1)
69-91: LGTM!Path resolution correctly migrated to Tauri's managed directory with proper error propagation. Scoped imports improve code organization.
plugins/db2/src/ext.rs (1)
21-48: LGTM!Path resolution correctly migrated to Tauri's managed directory with proper error propagation via
?.plugins/misc/src/commands.rs (6)
35-53: LGTM!Path resolution correctly uses Tauri's managed directory with proper error handling.
57-78: LGTM!Consistent path resolution pattern with appropriate error handling.
92-122: LGTM!Path resolution correctly migrated with proper error mapping to
AudioImportError::PathResolver.
126-143: LGTM!Consistent path resolution pattern maintained.
147-162: LGTM!Path resolution correctly implemented.
166-181: LGTM!Consistent path resolution pattern applied.
plugins/tracing/src/errors.rs (1)
3-16: LGTM!New error variant appropriately supports path resolution error handling.
…creation Correct the inverted isCloud logic so hyprnote cloud models and non-hyprnote providers are treated as cloud, ensuring health checks query the appropriate source (conn for cloud, local.data?.status for local). Replace the synchronous try/catch around config side effects with Promise.resolve(...).catch(...) so both sync throws and async rejections are logged. Make tinybase directory creation use async/await with try/catch and await mkdir so existence checks and directory creation complete before proceeding and errors are surfaced.
No description provided.