feat: add .env file auto-sync with port-forward state#578
feat: add .env file auto-sync with port-forward state#578ereli-sevenai wants to merge 1 commit intohcavarsan:mainfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds manual and automatic .env export/sync: new env_export utilities, settings model fields and async getters/setters, Tauri command + wiring, frontend UI/handlers and save-dialog flow, port-forward lifecycle triggers to auto-sync, docs and tests; plus widespread import formatting changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Frontend UI
participant Tauri as Tauri IPC
participant EnvExport as Env Export Util
participant Database as Database
participant FileSystem as File System
rect rgba(100,150,200,0.5)
Note over User,FileSystem: Manual Export Flow
User->>Frontend: Click "Export .env File"
Frontend->>Tauri: export_env_cmd(running_only: true)
Tauri->>EnvExport: generate_env_content(running_only)
EnvExport->>Database: Fetch configs & states
Database-->>EnvExport: Return data
EnvExport->>EnvExport: Build .env content (sanitize, dedupe)
EnvExport-->>Tauri: Return content
Tauri-->>Frontend: Content received
Frontend->>User: Show save dialog
User->>Frontend: Choose path
Frontend->>FileSystem: Write file
FileSystem-->>Frontend: Success
end
sequenceDiagram
participant User
participant Frontend as Frontend UI
participant Tauri as Tauri IPC
participant PortForward as Port Forward Handler
participant EnvExport as Env Export Util
participant Database as Database
participant FileSystem as File System
rect rgba(200,150,100,0.5)
Note over User,FileSystem: Auto-Sync Flow (on port-forward state change)
User->>Frontend: Start/stop port forward
Frontend->>Tauri: Port forward command
Tauri->>PortForward: Execute command
PortForward->>Database: Update state
Database-->>PortForward: Ack
PortForward->>EnvExport: write_env_file_if_enabled()
EnvExport->>Database: Read configs & running states
Database-->>EnvExport: Return data
EnvExport->>FileSystem: Write to configured path
FileSystem-->>EnvExport: Success/Err
EnvExport-->>PortForward: Result
PortForward-->>Frontend: Emit state change
Frontend->>User: Update UI
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![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
🤖 Fix all issues with AI agents
In `@crates/kftray-commons/src/models/settings_model.rs`:
- Around line 156-158: The code path in from_settings_manager that reads
settings.get("env_auto_sync_enabled") uses value.parse().unwrap_or(false) which
conflicts with the intended default true from default_env_auto_sync_enabled(),
Default::default(), and get_env_auto_sync_enabled(); change the fallback to the
canonical default (e.g., use unwrap_or(true) or call
default_env_auto_sync_enabled()) so malformed values produce the same default
true as the rest of the codebase and update the env_auto_sync_enabled assignment
in from_settings_manager accordingly.
In `@crates/kftray-commons/src/utils/env_export.rs`:
- Around line 166-180: sanitize_env_var_name currently uppercases and replaces
invalid chars but can return names starting with a digit (e.g., "3PROXY"), which
is invalid; update sanitize_env_var_name to ensure the resulting string does not
start with a digit by prepending an underscore when the first character is ASCII
digit (after performing the existing uppercase/replace/filter steps) so the
final env var always begins with a non-digit (use the existing function name
sanitize_env_var_name and operate on its output before returning).
- Around line 66-79: The current var_name construction (using
derive_env_var_name, sanitize_env_var_name, filtered_configs and name_counts)
still allows collisions when configs share both base name and namespace;
implement a second-pass dedup: after computing the initial var_name, track seen
names in a HashSet and if a var_name is already present, append a stable
discriminator (preferably sanitize_env_var_name(&config.local_port) or similar
unique field, falling back to an incrementing numeric suffix) to produce a
unique name; update the logic that produces var_name to consult this seen-set
and generate deterministic disambiguators so duplicate names no longer overwrite
each other in the generated .env.
In `@docs/kftray/USAGE.md`:
- Line 132: Update the documentation line to reflect the full fallback ordering
used by derive_env_var_name in env_export.rs: state that variable names are
derived from the service alias, or service name if no alias, then the target if
both are absent, and finally "unknown" as a last resort; ensure you mention the
uppercase conversion and dash-to-underscore replacement behavior remains
unchanged.
In `@frontend/src/components/SettingsModal/EnvAutoSyncSettings.tsx`:
- Around line 26-50: selectEnvPath currently logs debug messages and doesn't
call the dialog lifecycle helpers; update selectEnvPath to remove the three
console.log calls and wrap the save() call with await invoke('open_save_dialog')
before showing the dialog and await invoke('close_save_dialog') in a finally
block after the dialog completes (so focus/visibility is restored even on
error). Ensure you import/inject the invoke function used by the other handlers
and keep existing error handling and setEnvAutoSyncPath logic intact while
calling invoke('close_save_dialog') in every exit path.
🧹 Nitpick comments (9)
crates/kftray-tauri/src/commands/settings.rs (1)
212-218: Remove read-back verification — it's a debugging artifact.These lines perform two additional DB reads on every save purely for logging. This adds unnecessary overhead and the result is never acted upon (no error returned if verification fails). The
unwrap_or(false)on Line 213 would also silently mask a DB error.Consider removing this block or converting to
debug!-level logging gated behind a flag.Proposed fix: remove verification
set_env_auto_sync_path(&path_value).await.map_err(|e| { error!("Failed to set env_auto_sync_path: {e}"); format!("Failed to set env_auto_sync_path: {e}") })?; - // Verify the settings were saved correctly - let verify_enabled = get_env_auto_sync_enabled().await.unwrap_or(false); - let verify_path = get_env_auto_sync_path().await.ok().flatten(); - info!( - "Verified saved settings: enabled={}, path={:?}", - verify_enabled, verify_path - ); - info!("Successfully set env auto-sync settings"); Ok(())crates/kftray-commons/src/utils/env_export.rs (3)
111-160: Excessiveinfo!logging for a frequently-called function.
write_env_file_if_enabledis invoked on every port-forward start/stop and via a polling loop. The sixinfo!log lines (including the DB value readback on lines 114 and 130) will generate significant log noise. Consider downgrading most of these todebug!, keeping only the final "Auto-synced" message atinfo!.♻️ Suggested change
let enabled = match get_env_auto_sync_enabled().await { Ok(v) => { - info!("env_auto_sync_enabled from DB: {}", v); + debug!("env_auto_sync_enabled from DB: {}", v); v } ... if !enabled { - info!("Env auto-sync is disabled, skipping"); + debug!("Env auto-sync is disabled, skipping"); return Ok(()); } let path = match get_env_auto_sync_path().await { Ok(p) => { - info!("env_auto_sync_path from DB: {:?}", p); + debug!("env_auto_sync_path from DB: {:?}", p); p } ... let Some(file_path) = path else { - info!("Env auto-sync enabled but no path configured"); + debug!("Env auto-sync enabled but no path configured"); return Ok(()); }; let content = generate_env_content(true).await?; let line_count = content.lines().count(); - info!( + debug!( "Writing .env file to {} ({} lines, {} bytes)",(Also add
use log::{debug, error, info};at the top.)
153-156: Synchronousstd::fs::writein async context.
std::fs::writeblocks the current thread. Since this runs inside atokioasync task (called from Tauri commands and polling loops), it will block the runtime thread for the duration of the write. For small files this is usually acceptable, but consider usingtokio::fs::writefor consistency with the async runtime, especially if this will also be called from a polling loop.♻️ Proposed change
- std::fs::write(&file_path, &content).map_err(|e| { + tokio::fs::write(&file_path, &content).await.map_err(|e| { error!("Failed to write .env file to {}: {}", file_path, e); e.to_string() })?;
88-92: Missing trailing newline in generated.envcontent.
lines.join("\n")produces content without a trailing newline. Most tools and POSIX conventions expect text files to end with a newline character.♻️ Proposed fix
- Ok(lines.join("\n")) + let mut content = lines.join("\n"); + content.push('\n'); + Ok(content)frontend/src/components/Footer/index.tsx (1)
147-150: Consider disabling the menu item whenhandleExportEnvis not provided.Since
handleExportEnvis optional inFooterProps, clicking this item when the prop isundefinedwill silently do nothing. For consistency with other menu items (e.g., import is disabled whencredentialsSavedis true), consider disabling this item when the handler is absent.♻️ Proposed change
- <MenuItem value='export-env' onClick={handleExportEnv}> + <MenuItem value='export-env' onClick={handleExportEnv} disabled={!handleExportEnv}>frontend/src/components/SettingsModal/index.tsx (1)
381-399: Remove debugconsole.logstatements before merging.Lines 382-385 and 390 contain verbose debug logging (
console.log('Saving env auto-sync settings:...',console.log('Env auto-sync settings saved successfully')). Whileconsole.erroron failure is appropriate, the success/pre-save logs are debug artifacts that add noise in production.Same applies to line 148 in
loadEnvAutoSyncSettings.crates/kftray-tauri/src/commands/portforward.rs (2)
86-103: Redundant .env sync: both command handlers and the polling loop trigger writes.Every start/stop command (lines 136-205) calls
write_env_file_if_enabled()immediately after the operation. The polling loop at line 100 also calls the same function when it detects state changes (~500ms later). This results in at least two .env writes per operation.Since
write_env_file_if_enabledperforms DB reads + file I/O each time, consider either:
- Removing the per-command sync and relying solely on the polling loop (simplest, slight delay acceptable), or
- Removing the polling-based sync and keeping only the per-command sync (more responsive, less code in the loop).
Both approaches work; the current duplication is harmless but wasteful.
Also applies to: 136-141, 148-153, 160-165, 172-177, 184-189, 200-205
136-136: Remove unnecessary.clone()calls onconfigsparameter.The
configsparameter is cloned before being passed tostart_port_forwardanddeploy_and_forward_pod, but never used afterward. Since both functions take ownership (Vec<Config>, not&Vec<Config>), the value can be moved directly without cloning:
- Line 136: change
start_port_forward(configs.clone(), "udp")tostart_port_forward(configs, "udp")- Line 148: change
start_port_forward(configs.clone(), "tcp")tostart_port_forward(configs, "tcp")- Line 184: change
deploy_and_forward_pod(configs.clone())todeploy_and_forward_pod(configs)frontend/src/components/SettingsModal/EnvAutoSyncSettings.tsx (1)
28-39: Remove debugconsole.logstatements.Lines 28, 37, and 39 contain debug logging that should be removed before merging.
| if let Some(value) = settings.get("env_auto_sync_enabled") { | ||
| app_settings.env_auto_sync_enabled = value.parse().unwrap_or(false); | ||
| } |
There was a problem hiding this comment.
Inconsistent default for env_auto_sync_enabled in from_settings_manager.
Line 157 uses unwrap_or(false), but the default everywhere else is true:
default_env_auto_sync_enabled()returnstrue(Line 80)Default::default()sets it totrue(Line 98)get_env_auto_sync_enabled()insettings.rsdefaults totrue(Line 614/616)
If the stored value is somehow malformed, this path would produce false instead of the intended default true.
Proposed fix
if let Some(value) = settings.get("env_auto_sync_enabled") {
- app_settings.env_auto_sync_enabled = value.parse().unwrap_or(false);
+ app_settings.env_auto_sync_enabled = value.parse().unwrap_or(true);
}🤖 Prompt for AI Agents
In `@crates/kftray-commons/src/models/settings_model.rs` around lines 156 - 158,
The code path in from_settings_manager that reads
settings.get("env_auto_sync_enabled") uses value.parse().unwrap_or(false) which
conflicts with the intended default true from default_env_auto_sync_enabled(),
Default::default(), and get_env_auto_sync_enabled(); change the fallback to the
canonical default (e.g., use unwrap_or(true) or call
default_env_auto_sync_enabled()) so malformed values produce the same default
true as the rest of the codebase and update the env_auto_sync_enabled assignment
in from_settings_manager accordingly.
There was a problem hiding this comment.
agree with this one
unwrap_or(false) here and unwrap_or(true) in get_env_auto_sync_enabled mean two different code paths disagree on what happens with a bad stored value. i'd actually go further and suggest both should be false since the whole feature should be opt-in, but at minimum they need to match.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the get_env_auto_sync_enabled function implementation
rg -n -A 10 "pub async fn get_env_auto_sync_enabled" crates/kftray-commons/src/utils/settings.rs
Length of output: 450
@hcavarsan Good call—auto-writing to the filesystem should definitely be opt-in. I found all four locations that need alignment:
Locations to change from true → false:
settings_model.rs:80—default_env_auto_sync_enabled()currently returnstruesettings.rs:616—unwrap_or(true)for parse error fallbacksettings.rs:618—Ok(true)when the setting key is missing
Already correct:
4. settings_model.rs:157 — from_settings_manager already uses unwrap_or(false) ✓
Would you like me to plan the changes to make all defaults consistently false?
🧬 Code Graph Analysis Results
[crates/kftray-commons/src/utils/settings.rs:263-265]
/// The default() implementation delegates to Self::new().
fn default() -> Self {
Self::new()
}
| REDIS_PORT=6379 | ||
| ``` | ||
|
|
||
| The variable names are derived from the service alias (or service name if no alias is set), converted to uppercase with dashes replaced by underscores. |
There was a problem hiding this comment.
Documentation omits the target fallback in name derivation.
The code derives variable names with priority: alias → service → target → "unknown" (see derive_env_var_name in env_export.rs). This line only mentions alias and service name. Consider updating for accuracy.
📝 Suggested text
-The variable names are derived from the service alias (or service name if no alias is set), converted to uppercase with dashes replaced by underscores.
+The variable names are derived from the service alias, service name, or target (in that priority order), converted to uppercase with dashes replaced by underscores.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The variable names are derived from the service alias (or service name if no alias is set), converted to uppercase with dashes replaced by underscores. | |
| The variable names are derived from the service alias, service name, or target (in that priority order), converted to uppercase with dashes replaced by underscores. |
🤖 Prompt for AI Agents
In `@docs/kftray/USAGE.md` at line 132, Update the documentation line to reflect
the full fallback ordering used by derive_env_var_name in env_export.rs: state
that variable names are derived from the service alias, or service name if no
alias, then the target if both are absent, and finally "unknown" as a last
resort; ensure you mention the uppercase conversion and dash-to-underscore
replacement behavior remains unchanged.
There was a problem hiding this comment.
fair point. if the code does fall back to target for name derivation, the docs should mention it.
There was a problem hiding this comment.
@hcavarsan, thanks for confirming! Yes, including the target fallback in the documentation will make it more accurate and complete for users who rely on that field for naming. Looking forward to seeing the docs updated to reflect the full priority order.
🧠 Learnings used
Learnt from: hcavarsan
Repo: hcavarsan/kftray PR: 578
File: crates/kftray-commons/src/utils/env_export.rs:166-180
Timestamp: 2026-02-14T14:55:39.801Z
Learning: In kftray's env_export feature, environment variable names are derived from config fields (alias, service, or target) which cannot start with digits in this application's domain, so additional validation in sanitize_env_var_name to prepend underscores for digit-prefixed names is unnecessary.
ereli-sevenai
force-pushed
the
ereli/add_support_for_env_file_export
branch
from
2120362 to
f3fc8cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/kftray-commons/src/utils/env_export.rs`:
- Around line 112-141: The current frequent info-level logs in
write_env_file_if_enabled (calls to get_env_auto_sync_enabled and
get_env_auto_sync_path) are too noisy for a 500ms polling loop; change the
non-error, common no-op logs to a lower level (use debug! or trace!) —
specifically the messages logged after successful get_env_auto_sync_enabled
("env_auto_sync_enabled from DB: {}"), the early-return when !enabled ("Env
auto-sync is disabled, skipping"), the successful get_env_auto_sync_path
("env_auto_sync_path from DB: {:?}"), and the Some(path) else branch ("Env
auto-sync enabled but no path configured"); keep error! for the Err(e) branches
and retain current behavior in check_and_emit_changes.
- Around line 111-160: The function write_env_file_if_enabled is async but uses
the blocking std::fs::write which will block the Tokio runtime; replace the
synchronous call with tokio::fs::write(&file_path, &content).await and
propagate/map the async error the same way (log via error!("Failed to write .env
file to {}: {}", file_path, e) and return e.to_string()), ensuring tokio::fs is
imported and the await is used instead of the current std::fs::write sync call.
🧹 Nitpick comments (4)
frontend/src/components/SettingsModal/index.tsx (2)
142-156: Remove debugconsole.logstatements.Line 148 logs settings on every load. The rest of the component only uses
console.errorfor error paths. Thisconsole.logshould be removed to stay consistent.♻️ Suggested fix
const loadEnvAutoSyncSettings = async () => { try { const settings = await invoke<{ enabled: boolean; path: string | null }>( 'get_env_auto_sync_settings', ) - console.log('Loaded env auto-sync settings from DB:', settings) setEnvAutoSyncEnabled(settings.enabled) setEnvAutoSyncPath(settings.path || '') } catch (error) { console.error('Error loading env auto-sync settings:', error) setEnvAutoSyncEnabled(false) setEnvAutoSyncPath('') } }
381-399: Remove debugconsole.logstatements from save path.Lines 382–385 and 390 are development-time debug logs. Other settings save blocks (SSL on line 339, logs on line 365) don't log on success — only on error via
console.error.♻️ Suggested fix
try { - console.log('Saving env auto-sync settings:', { - enabled: envAutoSyncEnabled, - path: envAutoSyncPath, - }) await invoke('set_env_auto_sync_settings', { enabled: envAutoSyncEnabled, path: envAutoSyncPath || null, }) - console.log('Env auto-sync settings saved successfully') } catch (envError) { console.error('Error saving env auto-sync settings:', envError)crates/kftray-commons/src/models/settings_model.rs (1)
232-245: Tests don't cover the newenv_auto_sync_*fields.
test_default_app_settingsdoesn't assertenv_auto_sync_enabledorenv_auto_sync_path. Similarly,test_from_settings_managerandtest_to_settings_mapdon't exercise the new fields. Consider adding assertions to confirm round-trip correctness.crates/kftray-tauri/src/commands/portforward.rs (1)
86-103: Double env sync: polling loop + direct command calls.
write_env_file_if_enabled()is called both here (on every detected change in the 500ms polling loop) and directly in each start/stop command handler. This means every port-forward operation triggers at least two writes: one immediate and one ~500ms later when the polling loop detects the state change.This is functionally harmless (idempotent write), but it's worth being aware of. If the redundancy is intentional for reliability, a brief comment would clarify intent. If not, you could skip the call here and rely solely on command-level triggers (or vice versa).
| pub async fn write_env_file_if_enabled() -> Result<(), String> { | ||
| let enabled = match get_env_auto_sync_enabled().await { | ||
| Ok(v) => { | ||
| info!("env_auto_sync_enabled from DB: {}", v); | ||
| v | ||
| } | ||
| Err(e) => { | ||
| error!("Failed to get env_auto_sync_enabled: {}", e); | ||
| return Ok(()); | ||
| } | ||
| }; | ||
|
|
||
| if !enabled { | ||
| info!("Env auto-sync is disabled, skipping"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let path = match get_env_auto_sync_path().await { | ||
| Ok(p) => { | ||
| info!("env_auto_sync_path from DB: {:?}", p); | ||
| p | ||
| } | ||
| Err(e) => { | ||
| error!("Failed to get env_auto_sync_path: {}", e); | ||
| return Ok(()); | ||
| } | ||
| }; | ||
|
|
||
| let Some(file_path) = path else { | ||
| info!("Env auto-sync enabled but no path configured"); | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| let content = generate_env_content(true).await?; | ||
| let line_count = content.lines().count(); | ||
| info!( | ||
| "Writing .env file to {} ({} lines, {} bytes)", | ||
| file_path, | ||
| line_count, | ||
| content.len() | ||
| ); | ||
|
|
||
| std::fs::write(&file_path, &content).map_err(|e| { | ||
| error!("Failed to write .env file to {}: {}", file_path, e); | ||
| e.to_string() | ||
| })?; | ||
|
|
||
| info!("Auto-synced .env file to {}", file_path); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Blocking std::fs::write in async context will block the Tokio runtime thread.
write_env_file_if_enabled is async and called from Tokio tasks (the 500ms polling loop and command handlers), but std::fs::write on Line 153 is synchronous. This blocks the executor thread during the write. Use tokio::fs::write instead.
Proposed fix
- std::fs::write(&file_path, &content).map_err(|e| {
+ tokio::fs::write(&file_path, &content).await.map_err(|e| {
error!("Failed to write .env file to {}: {}", file_path, e);
e.to_string()
})?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn write_env_file_if_enabled() -> Result<(), String> { | |
| let enabled = match get_env_auto_sync_enabled().await { | |
| Ok(v) => { | |
| info!("env_auto_sync_enabled from DB: {}", v); | |
| v | |
| } | |
| Err(e) => { | |
| error!("Failed to get env_auto_sync_enabled: {}", e); | |
| return Ok(()); | |
| } | |
| }; | |
| if !enabled { | |
| info!("Env auto-sync is disabled, skipping"); | |
| return Ok(()); | |
| } | |
| let path = match get_env_auto_sync_path().await { | |
| Ok(p) => { | |
| info!("env_auto_sync_path from DB: {:?}", p); | |
| p | |
| } | |
| Err(e) => { | |
| error!("Failed to get env_auto_sync_path: {}", e); | |
| return Ok(()); | |
| } | |
| }; | |
| let Some(file_path) = path else { | |
| info!("Env auto-sync enabled but no path configured"); | |
| return Ok(()); | |
| }; | |
| let content = generate_env_content(true).await?; | |
| let line_count = content.lines().count(); | |
| info!( | |
| "Writing .env file to {} ({} lines, {} bytes)", | |
| file_path, | |
| line_count, | |
| content.len() | |
| ); | |
| std::fs::write(&file_path, &content).map_err(|e| { | |
| error!("Failed to write .env file to {}: {}", file_path, e); | |
| e.to_string() | |
| })?; | |
| info!("Auto-synced .env file to {}", file_path); | |
| Ok(()) | |
| } | |
| pub async fn write_env_file_if_enabled() -> Result<(), String> { | |
| let enabled = match get_env_auto_sync_enabled().await { | |
| Ok(v) => { | |
| info!("env_auto_sync_enabled from DB: {}", v); | |
| v | |
| } | |
| Err(e) => { | |
| error!("Failed to get env_auto_sync_enabled: {}", e); | |
| return Ok(()); | |
| } | |
| }; | |
| if !enabled { | |
| info!("Env auto-sync is disabled, skipping"); | |
| return Ok(()); | |
| } | |
| let path = match get_env_auto_sync_path().await { | |
| Ok(p) => { | |
| info!("env_auto_sync_path from DB: {:?}", p); | |
| p | |
| } | |
| Err(e) => { | |
| error!("Failed to get env_auto_sync_path: {}", e); | |
| return Ok(()); | |
| } | |
| }; | |
| let Some(file_path) = path else { | |
| info!("Env auto-sync enabled but no path configured"); | |
| return Ok(()); | |
| }; | |
| let content = generate_env_content(true).await?; | |
| let line_count = content.lines().count(); | |
| info!( | |
| "Writing .env file to {} ({} lines, {} bytes)", | |
| file_path, | |
| line_count, | |
| content.len() | |
| ); | |
| tokio::fs::write(&file_path, &content).await.map_err(|e| { | |
| error!("Failed to write .env file to {}: {}", file_path, e); | |
| e.to_string() | |
| })?; | |
| info!("Auto-synced .env file to {}", file_path); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In `@crates/kftray-commons/src/utils/env_export.rs` around lines 111 - 160, The
function write_env_file_if_enabled is async but uses the blocking std::fs::write
which will block the Tokio runtime; replace the synchronous call with
tokio::fs::write(&file_path, &content).await and propagate/map the async error
the same way (log via error!("Failed to write .env file to {}: {}", file_path,
e) and return e.to_string()), ensuring tokio::fs is imported and the await is
used instead of the current std::fs::write sync call.
There was a problem hiding this comment.
agree with this one...polling loop call path definitely should use tokio::fs::write instead.
There was a problem hiding this comment.
@hcavarsan, thank you for confirming! I'm glad we caught this before it could impact the runtime performance. The async I/O will ensure the polling loop and command handlers stay responsive during the file writes. 🐇
/\_/\
( o.o ) ✓
> ^ <
| let enabled = match get_env_auto_sync_enabled().await { | ||
| Ok(v) => { | ||
| info!("env_auto_sync_enabled from DB: {}", v); | ||
| v | ||
| } | ||
| Err(e) => { | ||
| error!("Failed to get env_auto_sync_enabled: {}", e); | ||
| return Ok(()); | ||
| } | ||
| }; | ||
|
|
||
| if !enabled { | ||
| info!("Env auto-sync is disabled, skipping"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let path = match get_env_auto_sync_path().await { | ||
| Ok(p) => { | ||
| info!("env_auto_sync_path from DB: {:?}", p); | ||
| p | ||
| } | ||
| Err(e) => { | ||
| error!("Failed to get env_auto_sync_path: {}", e); | ||
| return Ok(()); | ||
| } | ||
| }; | ||
|
|
||
| let Some(file_path) = path else { | ||
| info!("Env auto-sync enabled but no path configured"); | ||
| return Ok(()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Excessive info! logging on every invocation — called every 500ms from the polling loop.
Lines 114, 124, 130, and 140 all log at info level. Since write_env_file_if_enabled is invoked from check_and_emit_changes which runs every 500ms, this will produce 4+ info log lines every half second even when sync is disabled or no path is set. Consider using debug! or trace! for the common no-op paths.
Proposed fix
-use log::{error, info};
+use log::{debug, error, info}; let enabled = match get_env_auto_sync_enabled().await {
Ok(v) => {
- info!("env_auto_sync_enabled from DB: {}", v);
+ debug!("env_auto_sync_enabled from DB: {}", v);
v
}
...
};
if !enabled {
- info!("Env auto-sync is disabled, skipping");
+ debug!("Env auto-sync is disabled, skipping");
return Ok(());
}
let path = match get_env_auto_sync_path().await {
Ok(p) => {
- info!("env_auto_sync_path from DB: {:?}", p);
+ debug!("env_auto_sync_path from DB: {:?}", p);
p
}
...
};
let Some(file_path) = path else {
- info!("Env auto-sync enabled but no path configured");
+ debug!("Env auto-sync enabled but no path configured");
return Ok(());
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let enabled = match get_env_auto_sync_enabled().await { | |
| Ok(v) => { | |
| info!("env_auto_sync_enabled from DB: {}", v); | |
| v | |
| } | |
| Err(e) => { | |
| error!("Failed to get env_auto_sync_enabled: {}", e); | |
| return Ok(()); | |
| } | |
| }; | |
| if !enabled { | |
| info!("Env auto-sync is disabled, skipping"); | |
| return Ok(()); | |
| } | |
| let path = match get_env_auto_sync_path().await { | |
| Ok(p) => { | |
| info!("env_auto_sync_path from DB: {:?}", p); | |
| p | |
| } | |
| Err(e) => { | |
| error!("Failed to get env_auto_sync_path: {}", e); | |
| return Ok(()); | |
| } | |
| }; | |
| let Some(file_path) = path else { | |
| info!("Env auto-sync enabled but no path configured"); | |
| return Ok(()); | |
| let enabled = match get_env_auto_sync_enabled().await { | |
| Ok(v) => { | |
| debug!("env_auto_sync_enabled from DB: {}", v); | |
| v | |
| } | |
| Err(e) => { | |
| error!("Failed to get env_auto_sync_enabled: {}", e); | |
| return Ok(()); | |
| } | |
| }; | |
| if !enabled { | |
| debug!("Env auto-sync is disabled, skipping"); | |
| return Ok(()); | |
| } | |
| let path = match get_env_auto_sync_path().await { | |
| Ok(p) => { | |
| debug!("env_auto_sync_path from DB: {:?}", p); | |
| p | |
| } | |
| Err(e) => { | |
| error!("Failed to get env_auto_sync_path: {}", e); | |
| return Ok(()); | |
| } | |
| }; | |
| let Some(file_path) = path else { | |
| debug!("Env auto-sync enabled but no path configured"); | |
| return Ok(()); | |
| }; |
🤖 Prompt for AI Agents
In `@crates/kftray-commons/src/utils/env_export.rs` around lines 112 - 141, The
current frequent info-level logs in write_env_file_if_enabled (calls to
get_env_auto_sync_enabled and get_env_auto_sync_path) are too noisy for a 500ms
polling loop; change the non-error, common no-op logs to a lower level (use
debug! or trace!) — specifically the messages logged after successful
get_env_auto_sync_enabled ("env_auto_sync_enabled from DB: {}"), the
early-return when !enabled ("Env auto-sync is disabled, skipping"), the
successful get_env_auto_sync_path ("env_auto_sync_path from DB: {:?}"), and the
Some(path) else branch ("Env auto-sync enabled but no path configured"); keep
error! for the Err(e) branches and retain current behavior in
check_and_emit_changes.
There was a problem hiding this comment.
@hcavarsan, thank you for confirming! Changing those logs to debug! level will definitely help keep the log output cleaner given the frequent polling. Let me know if you need any assistance with the implementation.
ereli-sevenai
force-pushed
the
ereli/add_support_for_env_file_export
branch
from
c32bffd to
ef97651
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/kftray-commons/src/utils/env_export.rs`:
- Around line 88-92: The generated .env string currently returns
Ok(lines.join("\n")) which omits the trailing newline; change the return so the
joined string ends with a newline (for example replace Ok(lines.join("\n")) with
Ok(lines.join("\n") + "\n") or Ok(format!("{}\n", lines.join("\n")))) so the
output from this function includes the required final newline.
🧹 Nitpick comments (6)
crates/kftray-commons/src/utils/env_export.rs (1)
1-5: Minor: inconsistentHashMapimport style.
HashSetis imported at the top (line 1), butHashMapis used fully qualified on lines 48-49. Consider importing both fromstd::collectionsat the top for consistency.Proposed fix
-use std::collections::HashSet; +use std::collections::{HashMap, HashSet};- let mut name_counts: std::collections::HashMap<String, usize> = - std::collections::HashMap::new(); + let mut name_counts: HashMap<String, usize> = HashMap::new();Also applies to: 48-49
crates/kftray-tauri/src/commands/settings.rs (2)
212-218: Consider removing the post-write verification read-back.The verification read on every save adds two unnecessary DB round-trips. If this was added for debugging, consider either removing it or guarding it behind
debug!log level instead ofinfo!.Proposed fix
- // Verify the settings were saved correctly - let verify_enabled = get_env_auto_sync_enabled().await.unwrap_or(false); - let verify_path = get_env_auto_sync_path().await.ok().flatten(); - info!( - "Verified saved settings: enabled={}, path={:?}", - verify_enabled, verify_path - ); - info!("Successfully set env auto-sync settings");
197-210: Non-atomic update ofenabledandpathsettings.If
set_env_auto_sync_enabledsucceeds butset_env_auto_sync_pathfails, the settings are left in an inconsistent state (enabled changed, path unchanged). This matches the existing pattern for other settings in this codebase, so it's not a regression, but worth noting for a future improvement.frontend/src/components/SettingsModal/index.tsx (1)
381-399: Remove debugconsole.logstatements.Lines 382-385 and 390 have
console.logfor saving env auto-sync settings. The rest of the file only usesconsole.errorfor error cases — these look like leftover debug artifacts. Theconsole.logon line 148 inloadEnvAutoSyncSettingshas the same issue.🧹 Proposed cleanup
try { - console.log('Saving env auto-sync settings:', { - enabled: envAutoSyncEnabled, - path: envAutoSyncPath, - }) await invoke('set_env_auto_sync_settings', { enabled: envAutoSyncEnabled, path: envAutoSyncPath || null, }) - console.log('Env auto-sync settings saved successfully') } catch (envError) {And in
loadEnvAutoSyncSettings:- console.log('Loaded env auto-sync settings from DB:', settings) setEnvAutoSyncEnabled(settings.enabled)crates/kftray-tauri/src/commands/portforward.rs (2)
136-205: Consider extracting the repeated env-sync-after-action pattern.The same
write_env_file_if_enabled().await+ error log block is copy-pasted in 6 command handlers. A small helper would reduce duplication:async fn sync_env_after<T>(result: &Result<T, String>, context: &str) { if let Err(e) = write_env_file_if_enabled().await { error!("Failed to auto-sync .env file after {context}: {e}"); } }Also note that the polling loop in
check_and_emit_changes(line 100) will also trigger a sync when it detects the state change, so each operation effectively syncs twice. This is harmless (idempotent write) but worth being aware of.
136-141: Env sync fires even when the port-forward operation fails.On line 136, if
start_port_forwardreturnsErr, the env sync still runs. This is probably fine since the file reflects DB state regardless of this call's outcome, but if you add the helper suggested above, you could optionally skip sync on failure to avoid unnecessary I/O.
|
hey @ereli-sevenai , thanks for this. i went through the code and have some suggestions.. 1 - default to enabled might surprise users on upgrade: 2 - sync triggers are redundant... 3 - parse-failure defaults don't agree... 4 - i've been iterating on this locally after the PR was opened and ended up adding per-context support, k8s-style env var naming, and tui integration. if you're open to it i could push some changes to your branch that i think would be useful. |
2 participants
Changes
#577
Checklist before merging
Screenshots