From f748173809d24814a5d8296515352c0a8a9d8cc2 Mon Sep 17 00:00:00 2001 From: Jcode Date: Fri, 17 Jul 2026 02:02:49 -0700 Subject: [PATCH 0001/1688] fix(windows): upgrade a running launcher safely Allow the stable launcher path to be replaced while an existing Jcode process is still using the previous executable. Preserve rollback behavior, clean stale launcher backups during later installs and uninstall, and cover live replacement plus injected final-move failure cases. --- scripts/install.ps1 | 50 +++++++++++++++-- scripts/test_windows_launcher_install.ps1 | 65 +++++++++++++++++++++++ scripts/test_windows_setup_evaluation.ps1 | 6 +++ scripts/uninstall.ps1 | 35 +++++++++++- 4 files changed, 150 insertions(+), 6 deletions(-) diff --git a/scripts/install.ps1 b/scripts/install.ps1 index 14021c20e7..9fda7ba52f 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -431,6 +431,15 @@ function Set-JcodeProcessPath([string]$InstallDir) { return $update } +function Remove-JcodeStaleLauncherBackups { + param( + [Parameter(Mandatory = $true)][string]$LauncherDir + ) + + Get-ChildItem -LiteralPath $LauncherDir -Filter '.jcode-launcher-old-*.exe' -File -Force -ErrorAction SilentlyContinue | + Remove-Item -Force -ErrorAction SilentlyContinue +} + function Install-JcodeLauncher { param( [Parameter(Mandatory = $true)][string]$SourcePath, @@ -440,15 +449,48 @@ function Install-JcodeLauncher { $launcherDir = Split-Path -Parent $LauncherPath New-Item -ItemType Directory -Path $launcherDir -Force | Out-Null - $tempLauncher = Join-Path $launcherDir (".jcode-launcher-{0}.tmp.exe" -f ([guid]::NewGuid().ToString('N'))) + $operationId = [guid]::NewGuid().ToString('N') + $tempLauncher = Join-Path $launcherDir (".jcode-launcher-{0}.tmp.exe" -f $operationId) + $oldLauncher = Join-Path $launcherDir (".jcode-launcher-old-{0}.exe" -f $operationId) + $movedExistingLauncher = $false try { Copy-Item -Path $SourcePath -Destination $tempLauncher -Force - Move-Item -Path $tempLauncher -Destination $LauncherPath -Force + if (Test-Path -LiteralPath $LauncherPath) { + # Windows will not overwrite a loaded executable, but it does allow + # the directory entry to be renamed while the process keeps running + # from its existing file handle. Move the old launcher aside first, + # then atomically put the new binary at the stable PATH location. + Move-Item -LiteralPath $LauncherPath -Destination $oldLauncher + $movedExistingLauncher = $true + } + + try { + Move-Item -LiteralPath $tempLauncher -Destination $LauncherPath + } catch { + if ($movedExistingLauncher -and -not (Test-Path -LiteralPath $LauncherPath)) { + Move-Item -LiteralPath $oldLauncher -Destination $LauncherPath + $movedExistingLauncher = $false + } + throw + } + + if ($movedExistingLauncher) { + # Removal succeeds immediately for an idle launcher. If an older + # jcode process still has the renamed executable loaded, Windows + # keeps it until that process exits and the next install cleans it. + Remove-Item -LiteralPath $oldLauncher -Force -ErrorAction SilentlyContinue + } + + # Only prune backups after the stable path contains the new launcher. + # Doing this before replacement could delete another concurrent + # installer's rollback file during its short rename window. + Remove-JcodeStaleLauncherBackups -LauncherDir $launcherDir } finally { - Remove-Item -Path $tempLauncher -Force -ErrorAction SilentlyContinue + Remove-Item -LiteralPath $tempLauncher -Force -ErrorAction SilentlyContinue } - return $LauncherPath} + return $LauncherPath +} function Resolve-OptionalPath([string]$PathValue) { if (-not $PathValue) { diff --git a/scripts/test_windows_launcher_install.ps1 b/scripts/test_windows_launcher_install.ps1 index f022fa776e..d1d96270ff 100644 --- a/scripts/test_windows_launcher_install.ps1 +++ b/scripts/test_windows_launcher_install.ps1 @@ -137,6 +137,71 @@ try { Assert-Equal $false $upgradePath.Changed 'upgrade should not add another PATH entry when launcher dir is already present' Assert-PathCount $upgradePath.Path $installDir 1 'upgrade should preserve exactly one launcher PATH entry' + Write-Host 'test_running_launcher_can_be_replaced' + $runningDir = Join-Path $testRoot 'running-launcher' + New-Item -ItemType Directory -Path $runningDir -Force | Out-Null + $runningLauncher = Join-Path $runningDir 'jcode.exe' + $replacementLauncher = Join-Path $runningDir 'replacement.exe' + Copy-Item -LiteralPath $env:ComSpec -Destination $runningLauncher + Copy-Item -LiteralPath (Join-Path $env:WINDIR 'System32\where.exe') -Destination $replacementLauncher + $runningProcess = Start-Process -FilePath $runningLauncher -ArgumentList @('/d', '/q', '/c', 'ping -n 30 127.0.0.1 > nul') -WindowStyle Hidden -PassThru + try { + Start-Sleep -Milliseconds 500 + Assert-Equal $false $runningProcess.HasExited 'test launcher process should still be running before replacement' + + Install-JcodeLauncher -SourcePath $replacementLauncher -LauncherPath $runningLauncher | Out-Null + + Assert-Equal (Get-FileHash -LiteralPath $replacementLauncher -Algorithm SHA256).Hash (Get-FileHash -LiteralPath $runningLauncher -Algorithm SHA256).Hash 'live upgrade should place the replacement at the stable launcher path' + Assert-Equal $false $runningProcess.HasExited 'live upgrade should not terminate the process using the previous launcher' + $runningBackups = @(Get-ChildItem -LiteralPath $runningDir -Filter '.jcode-launcher-old-*.exe' -Force -ErrorAction SilentlyContinue) + Assert-Equal 1 $runningBackups.Count 'live upgrade should retain exactly one locked old launcher until the process exits' + } finally { + Stop-ProcessTree -ProcessId $runningProcess.Id + try { Wait-Process -Id $runningProcess.Id -Timeout 10 -ErrorAction SilentlyContinue } catch {} + } + Remove-JcodeStaleLauncherBackups -LauncherDir $runningDir + $runningBackups = @(Get-ChildItem -LiteralPath $runningDir -Filter '.jcode-launcher-old-*.exe' -Force -ErrorAction SilentlyContinue) + Assert-Equal 0 $runningBackups.Count 'stale live-upgrade launchers should be removable after the old process exits' + + Write-Host 'test_launcher_replacement_failure_rolls_back' + $rollbackDir = Join-Path $testRoot 'launcher-rollback' + New-Item -ItemType Directory -Path $rollbackDir -Force | Out-Null + $rollbackLauncher = Join-Path $rollbackDir 'jcode.exe' + $rollbackSource = Join-Path $rollbackDir 'replacement.exe' + Set-Content -LiteralPath $rollbackLauncher -Value 'known-good' -NoNewline + Set-Content -LiteralPath $rollbackSource -Value 'replacement' -NoNewline + $script:injectLauncherMoveFailure = $true + function Move-Item { + [CmdletBinding()] + param( + [string]$Path, + [string]$LiteralPath, + [Parameter(Mandatory = $true)][string]$Destination, + [switch]$Force + ) + $source = if ($PSBoundParameters.ContainsKey('LiteralPath')) { $LiteralPath } else { $Path } + if ($script:injectLauncherMoveFailure -and $source -like '*.tmp.exe' -and $Destination -eq $rollbackLauncher) { + $script:injectLauncherMoveFailure = $false + throw 'simulated final launcher move failure' + } + $moveArgs = @{ Destination = $Destination } + if ($PSBoundParameters.ContainsKey('LiteralPath')) { $moveArgs.LiteralPath = $LiteralPath } else { $moveArgs.Path = $Path } + if ($Force) { $moveArgs.Force = $true } + Microsoft.PowerShell.Management\Move-Item @moveArgs + } + $rollbackThrew = $false + try { + Install-JcodeLauncher -SourcePath $rollbackSource -LauncherPath $rollbackLauncher | Out-Null + } catch { + $rollbackThrew = $true + } finally { + Remove-Item Function:\Move-Item -ErrorAction SilentlyContinue + } + Assert-Equal $true $rollbackThrew 'launcher replacement should surface a final move failure' + Assert-Equal 'known-good' (Get-Content -LiteralPath $rollbackLauncher -Raw) 'launcher replacement should restore the previous stable launcher after a final move failure' + Assert-Equal 0 @(Get-ChildItem -LiteralPath $rollbackDir -Filter '.jcode-launcher-*.tmp.exe' -Force -ErrorAction SilentlyContinue).Count 'rollback should remove temporary launcher files' + Assert-Equal 0 @(Get-ChildItem -LiteralPath $rollbackDir -Filter '.jcode-launcher-old-*.exe' -Force -ErrorAction SilentlyContinue).Count 'rollback should restore rather than retain the previous launcher backup' + Write-Host 'test_uninstall_removes_launcher_and_only_jcode_path' $removeCurrentPath = "$installDir;C:\Keep;$installVariant;C:\Keep" $removeUpdate = Resolve-JcodePathUpdate -InstallDir $installDir -CurrentPath $removeCurrentPath -RemoveOnly diff --git a/scripts/test_windows_setup_evaluation.ps1 b/scripts/test_windows_setup_evaluation.ps1 index 59225c4812..11507f9637 100644 --- a/scripts/test_windows_setup_evaluation.ps1 +++ b/scripts/test_windows_setup_evaluation.ps1 @@ -411,6 +411,8 @@ try { Assert-Equal $false (Test-JcodeSafePurgePath $profile.UserProfile) 'the user profile must never be accepted as a purge target' Assert-Equal $false (Test-JcodeSafePurgePath $profile.Root) 'a parent workspace must never be accepted as a purge target' Assert-Equal $true (Test-JcodeManagedExecutablePath -ExecutablePath $profile.LauncherPath -LauncherPath $profile.LauncherPath -BuildsDir $profile.BuildsDir) 'the installed launcher should be recognized as managed' + Assert-Equal $true (Test-JcodeManagedExecutablePath -ExecutablePath (Join-Path $profile.InstallDir '.jcode-launcher-old-a1b2c3.exe') -LauncherPath $profile.LauncherPath -BuildsDir $profile.BuildsDir) 'a renamed live-upgrade launcher should be recognized as managed' + Assert-Equal $false (Test-JcodeManagedExecutablePath -ExecutablePath (Join-Path $profile.InstallDir 'other-tool.exe') -LauncherPath $profile.LauncherPath -BuildsDir $profile.BuildsDir) 'unrelated executables beside the launcher must not be terminated' Assert-Equal $true (Test-JcodeManagedExecutablePath -ExecutablePath (Join-Path $profile.BuildsDir 'stable\jcode.exe') -LauncherPath $profile.LauncherPath -BuildsDir $profile.BuildsDir) 'installed version binaries should be recognized as managed' Assert-Equal $false (Test-JcodeManagedExecutablePath -ExecutablePath (Join-Path $profile.Root 'development\jcode.exe') -LauncherPath $profile.LauncherPath -BuildsDir $profile.BuildsDir) 'unrelated development binaries must not be terminated' } @@ -423,6 +425,8 @@ try { New-Item -ItemType Directory -Path $profile.HotkeyDir -Force | Out-Null New-Item -ItemType Directory -Path (Split-Path -Parent $profile.StartupShortcutPath) -Force | Out-Null Set-Content -Path $profile.LauncherPath -Value 'installed launcher' -NoNewline + $oldLauncherPath = Join-Path $profile.InstallDir '.jcode-launcher-old-a1b2c3.exe' + Set-Content -Path $oldLauncherPath -Value 'previous running launcher' -NoNewline Set-Content -Path (Join-Path $profile.BuildsDir 'stable\jcode.exe') -Value 'stable build' -NoNewline Set-Content -Path (Join-Path $profile.JcodeHome 'config.toml') -Value 'kept = true' -NoNewline Set-Content -Path (Join-Path $profile.HotkeyDir 'jcode-hotkey.ps1') -Value 'legacy listener' -NoNewline @@ -436,6 +440,8 @@ try { $exitCode = Invoke-JcodeUninstall -InstallDir $profile.InstallDir -Yes Assert-Equal 0 $exitCode 'uninstall should complete successfully in the isolated profile' Assert-PathMissing $profile.LauncherPath 'uninstall should remove the launcher' + Assert-PathMissing $oldLauncherPath 'uninstall should remove renamed live-upgrade launchers' + Assert-PathMissing $profile.InstallDir 'uninstall should remove the empty launcher directory' Assert-PathMissing $profile.BuildsDir 'uninstall should remove installed build binaries' Assert-PathMissing $profile.StartupShortcutPath 'uninstall should remove the launch-hotkey Startup shortcut' Assert-PathMissing (Join-Path $profile.HotkeyDir 'jcode-hotkey.ps1') 'uninstall should remove legacy launch-hotkey artifacts' diff --git a/scripts/uninstall.ps1 b/scripts/uninstall.ps1 index f59eadfb4f..e764ac66a5 100644 --- a/scripts/uninstall.ps1 +++ b/scripts/uninstall.ps1 @@ -142,6 +142,16 @@ function Test-JcodeManagedExecutablePath([string]$ExecutablePath, [string]$Launc if (-not $executableKey) { return $false } if ($launcherKey -and $executableKey -eq $launcherKey) { return $true } + # A live upgrade may rename the loaded stable launcher before replacing it. + # Treat only that tightly-scoped backup pattern in the launcher directory as + # managed so uninstall can stop and remove it without touching other tools. + $launcherDirKey = ConvertTo-JcodePathKey (Split-Path -Parent $LauncherPath) + $executableDirKey = ConvertTo-JcodePathKey (Split-Path -Parent $ExecutablePath) + $executableName = Split-Path -Leaf $ExecutablePath + if ($launcherDirKey -and $executableDirKey -eq $launcherDirKey -and $executableName -like '.jcode-launcher-old-*.exe') { + return $true + } + $separator = [string][System.IO.Path]::DirectorySeparatorChar return [bool]($buildsKey -and $executableKey.StartsWith($buildsKey + $separator, [System.StringComparison]::OrdinalIgnoreCase)) } @@ -279,12 +289,19 @@ $userDataDir = if ($env:JCODE_HOME) { } $startupShortcutPath = Get-JcodeStartupShortcutPath $hotkeyArtifactPaths = @(Get-JcodeHotkeyArtifactPaths -UserDataDir $userDataDir) +$launcherBackupPaths = if (Test-Path -LiteralPath $InstallDir) { + @(Get-ChildItem -LiteralPath $InstallDir -Filter '.jcode-launcher-old-*.exe' -File -Force -ErrorAction SilentlyContinue | + ForEach-Object { $_.FullName }) +} else { + @() +} if ($Purge -and -not (Test-JcodeSafePurgePath $userDataDir)) { Write-Err "Refusing to purge unsafe JCODE_HOME path '$userDataDir'. Use a dedicated .jcode or jcode-* directory." } $targets = @() if (Test-Path -LiteralPath $launcherPath) { $targets += "$launcherPath (launcher)" } +foreach ($path in $launcherBackupPaths) { $targets += "$path (previous live-upgrade launcher)" } if (Test-Path -LiteralPath $buildsDir) { $targets += "$buildsDir (installed binaries)" } if (Test-Path -LiteralPath $startupShortcutPath) { $targets += "$startupShortcutPath (launch-hotkey startup shortcut)" } foreach ($path in $hotkeyArtifactPaths) { @@ -322,9 +339,16 @@ if (-not $Yes) { } try { - Get-CimInstance Win32_Process -Filter "Name = 'jcode.exe'" -ErrorAction SilentlyContinue | + $managedProcessIds = @(Get-CimInstance Win32_Process -Filter "Name = 'jcode.exe'" -ErrorAction SilentlyContinue | Where-Object { Test-JcodeManagedExecutablePath -ExecutablePath $_.ExecutablePath -LauncherPath $launcherPath -BuildsDir $buildsDir } | - ForEach-Object { Stop-Process -Id $_.ProcessId -Force -ErrorAction SilentlyContinue } + ForEach-Object { $_.ProcessId }) + foreach ($processId in $managedProcessIds) { + $process = Get-Process -Id $processId -ErrorAction SilentlyContinue + Stop-Process -Id $processId -Force -ErrorAction SilentlyContinue + if ($process) { + try { [void]$process.WaitForExit(10000) } catch {} + } + } } catch {} if (Test-Path -LiteralPath $startupShortcutPath) { @@ -351,6 +375,13 @@ if (Test-Path -LiteralPath $launcherPath) { Write-Info "Removed $launcherPath" } +foreach ($path in $launcherBackupPaths) { + if (Test-Path -LiteralPath $path) { + Remove-Item -LiteralPath $path -Force + Write-Info "Removed $path" + } +} + if (Test-Path -LiteralPath $InstallDir) { try { Remove-Item -LiteralPath $InstallDir -Force -ErrorAction SilentlyContinue } catch {} } From 4ee2a9e29b5c1183614d7f93e1f04d39d4f59fb5 Mon Sep 17 00:00:00 2001 From: Jcode Date: Fri, 17 Jul 2026 02:10:18 -0700 Subject: [PATCH 0002/1688] fix(tui): satisfy Rust 1.97 question-mark lint Use the Option question-mark idiom in the Claude takeover confirmation guard so the warnings-denied Quality Guardrails job passes on the current stable toolchain. --- crates/jcode-tui/src/tui/session_picker.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/jcode-tui/src/tui/session_picker.rs b/crates/jcode-tui/src/tui/session_picker.rs index 14b7b30cb5..1bb7ab5267 100644 --- a/crates/jcode-tui/src/tui/session_picker.rs +++ b/crates/jcode-tui/src/tui/session_picker.rs @@ -583,9 +583,7 @@ impl SessionPicker { code: KeyCode, modifiers: KeyModifiers, ) -> Option { - if self.pending_claude_takeover.is_none() { - return None; - } + self.pending_claude_takeover.as_ref()?; if code == KeyCode::Char('c') && modifiers.contains(KeyModifiers::CONTROL) { self.pending_claude_takeover = None; return Some(OverlayAction::Close); From 7582d147f81167d5066f7b61b7be0d048fc877ce Mon Sep 17 00:00:00 2001 From: jeremy <94247773+1jehuang@users.noreply.github.com> Date: Fri, 17 Jul 2026 03:32:58 -0700 Subject: [PATCH 0003/1688] tui: bound todo completion-gate nudges to stop infinite continuation loop Observed live: after a self-dev reload, an unattended session with a failing completion-confidence gate re-sent the same hidden continuation every ~5s indefinitely (one full API call per nudge) because schedule_auto_poke_followup_if_needed had no retry budget for the gate. Add TODO_COMPLETION_GATE_MAX_ATTEMPTS (5) tracked per auto-poke cycle: - consumed by each completion/spike gate nudge - reset when incomplete todos are still being poked (real progress), when auto-poke is re-armed, or when it is disabled - on exhaustion, disarm auto-poke and surface a visible warning instead of silently looping Regression test drives the gate to exhaustion against a stored low-confidence completed todo. --- crates/jcode-tui/src/tui/app.rs | 10 ++++ crates/jcode-tui/src/tui/app/commands.rs | 2 + crates/jcode-tui/src/tui/app/input.rs | 33 ++++++++++++- .../tui/app/tests/remote_events_reload_05.rs | 48 +++++++++++++++++++ crates/jcode-tui/src/tui/app/tui_lifecycle.rs | 2 + 5 files changed, 94 insertions(+), 1 deletion(-) diff --git a/crates/jcode-tui/src/tui/app.rs b/crates/jcode-tui/src/tui/app.rs index 72249f5850..160297cd85 100644 --- a/crates/jcode-tui/src/tui/app.rs +++ b/crates/jcode-tui/src/tui/app.rs @@ -859,6 +859,10 @@ pub struct App { /// final confidence increase. Low or missing completion confidence keeps /// retrying, but a spike gets one dedicated independent-validation turn. todo_confidence_spike_challenged: bool, + /// How many completion-confidence gate nudges the current auto-poke cycle + /// has sent. Without a budget, a model that stops updating its todos gets + /// nudged on every turn forever, silently burning an API call per tick. + todo_completion_gate_attempts: u8, // When armed by /overnight, automatically continue guarded follow-up turns until wake/wrap. overnight_auto_poke: Option, // Pending cross-provider resend after a failover warning/countdown. @@ -1541,6 +1545,12 @@ impl Provider for InertRuntimeProvider { impl App { const AUTO_RETRY_BASE_DELAY_SECS: u64 = 2; const AUTO_RETRY_MAX_ATTEMPTS: u8 = 3; + /// Budget for completion-confidence gate nudges per auto-poke cycle. + /// Observed live: a session that stopped updating its todos was re-nudged + /// with the same hidden continuation every ~5 seconds indefinitely, one + /// full API call per nudge. The counter resets whenever a nudge actually + /// changes the stored todos (progress) or auto-poke is re-armed. + const TODO_COMPLETION_GATE_MAX_ATTEMPTS: u8 = 5; /// Circuit breaker for credential failures: once this many consecutive /// turn errors classify as credential/auth failures, every automatic /// resend path (auto-retry, auto-poke, overnight poke, queued follow-ups) diff --git a/crates/jcode-tui/src/tui/app/commands.rs b/crates/jcode-tui/src/tui/app/commands.rs index d3dc9a7a63..15d812aabc 100644 --- a/crates/jcode-tui/src/tui/app/commands.rs +++ b/crates/jcode-tui/src/tui/app/commands.rs @@ -107,6 +107,7 @@ pub(super) fn disable_auto_poke(app: &mut App) -> usize { let cleared = clear_queued_poke_messages(app); app.auto_poke_incomplete_todos = false; app.todo_confidence_spike_challenged = false; + app.todo_completion_gate_attempts = 0; cleared } @@ -288,6 +289,7 @@ pub(super) fn activate_auto_poke(app: &mut App) -> PokeActivation { let incomplete = incomplete_poke_todos(app); app.auto_poke_incomplete_todos = true; app.todo_confidence_spike_challenged = false; + app.todo_completion_gate_attempts = 0; app.set_status_notice("Poke: ON"); if incomplete.is_empty() { diff --git a/crates/jcode-tui/src/tui/app/input.rs b/crates/jcode-tui/src/tui/app/input.rs index b03e852235..2c3683317c 100644 --- a/crates/jcode-tui/src/tui/app/input.rs +++ b/crates/jcode-tui/src/tui/app/input.rs @@ -1284,7 +1284,13 @@ impl App { super::commands::format_todo_completion_confidence(confidence_summary); let needs_spike_challenge = confidence_summary.confidence_spike_detected && !self.todo_confidence_spike_challenged; - if confidence_summary.completion_confidence_needs_validation || needs_spike_challenge { + let gate_budget_left = + self.todo_completion_gate_attempts < Self::TODO_COMPLETION_GATE_MAX_ATTEMPTS; + if (confidence_summary.completion_confidence_needs_validation || needs_spike_challenge) + && gate_budget_left + { + self.todo_completion_gate_attempts = + self.todo_completion_gate_attempts.saturating_add(1); let notice = if confidence_summary.completion_confidence_needs_validation { crate::telemetry::record_todo_gate(crate::telemetry::TodoGateKind::Completion); "🛑 Todo completion gate: completion confidence needs stronger validation." @@ -1302,8 +1308,30 @@ impl App { self.pending_queued_dispatch = true; return true; } + if (confidence_summary.completion_confidence_needs_validation || needs_spike_challenge) + && !gate_budget_left + { + // The gate keeps failing but the model is no longer making + // progress on it. Nudging again would loop forever, burning an + // API call per turn (observed live: an unattended session + // resent the same continuation every ~5s). Stop the cycle and + // surface the stall instead. + crate::logging::warn(&format!( + "Todo completion gate exhausted after {} attempts; stopping auto-poke to avoid an infinite continuation loop", + self.todo_completion_gate_attempts + )); + self.push_display_message(DisplayMessage::system( + "⚠️ Todo completion gate: validation still failing after repeated nudges. Auto-poke stopped; review the remaining todos manually.", + )); + self.auto_poke_incomplete_todos = false; + self.todo_confidence_spike_challenged = false; + self.todo_completion_gate_attempts = 0; + self.pending_queued_dispatch = false; + return false; + } self.auto_poke_incomplete_todos = false; self.todo_confidence_spike_challenged = false; + self.todo_completion_gate_attempts = 0; self.push_display_message(DisplayMessage::system(format!( "✅ Todos complete. Completion confidence: {}.", confidence_label @@ -1317,6 +1345,9 @@ impl App { incomplete.len(), if incomplete.len() == 1 { "" } else { "s" }, ))); + // Open todos mean the model is still iterating; completion-gate + // exhaustion should only trip when the gate itself stops moving. + self.todo_completion_gate_attempts = 0; self.queued_messages .push(super::commands::build_poke_message(&incomplete)); self.pending_queued_dispatch = true; diff --git a/crates/jcode-tui/src/tui/app/tests/remote_events_reload_05.rs b/crates/jcode-tui/src/tui/app/tests/remote_events_reload_05.rs index 122ff5240e..4852af2e18 100644 --- a/crates/jcode-tui/src/tui/app/tests/remote_events_reload_05.rs +++ b/crates/jcode-tui/src/tui/app/tests/remote_events_reload_05.rs @@ -146,6 +146,54 @@ fn test_reload_preserves_completed_confidence_spike_challenge() { }); } +#[test] +fn test_completion_gate_nudges_stop_after_budget_exhausted() { + with_temp_jcode_home(|| { + let mut app = create_test_app(); + app.auto_poke_incomplete_todos = true; + + // A completed todo with confidence below the gate threshold keeps the + // completion gate failing on every check. + crate::todo::save_todos( + &app.session.id, + &[crate::todo::TodoItem { + id: "todo-1".to_string(), + content: "Ship the fix".to_string(), + status: "completed".to_string(), + priority: "high".to_string(), + confidence: Some(50), + completion_confidence: Some(50), + confidence_history: vec![50], + ..Default::default() + }], + ) + .expect("save low-confidence completed todo"); + + // Each scheduled nudge consumes budget. Simulate the dispatch loop by + // clearing the queued state between iterations (as if the turn ran and + // the model made no todo progress). + for attempt in 0..App::TODO_COMPLETION_GATE_MAX_ATTEMPTS { + assert!( + app.schedule_auto_poke_followup_if_needed(), + "attempt {attempt} should still schedule a gate nudge" + ); + app.hidden_queued_system_messages.clear(); + app.pending_queued_dispatch = false; + } + + // Budget exhausted: the gate must stop scheduling and disarm auto-poke + // instead of looping forever (observed live as one API call per ~5s). + assert!( + !app.schedule_auto_poke_followup_if_needed(), + "exhausted gate must not schedule another nudge" + ); + assert!(!app.auto_poke_incomplete_todos); + assert!(!app.pending_queued_dispatch); + assert!(app.hidden_queued_system_messages.is_empty()); + assert_eq!(app.todo_completion_gate_attempts, 0); + }); +} + #[test] fn test_save_input_for_reload_removes_stale_file_when_state_is_empty() { let mut app = create_test_app(); diff --git a/crates/jcode-tui/src/tui/app/tui_lifecycle.rs b/crates/jcode-tui/src/tui/app/tui_lifecycle.rs index 16b21bbd65..eea97792e8 100644 --- a/crates/jcode-tui/src/tui/app/tui_lifecycle.rs +++ b/crates/jcode-tui/src/tui/app/tui_lifecycle.rs @@ -412,6 +412,7 @@ impl App { pending_turn: false, auto_poke_incomplete_todos: true, todo_confidence_spike_challenged: false, + todo_completion_gate_attempts: 0, overnight_auto_poke: None, pending_provider_failover: None, pending_fallback_offer: None, @@ -829,6 +830,7 @@ impl App { pending_turn: false, auto_poke_incomplete_todos: true, todo_confidence_spike_challenged: false, + todo_completion_gate_attempts: 0, overnight_auto_poke: None, pending_provider_failover: None, pending_fallback_offer: None, From 1b0e87ec08d4fb12884660a662cee25074f242af Mon Sep 17 00:00:00 2001 From: Jcode Date: Fri, 17 Jul 2026 04:05:20 -0700 Subject: [PATCH 0004/1688] test(windows): stabilize running-launcher fixture Use a directly launched ping.exe copy instead of a copied cmd.exe wrapper so the loaded-executable replacement regression reliably keeps its fixture process alive. --- scripts/test_windows_launcher_install.ps1 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/test_windows_launcher_install.ps1 b/scripts/test_windows_launcher_install.ps1 index d1d96270ff..0125fc23ef 100644 --- a/scripts/test_windows_launcher_install.ps1 +++ b/scripts/test_windows_launcher_install.ps1 @@ -142,9 +142,9 @@ try { New-Item -ItemType Directory -Path $runningDir -Force | Out-Null $runningLauncher = Join-Path $runningDir 'jcode.exe' $replacementLauncher = Join-Path $runningDir 'replacement.exe' - Copy-Item -LiteralPath $env:ComSpec -Destination $runningLauncher + Copy-Item -LiteralPath (Join-Path $env:WINDIR 'System32\ping.exe') -Destination $runningLauncher Copy-Item -LiteralPath (Join-Path $env:WINDIR 'System32\where.exe') -Destination $replacementLauncher - $runningProcess = Start-Process -FilePath $runningLauncher -ArgumentList @('/d', '/q', '/c', 'ping -n 30 127.0.0.1 > nul') -WindowStyle Hidden -PassThru + $runningProcess = Start-Process -FilePath $runningLauncher -ArgumentList @('-n', '30', '127.0.0.1') -WindowStyle Hidden -PassThru try { Start-Sleep -Milliseconds 500 Assert-Equal $false $runningProcess.HasExited 'test launcher process should still be running before replacement' From 3dfe257eaa36dcc6b9ee59bca37b971f8ad110bf Mon Sep 17 00:00:00 2001 From: Jcode Date: Fri, 17 Jul 2026 04:13:36 -0700 Subject: [PATCH 0005/1688] chore(ci): refresh code-size ratchet after merged growth Sync the six stale oversized-file baselines introduced on master by the todo telemetry and completion-gate changes. The Windows test-only change does not affect these counts. --- scripts/code_size_budget.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/scripts/code_size_budget.json b/scripts/code_size_budget.json index cc475f4565..ddf8edb053 100644 --- a/scripts/code_size_budget.json +++ b/scripts/code_size_budget.json @@ -56,18 +56,18 @@ "crates/jcode-provider-openrouter-runtime/src/lib.rs": 2701, "crates/jcode-render-core/src/math.rs": 1250, "crates/jcode-setup-hints/src/lib.rs": 2591, - "crates/jcode-telemetry-core/src/lib.rs": 2013, + "crates/jcode-telemetry-core/src/lib.rs": 2091, "crates/jcode-tui-mermaid/src/mermaid_cache_render.rs": 1333, "crates/jcode-tui-mermaid/src/mermaid_viewport.rs": 1351, "crates/jcode-tui-render/src/swarm_gallery.rs": 3044, - "crates/jcode-tui/src/tui/app.rs": 2371, + "crates/jcode-tui/src/tui/app.rs": 2381, "crates/jcode-tui/src/tui/app/auth.rs": 3355, "crates/jcode-tui/src/tui/app/auth_account_picker.rs": 1220, - "crates/jcode-tui/src/tui/app/commands.rs": 3357, + "crates/jcode-tui/src/tui/app/commands.rs": 3359, "crates/jcode-tui/src/tui/app/debug_bench.rs": 1281, "crates/jcode-tui/src/tui/app/helpers.rs": 1567, "crates/jcode-tui/src/tui/app/inline_interactive.rs": 3691, - "crates/jcode-tui/src/tui/app/input.rs": 3636, + "crates/jcode-tui/src/tui/app/input.rs": 3671, "crates/jcode-tui/src/tui/app/model_context.rs": 1956, "crates/jcode-tui/src/tui/app/navigation.rs": 1775, "crates/jcode-tui/src/tui/app/onboarding_flow_control.rs": 1604, @@ -76,7 +76,7 @@ "crates/jcode-tui/src/tui/app/remote/server_events.rs": 2800, "crates/jcode-tui/src/tui/app/state_ui.rs": 2208, "crates/jcode-tui/src/tui/app/state_ui_input_helpers.rs": 1889, - "crates/jcode-tui/src/tui/app/tui_lifecycle.rs": 1258, + "crates/jcode-tui/src/tui/app/tui_lifecycle.rs": 1260, "crates/jcode-tui/src/tui/app/tui_state.rs": 2371, "crates/jcode-tui/src/tui/app/turn.rs": 1480, "crates/jcode-tui/src/tui/backend.rs": 1721, @@ -97,7 +97,7 @@ "crates/jcode-tui/src/tui/ui_viewport.rs": 1361, "src/bin/memory_recall_bench.rs": 2667, "src/bin/tui_bench.rs": 1747, - "src/cli/commands.rs": 3297, + "src/cli/commands.rs": 3299, "src/cli/dispatch.rs": 1230, "src/cli/login.rs": 1395, "src/cli/provider_init.rs": 1823 From 4c1f5efb47b55372e30b8cf00f8cdb6bfba574aa Mon Sep 17 00:00:00 2001 From: jeremy <94247773+1jehuang@users.noreply.github.com> Date: Fri, 17 Jul 2026 13:28:56 -0700 Subject: [PATCH 0006/1688] fix(tui): load large remote histories linearly --- crates/jcode-tui/src/tui/app/remote.rs | 10 ++++ crates/jcode-tui/src/tui/app/remote_tests.rs | 49 ++++++++++++++++++++ crates/jcode-tui/src/tui/backend.rs | 48 ++++++++++++++++++- 3 files changed, 106 insertions(+), 1 deletion(-) diff --git a/crates/jcode-tui/src/tui/app/remote.rs b/crates/jcode-tui/src/tui/app/remote.rs index c369106f65..7a9c854a68 100644 --- a/crates/jcode-tui/src/tui/app/remote.rs +++ b/crates/jcode-tui/src/tui/app/remote.rs @@ -991,6 +991,16 @@ async fn recover_stuck_remote_history(app: &mut App, remote: &mut RemoteConnecti return false; } + // A large newline-delimited History event may take longer than the + // watchdog delay to arrive. Once any part of a frame is buffered, the + // response was not dropped: it is actively being assembled by the reader. + // Re-requesting here queues another complete (potentially tens-of-MB) + // History payload behind the first and can keep the connection saturated + // for minutes. Let the in-flight frame finish instead. + if remote.has_buffered_inbound_frame() { + return false; + } + if app.remote_history_recovery_attempts >= REMOTE_HISTORY_RECOVERY_MAX_ATTEMPTS { // We've exhausted re-requests. Surface a one-time actionable hint so the // user isn't stuck on a silent "loading session…" forever. diff --git a/crates/jcode-tui/src/tui/app/remote_tests.rs b/crates/jcode-tui/src/tui/app/remote_tests.rs index 2ebb0fecf9..eadfa2fdf6 100644 --- a/crates/jcode-tui/src/tui/app/remote_tests.rs +++ b/crates/jcode-tui/src/tui/app/remote_tests.rs @@ -822,6 +822,55 @@ fn remote_history_watchdog_rerequests_history_when_stuck() { )); } +/// A partial inbound frame proves that the original History response is in +/// flight. The watchdog must not queue another full response behind it. +#[test] +fn remote_history_watchdog_does_not_rerequest_while_frame_is_arriving() { + use std::time::{Duration, Instant}; + use tokio::io::{AsyncBufReadExt, AsyncWriteExt}; + + let mut app = create_test_app(); + app.is_remote = true; + app.remote_session_id = Some("session_large".to_string()); + app.remote_history_wait_started = Instant::now().checked_sub(Duration::from_secs(60)); + + let rt = tokio::runtime::Runtime::new().unwrap(); + rt.block_on(async { + let mut remote = crate::tui::backend::RemoteConnection::dummy(); + let peer = remote + .take_dummy_peer() + .expect("dummy remote should retain peer stream"); + let (reader, mut writer) = peer.into_split(); + let mut reader = tokio::io::BufReader::new(reader); + + // Deliberately omit the newline so next_event retains this partial + // History-sized frame when its future is cancelled by the tick. + writer + .write_all(b"{\"type\":\"history\",\"messages\":[") + .await + .expect("partial frame should reach remote"); + assert!( + tokio::time::timeout(Duration::from_millis(20), remote.next_event()) + .await + .is_err(), + "partial frame must remain incomplete" + ); + assert!(remote.has_buffered_inbound_frame()); + + let redraw = super::recover_stuck_remote_history(&mut app, &mut remote).await; + assert!(!redraw, "in-flight history should not trigger recovery"); + assert_eq!(app.remote_history_recovery_attempts, 0); + + let mut line = String::new(); + assert!( + tokio::time::timeout(Duration::from_millis(20), reader.read_line(&mut line)) + .await + .is_err(), + "watchdog must not write a duplicate GetHistory request" + ); + }); +} + /// Once history loads, the watchdog must clear its budget and do nothing. #[test] fn remote_history_watchdog_clears_budget_once_history_loads() { diff --git a/crates/jcode-tui/src/tui/backend.rs b/crates/jcode-tui/src/tui/backend.rs index 231c33415a..d08588a43f 100644 --- a/crates/jcode-tui/src/tui/backend.rs +++ b/crates/jcode-tui/src/tui/backend.rs @@ -245,6 +245,15 @@ pub struct RemoteConnection { /// `next_event` calls so a future cancelled by a `tokio::select!` peer /// branch never loses partially-read bytes. read_buffer: Vec, + /// First byte in `read_buffer` that has not yet been checked for a newline. + /// + /// Large History events can be tens of megabytes and arrive over thousands + /// of socket reads. Searching from byte zero after every read makes framing + /// quadratic and eventually backpressures the server writer. Keeping this + /// cursor makes each received byte participate in at most one newline scan. + read_buffer_scan_start: usize, + #[cfg(test)] + protocol_bytes_scanned: usize, has_loaded_history: bool, call_output_tokens_seen: u64, } @@ -313,6 +322,9 @@ impl RemoteConnection { next_request_id: 1, tool_diff: RemoteDiffTracker::default(), read_buffer: Vec::new(), + read_buffer_scan_start: 0, + #[cfg(test)] + protocol_bytes_scanned: 0, has_loaded_history: false, call_output_tokens_seen: 0, }; @@ -1047,6 +1059,7 @@ impl RemoteConnection { self.client_instance_id )); self.read_buffer.clear(); + self.read_buffer_scan_start = 0; } return RemoteRead::Disconnected(RemoteDisconnectReason::PeerClosed); } @@ -1060,9 +1073,26 @@ impl RemoteConnection { /// `\n`) from the persistent read buffer, leaving any partial remainder in /// place for the next read. fn take_buffered_line(&mut self) -> Option> { - let newline = self.read_buffer.iter().position(|&b| b == b'\n')?; + // Only inspect bytes appended since the previous unsuccessful scan. + // Without this cursor, a 27 MB History line delivered in 8 KB chunks + // causes roughly 45 GB of redundant memory scanning. + let scan_start = self.read_buffer_scan_start.min(self.read_buffer.len()); + let unscanned = &self.read_buffer[scan_start..]; + let relative_newline = unscanned.iter().position(|&b| b == b'\n'); + #[cfg(test)] + { + self.protocol_bytes_scanned += relative_newline.map_or(unscanned.len(), |i| i + 1); + } + let Some(relative_newline) = relative_newline else { + self.read_buffer_scan_start = self.read_buffer.len(); + return None; + }; + let newline = scan_start + relative_newline; let mut line: Vec = self.read_buffer.drain(..=newline).collect(); line.pop(); // drop trailing '\n' + // `position` stopped at the first newline, so none of the remaining + // bytes have been inspected yet. + self.read_buffer_scan_start = 0; // A single oversized line (e.g. a multi-megabyte `History` event) // permanently grows this persistent buffer. Once the line has been // split off, release the excess so each connection returns to a small @@ -1187,6 +1217,9 @@ impl RemoteConnection { next_request_id: 1, tool_diff: RemoteDiffTracker::default(), read_buffer: Vec::new(), + read_buffer_scan_start: 0, + #[cfg(test)] + protocol_bytes_scanned: 0, has_loaded_history: false, call_output_tokens_seen: 0, } @@ -1207,6 +1240,14 @@ impl RemoteConnection { self.has_loaded_history } + /// Whether the socket reader already holds part of an inbound protocol + /// frame. A history recovery request must not be sent in this state: the + /// original response is in flight, and another request only queues another + /// full copy behind it. + pub fn has_buffered_inbound_frame(&self) -> bool { + !self.read_buffer.is_empty() + } + /// Mark history as loaded pub fn mark_history_loaded(&mut self) { self.has_loaded_history = true; @@ -1533,6 +1574,7 @@ mod tests { detail: big_text.clone(), }; let encoded = crate::protocol::encode_event(&event); + let encoded_len = encoded.len(); // Feed the encoded event in small chunks from a background task, so the // reader sees a partially-available line for most of the test. @@ -1574,6 +1616,10 @@ mod tests { } other => panic!("expected intact event after cancellations, got {other:?}"), } + assert_eq!( + remote.protocol_bytes_scanned, encoded_len, + "fragmented frame assembly must inspect each protocol byte exactly once" + ); } /// A single logical event split across multiple socket writes (no trailing From 081b3d87b50a90fcba943df666a7522086403735 Mon Sep 17 00:00:00 2001 From: jeremy <94247773+1jehuang@users.noreply.github.com> Date: Fri, 17 Jul 2026 14:08:11 -0700 Subject: [PATCH 0007/1688] fix(tui): route /cancel and prompt input correctly (#496) Three input-routing fixes for interactive prompts: - /cancel (and /stop) now have a top-level dispatch that interrupts the in-flight turn like Ctrl+C. Previously the command was registered and advertised by login/API-key prompts, but typing it outside a pending prompt fell through to skill parsing and showed 'Unknown skill: /cancel'. - While a login/API-key/SSH prompt is pending, the command palette is suppressed. Typing '/' no longer opens the full command list; only /cancel is suggested since it is the one command those prompts accept. - Short digit-only input (e.g. '1') at an API key prompt is rejected as a likely menu selection instead of being silently saved as the key. Reported on Discord for the Windows onboarding flow. Closes #496 --- crates/jcode-tui/src/tui/app/auth.rs | 23 +++ crates/jcode-tui/src/tui/app/commands.rs | 28 ++++ crates/jcode-tui/src/tui/app/input.rs | 3 +- .../src/tui/app/state_ui_input_helpers.rs | 17 ++ crates/jcode-tui/src/tui/app/tests.rs | 1 + .../tui/app/tests/issue_496_input_routing.rs | 149 ++++++++++++++++++ 6 files changed, 220 insertions(+), 1 deletion(-) create mode 100644 crates/jcode-tui/src/tui/app/tests/issue_496_input_routing.rs diff --git a/crates/jcode-tui/src/tui/app/auth.rs b/crates/jcode-tui/src/tui/app/auth.rs index bf5c2d43a1..c7d222a242 100644 --- a/crates/jcode-tui/src/tui/app/auth.rs +++ b/crates/jcode-tui/src/tui/app/auth.rs @@ -2164,6 +2164,29 @@ impl App { }); return; } + // Real API keys are never short digit strings. Users sometimes + // type a menu number like `1` here, trying to select from a + // numbered list shown earlier; silently saving that as the key + // bricks the provider until they log in again (issue #496). + if !key.is_empty() && key.len() < 8 && key.chars().all(|c| c.is_ascii_digit()) { + self.push_display_message(DisplayMessage::error(format!( + "'{}' looks like a menu selection, not an API key. This prompt is waiting for the {} API key itself. Paste the key (see {}), or type /cancel to abort.", + key, provider, docs_url + ))); + self.pending_login = Some(PendingLogin::ApiKeyProfile { + provider_id, + provider, + auth_method, + docs_url, + env_file, + key_name, + default_model, + endpoint, + api_key_optional, + openai_compatible_profile, + }); + return; + } if key_name == "OPENROUTER_API_KEY" && !key.starts_with("sk-or-") { self.push_display_message(DisplayMessage::system( "OpenRouter keys typically start with sk-or-. Saving anyway...".to_string(), diff --git a/crates/jcode-tui/src/tui/app/commands.rs b/crates/jcode-tui/src/tui/app/commands.rs index 15d812aabc..878f2bfc94 100644 --- a/crates/jcode-tui/src/tui/app/commands.rs +++ b/crates/jcode-tui/src/tui/app/commands.rs @@ -863,6 +863,34 @@ fn handle_subagent_command(app: &mut App, trimmed: &str) -> bool { true } +/// `/cancel` (and `/stop`) interrupt the in-flight turn, mirroring Ctrl+C +/// while processing. The command has long been registered and advertised by +/// interactive prompts, but had no top-level dispatch, so typing it outside a +/// pending prompt fell through to skill parsing and produced +/// "Unknown skill: /cancel" (issue #496). +pub(super) fn handle_cancel_command(app: &mut App, trimmed: &str) -> bool { + if trimmed != "/cancel" && trimmed != "/stop" { + return false; + } + + if app.is_processing { + app.cancel_requested = true; + app.interleave_message = None; + app.pending_soft_interrupts.clear(); + app.pending_soft_interrupt_requests.clear(); + if app.cancel_overnight_for_interrupt() { + app.set_status_notice("Interrupting... Overnight cancelled"); + } else { + app.set_status_notice("Interrupting..."); + } + } else { + app.push_display_message(DisplayMessage::system( + "Nothing to cancel: no prompt or operation is in progress.".to_string(), + )); + } + true +} + pub(super) fn handle_help_command(app: &mut App, trimmed: &str) -> bool { if let Some(topic) = trimmed .strip_prefix("/help ") diff --git a/crates/jcode-tui/src/tui/app/input.rs b/crates/jcode-tui/src/tui/app/input.rs index 2c3683317c..2f3ff4ff94 100644 --- a/crates/jcode-tui/src/tui/app/input.rs +++ b/crates/jcode-tui/src/tui/app/input.rs @@ -3326,7 +3326,8 @@ impl App { } let trimmed = input.trim(); - let handled = commands::handle_help_command(self, trimmed) + let handled = commands::handle_cancel_command(self, trimmed) + || commands::handle_help_command(self, trimmed) || commands::handle_keys_command(self, trimmed) || commands::handle_ssh_command(self, trimmed) || commands::handle_session_command(self, trimmed) diff --git a/crates/jcode-tui/src/tui/app/state_ui_input_helpers.rs b/crates/jcode-tui/src/tui/app/state_ui_input_helpers.rs index ce8067b294..8083f9930a 100644 --- a/crates/jcode-tui/src/tui/app/state_ui_input_helpers.rs +++ b/crates/jcode-tui/src/tui/app/state_ui_input_helpers.rs @@ -1088,6 +1088,23 @@ impl App { /// Get command suggestions based on current input pub fn command_suggestions(&self) -> Vec<(String, &'static str)> { + // While an interactive prompt is waiting for typed input (API key, + // OAuth callback, account label, SSH target), the composer is an + // answer box, not a command line. Rendering the full command palette + // there is misleading (issue #496): the only command those prompts + // advertise is /cancel, so suggest exactly that and nothing else. + if self.pending_login.is_some() + || self.pending_account_input.is_some() + || self.pending_ssh_remote_name.is_some() + { + let input = self.input.trim_start(); + let typed = input.trim_end(); + if !typed.is_empty() && typed.starts_with('/') && "/cancel".starts_with(typed) { + return vec![("/cancel".into(), "Cancel the pending prompt")]; + } + return Vec::new(); + } + // While an inline picker preview is open for the command being typed, // the picker itself is the suggestion surface. Rendering the textual // suggestion list underneath would duplicate it (and its rows are not diff --git a/crates/jcode-tui/src/tui/app/tests.rs b/crates/jcode-tui/src/tui/app/tests.rs index a21d68a228..12ff5a3442 100644 --- a/crates/jcode-tui/src/tui/app/tests.rs +++ b/crates/jcode-tui/src/tui/app/tests.rs @@ -43,6 +43,7 @@ include!("tests/reasoning_region.rs"); include!("tests/smoothness_benchmark.rs"); include!("tests/hotkey_feedback_e2e.rs"); include!("tests/todo_card.rs"); +include!("tests/issue_496_input_routing.rs"); #[test] fn kv_cache_signature_prefix_match_allows_appended_messages() { diff --git a/crates/jcode-tui/src/tui/app/tests/issue_496_input_routing.rs b/crates/jcode-tui/src/tui/app/tests/issue_496_input_routing.rs new file mode 100644 index 0000000000..2eb2fc8299 --- /dev/null +++ b/crates/jcode-tui/src/tui/app/tests/issue_496_input_routing.rs @@ -0,0 +1,149 @@ +// Issue #496: input routing on interactive prompts. +// +// 1. `/cancel` typed at top level must never fall through to skill parsing +// ("Unknown skill: /cancel"); it interrupts a running turn or reports that +// nothing is in progress. +// 2. While an API-key prompt is pending, the command palette is suppressed +// (only /cancel is suggested), because the composer is an answer box. +// 3. A short digit string like "1" typed at the API-key prompt is a menu +// selection mistake, not a key; it must be rejected, not saved. + +#[test] +fn test_cancel_command_idle_reports_nothing_to_cancel() { + let mut app = create_test_app(); + + app.set_input_for_test("/cancel"); + app.submit_input(); + + let last = app + .display_messages() + .last() + .expect("cancel should produce a message"); + assert_eq!(last.role, "system"); + assert!( + last.content.contains("Nothing to cancel"), + "expected nothing-to-cancel notice, got: {}", + last.content + ); + // Regression: this used to fall through to skill parsing. + assert!( + !last.content.contains("Unknown skill"), + "'/cancel' must not be parsed as a skill: {}", + last.content + ); +} + +#[test] +fn test_cancel_command_processing_requests_interrupt() { + let mut app = create_test_app(); + app.is_processing = true; + + app.set_input_for_test("/cancel"); + app.submit_input(); + + assert!(app.cancel_requested, "processing turn must be interrupted"); + assert!( + !app.display_messages() + .iter() + .any(|m| m.content.contains("Unknown skill")), + "'/cancel' must not be parsed as a skill" + ); +} + +#[test] +fn test_stop_command_processing_requests_interrupt() { + let mut app = create_test_app(); + app.is_processing = true; + + app.set_input_for_test("/stop"); + app.submit_input(); + + assert!(app.cancel_requested, "'/stop' must interrupt like /cancel"); +} + +fn pending_api_key_login() -> crate::tui::app::PendingLogin { + crate::tui::app::PendingLogin::ApiKeyProfile { + provider_id: "openrouter".to_string(), + provider: "OpenRouter".to_string(), + auth_method: "api_key".to_string(), + docs_url: "https://openrouter.ai/keys".to_string(), + env_file: "openrouter.env".to_string(), + key_name: "OPENROUTER_API_KEY".to_string(), + default_model: None, + endpoint: None, + api_key_optional: false, + openai_compatible_profile: None, + } +} + +#[test] +fn test_command_palette_suppressed_while_api_key_prompt_pending() { + let mut app = create_test_app(); + app.pending_login = Some(pending_api_key_login()); + + // Typing a slash on the key prompt must not open the full palette. + app.set_input_for_test("/"); + let suggestions = app.command_suggestions(); + assert_eq!( + suggestions, + vec![("/cancel".to_string(), "Cancel the pending prompt")], + "only /cancel may be suggested while a login prompt is pending" + ); + + // Non-slash input (the key itself) gets no suggestions at all. + app.set_input_for_test("sk-or-abc"); + assert!(app.command_suggestions().is_empty()); + + // Prefixes of /cancel keep the single suggestion; other commands do not. + app.set_input_for_test("/can"); + assert_eq!(app.command_suggestions().len(), 1); + app.set_input_for_test("/model"); + assert!(app.command_suggestions().is_empty()); +} + +#[test] +fn test_menu_number_rejected_as_api_key() { + let mut app = create_test_app(); + app.pending_login = Some(pending_api_key_login()); + + app.set_input_for_test("1"); + app.submit_input(); + + let last = app + .display_messages() + .last() + .expect("menu-number input should produce an error message"); + assert_eq!(last.role, "error"); + assert!( + last.content.contains("menu selection"), + "expected menu-selection rejection, got: {}", + last.content + ); + // The prompt survives so the user can paste the real key. + assert!( + matches!( + app.pending_login, + Some(crate::tui::app::PendingLogin::ApiKeyProfile { .. }) + ), + "API key prompt must remain pending after rejecting menu-number input" + ); + // Nothing was persisted. + assert!(std::env::var("OPENROUTER_API_KEY").map_or(true, |v| v != "1")); +} + +#[test] +fn test_cancel_still_cancels_pending_api_key_prompt() { + let mut app = create_test_app(); + app.pending_login = Some(pending_api_key_login()); + + app.set_input_for_test("/cancel"); + app.submit_input(); + + assert!(app.pending_login.is_none(), "/cancel must clear the prompt"); + let last = app.display_messages().last().expect("message expected"); + assert!( + last.content.contains("Login cancelled"), + "expected login-cancelled notice, got: {}", + last.content + ); +} From 62f25e237d1709585e3fd3d6be61ab3bf6b35d8d Mon Sep 17 00:00:00 2001 From: jeremy <94247773+1jehuang@users.noreply.github.com> Date: Fri, 17 Jul 2026 14:46:15 -0700 Subject: [PATCH 0008/1688] fix(openrouter): flatten top-level schema combinators in tool parameters (#495) Fresh OpenRouter logins failed on their very first message with HTTP 400 'Provider returned error' whenever the default model routed to an Anthropic-family upstream (Anthropic, Google Vertex, Amazon Bedrock): tools.N.custom.input_schema: input_schema does not support oneOf, allOf, or anyOf at the top level The swarm tool models its spawn/non-spawn action branches with a top-level anyOf, which those upstreams reject for the whole request, bricking the provider right after onboarding. Flatten oneOf/anyOf/allOf at the top level of every tool parameters schema in the OpenRouter sanitizer, mirroring the direct Anthropic provider's anthropic_input_schema: merge branch properties as optional fields and promote required only from allOf branches. Nested combinators inside properties are untouched (upstreams accept those), and runtime tool deserialization remains the authority for action-specific constraints. Verified end-to-end with a fresh JCODE_HOME + real OpenRouter key: old binary 400s on the first turn, fixed binary completes it. Closes #495 --- .../jcode-provider-openrouter/src/request.rs | 168 ++++++++++++++++++ 1 file changed, 168 insertions(+) diff --git a/crates/jcode-provider-openrouter/src/request.rs b/crates/jcode-provider-openrouter/src/request.rs index 63aee0e5b4..b3c7bf0d21 100644 --- a/crates/jcode-provider-openrouter/src/request.rs +++ b/crates/jcode-provider-openrouter/src/request.rs @@ -78,10 +78,95 @@ pub fn sanitize_tool_parameters_schema(schema: &Value) -> Value { { obj.insert("type".to_string(), Value::String("object".to_string())); } + flatten_top_level_combinators(&mut sanitized); walk(&mut sanitized); sanitized } +/// Flatten `oneOf`/`anyOf`/`allOf` at the top level of a tool parameters +/// schema into a single object schema (issue #495). +/// +/// OpenRouter forwards tool schemas to whichever upstream serves the model, +/// and Anthropic-family backends (Anthropic, Google Vertex, Amazon Bedrock) +/// reject `input_schema` combinators at the top level with HTTP 400 +/// ("input_schema does not support oneOf, allOf, or anyOf at the top level"). +/// One such tool bricks every request, so first-time OpenRouter logins fail on +/// their first message when the registry contains a multi-action tool that +/// models its action branches with top-level `anyOf`. +/// +/// Mirror the direct Anthropic provider's `anthropic_input_schema`: keep the +/// common object shape, merge branch `properties` in as optional fields, and +/// only promote `required` from `allOf` branches (whose constraints all +/// apply). Runtime tool deserialization remains the authority for +/// action-specific constraints. Nested combinators inside properties are left +/// untouched; upstreams accept those. +fn flatten_top_level_combinators(schema: &mut Value) { + let Some(output) = schema.as_object_mut() else { + return; + }; + + let mut merged_properties = output + .get("properties") + .and_then(Value::as_object) + .cloned() + .unwrap_or_default(); + let mut all_of_required = Vec::new(); + let mut saw_combinator = false; + + for keyword in ["oneOf", "anyOf", "allOf"] { + let Some(branches) = output + .remove(keyword) + .and_then(|value| value.as_array().cloned()) + else { + continue; + }; + saw_combinator = true; + for branch in branches { + let Some(branch) = branch.as_object() else { + continue; + }; + if let Some(properties) = branch.get("properties").and_then(Value::as_object) { + for (name, property) in properties { + merged_properties + .entry(name.clone()) + .or_insert_with(|| property.clone()); + } + } + if keyword == "allOf" + && let Some(required) = branch.get("required").and_then(Value::as_array) + { + for name in required.iter().filter_map(Value::as_str) { + if !all_of_required.iter().any(|existing| existing == name) { + all_of_required.push(name.to_string()); + } + } + } + } + } + + if !saw_combinator { + return; + } + + output.insert("type".to_string(), Value::String("object".to_string())); + output.insert("properties".to_string(), Value::Object(merged_properties)); + if !all_of_required.is_empty() { + let required = output + .entry("required".to_string()) + .or_insert_with(|| Value::Array(Vec::new())); + if let Value::Array(required) = required { + for name in all_of_required { + if !required + .iter() + .any(|existing| existing.as_str() == Some(&name)) + { + required.push(Value::String(name)); + } + } + } + } +} + /// Build OpenAI-compatible chat `messages` for OpenRouter/direct compatible providers. /// /// This stays in the OpenRouter leaf crate so provider-specific message normalization, @@ -600,6 +685,89 @@ mod sanitize_schema_tests { use super::sanitize_tool_parameters_schema; use serde_json::json; + #[test] + fn top_level_any_of_is_flattened_for_anthropic_family_upstreams() { + // The swarm-tool shape from issue #495: top-level anyOf action + // branches make Anthropic/Vertex/Bedrock upstreams reject the whole + // request with HTTP 400, bricking fresh OpenRouter logins. + let schema = json!({ + "type": "object", + "properties": { + "action": {"type": "string"}, + }, + "required": ["action"], + "anyOf": [ + { + "type": "object", + "required": ["action", "label"], + "properties": { + "action": {"type": "string", "enum": ["spawn"]}, + "label": {"type": "string"} + } + }, + { + "type": "object", + "required": ["action"], + "properties": { + "action": {"type": "string", "enum": ["list"]} + } + } + ] + }); + + let sanitized = sanitize_tool_parameters_schema(&schema); + + for keyword in ["oneOf", "anyOf", "allOf"] { + assert!( + sanitized.get(keyword).is_none(), + "top-level {keyword} must be flattened away: {sanitized}" + ); + } + // Branch-only properties merge in as optional fields. + assert!(sanitized["properties"]["label"].is_object()); + // Pre-existing top-level shape is preserved. + assert_eq!(sanitized["type"], "object"); + assert_eq!(sanitized["required"], json!(["action"])); + // anyOf branch `required` must NOT be promoted (branches are + // alternatives, not conjunctions). + assert!( + !sanitized["required"] + .as_array() + .unwrap() + .iter() + .any(|v| v == "label"), + "anyOf branch required must not become unconditional: {sanitized}" + ); + } + + #[test] + fn top_level_all_of_promotes_required_fields() { + let schema = json!({ + "type": "object", + "properties": {"a": {"type": "string"}}, + "allOf": [ + { + "type": "object", + "required": ["b"], + "properties": {"b": {"type": "string"}} + } + ] + }); + + let sanitized = sanitize_tool_parameters_schema(&schema); + + assert!(sanitized.get("allOf").is_none()); + assert!(sanitized["properties"]["b"].is_object()); + assert!( + sanitized["required"] + .as_array() + .unwrap() + .iter() + .any(|v| v == "b"), + "allOf required applies unconditionally and must be promoted: {sanitized}" + ); + } + #[test] fn bare_object_schema_gains_empty_properties() { // The no-argument MCP tool shape from issue #446. From 8b4a9930bdbd4dc2f9cb095d4951da7b08e15317 Mon Sep 17 00:00:00 2001 From: jeremy <94247773+1jehuang@users.noreply.github.com> Date: Fri, 17 Jul 2026 15:03:21 -0700 Subject: [PATCH 0009/1688] fix(tui): keep gmail draft body/attachments through display compaction Gmail draft cards rendered '(empty body)' after storage compaction because compact_tool_input_for_display dropped the body and attachments fields from gmail tool inputs. Preserve the body (truncated to 4000 chars) and the attachments list so the draft card still shows content after reload. --- .../jcode-tui/src/tui/app/state_ui_storage.rs | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/crates/jcode-tui/src/tui/app/state_ui_storage.rs b/crates/jcode-tui/src/tui/app/state_ui_storage.rs index 27ab1e5643..ef1726a4e3 100644 --- a/crates/jcode-tui/src/tui/app/state_ui_storage.rs +++ b/crates/jcode-tui/src/tui/app/state_ui_storage.rs @@ -333,6 +333,26 @@ fn compact_tool_input_for_display(name: &str, input: &serde_json::Value) -> serd }) .unwrap_or(serde_json::Value::Null), ), + // Keep the draft/send body so the Gmail draft card still renders + // its content after storage compaction (previously dropped, which + // made every reloaded draft card show "(empty body)"). + ( + "body", + input + .get("body") + .and_then(|v| v.as_str()) + .map(|s| { + serde_json::Value::String(crate::util::truncate_str(s, 4000).to_string()) + }) + .unwrap_or(serde_json::Value::Null), + ), + ( + "attachments", + input + .get("attachments") + .cloned() + .unwrap_or(serde_json::Value::Null), + ), ]), "browser" => obj(vec![ ( @@ -661,6 +681,35 @@ mod tests { ); } + #[test] + fn compaction_keeps_gmail_draft_body_and_attachments_for_draft_card() { + let mut message = tool_message( + "gmail", + serde_json::json!({ + "action": "draft", + "to": "someone@example.com", + "subject": "Hello", + "body": "Dear someone,\n\nThis is the body.\n", + "attachments": ["/tmp/lease.pdf"] + }), + ); + compact_display_message_tool_data(&mut message); + let tool = message.tool_data.expect("tool data"); + assert_eq!( + tool.input.get("body").and_then(|v| v.as_str()), + Some("Dear someone,\n\nThis is the body.\n"), + "draft body must survive compaction so the draft card doesn't render '(empty body)'" + ); + assert_eq!( + tool.input + .get("attachments") + .and_then(|v| v.as_array()) + .map(|a| a.len()), + Some(1), + "attachments must survive compaction" + ); + } + #[test] fn compaction_keeps_browser_action_and_intent_for_transcript_summary() { let mut message = tool_message( From d63f735acd8b7f3d99b6c60f2d1f5be0b14a10e3 Mon Sep 17 00:00:00 2001 From: jeremy <94247773+1jehuang@users.noreply.github.com> Date: Fri, 17 Jul 2026 15:16:24 -0700 Subject: [PATCH 0010/1688] perf(images): optimize scrolling and bound memory --- crates/jcode-tui-mermaid/src/debug.rs | 11 +- crates/jcode-tui-mermaid/src/lib.rs | 464 ++++++++++- .../src/mermaid_cache_render.rs | 136 +++- crates/jcode-tui-mermaid/src/mermaid_debug.rs | 50 +- .../jcode-tui-mermaid/src/mermaid_runtime.rs | 13 +- .../src/mermaid_tests/part_02.rs | 228 +++++- .../jcode-tui-mermaid/src/mermaid_viewport.rs | 742 ++++++++++++++++-- .../jcode-tui-mermaid/src/mermaid_widget.rs | 8 +- .../tui/app/tests/scroll_copy_02/part_02.rs | 59 +- crates/jcode-tui/src/tui/app/tui_state.rs | 12 +- crates/jcode-tui/src/tui/mermaid.rs | 12 +- crates/jcode-tui/src/tui/mod.rs | 31 +- crates/jcode-tui/src/tui/ui.rs | 4 + crates/jcode-tui/src/tui/ui_inline_image.rs | 513 ++++++++++-- crates/jcode-tui/src/tui/ui_prepare.rs | 13 +- crates/jcode-tui/src/tui/ui_viewport.rs | 9 +- src/bin/tui_bench.rs | 16 + src/cli/terminal.rs | 11 +- 18 files changed, 2057 insertions(+), 275 deletions(-) diff --git a/crates/jcode-tui-mermaid/src/debug.rs b/crates/jcode-tui-mermaid/src/debug.rs index 598be178df..75b552d6ef 100644 --- a/crates/jcode-tui-mermaid/src/debug.rs +++ b/crates/jcode-tui-mermaid/src/debug.rs @@ -53,14 +53,17 @@ pub fn clear_cache() -> Result<(), String> { if let Ok(mut cache) = RENDER_CACHE.lock() { cache.entries.clear(); cache.order.clear(); + cache.width_miss_floor.clear(); } clear_layout_cache(); if let Ok(mut state) = IMAGE_STATE.lock() { state.clear(); } if let Ok(mut source) = SOURCE_CACHE.lock() { - source.entries.clear(); - source.order.clear(); + source.clear(); + } + if let Ok(mut fitted) = FITTED_SOURCE_CACHE.lock() { + fitted.clear(); } if let Ok(mut kitty) = KITTY_VIEWPORT_STATE.lock() { kitty.clear(); @@ -110,6 +113,7 @@ pub fn debug_image_state() -> Vec { ResizeMode::Scale => "Scale".to_string(), ResizeMode::Crop => "Crop".to_string(), ResizeMode::Viewport => "Viewport".to_string(), + ResizeMode::FitViewport => "FitViewport".to_string(), }, last_area: img_state .last_area @@ -175,6 +179,7 @@ pub fn debug_render(content: &str) -> TestRenderResult { ResizeMode::Scale => "Scale".to_string(), ResizeMode::Crop => "Crop".to_string(), ResizeMode::Viewport => "Viewport".to_string(), + ResizeMode::FitViewport => "FitViewport".to_string(), }) } else { None @@ -247,6 +252,7 @@ pub fn debug_test_resize_stability(hash: u64) -> serde_json::Value { ResizeMode::Scale => "Scale", ResizeMode::Crop => "Crop", ResizeMode::Viewport => "Viewport", + ResizeMode::FitViewport => "FitViewport", }) } else { None @@ -398,6 +404,7 @@ pub fn debug_test_scroll(content: Option<&str>) -> ScrollTestResult { ResizeMode::Scale => "Scale", ResizeMode::Crop => "Crop", ResizeMode::Viewport => "Viewport", + ResizeMode::FitViewport => "FitViewport", }; frame_info.resize_mode = Some(mode.to_string()); modes_seen.push(mode.to_string()); diff --git a/crates/jcode-tui-mermaid/src/lib.rs b/crates/jcode-tui-mermaid/src/lib.rs index 8d00e34629..86c3748958 100644 --- a/crates/jcode-tui-mermaid/src/lib.rs +++ b/crates/jcode-tui-mermaid/src/lib.rs @@ -51,13 +51,14 @@ use ratatui_image::{ protocol::StatefulProtocol, }; use serde::Serialize; +use std::borrow::Cow; use std::cell::Cell; -use std::collections::{HashMap, HashSet, VecDeque, hash_map::Entry}; +use std::collections::{HashMap, VecDeque, hash_map::Entry}; use std::fs; use std::hash::{Hash as _, Hasher}; use std::panic; use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU32, AtomicU64, Ordering}; use std::sync::{Arc, LazyLock, Mutex, OnceLock, mpsc}; use std::time::Instant; @@ -267,7 +268,7 @@ use cache_render::calculate_render_size; use cache_render::{ CachedDiagram, MermaidCache, RENDER_CACHE_MAX, RENDER_WIDTH_BUCKET_CELLS, bump_deferred_render_epoch, clear_layout_cache, get_cached_diagram, - get_cached_diagram_in_memory, layout_cache_usage, + get_cached_diagram_in_memory, get_cached_diagram_prefer_width, layout_cache_usage, }; use viewport_render::clear_image_area; use widget_render::{BORDER_WIDTH, draw_left_border, render_stateful_image_safe}; @@ -479,6 +480,14 @@ static SVG_FONT_DB: LazyLock> = LazyLock::new(|| { /// (see `ui_inline_image::prefetch`) so scrolling back through a transcript of /// inline screenshots reuses warm protocol state instead of re-encoding. const IMAGE_STATE_MAX: usize = 24; +/// Approximate source-pixel budget for `IMAGE_STATE`. +/// +/// `ratatui-image::StatefulProtocol` retains the original decoded image in +/// addition to protocol-specific encoded data. A count-only cap therefore lets +/// a handful of 4K screenshots pin hundreds of MiB. The source-pixel budget is +/// intentionally conservative; encoded buffers are extra, so keeping decoded +/// sources below this line keeps the real cache working set bounded too. +const IMAGE_STATE_MAX_SOURCE_BYTES: usize = 48 * 1024 * 1024; /// Maximum number of Kitty virtual-placement state entries to keep. /// @@ -493,6 +502,15 @@ const IMAGE_STATE_MAX: usize = 24; /// scroll working set for a screenshot-heavy session stays warm; the memory cost /// of the extra metadata entries is negligible. const KITTY_VIEWPORT_STATE_MAX: usize = 256; +/// Maximum encoded Kitty transmissions retained before their first draw. +/// +/// A prewarmed state temporarily owns a base64 PNG escape payload. Count-only +/// eviction is not sufficient because a handful of high-resolution images can +/// otherwise retain hundreds of MiB while they are still off screen. Once a +/// state is drawn this drops to zero and only its tiny terminal id metadata +/// remains. As with the other byte-bounded caches, one oversized newest entry is +/// retained so a single large image can still make forward progress. +const KITTY_VIEWPORT_PENDING_MAX_BYTES: usize = 32 * 1024 * 1024; /// Image state cache - holds StatefulProtocol for each rendered image /// Keyed by content hash; source_path guards prevent stale reuse when @@ -504,12 +522,56 @@ static IMAGE_STATE: LazyLock> = static SOURCE_CACHE: LazyLock> = LazyLock::new(|| Mutex::new(SourceImageCache::new())); +/// Cache images pre-scaled to their inline placeholder geometry. Non-Kitty +/// protocols cannot re-address a terminal-retained image like Kitty can, but +/// keeping this bounded decoded source lets scroll-only updates crop the visible +/// rows without re-decoding or re-scaling the complete screenshot. +static FITTED_SOURCE_CACHE: LazyLock> = + LazyLock::new(|| Mutex::new(FittedSourceCache::new())); + /// Cache Kitty-specific viewport state so scroll-only updates can reuse the /// same transmitted image data and adjust placeholders instead of rebuilding a /// fresh cropped protocol payload on every tick. static KITTY_VIEWPORT_STATE: LazyLock> = LazyLock::new(|| Mutex::new(KittyViewportCache::new())); +/// Terminal image ids whose Kitty allocations should be deleted on the next +/// image draw. Eviction can happen while only cache locks are available, so the +/// actual escape sequence is deferred until a render buffer is being built. +static KITTY_PENDING_DELETE_IDS: LazyLock>> = + LazyLock::new(|| Mutex::new(VecDeque::new())); + +/// Monotonic process-local Kitty image id allocator. Folding a 64-bit content +/// hash into 32 bits allowed unrelated images to alias and overwrite each other. +static NEXT_KITTY_IMAGE_ID: AtomicU32 = AtomicU32::new(1); + +fn queue_kitty_delete(unique_id: u32) { + if unique_id == 0 { + return; + } + if let Ok(mut pending) = KITTY_PENDING_DELETE_IDS.lock() + && !pending.contains(&unique_id) + { + pending.push_back(unique_id); + } +} + +fn queue_kitty_delete_if_transmitted(state: &KittyViewportState) { + // A pending transmit has never reached the terminal, so there is no terminal + // allocation to reclaim. Skipping it also keeps the deferred-delete queue + // bounded naturally during large offscreen prewarm bursts. + if state.pending_transmit.is_none() { + queue_kitty_delete(state.unique_id); + } +} + +fn take_kitty_delete_ids() -> Vec { + KITTY_PENDING_DELETE_IDS + .lock() + .map(|mut pending| pending.drain(..).collect()) + .unwrap_or_default() +} + /// Last render state for skip-redundant-render optimization static LAST_RENDER: LazyLock>> = LazyLock::new(|| Mutex::new(HashMap::new())); @@ -541,6 +603,9 @@ const ACTIVE_DIAGRAMS_MAX: usize = 128; struct ImageState { protocol: StatefulProtocol, source_path: PathBuf, + /// Exact bytes retained by the protocol's decoded source image before any + /// protocol-specific encoding overhead. + source_bytes: usize, /// The area this was last rendered to (for change detection) last_area: Option, /// Resize mode locked at creation time (prevents flickering on scroll) @@ -555,6 +620,7 @@ struct ImageState { struct ImageStateCache { entries: HashMap, order: VecDeque, + total_source_bytes: usize, } impl ImageStateCache { @@ -562,6 +628,7 @@ impl ImageStateCache { Self { entries: HashMap::new(), order: VecDeque::new(), + total_source_bytes: 0, } } @@ -586,22 +653,35 @@ impl ImageStateCache { } fn insert(&mut self, hash: u64, state: ImageState) { - if let std::collections::hash_map::Entry::Occupied(mut entry) = self.entries.entry(hash) { - entry.insert(state); - self.touch(hash); - } else { - self.entries.insert(hash, state); - self.order.push_back(hash); - while self.order.len() > IMAGE_STATE_MAX { - if let Some(old) = self.order.pop_front() { - self.entries.remove(&old); - } + if let Some(old) = self.entries.remove(&hash) { + self.total_source_bytes = self.total_source_bytes.saturating_sub(old.source_bytes); + if let Some(pos) = self.order.iter().position(|h| *h == hash) { + self.order.remove(pos); + } + } + self.total_source_bytes = self.total_source_bytes.saturating_add(state.source_bytes); + self.entries.insert(hash, state); + self.order.push_back(hash); + // Keep one oversized image rather than immediately evicting the state + // that the caller is about to draw and entering a decode/rebuild loop. + while (self.order.len() > IMAGE_STATE_MAX + || self.total_source_bytes > IMAGE_STATE_MAX_SOURCE_BYTES) + && self.order.len() > 1 + { + if let Some(old) = self.order.pop_front() + && let Some(old_state) = self.entries.remove(&old) + { + self.total_source_bytes = self + .total_source_bytes + .saturating_sub(old_state.source_bytes); } } } fn remove(&mut self, hash: &u64) { - self.entries.remove(hash); + if let Some(old) = self.entries.remove(hash) { + self.total_source_bytes = self.total_source_bytes.saturating_sub(old.source_bytes); + } if let Some(pos) = self.order.iter().position(|h| h == hash) { self.order.remove(pos); } @@ -610,6 +690,7 @@ impl ImageStateCache { fn clear(&mut self) { self.entries.clear(); self.order.clear(); + self.total_source_bytes = 0; } fn iter(&self) -> impl Iterator { @@ -617,7 +698,7 @@ impl ImageStateCache { } } -#[derive(Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] struct ViewportState { scroll_x_px: u32, scroll_y_px: u32, @@ -632,6 +713,7 @@ enum ResizeMode { Scale, Crop, Viewport, + FitViewport, } /// Cache decoded source images for fast viewport cropping. @@ -640,15 +722,47 @@ enum ResizeMode { /// so scrolling back over recently seen screenshots reuses the decoded pixels /// instead of re-opening and re-decoding the cached PNG from disk. const SOURCE_CACHE_MAX: usize = 16; +/// Exact decoded-byte budget for source images used by viewport and fit paths. +/// Count-only bounding is unsafe for heterogeneous images: sixteen 4K RGBA +/// screenshots are already roughly 500 MiB before allocator overhead. +const SOURCE_CACHE_MAX_BYTES: usize = 48 * 1024 * 1024; + +/// Pre-scaled sources are normally much smaller than their originals because +/// they are bounded by the inline placeholder. Keep a modest working set for +/// back-scrolling while preventing terminal resizes from accumulating variants. +const FITTED_SOURCE_CACHE_MAX: usize = 16; +const FITTED_SOURCE_CACHE_MAX_BYTES: usize = 32 * 1024 * 1024; struct SourceImageEntry { path: PathBuf, image: Arc, + decoded_bytes: usize, } struct SourceImageCache { order: VecDeque, entries: HashMap, + total_decoded_bytes: usize, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +struct FittedSourceKey { + hash: u64, + target_cols: u16, + target_rows: u16, + font_size: (u16, u16), +} + +struct FittedSourceEntry { + source_path: PathBuf, + image: Arc, + decoded_bytes: usize, +} + +struct FittedSourceCache { + order: VecDeque, + entries: HashMap, + total_decoded_bytes: usize, } struct KittyViewportState { @@ -659,6 +773,10 @@ struct KittyViewportState { full_cols: u16, full_rows: u16, pending_transmit: Option, + /// Exact bytes currently held by `pending_transmit`. Stored explicitly so + /// cache accounting remains cheap and tests can exercise budget eviction + /// without allocating multi-megabyte strings. + pending_transmit_bytes: usize, /// `Some((cols, rows))` when this entry was built by the inline fit path /// (image pre-scaled to fit a placeholder region); `None` for the zoomable /// diagram viewport path. Keeps the two users of this cache from @@ -668,22 +786,34 @@ struct KittyViewportState { struct KittyViewportCache { entries: HashMap, - order: VecDeque, + /// Monotonic recency stamps make the per-frame hit path O(1). Finding the + /// oldest entry is O(n) only during insertion/eviction, where PNG scaling + /// and encoding dominate and the cache is capped at 256 entries. + recency: HashMap, + clock: u64, + total_pending_transmit_bytes: usize, } impl KittyViewportCache { fn new() -> Self { Self { entries: HashMap::new(), - order: VecDeque::new(), + recency: HashMap::new(), + clock: 0, + total_pending_transmit_bytes: 0, } } fn touch(&mut self, hash: u64) { - if let Some(pos) = self.order.iter().position(|h| *h == hash) { - self.order.remove(pos); - } - self.order.push_back(hash); + self.clock = self.clock.saturating_add(1); + self.recency.insert(hash, self.clock); + } + + fn oldest_hash(&self) -> Option { + self.recency + .iter() + .min_by_key(|(_, stamp)| *stamp) + .map(|(hash, _)| *hash) } fn get_mut(&mut self, hash: u64) -> Option<&mut KittyViewportState> { @@ -696,31 +826,70 @@ impl KittyViewportCache { } fn insert(&mut self, hash: u64, state: KittyViewportState) { + let pending_bytes = state.pending_transmit_bytes; if let std::collections::hash_map::Entry::Occupied(mut entry) = self.entries.entry(hash) { - entry.insert(state); + let old = entry.insert(state); + self.total_pending_transmit_bytes = self + .total_pending_transmit_bytes + .saturating_sub(old.pending_transmit_bytes) + .saturating_add(pending_bytes); + if entry.get().unique_id != old.unique_id { + queue_kitty_delete_if_transmitted(&old); + } self.touch(hash); } else { self.entries.insert(hash, state); - self.order.push_back(hash); - while self.order.len() > KITTY_VIEWPORT_STATE_MAX { - if let Some(old) = self.order.pop_front() { - self.entries.remove(&old); - } + self.total_pending_transmit_bytes = self + .total_pending_transmit_bytes + .saturating_add(pending_bytes); + self.touch(hash); + } + while (self.entries.len() > KITTY_VIEWPORT_STATE_MAX + || self.total_pending_transmit_bytes > KITTY_VIEWPORT_PENDING_MAX_BYTES) + && self.entries.len() > 1 + { + if let Some(old) = self.oldest_hash() + && let Some(old_state) = self.entries.remove(&old) + { + self.recency.remove(&old); + self.total_pending_transmit_bytes = self + .total_pending_transmit_bytes + .saturating_sub(old_state.pending_transmit_bytes); + queue_kitty_delete_if_transmitted(&old_state); } } } + fn take_pending_transmit(&mut self, hash: u64) -> Option<(u32, Option)> { + let state = self.get_mut(hash)?; + let unique_id = state.unique_id; + let pending = state.pending_transmit.take(); + let pending_bytes = std::mem::take(&mut state.pending_transmit_bytes); + self.total_pending_transmit_bytes = self + .total_pending_transmit_bytes + .saturating_sub(pending_bytes); + Some((unique_id, pending)) + } + #[cfg(feature = "renderer")] fn remove(&mut self, hash: &u64) { - self.entries.remove(hash); - if let Some(pos) = self.order.iter().position(|h| h == hash) { - self.order.remove(pos); + if let Some(state) = self.entries.remove(hash) { + self.total_pending_transmit_bytes = self + .total_pending_transmit_bytes + .saturating_sub(state.pending_transmit_bytes); + queue_kitty_delete_if_transmitted(&state); } + self.recency.remove(hash); } fn clear(&mut self) { + for state in self.entries.values() { + queue_kitty_delete_if_transmitted(state); + } self.entries.clear(); - self.order.clear(); + self.recency.clear(); + self.clock = 0; + self.total_pending_transmit_bytes = 0; } } @@ -729,6 +898,7 @@ impl SourceImageCache { Self { order: VecDeque::new(), entries: HashMap::new(), + total_decoded_bytes: 0, } } @@ -755,29 +925,176 @@ impl SourceImageCache { } fn insert(&mut self, hash: u64, path: PathBuf, image: DynamicImage) -> Arc { + let decoded_bytes = image.as_bytes().len(); + self.insert_with_decoded_bytes(hash, path, image, decoded_bytes) + } + + fn insert_with_decoded_bytes( + &mut self, + hash: u64, + path: PathBuf, + image: DynamicImage, + decoded_bytes: usize, + ) -> Arc { + self.remove(hash); let arc = Arc::new(image); + self.total_decoded_bytes = self.total_decoded_bytes.saturating_add(decoded_bytes); self.entries.insert( hash, SourceImageEntry { path, image: arc.clone(), + decoded_bytes, }, ); self.touch(hash); - while self.order.len() > SOURCE_CACHE_MAX { - if let Some(old) = self.order.pop_front() { - self.entries.remove(&old); + // Preserve one oversized source so a single large image can still draw + // without thrashing between decode and immediate eviction. + while (self.order.len() > SOURCE_CACHE_MAX + || self.total_decoded_bytes > SOURCE_CACHE_MAX_BYTES) + && self.order.len() > 1 + { + if let Some(old) = self.order.pop_front() + && let Some(old_entry) = self.entries.remove(&old) + { + self.total_decoded_bytes = self + .total_decoded_bytes + .saturating_sub(old_entry.decoded_bytes); } } arc } fn remove(&mut self, hash: u64) { - self.entries.remove(&hash); + if let Some(old) = self.entries.remove(&hash) { + self.total_decoded_bytes = self.total_decoded_bytes.saturating_sub(old.decoded_bytes); + } if let Some(pos) = self.order.iter().position(|h| *h == hash) { self.order.remove(pos); } } + + fn remove_if_path(&mut self, hash: u64, expected_path: &Path) { + if self + .entries + .get(&hash) + .is_some_and(|entry| entry.path == expected_path) + { + self.remove(hash); + } + } + + fn clear(&mut self) { + self.entries.clear(); + self.order.clear(); + self.total_decoded_bytes = 0; + } +} + +impl FittedSourceCache { + fn new() -> Self { + Self { + order: VecDeque::new(), + entries: HashMap::new(), + total_decoded_bytes: 0, + } + } + + fn touch(&mut self, key: FittedSourceKey) { + if let Some(pos) = self.order.iter().position(|entry| *entry == key) { + self.order.remove(pos); + } + self.order.push_back(key); + } + + fn get(&mut self, key: FittedSourceKey, source_path: &Path) -> Option> { + let image = match self.entries.get(&key) { + Some(entry) if entry.source_path == source_path => Some(entry.image.clone()), + Some(_) => { + self.remove_key(key); + None + } + None => None, + }; + if image.is_some() { + self.touch(key); + } + image + } + + fn insert( + &mut self, + key: FittedSourceKey, + source_path: PathBuf, + image: DynamicImage, + ) -> Arc { + let decoded_bytes = image.as_bytes().len(); + self.insert_with_decoded_bytes(key, source_path, image, decoded_bytes) + } + + fn insert_with_decoded_bytes( + &mut self, + key: FittedSourceKey, + source_path: PathBuf, + image: DynamicImage, + decoded_bytes: usize, + ) -> Arc { + // Only one placeholder geometry per image is useful after a resize. Drop + // older variants immediately rather than waiting for global LRU pressure. + self.remove_hash(key.hash); + let image = Arc::new(image); + self.total_decoded_bytes = self.total_decoded_bytes.saturating_add(decoded_bytes); + self.entries.insert( + key, + FittedSourceEntry { + source_path, + image: image.clone(), + decoded_bytes, + }, + ); + self.order.push_back(key); + // Preserve one oversized fitted source so the image remains drawable + // instead of cycling through scale -> immediate eviction on every frame. + while (self.order.len() > FITTED_SOURCE_CACHE_MAX + || self.total_decoded_bytes > FITTED_SOURCE_CACHE_MAX_BYTES) + && self.order.len() > 1 + { + if let Some(old) = self.order.pop_front() + && let Some(entry) = self.entries.remove(&old) + { + self.total_decoded_bytes = + self.total_decoded_bytes.saturating_sub(entry.decoded_bytes); + } + } + image + } + + fn remove_key(&mut self, key: FittedSourceKey) { + if let Some(entry) = self.entries.remove(&key) { + self.total_decoded_bytes = self.total_decoded_bytes.saturating_sub(entry.decoded_bytes); + } + if let Some(pos) = self.order.iter().position(|entry| *entry == key) { + self.order.remove(pos); + } + } + + fn remove_hash(&mut self, hash: u64) { + let keys: Vec<_> = self + .entries + .keys() + .filter(|key| key.hash == hash) + .copied() + .collect(); + for key in keys { + self.remove_key(key); + } + } + + fn clear(&mut self) { + self.entries.clear(); + self.order.clear(); + self.total_decoded_bytes = 0; + } } /// Track what was rendered last frame for skip-redundant optimization @@ -907,13 +1224,28 @@ pub struct MermaidMemoryProfile { /// Number of image protocol states currently cached. pub image_state_entries: usize, pub image_state_limit: usize, + /// Maximum decoded source bytes retained by image protocol states. + pub image_state_source_limit_bytes: usize, /// Lower-bound estimate for image protocol buffers (derived from source PNG dimensions). pub image_state_protocol_min_estimate_bytes: u64, /// Number of decoded source images cached for viewport panning. pub source_cache_entries: usize, pub source_cache_limit: usize, + /// Maximum exact decoded bytes retained by the source-image cache. + pub source_cache_limit_bytes: usize, /// Estimated decoded source image bytes (RGBA estimate). pub source_cache_decoded_estimate_bytes: u64, + /// Number of non-Kitty sources pre-scaled to inline placeholder geometry. + pub fitted_source_cache_entries: usize, + pub fitted_source_cache_limit: usize, + pub fitted_source_cache_limit_bytes: usize, + /// Exact decoded bytes held by pre-scaled non-Kitty sources. + pub fitted_source_cache_decoded_bytes: u64, + /// Kitty virtual-placement states and exact not-yet-drawn transmit bytes. + pub kitty_viewport_state_entries: usize, + pub kitty_viewport_state_limit: usize, + pub kitty_pending_transmit_bytes: u64, + pub kitty_pending_transmit_limit_bytes: usize, /// Number of active diagrams in the pinned-diagram list. pub active_diagrams: usize, pub active_diagrams_limit: usize, @@ -994,6 +1326,14 @@ pub struct ImageScrollBenchmark { pub fit_protocol_rebuilds: u64, /// Cheap fit-state reuse hits during the scroll. pub fit_state_reuse_hits: u64, + /// Exact decoded source bytes retained by cached protocol states. + pub retained_image_state_source_bytes: u64, + /// Exact decoded bytes retained by full source images after the benchmark. + pub retained_source_cache_decoded_bytes: u64, + /// Exact decoded bytes retained by pre-scaled non-Kitty sources. + pub retained_fitted_source_decoded_bytes: u64, + /// Lower-bound total for Mermaid/image-owned memory after the benchmark. + pub retained_working_set_estimate_bytes: u64, } #[derive(Debug, Clone, Default, Serialize)] @@ -1027,10 +1367,14 @@ fn hash_content(content: &str) -> u64 { /// Get PNG dimensions from file fn get_png_dimensions(path: &Path) -> Option<(u32, u32)> { - let data = fs::read(path).ok()?; - if data.len() > 24 && &data[0..8] == b"\x89PNG\r\n\x1a\n" { - let width = u32::from_be_bytes([data[16], data[17], data[18], data[19]]); - let height = u32::from_be_bytes([data[20], data[21], data[22], data[23]]); + use std::io::Read as _; + + let mut header = [0u8; 24]; + let mut file = fs::File::open(path).ok()?; + file.read_exact(&mut header).ok()?; + if &header[0..8] == b"\x89PNG\r\n\x1a\n" { + let width = u32::from_be_bytes([header[16], header[17], header[18], header[19]]); + let height = u32::from_be_bytes([header[20], header[21], header[22], header[23]]); return Some((width, height)); } None @@ -1100,14 +1444,52 @@ pub fn clear_image_state() { state.clear(); } if let Ok(mut source) = SOURCE_CACHE.lock() { - source.entries.clear(); - source.order.clear(); + source.clear(); + } + if let Ok(mut fitted) = FITTED_SOURCE_CACHE.lock() { + fitted.clear(); + } + if let Ok(mut kitty) = KITTY_VIEWPORT_STATE.lock() { + kitty.clear(); } if let Ok(mut last) = LAST_RENDER.lock() { last.clear(); } } +/// Take terminal control sequences that delete Kitty image allocations evicted +/// from the in-process caches. Exposed for TUI teardown, where there may be no +/// later frame in which an image widget can carry the deferred cleanup. +pub fn take_terminal_image_cleanup_payload() -> String { + viewport_render::take_kitty_delete_payloads() +} + +/// Attach pending Kitty deletion commands to the first cell of an ordinary TUI +/// frame. Escape sequences are zero-width, so preserving the original symbol +/// keeps the rendered frame visually unchanged even when no image remains. +pub fn render_pending_terminal_image_cleanup(buf: &mut Buffer) -> bool { + let area = *buf.area(); + if area.width == 0 || area.height == 0 { + return false; + } + let payload = take_terminal_image_cleanup_payload(); + if payload.is_empty() { + return false; + } + let Some(cell) = buf.cell_mut((area.left(), area.top())) else { + return false; + }; + let existing = cell.symbol().to_string(); + let mut symbol = String::with_capacity(payload.len() + existing.len()); + symbol.push_str(&payload); + symbol.push_str(&existing); + cell.set_symbol(&symbol); + true +} + +#[cfg(test)] +static IMAGE_TEST_LOCK: Mutex<()> = Mutex::new(()); + #[cfg(test)] #[path = "mermaid_tests.rs"] mod tests; diff --git a/crates/jcode-tui-mermaid/src/mermaid_cache_render.rs b/crates/jcode-tui-mermaid/src/mermaid_cache_render.rs index f291017f1c..6922f68030 100644 --- a/crates/jcode-tui-mermaid/src/mermaid_cache_render.rs +++ b/crates/jcode-tui-mermaid/src/mermaid_cache_render.rs @@ -37,6 +37,11 @@ pub(super) struct MermaidCache { pub(super) order: VecDeque<(u64, RenderProfile)>, /// Cache directory pub(super) cache_dir: PathBuf, + /// Largest requested width for which disk discovery found no satisfying + /// rendition. This preserves the zero-I/O steady-state fallback for uploads + /// while allowing a pane expansion to discover an already-rendered wider PNG + /// exactly once. A later insert clears the memo for that content hash. + pub(super) width_miss_floor: HashMap, } #[derive(Clone)] @@ -59,6 +64,7 @@ impl MermaidCache { entries: HashMap::new(), order: VecDeque::new(), cache_dir, + width_miss_floor: HashMap::new(), } } @@ -152,18 +158,71 @@ impl MermaidCache { /// profile, while the draw thread runs outside that aspect scope, so an /// exact-profile lookup would never find it. fn get_in_memory_any_profile(&mut self, hash: u64) -> Option { + self.get_in_memory_any_profile_for_width(hash, None) + } + + fn get_in_memory_any_profile_for_width( + &mut self, + hash: u64, + min_width: Option, + ) -> Option { let key = self .order .iter() .rev() - .find(|(entry_hash, _)| *entry_hash == hash) + .find(|key| { + key.0 == hash + && self + .entries + .get(key) + .is_some_and(|entry| cached_width_satisfies(entry.width, min_width)) + }) .copied()?; let existing = self.entries.get(&key).cloned()?; self.touch(key); Some(existing) } + fn get_preferred_width_or_any_in_memory( + &mut self, + hash: u64, + min_width: Option, + ) -> Option { + if let Some(existing) = self.get_in_memory_any_profile_for_width(hash, min_width) { + return Some(existing); + } + let Some(min_width) = min_width.filter(|width| *width > 0) else { + return self.get_in_memory_any_profile(hash); + }; + + let already_missed = self + .width_miss_floor + .get(&hash) + .is_some_and(|missed_width| *missed_width >= min_width); + if !already_missed { + if let Some(found) = self.discover_on_disk(hash, Some(min_width), None) { + let profile = parse_cache_filename(&found.path) + .map(|(_, _, profile)| profile) + .unwrap_or_default(); + self.insert(hash, profile, found.clone()); + return Some(found); + } + if self.width_miss_floor.len() >= RENDER_CACHE_MAX + && !self.width_miss_floor.contains_key(&hash) + { + self.width_miss_floor.clear(); + } + self.width_miss_floor + .entry(hash) + .and_modify(|width| *width = (*width).max(min_width)) + .or_insert(min_width); + } + + self.get_in_memory_any_profile(hash) + } + pub(super) fn insert(&mut self, hash: u64, profile: RenderProfile, diagram: CachedDiagram) { + self.width_miss_floor.remove(&hash); let key = (hash, profile); if let std::collections::hash_map::Entry::Occupied(mut entry) = self.entries.entry(key) { entry.insert(diagram); @@ -643,6 +702,16 @@ pub(super) fn get_cached_diagram_in_memory(hash: u64) -> Option { .or_else(|| cache.get_in_memory_any_profile(hash)) } +pub(super) fn get_cached_diagram_prefer_width( + hash: u64, + min_width: Option, +) -> Option { + RENDER_CACHE + .lock() + .ok()? + .get_preferred_width_or_any_in_memory(hash, min_width) +} + fn get_cached_diagram_for_profile( hash: u64, min_width: Option, @@ -667,6 +736,9 @@ fn invalidate_cached_image(hash: u64) { if let Ok(mut source) = SOURCE_CACHE.lock() { source.remove(hash); } + if let Ok(mut fitted) = FITTED_SOURCE_CACHE.lock() { + fitted.remove_hash(hash); + } } /// Result of attempting to render a mermaid diagram @@ -1331,3 +1403,65 @@ mod font_prewarm_tests { ); } } + +#[cfg(test)] +mod width_selection_tests { + use super::*; + + fn test_cache(name: &str) -> MermaidCache { + let cache_dir = std::env::temp_dir().join(format!( + "jcode-mermaid-width-{name}-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let _ = fs::remove_dir_all(&cache_dir); + fs::create_dir_all(&cache_dir).expect("create cache fixture"); + MermaidCache { + entries: HashMap::new(), + order: VecDeque::new(), + cache_dir, + width_miss_floor: HashMap::new(), + } + } + + #[test] + fn pane_expansion_prefers_wider_disk_rendition_then_memoizes_misses() { + const HASH: u64 = 0xA11C_E55D_1A6A_0001; + let mut cache = test_cache("prefer-wide"); + let narrow = CachedDiagram { + path: cache.cache_dir.join(format!("{HASH:016x}_w100.png")), + width: 100, + height: 50, + }; + cache.insert( + HASH, + RenderProfile { + preferred_aspect_per_mille: Some(1000), + }, + narrow, + ); + let wide_path = cache.cache_dir.join(format!("{HASH:016x}_w220.png")); + fs::write(&wide_path, []).expect("write wider cache fixture"); + + let selected = cache + .get_preferred_width_or_any_in_memory(HASH, Some(200)) + .expect("wider rendition"); + assert_eq!(selected.width, 220); + + fs::remove_file(&wide_path).expect("remove wider fixture"); + cache.entries.retain(|_, entry| entry.width == 100); + cache.order.retain(|key| cache.entries.contains_key(key)); + let selected = cache + .get_preferred_width_or_any_in_memory(HASH, Some(300)) + .expect("narrow fallback"); + assert_eq!(selected.width, 100); + assert_eq!(cache.width_miss_floor.get(&HASH), Some(&300)); + let selected_again = cache + .get_preferred_width_or_any_in_memory(HASH, Some(300)) + .expect("memoized narrow fallback"); + assert_eq!(selected_again.width, 100); + assert_eq!(cache.width_miss_floor.get(&HASH), Some(&300)); + + let _ = fs::remove_dir_all(&cache.cache_dir); + } +} diff --git a/crates/jcode-tui-mermaid/src/mermaid_debug.rs b/crates/jcode-tui-mermaid/src/mermaid_debug.rs index 4fa91b5606..2440e1df7c 100644 --- a/crates/jcode-tui-mermaid/src/mermaid_debug.rs +++ b/crates/jcode-tui-mermaid/src/mermaid_debug.rs @@ -113,7 +113,13 @@ pub fn debug_memory_profile() -> MermaidMemoryProfile { process_virtual_bytes: process_mem.virtual_bytes, render_cache_limit: RENDER_CACHE_MAX, image_state_limit: IMAGE_STATE_MAX, + image_state_source_limit_bytes: IMAGE_STATE_MAX_SOURCE_BYTES, source_cache_limit: SOURCE_CACHE_MAX, + source_cache_limit_bytes: SOURCE_CACHE_MAX_BYTES, + fitted_source_cache_limit: FITTED_SOURCE_CACHE_MAX, + fitted_source_cache_limit_bytes: FITTED_SOURCE_CACHE_MAX_BYTES, + kitty_viewport_state_limit: KITTY_VIEWPORT_STATE_MAX, + kitty_pending_transmit_limit_bytes: KITTY_VIEWPORT_PENDING_MAX_BYTES, active_diagrams_limit: ACTIVE_DIAGRAMS_MAX, cache_disk_limit_bytes: CACHE_MAX_SIZE_BYTES, cache_disk_max_age_secs: CACHE_MAX_AGE_SECS, @@ -143,28 +149,22 @@ pub fn debug_memory_profile() -> MermaidMemoryProfile { if let Ok(state) = IMAGE_STATE.lock() { out.image_state_entries = state.entries.len(); - let mut seen_paths: HashSet = HashSet::new(); - for (_, image_state) in state.iter() { - if seen_paths.insert(image_state.source_path.clone()) - && let Some((w, h)) = get_png_dimensions(&image_state.source_path) - { - out.image_state_protocol_min_estimate_bytes = out - .image_state_protocol_min_estimate_bytes - .saturating_add(rgba_bytes_estimate(w, h)); - } - } + out.image_state_protocol_min_estimate_bytes = state.total_source_bytes as u64; } if let Ok(source) = SOURCE_CACHE.lock() { out.source_cache_entries = source.entries.len(); - for entry in source.entries.values() { - out.source_cache_decoded_estimate_bytes = out - .source_cache_decoded_estimate_bytes - .saturating_add(rgba_bytes_estimate( - entry.image.width(), - entry.image.height(), - )); - } + out.source_cache_decoded_estimate_bytes = source.total_decoded_bytes as u64; + } + + if let Ok(fitted) = FITTED_SOURCE_CACHE.lock() { + out.fitted_source_cache_entries = fitted.entries.len(); + out.fitted_source_cache_decoded_bytes = fitted.total_decoded_bytes as u64; + } + + if let Ok(kitty) = KITTY_VIEWPORT_STATE.lock() { + out.kitty_viewport_state_entries = kitty.entries.len(); + out.kitty_pending_transmit_bytes = kitty.total_pending_transmit_bytes as u64; } out.active_diagrams = active_diagram_count(); @@ -178,6 +178,8 @@ pub fn debug_memory_profile() -> MermaidMemoryProfile { .render_cache_metadata_estimate_bytes .saturating_add(out.image_state_protocol_min_estimate_bytes) .saturating_add(out.source_cache_decoded_estimate_bytes) + .saturating_add(out.fitted_source_cache_decoded_bytes) + .saturating_add(out.kitty_pending_transmit_bytes) .saturating_add(layout_bytes); out @@ -381,6 +383,7 @@ pub fn debug_image_scroll_benchmark( // Force a Kitty picker so the stable-fit fast path (the one used for real // inline screenshots) is exercised even in a headless benchmark process. force_test_kitty_picker(); + clear_image_state(); let images = images.clamp(1, 4096); let frames = frames.clamp(1, 100_000); @@ -472,6 +475,7 @@ pub fn debug_image_scroll_benchmark( let stat_after = super::cache_stat_syscalls(); let stats_after = debug_stats(); + let memory_after = debug_memory_profile(); let stat_syscalls = stat_after.saturating_sub(stat_before); ImageScrollBenchmark { @@ -489,6 +493,10 @@ pub fn debug_image_scroll_benchmark( fit_state_reuse_hits: stats_after .fit_state_reuse_hits .saturating_sub(stats_before.fit_state_reuse_hits), + retained_image_state_source_bytes: memory_after.image_state_protocol_min_estimate_bytes, + retained_source_cache_decoded_bytes: memory_after.source_cache_decoded_estimate_bytes, + retained_fitted_source_decoded_bytes: memory_after.fitted_source_cache_decoded_bytes, + retained_working_set_estimate_bytes: memory_after.mermaid_working_set_estimate_bytes, } } @@ -511,12 +519,6 @@ fn scan_cache_dir_png_usage(cache_dir: &Path) -> (usize, u64) { (file_count, total_bytes) } -fn rgba_bytes_estimate(width: u32, height: u32) -> u64 { - (width as u64) - .saturating_mul(height as u64) - .saturating_mul(4) -} - fn max_opt_u64(a: Option, b: Option) -> Option { match (a, b) { (Some(x), Some(y)) => Some(x.max(y)), diff --git a/crates/jcode-tui-mermaid/src/mermaid_runtime.rs b/crates/jcode-tui-mermaid/src/mermaid_runtime.rs index d9b6129ba9..afcdb9dbc1 100644 --- a/crates/jcode-tui-mermaid/src/mermaid_runtime.rs +++ b/crates/jcode-tui-mermaid/src/mermaid_runtime.rs @@ -259,8 +259,9 @@ pub(crate) fn prewarm_svg_font_db_async() { /// By default jcode uses environment-based detection and never blocks startup /// on terminal capability responses. Set JCODE_MERMAID_PICKER_PROBE=1 to run an /// authoritative stdio probe when a multiplexer masks the outer terminal, or -/// =0 to explicitly retain the fast path. Also triggers cache eviction on first -/// call. +/// =0 to explicitly retain the fast path. Cache eviction runs once before image +/// rendering can begin so it cannot delete a file between materialization and +/// in-memory cache registration. pub fn init_picker() { PICKER.get_or_init(|| { let env_protocol = infer_protocol_from_env( @@ -291,10 +292,10 @@ pub fn init_picker() { // init_picker() runs on every TUI startup, and the font load is only // needed if a mermaid diagram is actually rendered; see // prewarm_svg_font_db_async() for the lazy trigger. - // Evict old cache files once per process - CACHE_EVICTED.get_or_init(|| { - evict_old_cache(); - }); + // This is intentionally synchronous. An asynchronous pass can observe an + // image file after it is written but before it is registered in RENDER_CACHE, + // delete it, and leave stale in-memory metadata that suppresses recovery. + CACHE_EVICTED.get_or_init(evict_old_cache); } /// Force the global picker into Kitty protocol for deterministic benchmarks and diff --git a/crates/jcode-tui-mermaid/src/mermaid_tests/part_02.rs b/crates/jcode-tui-mermaid/src/mermaid_tests/part_02.rs index d61b2c4c70..eca001c9c9 100644 --- a/crates/jcode-tui-mermaid/src/mermaid_tests/part_02.rs +++ b/crates/jcode-tui-mermaid/src/mermaid_tests/part_02.rs @@ -97,9 +97,8 @@ fn mmdr_size_api_fits_natural_aspect_into_target_canvas() { // Wide linear chain: natural layout is much wider than the ~4:3 target // box, so forcing the raw target canvas would letterbox ~80% of the PNG // with transparent padding above and below the ink. - let content = format!( - "flowchart LR\nA[Start {unique}] --> B[Step] --> C[Step] --> D[Step] --> E[End]" - ); + let content = + format!("flowchart LR\nA[Start {unique}] --> B[Step] --> C[Step] --> D[Step] --> E[End]"); let result = super::render_mermaid_untracked(&content, Some(100)); let (width, height) = match result { @@ -149,6 +148,7 @@ fn mmdr_size_api_fits_natural_aspect_into_target_canvas() { /// test pins the corrected steady-state behavior via the image-scroll benchmark. #[test] fn image_scroll_steady_state_has_no_per_frame_stats_or_rebuilds() { + let _stats_guard = render_stats_test_lock(); // 60 images > the historical fit-state cap (24); 800 frames is plenty of // steady-state scrolling to surface any per-frame stat/rebuild regression. let result = super::debug_image_scroll_benchmark(60, 800, 3); @@ -176,6 +176,45 @@ fn image_scroll_steady_state_has_no_per_frame_stats_or_rebuilds() { (result.frames * result.visible_per_frame) as u64, "expected one cheap fit-state reuse per visible image per frame" ); + assert_eq!( + result.retained_source_cache_decoded_bytes, 0, + "Kitty owns transmitted pixels, so decoded full sources should be released" + ); + assert!( + result.retained_image_state_source_bytes <= crate::IMAGE_STATE_MAX_SOURCE_BYTES as u64, + "protocol source memory must remain within its byte budget" + ); +} + +#[test] +fn warm_fit_draws_do_not_stat_the_render_cache() { + let _stats_guard = render_stats_test_lock(); + use base64::Engine as _; + use image::ImageEncoder as _; + + crate::force_test_kitty_picker(); + let image = image::RgbaImage::from_pixel(32, 16, image::Rgba([17, 29, 43, 255])); + let mut png = Vec::new(); + image::codecs::png::PngEncoder::new(&mut png) + .write_image(image.as_raw(), 32, 16, image::ExtendedColorType::Rgba8) + .expect("encode fixture"); + let b64 = base64::engine::general_purpose::STANDARD.encode(png); + let (hash, _, _) = crate::materialize_inline_image("image/png", &b64).expect("materialize"); + let area = ratatui::layout::Rect::new(0, 0, 20, 6); + let mut buf = ratatui::buffer::Buffer::empty(area); + + // The first expanded draw may probe disk once for a wider cached rendition. + // Measure after that preference decision has been memoized: steady-state + // draws must never repeat the lookup. + crate::render_image_widget_fit(hash, area, &mut buf, false, true); + let before = crate::cache_stat_syscalls(); + crate::render_image_widget_fit(hash, area, &mut buf, false, true); + crate::render_image_widget_fit(hash, area, &mut buf, false, true); + assert_eq!( + crate::cache_stat_syscalls() - before, + 0, + "warm draw paths must stay entirely in memory" + ); } /// `evict_old_cache` used to look only at `*.png`, so inline images cached in @@ -223,6 +262,144 @@ fn bounded_bookkeeping_insert_caps_map_growth() { assert_eq!(map.len(), before, "existing-key update must not clear"); } +#[test] +fn source_image_cache_enforces_decoded_byte_budget() { + let mut cache = crate::SourceImageCache::new(); + let accounted = crate::SOURCE_CACHE_MAX_BYTES / 2 + 1; + for hash in 1..=2u64 { + cache.insert_with_decoded_bytes( + hash, + std::path::PathBuf::from(format!("source-{hash}.png")), + image::DynamicImage::new_rgba8(1, 1), + accounted, + ); + } + assert!(cache.total_decoded_bytes <= crate::SOURCE_CACHE_MAX_BYTES); + assert_eq!(cache.entries.len(), 1, "oldest decoded source should evict"); + assert!(cache.entries.contains_key(&2)); + + let mut oversized = crate::SourceImageCache::new(); + oversized.insert_with_decoded_bytes( + 9, + std::path::PathBuf::from("oversized.png"), + image::DynamicImage::new_rgba8(1, 1), + crate::SOURCE_CACHE_MAX_BYTES + 1, + ); + assert_eq!( + oversized.entries.len(), + 1, + "single oversized source must remain drawable" + ); +} + +#[test] +fn fitted_source_cache_enforces_budget_and_replaces_resize_variants() { + let mut cache = crate::FittedSourceCache::new(); + let accounted = crate::FITTED_SOURCE_CACHE_MAX_BYTES / 2 + 1; + for hash in 1..=2u64 { + let key = crate::FittedSourceKey { + hash, + target_cols: 80, + target_rows: 16, + font_size: (8, 16), + }; + cache.insert_with_decoded_bytes( + key, + std::path::PathBuf::from(format!("fitted-{hash}.png")), + image::DynamicImage::new_rgba8(1, 1), + accounted, + ); + } + assert!(cache.total_decoded_bytes <= crate::FITTED_SOURCE_CACHE_MAX_BYTES); + assert_eq!(cache.entries.len(), 1, "oldest fitted source should evict"); + assert!(cache.entries.keys().any(|key| key.hash == 2)); + + let replacement = crate::FittedSourceKey { + hash: 2, + target_cols: 96, + target_rows: 20, + font_size: (8, 16), + }; + cache.insert_with_decoded_bytes( + replacement, + std::path::PathBuf::from("fitted-2.png"), + image::DynamicImage::new_rgba8(1, 1), + 4, + ); + assert_eq!( + cache.entries.keys().filter(|key| key.hash == 2).count(), + 1, + "terminal resize must replace rather than accumulate fitted variants" + ); + assert!(cache.entries.contains_key(&replacement)); +} + +#[test] +fn fitted_scroll_crop_tracks_arbitrary_middle_rows_without_rescaling() { + // 800x320 at 10x20 pixels/cell occupies 80x16 cells. Scrolling four + // rows down into a five-row viewport must crop source pixels 80..180, not + // squeeze the full image or jump to its bottom edge. + assert_eq!( + super::viewport_render::fitted_scroll_crop(800, 320, (10, 20), 80, 5, 4), + Some((80, 100, 80, 5)) + ); + // The final partial row is bounded by the actual source height. + assert_eq!( + super::viewport_render::fitted_scroll_crop(800, 305, (10, 20), 80, 4, 15), + Some((300, 5, 80, 1)) + ); + assert_eq!( + super::viewport_render::fitted_scroll_crop(800, 320, (10, 20), 80, 5, 16), + None, + "rows entirely below the fitted image should remain blank" + ); +} + +#[test] +fn image_state_cache_enforces_source_byte_budget() { + crate::force_test_kitty_picker(); + let picker = crate::PICKER + .get() + .and_then(|picker| picker.as_ref()) + .expect("test picker"); + let mut cache = crate::ImageStateCache::new(); + let accounted = crate::IMAGE_STATE_MAX_SOURCE_BYTES / 2 + 1; + for hash in 1..=2u64 { + let protocol = picker.new_resize_protocol(image::DynamicImage::new_rgba8(1, 1)); + cache.insert( + hash, + crate::ImageState { + protocol, + source_path: std::path::PathBuf::from(format!("state-{hash}.png")), + source_bytes: accounted, + last_area: None, + resize_mode: crate::ResizeMode::Fit, + last_crop_top: false, + last_viewport: None, + }, + ); + } + assert!(cache.total_source_bytes <= crate::IMAGE_STATE_MAX_SOURCE_BYTES); + assert_eq!(cache.entries.len(), 1, "oldest protocol state should evict"); + assert!(cache.entries.contains_key(&2)); +} + +#[test] +fn png_dimension_probe_needs_only_the_header() { + let path = std::env::temp_dir().join(format!( + "jcode-png-header-{}-{}.png", + std::process::id(), + std::thread::current().name().unwrap_or("test") + )); + let mut header = [0u8; 24]; + header[..8].copy_from_slice(b"\x89PNG\r\n\x1a\n"); + header[16..20].copy_from_slice(&123u32.to_be_bytes()); + header[20..24].copy_from_slice(&456u32.to_be_bytes()); + std::fs::write(&path, header).expect("write header fixture"); + assert_eq!(crate::get_png_dimensions(&path), Some((123, 456))); + let _ = std::fs::remove_file(path); +} + /// Inline-fit geometry must preserve aspect ratio, respect the row cap, and /// return a marker-parsable placeholder that survives leading padding spans /// (centered mode inserts one). @@ -249,7 +426,9 @@ fn inline_fit_geometry_and_marker_roundtrip() { // A leading whitespace span (centered-mode padding) must not break parsing. let mut padded = lines[0].clone(); - padded.spans.insert(0, Span::styled(" ", Style::default())); + padded + .spans + .insert(0, Span::styled(" ", Style::default())); assert_eq!( crate::parse_inline_image_placeholder(&padded), Some((0xabcdef, rows, cols)), @@ -269,9 +448,8 @@ fn inline_transcript_aspect_goal_produces_expected_bucketed_profile() { assert_eq!(goal, Some(1.75)); // The goal flows through the standard profile bucketing (per-mille). - let bucket = crate::with_preferred_aspect_ratio(goal, || { - crate::current_preferred_aspect_ratio_bucket() - }); + let bucket = + crate::with_preferred_aspect_ratio(goal, || crate::current_preferred_aspect_ratio_bucket()); assert_eq!(bucket, Some(1750)); // Narrow terminals floor at the 4:3 sizing default instead of requesting @@ -297,7 +475,10 @@ fn inline_transcript_aspect_goal_is_stable_under_resize_jitter() { let b = crate::inline_transcript_aspect_goal_with_font(121, 40, Some((8, 16))); let c = crate::inline_transcript_aspect_goal_with_font(120, 39, Some((8, 16))); assert_eq!(a, b, "1-col width jitter must stay in the same aspect step"); - assert_eq!(a, c, "1-row height jitter must stay in the same aspect step"); + assert_eq!( + a, c, + "1-row height jitter must stay in the same aspect step" + ); let bucket_a = crate::preferred_aspect_ratio_bucket(a); let bucket_b = crate::preferred_aspect_ratio_bucket(b); @@ -331,19 +512,14 @@ fn inline_transcript_aspect_goal_no_geometry_yields_none() { #[test] fn transcript_profile_keeps_pinned_pane_aspect_when_present() { let pane_aspect = Some(0.5); - let combined = crate::transcript_preferred_aspect_ratio_with_font( - pane_aspect, - 120, - 40, - Some((8, 16)), - ); + let combined = + crate::transcript_preferred_aspect_ratio_with_font(pane_aspect, 120, 40, Some((8, 16))); assert_eq!( combined, pane_aspect, "pinned pane aspect must win over the inline goal" ); - let fallback = - crate::transcript_preferred_aspect_ratio_with_font(None, 120, 40, Some((8, 16))); + let fallback = crate::transcript_preferred_aspect_ratio_with_font(None, 120, 40, Some((8, 16))); assert_eq!(fallback, Some(1.75), "no pane -> inline goal"); let no_geometry = crate::transcript_preferred_aspect_ratio_with_font(None, 120, 40, None); @@ -354,8 +530,9 @@ fn transcript_profile_keeps_pinned_pane_aspect_when_present() { /// `MermaidDebugStats` (`last_*` fields and counters), which concurrent /// renders in sibling tests would otherwise clobber. fn render_stats_test_lock() -> std::sync::MutexGuard<'static, ()> { - static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); - LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()) + crate::IMAGE_TEST_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) } /// Layout is terminal-width independent: rendering the same source at two @@ -539,9 +716,15 @@ fn layout_cache_evicts_lru_and_clears_on_theme_change() { // Touch entry 0 so it becomes most-recently used, then overflow: entry 1 // (now the LRU) must be evicted, entry 0 retained. assert!(cache.get(&key(0, 1)).is_some()); - cache.insert(key(super::cache_render::LAYOUT_CACHE_MAX as u64, 1), empty_layout()); + cache.insert( + key(super::cache_render::LAYOUT_CACHE_MAX as u64, 1), + empty_layout(), + ); assert_eq!(cache.entries.len(), super::cache_render::LAYOUT_CACHE_MAX); - assert!(cache.get(&key(0, 1)).is_some(), "recently used entry survives"); + assert!( + cache.get(&key(0, 1)).is_some(), + "recently used entry survives" + ); assert!(cache.get(&key(1, 1)).is_none(), "LRU entry is evicted"); // Theme change: a lookup with a new theme fingerprint clears stale entries. @@ -592,7 +775,10 @@ fn streaming_preview_then_final_registration_does_not_double_count() { assert_eq!(combined[0].hash, HASH); // While the preview is still live, the preview entry wins (it is pushed // first and the registered duplicate is filtered by hash). - assert_eq!(combined[0].width, 100, "live preview entry shadows the registered one"); + assert_eq!( + combined[0].width, 100, + "live preview entry shadows the registered one" + ); let registered = super::snapshot_active_diagrams(); assert_eq!(registered.len(), 1, "exactly one registered entry"); assert_eq!(registered[0].hash, HASH); diff --git a/crates/jcode-tui-mermaid/src/mermaid_viewport.rs b/crates/jcode-tui-mermaid/src/mermaid_viewport.rs index b0511db4d5..d313cc2381 100644 --- a/crates/jcode-tui-mermaid/src/mermaid_viewport.rs +++ b/crates/jcode-tui-mermaid/src/mermaid_viewport.rs @@ -15,6 +15,91 @@ fn load_source_image(hash: u64, path: &Path) -> Option> { Some(Arc::new(img)) } +/// Drop a decoded source once a stable Kitty fit transmission has been built. +/// The terminal retains the transmitted pixels and the on-disk cache retains +/// the source bytes, so keeping the full decoded image as well only adds tens of +/// MiB to long screenshot-heavy conversations. +fn release_source_image(hash: u64, path: &Path) { + if let Ok(mut cache) = SOURCE_CACHE.lock() { + cache.remove_if_path(hash, path); + } +} + +fn fitted_source_key( + hash: u64, + target_cols: u16, + target_rows: u16, + font_size: (u16, u16), +) -> FittedSourceKey { + FittedSourceKey { + hash, + target_cols, + target_rows, + font_size, + } +} + +/// A fitted source is draw-ready when either its scaled variant is cached or +/// the decoded original already fits the target and therefore needs no resize. +fn fitted_source_is_ready( + hash: u64, + source_path: &Path, + target_cols: u16, + target_rows: u16, + font_size: (u16, u16), +) -> bool { + let key = fitted_source_key(hash, target_cols, target_rows, font_size); + if let Ok(mut cache) = FITTED_SOURCE_CACHE.lock() + && cache.get(key, source_path).is_some() + { + return true; + } + + let max_w_px = (target_cols as u32).saturating_mul(font_size.0.max(1) as u32); + let max_h_px = (target_rows as u32).saturating_mul(font_size.1.max(1) as u32); + SOURCE_CACHE + .lock() + .ok() + .and_then(|cache| { + cache.entries.get(&hash).map(|entry| { + entry.path == source_path + && entry.image.width() <= max_w_px + && entry.image.height() <= max_h_px + }) + }) + .unwrap_or(false) +} + +fn load_fitted_source( + hash: u64, + source_path: &Path, + target_cols: u16, + target_rows: u16, + font_size: (u16, u16), +) -> Option> { + let key = fitted_source_key(hash, target_cols, target_rows, font_size); + if let Ok(mut cache) = FITTED_SOURCE_CACHE.lock() + && let Some(image) = cache.get(key, source_path) + { + return Some(image); + } + + let source = load_source_image(hash, source_path)?; + let scaled = match scale_to_fit_box(source.as_ref(), target_cols, target_rows, font_size) { + Cow::Borrowed(_) => return Some(source), + Cow::Owned(image) => image, + }; + let fitted = FITTED_SOURCE_CACHE + .lock() + .ok() + .map(|mut cache| cache.insert(key, source_path.to_path_buf(), scaled))?; + drop(source); + // The fitted variant now owns the pixels needed for scrolling. Keeping the + // full decoded original as well would double the resident cost. + release_source_image(hash, source_path); + Some(fitted) +} + pub(super) fn viewport_crop_should_scale_to_area( crop_w: u32, crop_h: u32, @@ -25,8 +110,16 @@ pub(super) fn viewport_crop_should_scale_to_area( } fn kitty_viewport_unique_id(hash: u64) -> u32 { - let mixed = (hash as u32) ^ ((hash >> 32) as u32) ^ 0x4B49_5459; - mixed.max(1) + let _ = hash; + loop { + let sequence = NEXT_KITTY_IMAGE_ID.fetch_add(1, Ordering::Relaxed) & 0x00FF_FFFF; + if sequence != 0 { + // Reserve the 0x4Axxxxxx namespace ('J') for jcode's custom virtual + // placements so these ids do not collide with ratatui-image's Kitty + // protocol states in the same terminal process. + return 0x4A00_0000 | sequence; + } + } } fn kitty_is_tmux() -> bool { @@ -35,29 +128,36 @@ fn kitty_is_tmux() -> bool { } fn kitty_transmit_payload(bytes: &[u8], id: u32, format: &str) -> String { - let (start, escape, end) = Parser::escape_tmux(kitty_is_tmux()); - let mut data = String::from(start); + use std::fmt::Write as _; + let (start, escape, end) = Parser::escape_tmux(kitty_is_tmux()); let chunks = bytes.chunks(4096 / 4 * 3); let chunk_count = chunks.len(); + let encoded_len = bytes.len().saturating_add(2) / 3 * 4; + let mut data = String::with_capacity( + start + .len() + .saturating_add(end.len()) + .saturating_add(encoded_len) + .saturating_add(chunk_count.saturating_mul(48)), + ); + data.push_str(start); for (i, chunk) in chunks.enumerate() { - let payload = base64::engine::general_purpose::STANDARD.encode(chunk); data.push_str(escape); match i { 0 => { let more = if chunk_count > 1 { 1 } else { 0 }; - data.push_str(&format!( - "_Gq=2,i={id},a=T,U=1,{format},t=d,m={more};{payload}" - )); + let _ = write!(data, "_Gq=2,i={id},a=T,U=1,{format},t=d,m={more};"); } n if n + 1 == chunk_count => { - data.push_str(&format!("_Gq=2,m=0;{payload}")); + data.push_str("_Gq=2,m=0;"); } _ => { - data.push_str(&format!("_Gq=2,m=1;{payload}")); + data.push_str("_Gq=2,m=1;"); } } + base64::engine::general_purpose::STANDARD.encode_string(chunk, &mut data); data.push_str(escape); data.push('\\'); } @@ -66,6 +166,29 @@ fn kitty_transmit_payload(bytes: &[u8], id: u32, format: &str) -> String { data } +fn kitty_delete_image_payload(id: u32) -> String { + use std::fmt::Write as _; + + let (start, escape, end) = Parser::escape_tmux(kitty_is_tmux()); + let mut data = String::with_capacity(start.len() + end.len() + 48); + data.push_str(start); + data.push_str(escape); + let _ = write!(data, "_Gq=2,a=d,d=I,i={id}"); + data.push_str(escape); + data.push('\\'); + data.push_str(end); + data +} + +pub(super) fn take_kitty_delete_payloads() -> String { + let ids = take_kitty_delete_ids(); + let mut payloads = String::with_capacity(ids.len().saturating_mul(48)); + for id in ids { + payloads.push_str(&kitty_delete_image_payload(id)); + } + payloads +} + fn kitty_transmit_virtual(img: &DynamicImage, id: u32) -> String { // Kitty accepts PNG payloads directly (`f=100`). Mermaid and generated // images compress especially well, while the previous raw RGBA upload sent @@ -84,12 +207,30 @@ fn kitty_transmit_virtual(img: &DynamicImage, id: u32) -> String { kitty_transmit_payload(rgba.as_raw(), id, &format!("f=32,s={w},v={h}")) } -fn kitty_scaled_image_for_zoom(source: &DynamicImage, zoom_percent: u8) -> DynamicImage { +/// Reuse an already-compressed PNG when the transmitted pixels are byte-for-byte +/// the on-disk source. This avoids a full DynamicImage -> PNG deflate pass for +/// unscaled Mermaid diagrams and small PNG screenshots. +fn kitty_transmit_png_path(path: &Path, id: u32) -> Option { + if !path + .extension() + .and_then(|extension| extension.to_str()) + .is_some_and(|extension| extension.eq_ignore_ascii_case("png")) + { + return None; + } + let bytes = fs::read(path).ok()?; + if !bytes.starts_with(b"\x89PNG\r\n\x1a\n") { + return None; + } + Some(kitty_transmit_payload(&bytes, id, "f=100")) +} + +fn kitty_scaled_image_for_zoom(source: &DynamicImage, zoom_percent: u8) -> Cow<'_, DynamicImage> { use image::imageops::FilterType; let zoom = zoom_percent.clamp(50, 200) as u32; if zoom == 100 { - return source.clone(); + return Cow::Borrowed(source); } let scaled_w = ((source.width() as u64).saturating_mul(zoom as u64) / 100) @@ -98,7 +239,7 @@ fn kitty_scaled_image_for_zoom(source: &DynamicImage, zoom_percent: u8) -> Dynam let scaled_h = ((source.height() as u64).saturating_mul(zoom as u64) / 100) .max(1) .min(u32::MAX as u64) as u32; - source.resize_exact(scaled_w, scaled_h, FilterType::Nearest) + Cow::Owned(source.resize_exact(scaled_w, scaled_h, FilterType::Nearest)) } fn div_ceil_u32_local(value: u32, divisor: u32) -> u32 { @@ -108,7 +249,7 @@ fn div_ceil_u32_local(value: u32, divisor: u32) -> u32 { .unwrap_or(value) } -fn kitty_full_rect_for_image(img: &DynamicImage, font_size: (u16, u16)) -> (u16, u16) { +fn cell_rect_for_image(img: &DynamicImage, font_size: (u16, u16)) -> (u16, u16) { ( div_ceil_u32_local(img.width().max(1), font_size.0.max(1) as u32).min(u16::MAX as u32) as u16, @@ -136,7 +277,7 @@ pub(super) fn ensure_kitty_viewport_state( } let scaled = kitty_scaled_image_for_zoom(source, zoom_percent); - let (full_cols, full_rows) = kitty_full_rect_for_image(&scaled, font_size); + let (full_cols, full_rows) = cell_rect_for_image(&scaled, font_size); if full_cols == 0 || full_rows == 0 { return None; } @@ -146,6 +287,13 @@ pub(super) fn ensure_kitty_viewport_state( .map(|state| state.unique_id) .unwrap_or_else(|| kitty_viewport_unique_id(hash)); + let pending_transmit = if zoom_percent == 100 { + kitty_transmit_png_path(source_path, unique_id) + .unwrap_or_else(|| kitty_transmit_virtual(&scaled, unique_id)) + } else { + kitty_transmit_virtual(&scaled, unique_id) + }; + let pending_transmit_bytes = pending_transmit.len(); cache.insert( hash, KittyViewportState { @@ -155,7 +303,8 @@ pub(super) fn ensure_kitty_viewport_state( unique_id, full_cols, full_rows, - pending_transmit: Some(kitty_transmit_virtual(&scaled, unique_id)), + pending_transmit: Some(pending_transmit), + pending_transmit_bytes, fit_target: None, }, ); @@ -209,12 +358,18 @@ pub(super) fn ensure_kitty_fit_state( // worker against every scroll frame and reintroduce the very stall the // off-thread prewarm exists to avoid. let scaled = scale_to_fit_box(source, target_cols, target_rows, font_size); - let (full_cols, full_rows) = kitty_full_rect_for_image(&scaled, font_size); + let (full_cols, full_rows) = cell_rect_for_image(&scaled, font_size); if full_cols == 0 || full_rows == 0 { return None; } let unique_id = existing_unique_id.unwrap_or_else(|| kitty_viewport_unique_id(hash)); - let pending_transmit = kitty_transmit_virtual(&scaled, unique_id); + let pending_transmit = if matches!(&scaled, Cow::Borrowed(_)) { + kitty_transmit_png_path(source_path, unique_id) + .unwrap_or_else(|| kitty_transmit_virtual(&scaled, unique_id)) + } else { + kitty_transmit_virtual(&scaled, unique_id) + }; + let pending_transmit_bytes = pending_transmit.len(); let mut cache = KITTY_VIEWPORT_STATE.lock().ok()?; // Re-check under the lock: another thread may have built matching state @@ -237,6 +392,7 @@ pub(super) fn ensure_kitty_fit_state( full_cols, full_rows, pending_transmit: Some(pending_transmit), + pending_transmit_bytes, fit_target: Some((target_cols, target_rows)), }, ); @@ -252,18 +408,18 @@ pub(super) fn ensure_kitty_fit_state( /// Scale a source image once to fit a `(cols, rows)` cell box at `font_size`, /// preserving aspect ratio. Returns a clone when the source already fits. -fn scale_to_fit_box( - source: &DynamicImage, +fn scale_to_fit_box<'a>( + source: &'a DynamicImage, target_cols: u16, target_rows: u16, font_size: (u16, u16), -) -> DynamicImage { +) -> Cow<'a, DynamicImage> { let max_w_px = (target_cols as u32).saturating_mul(font_size.0.max(1) as u32); let max_h_px = (target_rows as u32).saturating_mul(font_size.1.max(1) as u32); if source.width() <= max_w_px && source.height() <= max_h_px { - source.clone() + Cow::Borrowed(source) } else { - source.resize(max_w_px, max_h_px, image::imageops::FilterType::Triangle) + Cow::Owned(source.resize(max_w_px, max_h_px, image::imageops::FilterType::Triangle)) } } @@ -284,12 +440,11 @@ pub(super) fn render_kitty_virtual_viewport( Ok(cache) => cache, Err(_) => return false, }; - let Some(state) = cache.get_mut(hash) else { + let Some((unique_id, mut pending_transmit)) = cache.take_pending_transmit(hash) else { return false; }; - let unique_id = state.unique_id; - let pending_transmit = state.pending_transmit.take(); drop(cache); + let mut pending_deletes = Some(take_kitty_delete_payloads()); if pending_transmit.is_none() && let Ok(mut dbg) = MERMAID_DEBUG.lock() @@ -315,7 +470,9 @@ pub(super) fn render_kitty_virtual_viewport( } let mut symbol = if row == 0 { - pending_transmit.clone().unwrap_or_default() + let mut symbol = pending_deletes.take().unwrap_or_default(); + symbol.push_str(&pending_transmit.take().unwrap_or_default()); + symbol } else { String::new() }; @@ -692,25 +849,24 @@ fn probe_kitty_fit_state( .then_some((state.unique_id, state.full_cols, state.full_rows)) } -/// Readiness of the Kitty stable-fit fast path for an inline image at a given -/// placeholder geometry. +/// Readiness of the stable-fit path for an inline image at a given placeholder +/// geometry. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum InlineFitReadiness { - /// Scale + transmit state already exists for this geometry; drawing is a - /// cheap placeholder re-address. + /// Protocol state (Kitty) or a pre-scaled decoded source (other protocols) + /// already exists for this geometry. Ready, - /// Kitty is active but the scaled/encoded state is missing. Building it on - /// the UI thread costs tens of milliseconds for a large screenshot, so - /// callers should prewarm it off-thread via [`prewarm_inline_fit_state`]. + /// The scaled state is missing. Building it on the UI thread costs tens of + /// milliseconds for a large screenshot, so callers should prewarm it + /// off-thread via [`prewarm_inline_fit_state`]. NeedsPrewarm, - /// The stable-fit fast path does not apply (no picker, non-Kitty protocol, - /// or video export); callers should use the regular synchronous path. + /// The stable-fit fast path does not apply (no picker or video export). Unsupported, } -/// Check whether the Kitty stable-fit state for `hash` is ready at the -/// `(target_cols, target_rows)` placeholder geometry. Cheap: a couple of mutex -/// lookups, no decoding and no filesystem reads beyond a cache-path stat. +/// Check whether stable-fit state for `hash` is ready at the +/// `(target_cols, target_rows)` placeholder geometry. Cheap: bounded in-memory +/// cache probes only, with no decoding or filesystem access. pub fn inline_fit_readiness( hash: u64, target_cols: u16, @@ -724,9 +880,6 @@ pub fn inline_fit_readiness( Some(picker) => picker, None => return InlineFitReadiness::Unsupported, }; - if picker.protocol_type() != ProtocolType::Kitty { - return InlineFitReadiness::Unsupported; - } // Hot path: runs on the UI thread for every visible and prefetched image, // every frame. Use the in-memory-only cache lookup so steady-state scrolling // never pays a `path.exists()` stat syscall per image per frame. @@ -735,25 +888,36 @@ pub fn inline_fit_readiness( }; let border_width = if draw_border { BORDER_WIDTH } else { 0 }; let fit_cols = target_cols.saturating_sub(border_width); - if probe_kitty_fit_state( - hash, - &cached.path, - fit_cols, - target_rows, - picker.font_size(), - ) - .is_some() - { + let ready = if picker.protocol_type() == ProtocolType::Kitty { + probe_kitty_fit_state( + hash, + &cached.path, + fit_cols, + target_rows, + picker.font_size(), + ) + .is_some() + } else { + fitted_source_is_ready( + hash, + &cached.path, + fit_cols, + target_rows, + picker.font_size(), + ) + }; + if ready { InlineFitReadiness::Ready } else { InlineFitReadiness::NeedsPrewarm } } -/// Build (decode + scale + escape-encode) the Kitty stable-fit state for an -/// inline image, mirroring the geometry math of -/// [`render_image_widget_fit_stable`]. Intended to run off the UI thread so the -/// first visible frame of a large image does not hitch the render loop. +/// Build the protocol-specific stable-fit state for an inline image, mirroring +/// the geometry math of [`render_image_widget_fit_stable`]. Intended to run off +/// the UI thread so the first visible frame of a large image does not hitch the +/// render loop. Kitty builds its transmit state; other protocols retain a +/// byte-bounded pre-scaled source for cheap visible-row crops. /// Returns true when the state exists afterwards. pub fn prewarm_inline_fit_state( hash: u64, @@ -765,22 +929,22 @@ pub fn prewarm_inline_fit_state( Some(picker) => picker, None => return false, }; - if picker.protocol_type() != ProtocolType::Kitty { - return false; - } let Some(cached) = get_cached_diagram(hash, None) else { return false; }; let font_size = picker.font_size(); let border_width = if draw_border { BORDER_WIDTH } else { 0 }; let fit_cols = target_cols.saturating_sub(border_width); + if picker.protocol_type() != ProtocolType::Kitty { + return load_fitted_source(hash, &cached.path, fit_cols, target_rows, font_size).is_some(); + } if probe_kitty_fit_state(hash, &cached.path, fit_cols, target_rows, font_size).is_some() { return true; } let Some(source) = load_source_image(hash, &cached.path) else { return false; }; - ensure_kitty_fit_state( + let prepared = ensure_kitty_fit_state( hash, &cached.path, source.as_ref(), @@ -788,7 +952,10 @@ pub fn prewarm_inline_fit_state( target_rows, font_size, ) - .is_some() + .is_some(); + drop(source); + release_source_image(hash, &cached.path); + prepared } /// Draw a rounded left border that hugs the image's real extent: `╭` on the @@ -818,6 +985,172 @@ fn draw_fitted_left_border(buf: &mut Buffer, area: Rect, skip_rows: u16, full_ro } } +/// Convert a row scroll over a fitted image into an exact source-pixel crop and +/// visible cell extent. Kept protocol-independent so the geometry can be tested +/// without a terminal. +pub(super) fn fitted_scroll_crop( + image_width: u32, + image_height: u32, + font_size: (u16, u16), + available_cols: u16, + available_rows: u16, + skip_rows: u16, +) -> Option<(u32, u32, u16, u16)> { + if image_width == 0 || image_height == 0 || available_cols == 0 || available_rows == 0 { + return None; + } + let font_w = font_size.0.max(1) as u32; + let font_h = font_size.1.max(1) as u32; + let full_cols = div_ceil_u32_local(image_width, font_w).min(u16::MAX as u32) as u16; + let full_rows = div_ceil_u32_local(image_height, font_h).min(u16::MAX as u32) as u16; + if skip_rows >= full_rows { + return None; + } + let visible_cols = available_cols.min(full_cols); + let visible_rows = available_rows.min(full_rows.saturating_sub(skip_rows)); + if visible_cols == 0 || visible_rows == 0 { + return None; + } + let scroll_y_px = (skip_rows as u32).saturating_mul(font_h).min(image_height); + let crop_h_px = (visible_rows as u32) + .saturating_mul(font_h) + .min(image_height.saturating_sub(scroll_y_px)); + (crop_h_px > 0).then_some((scroll_y_px, crop_h_px, visible_cols, visible_rows)) +} + +#[allow(clippy::too_many_arguments)] +fn render_non_kitty_fit_stable( + hash: u64, + area: Rect, + buf: &mut Buffer, + target_cols: u16, + target_rows: u16, + skip_rows: u16, + centered: bool, + draw_border: bool, + picker: &Picker, + cached: CachedDiagram, +) -> bool { + let border_width = if draw_border { BORDER_WIDTH } else { 0 }; + let fit_cols = target_cols.saturating_sub(border_width); + let font_size = picker.font_size(); + let source_path = cached.path; + let Some(source) = load_fitted_source(hash, &source_path, fit_cols, target_rows, font_size) + else { + return false; + }; + let (full_cols, full_rows) = cell_rect_for_image(source.as_ref(), font_size); + + let mut available = Rect { + x: area.x + border_width, + y: area.y, + width: area.width - border_width, + height: area.height, + }; + let Some((scroll_y_px, crop_h_px, visible_cols, visible_rows)) = fitted_scroll_crop( + source.width(), + source.height(), + font_size, + available.width, + available.height, + skip_rows, + ) else { + clear_image_area(area, buf); + return true; + }; + + if draw_border { + draw_fitted_left_border(buf, area, skip_rows, full_rows); + } + if centered && full_cols < available.width { + available.x += (available.width - full_cols) / 2; + } + let render_area = Rect { + x: available.x, + y: available.y, + width: visible_cols, + height: visible_rows, + }; + let viewport = ViewportState { + scroll_x_px: 0, + scroll_y_px, + view_w_px: source.width(), + view_h_px: crop_h_px, + }; + + { + let mut state = IMAGE_STATE + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let needs_reset = state + .get(&hash) + .map(|state| { + state.resize_mode != ResizeMode::FitViewport + || state.source_path.as_path() != source_path.as_path() + }) + .unwrap_or(false); + if needs_reset { + state.remove(&hash); + } + if let Some(image_state) = state.get_mut(hash) + && image_state.last_viewport == Some(viewport) + { + if let Ok(mut debug) = MERMAID_DEBUG.lock() { + debug.stats.image_state_hits += 1; + debug.stats.fit_state_reuse_hits += 1; + } + if !render_stateful_image_safe( + hash, + render_area, + buf, + &mut image_state.protocol, + Resize::Fit(None), + ) { + return false; + } + image_state.last_area = Some(render_area); + return true; + } + } + + // Non-Kitty protocols must encode a new payload when the visible slice + // changes, but the expensive full-image decode and scale have already been + // completed off-thread and retained in FITTED_SOURCE_CACHE. + let cropped = source.crop_imm(0, scroll_y_px, source.width(), crop_h_px); + let source_bytes = cropped.as_bytes().len(); + let protocol = picker.new_resize_protocol(cropped); + if let Ok(mut debug) = MERMAID_DEBUG.lock() { + debug.stats.image_state_misses += 1; + debug.stats.fit_protocol_rebuilds += 1; + } + + let mut state = IMAGE_STATE + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + state.insert( + hash, + ImageState { + protocol, + source_path, + source_bytes, + last_area: Some(render_area), + resize_mode: ResizeMode::FitViewport, + last_crop_top: false, + last_viewport: Some(viewport), + }, + ); + let Some(image_state) = state.get_mut(hash) else { + return false; + }; + render_stateful_image_safe( + hash, + render_area, + buf, + &mut image_state.protocol, + Resize::Fit(None), + ) +} + /// Render an inline raster image scaled-to-fit a fixed placeholder box, with /// stable pixels while scrolling. /// @@ -825,9 +1158,9 @@ fn draw_fitted_left_border(buf: &mut Buffer, area: Rect, skip_rows: u16, full_ro /// at prepare time; `skip_rows` is how many of the image's top rows are /// scrolled off-screen. On Kitty this reuses one transmitted image and just /// re-addresses rows via unicode placeholders, so partial visibility never -/// rescales or retransmits. Returns true when handled; callers should fall -/// back to `render_image_widget_fit` when it returns false (non-Kitty -/// protocols or oversized images). +/// rescales or retransmits. Other protocols reuse a pre-scaled source and only +/// crop/re-encode when the visible row slice changes. Returns true when handled; +/// callers should fall back to `render_image_widget_fit` on an actual failure. #[allow(clippy::too_many_arguments)] pub fn render_image_widget_fit_stable( hash: u64, @@ -858,10 +1191,6 @@ pub fn render_image_widget_fit_stable( Some(picker) => picker, None => return false, }; - if picker.protocol_type() != ProtocolType::Kitty { - return false; - } - // Hot path: runs on the UI thread for every visible image every frame. // The in-memory-only lookup avoids a per-frame `path.exists()` stat; a // genuinely missing file degrades gracefully below when the source decode @@ -870,6 +1199,20 @@ pub fn render_image_widget_fit_stable( Some(cached) => cached, None => return false, }; + if picker.protocol_type() != ProtocolType::Kitty { + return render_non_kitty_fit_stable( + hash, + area, + buf, + target_cols, + target_rows, + skip_rows, + centered, + draw_border, + picker, + cached, + ); + } let source_path = cached.path; let font_size = picker.font_size(); let fit_cols = target_cols.saturating_sub(border_width); @@ -879,14 +1222,17 @@ pub fn render_image_widget_fit_stable( let placement = probe_kitty_fit_state(hash, &source_path, fit_cols, target_rows, font_size) .or_else(|| { let source = load_source_image(hash, &source_path)?; - ensure_kitty_fit_state( + let placement = ensure_kitty_fit_state( hash, &source_path, source.as_ref(), fit_cols, target_rows, font_size, - ) + ); + drop(source); + release_source_image(hash, &source_path); + placement }); let Some((_, full_cols, full_rows)) = placement else { return false; @@ -1006,7 +1352,8 @@ pub fn render_image_widget_viewport_precise( None => return 0, }; - let cached = match get_cached_diagram(hash, None) { + let cached = match get_cached_diagram_in_memory(hash).or_else(|| get_cached_diagram(hash, None)) + { Some(cached) => cached, None => return 0, }; @@ -1148,6 +1495,7 @@ pub fn render_image_widget_viewport_precise( if let Ok(mut dbg) = MERMAID_DEBUG.lock() { dbg.stats.viewport_protocol_rebuilds += 1; } + let source_bytes = cropped.as_bytes().len(); let protocol = picker.new_resize_protocol(cropped); let mut state = IMAGE_STATE @@ -1158,6 +1506,7 @@ pub fn render_image_widget_viewport_precise( ImageState { protocol, source_path, + source_bytes, last_area: Some(image_area), resize_mode: ResizeMode::Viewport, last_crop_top: false, @@ -1244,6 +1593,255 @@ mod kitty_viewport_leak_tests { ); } + #[test] + fn kitty_ids_are_process_unique_and_delete_payload_targets_one_image() { + let first = kitty_viewport_unique_id(0x0000_0001_FFFF_FFFF); + let second = kitty_viewport_unique_id(0xFFFF_FFFF_0000_0001); + assert_ne!( + first, second, + "64-bit hash folds must never alias Kitty ids" + ); + + let delete = kitty_delete_image_payload(first); + assert!(delete.contains("a=d,d=I")); + assert!(delete.contains(&format!("i={first}"))); + } + + #[test] + fn kitty_cache_eviction_queues_terminal_image_deletion() { + let _guard = crate::IMAGE_TEST_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let _ = take_kitty_delete_ids(); + let mut cache = KittyViewportCache::new(); + for index in 0..KITTY_VIEWPORT_STATE_MAX { + cache.insert( + index as u64, + KittyViewportState { + source_path: PathBuf::from(format!("/test/evict-{index}.png")), + zoom_percent: 100, + font_size: (8, 16), + unique_id: 10_000 + index as u32, + full_cols: 10, + full_rows: 10, + pending_transmit: None, + pending_transmit_bytes: 0, + fit_target: Some((10, 10)), + }, + ); + } + cache.get_mut(0).expect("touch oldest state"); + let index = KITTY_VIEWPORT_STATE_MAX; + cache.insert( + index as u64, + KittyViewportState { + source_path: PathBuf::from(format!("/test/evict-{index}.png")), + zoom_percent: 100, + font_size: (8, 16), + unique_id: 10_000 + index as u32, + full_cols: 10, + full_rows: 10, + pending_transmit: None, + pending_transmit_bytes: 0, + fit_target: Some((10, 10)), + }, + ); + assert_eq!(cache.entries.len(), KITTY_VIEWPORT_STATE_MAX); + assert!( + cache.entries.contains_key(&0), + "recently touched state stays hot" + ); + assert!( + !cache.entries.contains_key(&1), + "least-recent state is evicted" + ); + assert_eq!(take_kitty_delete_ids(), vec![10_001]); + } + + #[test] + fn kitty_cache_bounds_not_yet_drawn_transmissions_by_bytes() { + let _guard = crate::IMAGE_TEST_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let _ = take_kitty_delete_ids(); + let mut cache = KittyViewportCache::new(); + let accounted = KITTY_VIEWPORT_PENDING_MAX_BYTES / 2 + 1; + for index in 0..3u64 { + cache.insert( + index, + KittyViewportState { + source_path: PathBuf::from(format!("/test/pending-{index}.png")), + zoom_percent: 100, + font_size: (8, 16), + unique_id: 20_000 + index as u32, + full_cols: 10, + full_rows: 10, + pending_transmit: Some(String::from("synthetic")), + pending_transmit_bytes: accounted, + fit_target: Some((10, 10)), + }, + ); + } + assert_eq!( + cache.entries.len(), + 1, + "byte budget should evict old prewarms" + ); + assert_eq!(cache.total_pending_transmit_bytes, accounted); + assert!( + take_kitty_delete_ids().is_empty(), + "never-drawn prewarms have no terminal allocation to delete" + ); + + let (_, pending) = cache.take_pending_transmit(2).expect("newest state"); + assert_eq!(pending.as_deref(), Some("synthetic")); + assert_eq!(cache.total_pending_transmit_bytes, 0); + } + + #[test] + fn pending_terminal_cleanup_renders_without_changing_visible_cell_text() { + let _guard = crate::IMAGE_TEST_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let _ = take_kitty_delete_ids(); + queue_kitty_delete(42_424); + let mut buf = Buffer::empty(Rect::new(0, 0, 1, 1)); + buf.cell_mut((0, 0)).expect("fixture cell").set_symbol("X"); + + assert!(crate::render_pending_terminal_image_cleanup(&mut buf)); + let symbol = buf.cell((0, 0)).expect("cleanup cell").symbol(); + assert!(symbol.contains("a=d,d=I,i=42424")); + assert!(symbol.ends_with('X'), "visible symbol must be preserved"); + assert!(take_kitty_delete_ids().is_empty()); + } + + #[test] + fn unchanged_png_transmit_reuses_exact_file_bytes() { + use image::ImageEncoder as _; + + let pixels = image::RgbaImage::from_pixel(7, 5, image::Rgba([9, 71, 203, 255])); + let mut png = Vec::new(); + image::codecs::png::PngEncoder::new(&mut png) + .write_image(pixels.as_raw(), 7, 5, image::ExtendedColorType::Rgba8) + .expect("encode fixture"); + let path = std::env::temp_dir().join(format!( + "jcode-kitty-source-{}-{}.png", + std::process::id(), + std::thread::current().name().unwrap_or("test") + )); + fs::write(&path, &png).expect("write fixture"); + + let actual = kitty_transmit_png_path(&path, 19).expect("direct PNG transmit"); + let expected = kitty_transmit_payload(&png, 19, "f=100"); + assert_eq!(actual, expected, "source PNG bytes must not be re-encoded"); + + let _ = fs::remove_file(path); + } + + #[test] + fn halfblocks_partial_scroll_reuses_fitted_source_and_identical_slice() { + let _guard = crate::IMAGE_TEST_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + const HASH: u64 = 0x4841_4C46_424C_4B53; + let path = PathBuf::from("/test/halfblocks-scroll.png"); + let source = DynamicImage::ImageRgba8(image::ImageBuffer::from_pixel( + 160, + 128, + image::Rgba([31, 127, 223, 255]), + )); + SOURCE_CACHE + .lock() + .unwrap() + .insert(HASH, path.clone(), source); + FITTED_SOURCE_CACHE.lock().unwrap().remove_hash(HASH); + IMAGE_STATE.lock().unwrap().remove(&HASH); + + #[allow(deprecated)] + let mut picker = Picker::from_fontsize((2, 4)); + picker.set_protocol_type(ProtocolType::Halfblocks); + let cached = CachedDiagram { + path: path.clone(), + width: 160, + height: 128, + }; + let area = Rect::new(0, 0, 42, 5); + let mut buf = Buffer::empty(Rect::new(0, 0, 42, 16)); + let stats_before = debug_stats(); + + assert!(render_non_kitty_fit_stable( + HASH, + area, + &mut buf, + 42, + 16, + 4, + false, + true, + &picker, + cached.clone(), + )); + let first_stats = debug_stats(); + assert_eq!( + first_stats + .fit_protocol_rebuilds + .saturating_sub(stats_before.fit_protocol_rebuilds), + 1, + "the first visible slice should build one protocol" + ); + { + let state = IMAGE_STATE.lock().unwrap(); + let state = state.get(&HASH).expect("halfblocks viewport state"); + assert_eq!(state.resize_mode, ResizeMode::FitViewport); + assert_eq!( + state.last_viewport, + Some(ViewportState { + scroll_x_px: 0, + scroll_y_px: 16, + view_w_px: 80, + view_h_px: 20, + }) + ); + } + assert!( + SOURCE_CACHE.lock().unwrap().entries.get(&HASH).is_none(), + "full decoded original should be released after fitting" + ); + assert_eq!( + FITTED_SOURCE_CACHE + .lock() + .unwrap() + .entries + .keys() + .filter(|key| key.hash == HASH) + .count(), + 1 + ); + + // Rendering the identical scroll slice must reuse protocol state rather + // than recropping/re-encoding it. + assert!(render_non_kitty_fit_stable( + HASH, area, &mut buf, 42, 16, 4, false, true, &picker, cached, + )); + let second_stats = debug_stats(); + assert_eq!( + second_stats + .fit_protocol_rebuilds + .saturating_sub(first_stats.fit_protocol_rebuilds), + 0 + ); + assert_eq!( + second_stats + .fit_state_reuse_hits + .saturating_sub(first_stats.fit_state_reuse_hits), + 1 + ); + + IMAGE_STATE.lock().unwrap().remove(&HASH); + SOURCE_CACHE.lock().unwrap().remove(HASH); + FITTED_SOURCE_CACHE.lock().unwrap().remove_hash(HASH); + } + /// Seed `KITTY_VIEWPORT_STATE` with a fit entry so the emitter has an id to /// address without needing a real terminal/transmit. fn seed_state(hash: u64, full_cols: u16, full_rows: u16) { @@ -1258,6 +1856,7 @@ mod kitty_viewport_leak_tests { full_cols, full_rows, pending_transmit: Some(String::from("\x1b_Gtransmit\x1b\\")), + pending_transmit_bytes: "\x1b_Gtransmit\x1b\\".len(), fit_target: Some((full_cols, full_rows)), }, ); @@ -1277,6 +1876,9 @@ mod kitty_viewport_leak_tests { /// rows above the image area that stand in for the label/tag line. #[test] fn placeholders_never_leak_above_image_area() { + let _guard = crate::IMAGE_TEST_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); let hash = 0xDEAD_BEEF_u64; let full_cols = 20; let full_rows = 30; diff --git a/crates/jcode-tui-mermaid/src/mermaid_widget.rs b/crates/jcode-tui-mermaid/src/mermaid_widget.rs index bfc851747d..df3e398a4e 100644 --- a/crates/jcode-tui-mermaid/src/mermaid_widget.rs +++ b/crates/jcode-tui-mermaid/src/mermaid_widget.rs @@ -124,7 +124,7 @@ pub fn render_image_widget( .get() .and_then(|p| p.as_ref()) .map(|picker| image_area.width as u32 * picker.font_size().0 as u32); - let cached = get_cached_diagram(hash, min_cached_width); + let cached = get_cached_diagram_prefer_width(hash, min_cached_width); let (img_width, path) = if let Some(cached) = cached { (cached.width, Some(cached.path)) } else { @@ -239,6 +239,7 @@ pub fn render_image_widget( if let Ok(mut dbg) = MERMAID_DEBUG.lock() { dbg.stats.image_state_misses += 1; } + let source_bytes = img.as_bytes().len(); let protocol = picker.new_resize_protocol(img); let mut state = IMAGE_STATE @@ -249,6 +250,7 @@ pub fn render_image_widget( ImageState { protocol, source_path: path.clone(), + source_bytes, last_area: Some(render_area), resize_mode: ResizeMode::Crop, last_crop_top: false, @@ -352,7 +354,7 @@ fn render_image_widget_fit_inner( .and_then(|p| p.as_ref()) .map(|picker| image_area.width as u32 * picker.font_size().0 as u32) }; - let cached = get_cached_diagram(hash, min_cached_width); + let cached = get_cached_diagram_prefer_width(hash, min_cached_width); let (img_width, path) = if let Some(cached) = cached { (cached.width, Some(cached.path)) } else { @@ -464,6 +466,7 @@ fn render_image_widget_fit_inner( } else { Resize::Fit(None) }; + let source_bytes = img.as_bytes().len(); let protocol = picker.new_resize_protocol(img); let mut state = IMAGE_STATE @@ -474,6 +477,7 @@ fn render_image_widget_fit_inner( ImageState { protocol, source_path: path.clone(), + source_bytes, last_area: Some(render_area), resize_mode: target_mode, last_crop_top: false, diff --git a/crates/jcode-tui/src/tui/app/tests/scroll_copy_02/part_02.rs b/crates/jcode-tui/src/tui/app/tests/scroll_copy_02/part_02.rs index 50d205ffd1..73eb93edb4 100644 --- a/crates/jcode-tui/src/tui/app/tests/scroll_copy_02/part_02.rs +++ b/crates/jcode-tui/src/tui/app/tests/scroll_copy_02/part_02.rs @@ -120,7 +120,10 @@ fn test_alt_shift_i_toggles_inline_images_and_persists() { KeyCode::Char('I'), KeyModifiers::ALT | KeyModifiers::SHIFT, )); - assert!(app.inline_images_visible, "second toggle should show images"); + assert!( + app.inline_images_visible, + "second toggle should show images" + ); assert!(crate::tui::app::ui_prefs::inline_images_visible()); if let Some(prev_home) = prev_home { @@ -163,6 +166,40 @@ fn text_only_transcript_updates_keep_inline_image_signature_cached() { ); } +#[test] +fn inline_image_signature_distinguishes_labels_and_same_prefix_payloads() { + use std::hash::Hasher as _; + + let signature = |image: &crate::session::RenderedImage| { + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + crate::tui::hash_rendered_image_signature_fields(image, &mut hasher); + hasher.finish() + }; + let base = crate::session::RenderedImage { + media_type: "image/png".to_string(), + data: format!("{}tail-a", "A".repeat(128)), + label: Some("first.png".to_string()), + source: crate::session::RenderedImageSource::UserInput, + anchor: None, + }; + let mut changed_tail = base.clone(); + changed_tail.data = format!("{}tail-b", "A".repeat(128)); + let mut changed_label = base.clone(); + changed_label.label = Some("second.png".to_string()); + let middle_base = crate::session::RenderedImage { + data: format!("{}middle-a{}", "A".repeat(128), "Z".repeat(128)), + ..base.clone() + }; + let middle_changed = crate::session::RenderedImage { + data: format!("{}middle-b{}", "A".repeat(128), "Z".repeat(128)), + ..middle_base.clone() + }; + + assert_ne!(signature(&base), signature(&changed_tail)); + assert_ne!(signature(&base), signature(&changed_label)); + assert_ne!(signature(&middle_base), signature(&middle_changed)); +} + #[test] fn test_alt_shift_i_is_inert_without_inline_images() { let _render_lock = scroll_render_test_lock(); @@ -225,7 +262,9 @@ fn make_edit_badge_test_app( "old_string": old_string, "new_string": new_string, }), - intent: None, thought_signature: None, }, + intent: None, + thought_signature: None, + }, ), ]; app.bump_display_messages_version(); @@ -429,7 +468,9 @@ fn test_expand_badge_shortcut_opens_full_inline_from_non_inline_mode() { "old_string": "old line\n", "new_string": "new line\n", }), - intent: None, thought_signature: None, }, + intent: None, + thought_signature: None, + }, )); app.bump_display_messages_version(); app.diff_mode = crate::config::DiffDisplayMode::Off; @@ -458,7 +499,9 @@ fn test_expand_badge_shortcut_uses_display_messages_when_edit_count_is_stale() { "old_string": "old line\n", "new_string": "new line\n", }), - intent: None, thought_signature: None, }, + intent: None, + thought_signature: None, + }, )); app.bump_display_messages_version(); app.diff_mode = crate::config::DiffDisplayMode::Off; @@ -705,7 +748,8 @@ fn test_click_on_inline_image_label_line_cycles_level() { "expected a Fit image region anchored under the label line" ); - let prepared = std::sync::Arc::new(PreparedChatFrame::from_single(std::sync::Arc::new(section))); + let prepared = + std::sync::Arc::new(PreparedChatFrame::from_single(std::sync::Arc::new(section))); let visible_end = prepared.wrapped_plain_line_count(); let content_area = Rect::new(0, 0, chat_width, visible_end as u16 + 1); @@ -863,8 +907,8 @@ const REPRO_TINY_PNG_B64: &str = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAA /// path actually used in production, not the isolated `build_section` helper. #[test] fn test_real_draw_click_on_body_anchored_image_label_cycles_level() { - use crate::tui::ui::inline_image_ui::ImageExpandLevel; use crate::message::{ContentBlock, Role}; + use crate::tui::ui::inline_image_ui::ImageExpandLevel; let _render_lock = scroll_render_test_lock(); let mut app = create_test_app(); @@ -978,8 +1022,7 @@ fn test_real_draw_click_on_body_anchored_image_label_cycles_level() { } } } - let (badge_col, badge_row) = - badge.expect("image label cell should be visible in the frame"); + let (badge_col, badge_row) = badge.expect("image label cell should be visible in the frame"); assert_eq!( app.image_expand_level(image_id), diff --git a/crates/jcode-tui/src/tui/app/tui_state.rs b/crates/jcode-tui/src/tui/app/tui_state.rs index e5ec39aa01..a0eb4b9c78 100644 --- a/crates/jcode-tui/src/tui/app/tui_state.rs +++ b/crates/jcode-tui/src/tui/app/tui_state.rs @@ -560,19 +560,11 @@ impl crate::tui::TuiState for App { if let Some(signature) = self.side_pane_images_signature_cache.get() { return signature; } - use std::hash::{Hash, Hasher}; + use std::hash::Hasher; let images = self.side_pane_images(); let mut hasher = std::collections::hash_map::DefaultHasher::new(); for image in &images { - image.media_type.hash(&mut hasher); - image.data.len().hash(&mut hasher); - image - .data - .as_bytes() - .iter() - .take(64) - .for_each(|b| b.hash(&mut hasher)); - crate::tui::hash_rendered_image_anchor(image.anchor.as_ref(), &mut hasher); + crate::tui::hash_rendered_image_signature_fields(image, &mut hasher); } let signature = (images.len(), hasher.finish()); self.side_pane_images_signature_cache.set(Some(signature)); diff --git a/crates/jcode-tui/src/tui/mermaid.rs b/crates/jcode-tui/src/tui/mermaid.rs index bf7997ab49..c26e01c2a4 100644 --- a/crates/jcode-tui/src/tui/mermaid.rs +++ b/crates/jcode-tui/src/tui/mermaid.rs @@ -24,12 +24,12 @@ pub use jcode_tui_mermaid::{ render_image_widget_fit_stable, render_image_widget_scale, render_image_widget_viewport, render_image_widget_viewport_precise, render_mermaid, render_mermaid_deferred, render_mermaid_deferred_with_registration, render_mermaid_deferred_with_stream_scope, - render_mermaid_sized, render_mermaid_untracked, reset_debug_stats, restore_active_diagrams, - result_to_content, result_to_lines, set_log_hooks, set_memory_snapshot_hook, - set_render_completed_hook, set_streaming_preview_diagram, set_video_export_mode, - snapshot_active_diagrams, transcript_preferred_aspect_ratio, - transcript_preferred_aspect_ratio_with_font, with_image_protocol_override, - with_preferred_aspect_ratio, write_video_export_marker, + render_mermaid_sized, render_mermaid_untracked, render_pending_terminal_image_cleanup, + reset_debug_stats, restore_active_diagrams, result_to_content, result_to_lines, set_log_hooks, + set_memory_snapshot_hook, set_render_completed_hook, set_streaming_preview_diagram, + set_video_export_mode, snapshot_active_diagrams, take_terminal_image_cleanup_payload, + transcript_preferred_aspect_ratio, transcript_preferred_aspect_ratio_with_font, + with_image_protocol_override, with_preferred_aspect_ratio, write_video_export_marker, }; pub use jcode_tui_mermaid::{ImageScrollBenchmark, cache_stat_syscalls}; diff --git a/crates/jcode-tui/src/tui/mod.rs b/crates/jcode-tui/src/tui/mod.rs index 7a9e52c4c6..fc5657481b 100644 --- a/crates/jcode-tui/src/tui/mod.rs +++ b/crates/jcode-tui/src/tui/mod.rs @@ -138,6 +138,22 @@ pub(crate) fn hash_rendered_image_anchor( } } +/// Hash every field that affects inline image rendering. The production App +/// memoizes this signature until its image set changes, so exact payload hashing +/// happens on image updates rather than during scrolling. Correctness matters +/// here: sampling can miss a same-length change and reuse a stale prepared frame. +pub(crate) fn hash_rendered_image_signature_fields( + image: &crate::session::RenderedImage, + hasher: &mut impl std::hash::Hasher, +) { + use std::hash::Hash; + + image.media_type.hash(hasher); + image.data.hash(hasher); + image.label.hash(hasher); + hash_rendered_image_anchor(image.anchor.as_ref(), hasher); +} + /// Trait for TUI state consumed by the shared renderer. /// /// This is a wide (114-method) presentation interface: the read-only surface the @@ -163,22 +179,11 @@ pub trait TuiState { /// The default implementation derives it from `side_pane_images`; overrides /// can provide a cheaper path. fn side_pane_images_signature(&self) -> (usize, u64) { - use std::hash::{Hash, Hasher}; + use std::hash::Hasher; let images = self.side_pane_images(); let mut hasher = std::collections::hash_map::DefaultHasher::new(); for image in &images { - image.media_type.hash(&mut hasher); - image.data.len().hash(&mut hasher); - // A short prefix is enough to distinguish distinct payloads cheaply. - image - .data - .as_bytes() - .iter() - .take(64) - .for_each(|b| b.hash(&mut hasher)); - // The anchor determines where the image renders in the transcript, - // so anchor changes must invalidate prepared frames too. - hash_rendered_image_anchor(image.anchor.as_ref(), &mut hasher); + hash_rendered_image_signature_fields(image, &mut hasher); } (images.len(), hasher.finish()) } diff --git a/crates/jcode-tui/src/tui/ui.rs b/crates/jcode-tui/src/tui/ui.rs index 7c159f6ec8..6f4349b083 100644 --- a/crates/jcode-tui/src/tui/ui.rs +++ b/crates/jcode-tui/src/tui/ui.rs @@ -2445,6 +2445,10 @@ pub fn draw(frame: &mut Frame, app: &dyn TuiState) { // Doing this at the buffer level covers every widget and overlay without // touching individual color call sites. jcode_tui_style::adapt_buffer_for_theme(frame.buffer_mut()); + // Cache eviction/clearing can outlive the last visible image. Carry Kitty + // deletion commands on any completed frame so terminal-side pixel storage + // is reclaimed even when no image widget renders again. + crate::tui::mermaid::render_pending_terminal_image_cleanup(frame.buffer_mut()); } fn draw_inner(frame: &mut Frame, app: &dyn TuiState) { diff --git a/crates/jcode-tui/src/tui/ui_inline_image.rs b/crates/jcode-tui/src/tui/ui_inline_image.rs index 6e5d2c42d0..8b94419755 100644 --- a/crates/jcode-tui/src/tui/ui_inline_image.rs +++ b/crates/jcode-tui/src/tui/ui_inline_image.rs @@ -21,6 +21,7 @@ use jcode_tui_messages::{ImageRegion, ImageRegionRender, PreparedMessages}; use ratatui::style::{Modifier, Style}; use ratatui::text::{Line, Span}; use std::collections::{HashMap, HashSet}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{LazyLock, Mutex, OnceLock, mpsc}; /// One image to render inline, resolved from a `RenderedImage`. @@ -115,6 +116,16 @@ impl ImageExpandLevels for AppExpandLevels<'_> { static PAYLOAD_REGISTRY: LazyLock> = LazyLock::new(|| Mutex::new(PayloadRegistry::new())); +/// Payloads can be dropped when images are hidden or when the staging byte +/// budget is exceeded. Prepared frames intentionally retain only image ids, so +/// a later draw asks the next prepare pass to restage the missing source from the +/// App's canonical image list instead of leaving a cold image blank forever. +static PAYLOAD_RESTAGE_IDS: LazyLock>> = + LazyLock::new(|| Mutex::new(HashSet::new())); +static PAYLOAD_RESTAGE_PENDING: AtomicBool = AtomicBool::new(false); +static PAYLOAD_RESTAGE_ALL: AtomicBool = AtomicBool::new(false); +const PAYLOAD_RESTAGE_MAX: usize = 512; + const PAYLOAD_REGISTRY_MAX: usize = 512; /// Byte budget for the payload registry. Entries hold the *full base64 /// payload* (a 5 MB screenshot is ~6.7 MB of base64), so a pure entry-count @@ -182,6 +193,12 @@ impl PayloadRegistry { } } } + + fn clear(&mut self) { + self.map.clear(); + self.order.clear(); + self.total_bytes = 0; + } } /// Record an image payload so [`materialize_visible`] can decode it on demand. @@ -191,6 +208,10 @@ impl PayloadRegistry { /// the base64 copy again would just hold multi-megabyte payloads resident /// twice. [`materialize_visible`] rediscovers evicted entries from disk. pub(crate) fn register_payload(id: u64, media_type: &str, data_b64: &str) { + if let Ok(mut requested) = PAYLOAD_RESTAGE_IDS.lock() { + requested.remove(&id); + PAYLOAD_RESTAGE_PENDING.store(!requested.is_empty(), Ordering::Release); + } if mermaid::inline_image_is_materialized(id) { return; } @@ -200,8 +221,13 @@ pub(crate) fn register_payload(id: u64, media_type: &str, data_b64: &str) { }; // A fresh payload may succeed where a previously evicted/corrupt one // failed, so give the prewarm pipeline its retries back. - if newly_inserted && let Ok(mut failures) = PREWARM_FAILURES.lock() { - failures.remove(&id); + if newly_inserted { + // The id hashes the complete payload, so re-registering the same id is + // not fresh content. Keep ID-wide decode failures capped across staging + // eviction; geometry preparation may still be retried. + if let Ok(mut failures) = PREWARM_FIT_FAILURES.lock() { + failures.retain(|req, _| req.id != id); + } } } @@ -213,6 +239,82 @@ fn release_payload(id: u64) { } } +fn clear_staged_payloads() { + let mut cleared = false; + if let Ok(mut reg) = PAYLOAD_REGISTRY.lock() { + cleared |= !reg.map.is_empty(); + reg.clear(); + } + // `pin_images == false` returns before computing a replacement cache key. + // Without invalidating this cache, re-enabling images can hit an old + // stage_payloads=true result even though its payload registry was cleared, + // permanently leaving unmaterialized images with nothing to decode. + if let Ok(mut cache) = ANCHORED_CACHE.lock() { + cleared |= cache.take().is_some(); + } + if cleared { + PAYLOAD_RESTAGE_ALL.store(true, Ordering::Release); + } +} + +fn request_payload_restage(id: u64) { + let mut newly_requested = false; + if let Ok(mut requested) = PAYLOAD_RESTAGE_IDS.lock() { + if requested.len() >= PAYLOAD_RESTAGE_MAX && !requested.contains(&id) { + requested.clear(); + PAYLOAD_RESTAGE_ALL.store(true, Ordering::Release); + PAYLOAD_RESTAGE_PENDING.store(false, Ordering::Release); + newly_requested = true; + } else { + newly_requested = requested.insert(id); + PAYLOAD_RESTAGE_PENDING.store(true, Ordering::Release); + } + } + if newly_requested { + // Materialization normally happens on the worker. Wake the UI so the + // next prepare pass can recover the evicted source from App state. + crate::bus::Bus::global().publish(crate::bus::BusEvent::MermaidRenderCompleted); + } +} + +/// Rare recovery path for payloads removed by the staging byte budget or an +/// image visibility toggle. The steady-state fast path is one relaxed atomic +/// load; only an actual miss clones/scans the App's image list. +pub(crate) fn restage_requested_payloads(app: &dyn crate::tui::TuiState) { + if !app.pin_images() || !app.inline_images_visible() { + return; + } + let restage_all = PAYLOAD_RESTAGE_ALL.swap(false, Ordering::AcqRel); + if !restage_all && !PAYLOAD_RESTAGE_PENDING.load(Ordering::Acquire) { + return; + } + let requested = if restage_all { + HashSet::new() + } else { + PAYLOAD_RESTAGE_IDS + .lock() + .map(|ids| ids.clone()) + .unwrap_or_default() + }; + if !restage_all && requested.is_empty() { + PAYLOAD_RESTAGE_PENDING.store(false, Ordering::Release); + return; + } + + let mut restored = HashSet::new(); + for image in app.side_pane_images() { + let id = mermaid::inline_image_id(&image.media_type, &image.data); + if restage_all || requested.contains(&id) { + register_payload(id, &image.media_type, &image.data); + restored.insert(id); + } + } + if let Ok(mut pending) = PAYLOAD_RESTAGE_IDS.lock() { + pending.retain(|id| !restored.contains(id)); + PAYLOAD_RESTAGE_PENDING.store(!pending.is_empty(), Ordering::Release); + } +} + /// Ensure the image with `id` is materialized (decoded + cached) so it can be /// drawn. Returns true on success. /// @@ -240,24 +342,36 @@ pub(crate) fn materialize_visible(id: u64) -> bool { if mermaid::rediscover_inline_image(id).is_some() { return true; } - mermaid::get_cached_path(id).is_some() + if mermaid::get_cached_path(id).is_some() { + return true; + } + request_payload_restage(id); + false } /// One pending prewarm request: build everything needed to draw image `id` /// at the given placeholder geometry (decode payload, write cache file, scale /// to the target box, escape-encode for Kitty). -#[derive(Clone, Copy, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] struct PrewarmRequest { id: u64, target_cols: u16, target_rows: u16, } -static PREWARM_TX: OnceLock> = OnceLock::new(); +const PREWARM_QUEUE_CAPACITY: usize = 32; +static PREWARM_TX: OnceLock> = OnceLock::new(); /// Requests queued or in flight, so a 60fps scroll doesn't enqueue the same -/// image dozens of times before the worker finishes it. -static PREWARM_INFLIGHT: LazyLock>> = +/// image dozens of times before the worker finishes it. Dedup by image id, not +/// exact geometry: during a live resize only one request per image may consume +/// CPU; completion triggers a repaint that queues the newest geometry if needed. +static PREWARM_INFLIGHT: LazyLock>> = LazyLock::new(|| Mutex::new(HashSet::new())); +/// A full bounded channel must not drop the newest visible request. Keep one +/// coalesced overflow request; the worker drains it after every queued job. +static PREWARM_OVERFLOW: LazyLock>> = + LazyLock::new(|| Mutex::new(None)); +static PREWARM_RETRY_NEEDED: AtomicBool = AtomicBool::new(false); /// Per-image count of failed materialize attempts. Without this memo an /// undecodable payload (or one evicted from the registry) would loop forever: @@ -265,23 +379,31 @@ static PREWARM_INFLIGHT: LazyLock>> = /// repaint reschedules the same prewarm. After /// [`PREWARM_FAILURE_MAX_ATTEMPTS`] failures the id is skipped until /// [`register_payload`] sees a fresh payload for it. -static PREWARM_FAILURES: LazyLock>> = +static PREWARM_MATERIALIZE_FAILURES: LazyLock>> = + LazyLock::new(|| Mutex::new(HashMap::new())); +static PREWARM_FIT_FAILURES: LazyLock>> = LazyLock::new(|| Mutex::new(HashMap::new())); const PREWARM_FAILURE_MAX_ATTEMPTS: u8 = 3; /// Bound the failure memo; it only holds pathological ids, so if it fills up /// something is systemically wrong and starting over is harmless. const PREWARM_FAILURES_MAX: usize = 512; -fn prewarm_failures_exhausted(id: u64) -> bool { - PREWARM_FAILURES +fn prewarm_failures_exhausted(req: PrewarmRequest) -> bool { + let materialize_exhausted = PREWARM_MATERIALIZE_FAILURES .lock() .ok() - .and_then(|failures| failures.get(&id).copied()) - .is_some_and(|count| count >= PREWARM_FAILURE_MAX_ATTEMPTS) + .and_then(|failures| failures.get(&req.id).copied()) + .is_some_and(|count| count >= PREWARM_FAILURE_MAX_ATTEMPTS); + materialize_exhausted + || PREWARM_FIT_FAILURES + .lock() + .ok() + .and_then(|failures| failures.get(&req).copied()) + .is_some_and(|count| count >= PREWARM_FAILURE_MAX_ATTEMPTS) } -fn record_prewarm_failure(id: u64) { - if let Ok(mut failures) = PREWARM_FAILURES.lock() { +fn record_materialize_failure(id: u64) { + if let Ok(mut failures) = PREWARM_MATERIALIZE_FAILURES.lock() { if failures.len() >= PREWARM_FAILURES_MAX && !failures.contains_key(&id) { failures.clear(); } @@ -289,16 +411,34 @@ fn record_prewarm_failure(id: u64) { *count = count.saturating_add(1); if *count == PREWARM_FAILURE_MAX_ATTEMPTS { crate::logging::warn(&format!( - "inline image {id:#018x} failed to materialize {PREWARM_FAILURE_MAX_ATTEMPTS} times; \ - suspending prewarm until its payload is re-registered" + "inline image {id:#018x} failed to decode/materialize {} times; \ + suspending all geometries until its payload is re-registered", + PREWARM_FAILURE_MAX_ATTEMPTS )); } } } -fn prewarm_sender() -> &'static mpsc::Sender { +fn record_fit_failure(req: PrewarmRequest) { + if let Ok(mut failures) = PREWARM_FIT_FAILURES.lock() { + if failures.len() >= PREWARM_FAILURES_MAX && !failures.contains_key(&req) { + failures.clear(); + } + let count = failures.entry(req).or_insert(0); + *count = count.saturating_add(1); + if *count == PREWARM_FAILURE_MAX_ATTEMPTS { + crate::logging::warn(&format!( + "inline image {:#018x} failed to fit at {}x{} {} times; \ + suspending this geometry until its payload is re-registered", + req.id, req.target_cols, req.target_rows, PREWARM_FAILURE_MAX_ATTEMPTS + )); + } + } +} + +fn prewarm_sender() -> &'static mpsc::SyncSender { PREWARM_TX.get_or_init(|| { - let (tx, rx) = mpsc::channel::(); + let (tx, rx) = mpsc::sync_channel::(PREWARM_QUEUE_CAPACITY); if let Err(err) = std::thread::Builder::new() .name("jcode-inline-image-prewarm".to_string()) .spawn(move || prewarm_worker(rx)) @@ -312,33 +452,102 @@ fn prewarm_sender() -> &'static mpsc::Sender { }) } -fn prewarm_worker(rx: mpsc::Receiver) { - for req in rx { - let materialized = materialize_visible(req.id); - if materialized { - mermaid::prewarm_inline_fit_state(req.id, req.target_cols, req.target_rows, true); - if let Ok(mut failures) = PREWARM_FAILURES.lock() { - failures.remove(&req.id); - } +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum PrewarmOutcome { + Prepared, + SourceMissing, + MaterializeFailed, + FitFailed, +} + +fn payload_restage_requested(id: u64) -> bool { + PAYLOAD_RESTAGE_ALL.load(Ordering::Acquire) + || PAYLOAD_RESTAGE_IDS + .lock() + .map(|requested| requested.contains(&id)) + .unwrap_or(false) +} + +fn prepare_prewarm_request(req: PrewarmRequest) -> PrewarmOutcome { + if !materialize_visible(req.id) { + return if payload_restage_requested(req.id) { + PrewarmOutcome::SourceMissing } else { - record_prewarm_failure(req.id); + PrewarmOutcome::MaterializeFailed + }; + } + match mermaid::inline_fit_readiness(req.id, req.target_cols, req.target_rows, true) { + mermaid::InlineFitReadiness::Ready | mermaid::InlineFitReadiness::Unsupported => { + PrewarmOutcome::Prepared } - if let Ok(mut inflight) = PREWARM_INFLIGHT.lock() { - inflight.remove(&req); + mermaid::InlineFitReadiness::NeedsPrewarm => { + if mermaid::prewarm_inline_fit_state(req.id, req.target_cols, req.target_rows, true) { + PrewarmOutcome::Prepared + } else { + PrewarmOutcome::FitFailed + } } - if !materialized { - // Nothing new to draw; nudging a repaint would only make the - // draw path reschedule this same failed request. - continue; + } +} + +fn finish_prewarm_request(req: PrewarmRequest) { + let outcome = prepare_prewarm_request(req); + match outcome { + PrewarmOutcome::Prepared => { + if let Ok(mut failures) = PREWARM_MATERIALIZE_FAILURES.lock() { + failures.remove(&req.id); + } + if let Ok(mut failures) = PREWARM_FIT_FAILURES.lock() { + failures.remove(&req); + } } - // Nudge the UI exactly like a finished deferred Mermaid render so the - // placeholder fills in on the next frame without user input. The - // prepared placeholder geometry is unchanged, so no prepare-cache - // invalidation is needed - just a repaint. + PrewarmOutcome::SourceMissing => {} + PrewarmOutcome::MaterializeFailed => record_materialize_failure(req.id), + PrewarmOutcome::FitFailed => record_fit_failure(req), + } + if let Ok(mut inflight) = PREWARM_INFLIGHT.lock() { + inflight.remove(&req.id); + } + if outcome == PrewarmOutcome::Prepared { crate::bus::Bus::global().publish(crate::bus::BusEvent::MermaidRenderCompleted); } } +fn prewarm_worker(rx: mpsc::Receiver) { + for req in rx { + finish_prewarm_request(req); + // A producer never blocks the UI on a full channel. Drain the newest + // coalesced overflow request before waiting on the channel again. + while let Some(overflow) = PREWARM_OVERFLOW + .lock() + .ok() + .and_then(|mut pending| pending.take()) + { + finish_prewarm_request(overflow); + } + if PREWARM_RETRY_NEEDED.swap(false, Ordering::AcqRel) { + // At least one older coalesced request was displaced. Space has now + // been made, so repaint once and let the current viewport resubmit + // whichever geometry is still relevant. + crate::bus::Bus::global().publish(crate::bus::BusEvent::MermaidRenderCompleted); + } + } +} + +fn coalesce_overflow(req: PrewarmRequest) { + let displaced = PREWARM_OVERFLOW + .lock() + .ok() + .and_then(|mut pending| pending.replace(req)); + if let Some(displaced) = displaced + && displaced.id != req.id + && let Ok(mut inflight) = PREWARM_INFLIGHT.lock() + { + inflight.remove(&displaced.id); + PREWARM_RETRY_NEEDED.store(true, Ordering::Release); + } +} + /// Make sure image `id` can be drawn cheaply this frame. /// /// Returns true when the draw path can run now without heavy work (image @@ -359,8 +568,8 @@ pub(crate) fn ensure_drawable(id: u64, target_cols: u16, target_rows: u16) -> bo match readiness { mermaid::InlineFitReadiness::Ready => true, mermaid::InlineFitReadiness::Unsupported => { - // Non-Kitty fallback renderers manage their own protocol state; - // just make sure the bytes are decoded, off-thread if possible. + // No picker or video export. Materialization is still useful so a + // picker becoming available later can draw without decoding here. if materialized { true } else { @@ -376,25 +585,43 @@ pub(crate) fn ensure_drawable(id: u64, target_cols: u16, target_rows: u16) -> bo } fn schedule_prewarm(id: u64, target_cols: u16, target_rows: u16) { - if prewarm_failures_exhausted(id) { - return; - } let req = PrewarmRequest { id, target_cols, target_rows, }; + if prewarm_failures_exhausted(req) { + return; + } if let Ok(mut inflight) = PREWARM_INFLIGHT.lock() - && !inflight.insert(req) + && !inflight.insert(id) { return; } - if prewarm_sender().send(req).is_err() { - // Worker unavailable: fall back to synchronous work on next frame. - if let Ok(mut inflight) = PREWARM_INFLIGHT.lock() { - inflight.remove(&req); + match prewarm_sender().try_send(req) { + Ok(()) => {} + Err(mpsc::TrySendError::Full(req)) => { + // Keep the UI thread non-blocking without dropping the newest + // request. The single worker drains this coalesced slot after every + // channel job, including failed ones that do not publish a repaint. + coalesce_overflow(req); + } + Err(mpsc::TrySendError::Disconnected(req)) => { + if let Ok(mut inflight) = PREWARM_INFLIGHT.lock() { + inflight.remove(&id); + } + // Thread creation failure is rare; preserve correctness by doing the + // complete preparation synchronously instead of leaving Kitty images + // permanently blank after materialization alone. + match prepare_prewarm_request(req) { + PrewarmOutcome::Prepared => { + crate::bus::Bus::global().publish(crate::bus::BusEvent::MermaidRenderCompleted); + } + PrewarmOutcome::SourceMissing => {} + PrewarmOutcome::MaterializeFailed => record_materialize_failure(req.id), + PrewarmOutcome::FitFailed => record_fit_failure(req), + } } - materialize_visible(id); } } @@ -414,8 +641,7 @@ pub(crate) fn prefetch(id: u64, target_cols: u16, target_rows: u16) { mermaid::InlineFitReadiness::NeedsPrewarm }; match readiness { - // Already drawable, or a protocol that builds its state synchronously - // at draw time (nothing useful to prewarm ahead). + // Already drawable, or no terminal protocol is currently available. mermaid::InlineFitReadiness::Ready | mermaid::InlineFitReadiness::Unsupported => {} mermaid::InlineFitReadiness::NeedsPrewarm => { schedule_prewarm(id, target_cols, target_rows); @@ -423,9 +649,14 @@ pub(crate) fn prefetch(id: u64, target_cols: u16, target_rows: u16) { } } -fn resolve_item(image: &crate::session::RenderedImage) -> Option { +fn resolve_item( + image: &crate::session::RenderedImage, + stage_payload: bool, +) -> Option { let (id, width, height) = mermaid::inline_image_dims(&image.media_type, &image.data)?; - register_payload(id, &image.media_type, &image.data); + if stage_payload { + register_payload(id, &image.media_type, &image.data); + } let label = image .label .clone() @@ -505,12 +736,20 @@ impl AnchoredInlineImages { /// Resolve rendered images into anchored buckets (tool call / user prompt / /// unanchored). Same lazy header-only cost profile as [`resolve_item`]. +#[cfg(test)] pub(crate) fn resolve_anchored_items( images: &[crate::session::RenderedImage], +) -> AnchoredInlineImages { + resolve_anchored_items_inner(images, true) +} + +fn resolve_anchored_items_inner( + images: &[crate::session::RenderedImage], + stage_payloads: bool, ) -> AnchoredInlineImages { let mut result = AnchoredInlineImages::default(); for image in images { - let Some(item) = resolve_item(image) else { + let Some(item) = resolve_item(image, stage_payloads) else { continue; }; match &image.anchor { @@ -530,7 +769,8 @@ pub(crate) fn resolve_anchored_items( /// signature. Resolving hashes every image payload (for ids), so body /// preparation must not redo it per rebuild; the signature is already cached /// per transcript version on the app side. -type AnchoredCache = Mutex)>>; +type AnchoredCacheKey = ((usize, u64), bool); +type AnchoredCache = Mutex)>>; static ANCHORED_CACHE: LazyLock = LazyLock::new(|| Mutex::new(None)); /// Resolve the app's images into anchored buckets, cached by the image-set @@ -540,21 +780,31 @@ pub(crate) fn resolve_anchored_items_cached( app: &dyn crate::tui::TuiState, ) -> std::sync::Arc { if !app.pin_images() { + clear_staged_payloads(); return std::sync::Arc::new(AnchoredInlineImages::default()); } let signature = app.side_pane_images_signature(); if signature.0 == 0 { + clear_staged_payloads(); return std::sync::Arc::new(AnchoredInlineImages::default()); } + let stage_payloads = app.inline_images_visible(); + if !stage_payloads { + clear_staged_payloads(); + } + let key = (signature, stage_payloads); if let Ok(cache) = ANCHORED_CACHE.lock() && let Some((cached_sig, cached)) = cache.as_ref() - && *cached_sig == signature + && *cached_sig == key { return cached.clone(); } - let resolved = std::sync::Arc::new(resolve_anchored_items(&app.side_pane_images())); + let resolved = std::sync::Arc::new(resolve_anchored_items_inner( + &app.side_pane_images(), + stage_payloads, + )); if let Ok(mut cache) = ANCHORED_CACHE.lock() { - *cache = Some((signature, resolved.clone())); + *cache = Some((key, resolved.clone())); } resolved } @@ -1087,23 +1337,101 @@ mod tests { ); } - /// Re-registering a payload must clear the prewarm failure memo so a fresh - /// payload gets its decode retries back. + /// Re-registering a staged source may retry geometry preparation, but its + /// stable content id keeps corrupt-payload decode failures capped. #[test] - fn reregistering_payload_resets_prewarm_failures() { + fn reregistering_payload_resets_fit_failures() { const ID: u64 = 0xFA11_ED01; + let req = PrewarmRequest { + id: ID, + target_cols: 80, + target_rows: 16, + }; for _ in 0..PREWARM_FAILURE_MAX_ATTEMPTS { - record_prewarm_failure(ID); + record_fit_failure(req); } assert!( - prewarm_failures_exhausted(ID), + prewarm_failures_exhausted(req), "failure memo should suspend prewarm after max attempts" ); register_payload(ID, "image/png", "BBBB"); assert!( - !prewarm_failures_exhausted(ID), - "fresh payload registration must reset the failure memo" + !prewarm_failures_exhausted(req), + "restaging may retry geometry preparation" + ); + PAYLOAD_REGISTRY.lock().unwrap().remove(ID); + } + + #[test] + fn prewarm_failures_are_scoped_to_geometry() { + let failed = PrewarmRequest { + id: 0xFA11_ED02, + target_cols: 80, + target_rows: 16, + }; + for _ in 0..PREWARM_FAILURE_MAX_ATTEMPTS { + record_fit_failure(failed); + } + let resized = PrewarmRequest { + target_cols: 100, + ..failed + }; + assert!(prewarm_failures_exhausted(failed)); + assert!( + !prewarm_failures_exhausted(resized), + "a failed stale resize must not block preparation at the new geometry" + ); + PREWARM_FIT_FAILURES.lock().unwrap().remove(&failed); + } + + #[test] + fn materialize_failures_apply_across_resize_geometries() { + let failed = PrewarmRequest { + id: 0xFA11_ED03, + target_cols: 80, + target_rows: 16, + }; + for _ in 0..PREWARM_FAILURE_MAX_ATTEMPTS { + record_materialize_failure(failed.id); + } + let resized = PrewarmRequest { + target_cols: 120, + target_rows: 24, + ..failed + }; + assert!(prewarm_failures_exhausted(failed)); + assert!( + prewarm_failures_exhausted(resized), + "a corrupt payload must not get three more full decodes after every resize" + ); + register_payload(failed.id, "image/png", "BBBB"); + assert!( + prewarm_failures_exhausted(resized), + "restaging identical content must not reset its ID-wide decode cap" ); + PAYLOAD_REGISTRY.lock().unwrap().remove(failed.id); + PREWARM_MATERIALIZE_FAILURES + .lock() + .unwrap() + .remove(&failed.id); + } + + #[test] + fn overflow_slot_keeps_the_newest_request() { + let first = PrewarmRequest { + id: 1, + target_cols: 80, + target_rows: 16, + }; + let newest = PrewarmRequest { + id: 2, + target_cols: 100, + target_rows: 20, + }; + let mut pending = None; + assert!(pending.replace(first).is_none()); + assert_eq!(pending.replace(newest), Some(first)); + assert_eq!(pending, Some(newest)); } /// Materialization must release the staged base64 payload (the decoded @@ -1197,6 +1525,63 @@ mod tests { assert_eq!(anchored.unanchored.len(), 1); } + #[test] + fn resolving_hidden_images_does_not_stage_payload_bytes() { + use base64::Engine as _; + use image::ImageEncoder as _; + + clear_staged_payloads(); + let pixels = image::RgbaImage::from_pixel(3, 2, image::Rgba([91, 37, 211, 255])); + let mut png = Vec::new(); + image::codecs::png::PngEncoder::new(&mut png) + .write_image(pixels.as_raw(), 3, 2, image::ExtendedColorType::Rgba8) + .expect("encode fixture"); + let data = base64::engine::general_purpose::STANDARD.encode(png); + let image = crate::session::RenderedImage { + media_type: "image/png".to_string(), + data, + label: Some("hidden-fixture.png".to_string()), + source: crate::session::RenderedImageSource::ToolResult { + tool_name: "read".to_string(), + }, + anchor: None, + }; + let id = mermaid::inline_image_id(&image.media_type, &image.data); + + let resolved = resolve_anchored_items_inner(std::slice::from_ref(&image), false); + assert_eq!(resolved.unanchored.len(), 1, "metadata still resolves"); + assert!( + PAYLOAD_REGISTRY.lock().unwrap().get(id).is_none(), + "hidden image payload must not be copied into the staging registry" + ); + + let visible = resolve_anchored_items_inner(std::slice::from_ref(&image), true); + assert_eq!(visible.unanchored.len(), 1); + assert!( + PAYLOAD_REGISTRY.lock().unwrap().get(id).is_some(), + "visible image payload should be staged for lazy materialization" + ); + PAYLOAD_REGISTRY.lock().unwrap().remove(id); + } + + #[test] + fn clearing_staged_payloads_invalidates_resolved_image_cache() { + PAYLOAD_RESTAGE_ALL.store(false, Ordering::Release); + *ANCHORED_CACHE.lock().unwrap() = Some(( + ((1, 99), true), + std::sync::Arc::new(AnchoredInlineImages::default()), + )); + clear_staged_payloads(); + assert!( + ANCHORED_CACHE.lock().unwrap().is_none(), + "showing images again must resolve and restage their payloads" + ); + assert!( + PAYLOAD_RESTAGE_ALL.load(Ordering::Acquire), + "a prepared-frame cache hit must know that every payload needs restaging" + ); + } + #[test] fn unplaced_items_falls_back_for_missing_anchor_targets() { use jcode_tui_messages::DisplayMessage; diff --git a/crates/jcode-tui/src/tui/ui_prepare.rs b/crates/jcode-tui/src/tui/ui_prepare.rs index e9d909bfcc..7f62658452 100644 --- a/crates/jcode-tui/src/tui/ui_prepare.rs +++ b/crates/jcode-tui/src/tui/ui_prepare.rs @@ -650,6 +650,10 @@ pub(super) fn prepare_messages( width: u16, height: u16, ) -> Arc { + // A cached prepared frame intentionally owns only image ids. Recover any + // staged source evicted by the byte budget or a visibility toggle before an + // exact frame-cache hit can bypass the normal anchored-image resolver. + super::inline_image_ui::restage_requested_payloads(app); if cfg!(test) { return Arc::new(prepare_messages_inner(app, width, height)); } @@ -666,7 +670,14 @@ pub(super) fn prepare_messages( streaming_text_len: app.streaming_text().len(), streaming_text_hash: super::hash_text_for_cache(app.streaming_text()), batch_progress_hash: active_batch_progress_hash(app), - inline_images_signature: app.side_pane_images_signature(), + // An unpinned transcript must not reuse a previously prepared frame + // containing anchored images. With no images, both modes are visually + // identical and `(0, 0)` reuse is safe. + inline_images_signature: if app.pin_images() { + app.side_pane_images_signature() + } else { + (0, 0) + }, inline_images_visible: app.inline_images_visible(), expanded_images_version: app.expanded_images_version(), swarm_members_signature: swarm_members_signature(&app.swarm_members_for_transcript()), diff --git a/crates/jcode-tui/src/tui/ui_viewport.rs b/crates/jcode-tui/src/tui/ui_viewport.rs index e7e29a073b..1df22e515d 100644 --- a/crates/jcode-tui/src/tui/ui_viewport.rs +++ b/crates/jcode-tui/src/tui/ui_viewport.rs @@ -930,8 +930,9 @@ pub(super) fn draw_messages( let rows = if is_fit { // Stable fit: scale once to the placeholder box and // reuse the transmitted pixels for every frame. - // Falls back to the per-area fit renderer on - // non-Kitty protocols. + // Kitty re-addresses terminal-retained pixels; + // other protocols crop a pre-scaled source so the + // visible slice never changes the image's scale. if crate::tui::mermaid::render_image_widget_fit_stable( hash, image_area, @@ -986,8 +987,8 @@ pub(super) fn draw_messages( }; if is_fit { // Top scrolled off: keep the same scaled pixels and - // skip the hidden rows instead of rescaling into - // the smaller visible portion. + // skip the hidden rows instead of rescaling into the + // smaller visible portion on every protocol. let skip_rows = (visible_start - abs_idx) as u16; if !crate::tui::mermaid::render_image_widget_fit_stable( hash, diff --git a/src/bin/tui_bench.rs b/src/bin/tui_bench.rs index 33408bf2c9..c28f5b72ab 100644 --- a/src/bin/tui_bench.rs +++ b/src/bin/tui_bench.rs @@ -1301,6 +1301,22 @@ fn main() -> Result<()> { println!("visible_draw_skips: {}", result.visible_draw_skips); println!("fit_protocol_rebuilds: {}", result.fit_protocol_rebuilds); println!("fit_state_reuse_hits: {}", result.fit_state_reuse_hits); + println!( + "retained_image_state_source_bytes: {}", + result.retained_image_state_source_bytes + ); + println!( + "retained_source_cache_decoded_bytes: {}", + result.retained_source_cache_decoded_bytes + ); + println!( + "retained_fitted_source_decoded_bytes: {}", + result.retained_fitted_source_decoded_bytes + ); + println!( + "retained_working_set_estimate_bytes: {}", + result.retained_working_set_estimate_bytes + ); return Ok(()); } diff --git a/src/cli/terminal.rs b/src/cli/terminal.rs index 8d459f3e8f..39e48859c2 100644 --- a/src/cli/terminal.rs +++ b/src/cli/terminal.rs @@ -373,6 +373,15 @@ fn cleanup_tui_runtime(state: &TuiRuntimeState, restore_terminal: bool) { state.keyboard_enhanced, state.focus_change, )); + crate::tui::mermaid::clear_image_state(); + let image_cleanup = crate::tui::mermaid::take_terminal_image_cleanup_payload(); + if !image_cleanup.is_empty() { + use std::io::Write as _; + let mut stdout = std::io::stdout().lock(); + let _ = stdout.write_all(image_cleanup.as_bytes()); + let _ = stdout.flush(); + } + if restore_terminal { let _ = crossterm::execute!(std::io::stdout(), crossterm::event::DisableBracketedPaste); if state.focus_change { @@ -391,8 +400,6 @@ fn cleanup_tui_runtime(state: &TuiRuntimeState, restore_terminal: bool) { } ratatui::restore(); } - - crate::tui::mermaid::clear_image_state(); } fn cleanup_tui_runtime_for_run_result( From db24457cfd901ffc40a0cae7b31867e734b562e8 Mon Sep 17 00:00:00 2001 From: jeremy <94247773+1jehuang@users.noreply.github.com> Date: Fri, 17 Jul 2026 15:16:25 -0700 Subject: [PATCH 0011/1688] fix(config): migrate legacy baked swarm_spawn_mode "visible" to "inline" The old visible default got baked into config.toml by any Config::save(), so existing users stayed on window-per-agent spawning after the default changed to inline. Add a one-time marker-gated startup migration that rewrites only that line, preserving the rest of the file, and never touches an explicit post-migration choice. --- crates/jcode-base/src/config/config_file.rs | 90 +++++++++++++++++++++ crates/jcode-base/src/config_tests.rs | 73 +++++++++++++++++ src/cli/dispatch.rs | 6 ++ 3 files changed, 169 insertions(+) diff --git a/crates/jcode-base/src/config/config_file.rs b/crates/jcode-base/src/config/config_file.rs index 6f01264e66..a34a3bf65a 100644 --- a/crates/jcode-base/src/config/config_file.rs +++ b/crates/jcode-base/src/config/config_file.rs @@ -325,6 +325,96 @@ impl Config { false } + /// One-time migration: flip a persisted legacy `swarm_spawn_mode = + /// "visible"` to the current `"inline"` default. + /// + /// Historically `visible` was the default, and any full-config + /// `Config::save()` (model switches, display toggles, ...) baked that + /// then-default into the user's config.toml. When the default changed to + /// `inline`, those users stayed pinned to `visible` forever. This rewrites + /// exactly that one line (preserving the rest of the file byte-for-byte) + /// and drops a marker so it runs at most once. A user who explicitly sets + /// `visible` after the migration is never flipped again. + /// + /// Returns `true` when it rewrote the config. Best-effort: errors are + /// logged and swallowed. + pub fn migrate_legacy_swarm_spawn_mode_once() -> bool { + let Ok(dir) = jcode_dir() else { + return false; + }; + let marker = dir.join("migrations").join("swarm-spawn-mode-inline"); + if marker.exists() { + return false; + } + let write_marker = || { + if let Some(parent) = marker.parent() { + let _ = std::fs::create_dir_all(parent); + } + let _ = std::fs::write( + &marker, + "swarm_spawn_mode default migration: visible -> inline\n", + ); + }; + + let path = dir.join("config.toml"); + let Ok(content) = std::fs::read_to_string(&path) else { + // No config file (fresh install): nothing to migrate. + write_marker(); + return false; + }; + + let mut changed = false; + let migrated: Vec = content + .lines() + .map(|line| { + if changed { + return line.to_string(); + } + let trimmed = line.trim_start(); + let Some(rest) = trimmed.strip_prefix("swarm_spawn_mode") else { + return line.to_string(); + }; + let Some(value) = rest.trim_start().strip_prefix('=') else { + return line.to_string(); + }; + let value = value.trim().trim_matches(|c| c == '"' || c == '\''); + if matches!(value, "visible" | "headed") { + changed = true; + let indent = &line[..line.len() - trimmed.len()]; + format!("{indent}swarm_spawn_mode = \"inline\"") + } else { + line.to_string() + } + }) + .collect(); + + if !changed { + write_marker(); + return false; + } + + let mut new_content = migrated.join("\n"); + if content.ends_with('\n') { + new_content.push('\n'); + } + match std::fs::write(&path, new_content) { + Ok(()) => { + Self::invalidate_cache(); + write_marker(); + crate::logging::info( + "Migrated legacy swarm_spawn_mode \"visible\" to \"inline\" in config.toml", + ); + true + } + Err(err) => { + crate::logging::warn(&format!( + "swarm_spawn_mode migration failed to write config: {err}" + )); + false + } + } + } + fn normalize_external_auth_source_id(source_id: &str) -> String { source_id.trim().to_ascii_lowercase() } diff --git a/crates/jcode-base/src/config_tests.rs b/crates/jcode-base/src/config_tests.rs index 505c6abea7..37447947c6 100644 --- a/crates/jcode-base/src/config_tests.rs +++ b/crates/jcode-base/src/config_tests.rs @@ -991,3 +991,76 @@ fn populate_context_limits_from_config_seeds_qualified_runtime_model_shapes() { "profile-qualified slash-path spec must resolve the configured context_window" ); } + +#[test] +fn migrate_legacy_swarm_spawn_mode_flips_visible_to_inline_once() { + let _guard = crate::storage::lock_test_env(); + let prev_home = std::env::var_os("JCODE_HOME"); + let dir = tempfile::TempDir::new().expect("tempdir"); + crate::env::set_var("JCODE_HOME", dir.path()); + + let config_path = dir.path().join("config.toml"); + let original = "[display]\ncentered = true\n\n[agents]\nswarm_spawn_mode = \"visible\"\nswarm_max_concurrent_agents = 32\n"; + std::fs::write(&config_path, original).expect("write config"); + + assert!( + Config::migrate_legacy_swarm_spawn_mode_once(), + "migration should rewrite a legacy visible spawn mode" + ); + let migrated = std::fs::read_to_string(&config_path).expect("read config"); + assert!( + migrated.contains("swarm_spawn_mode = \"inline\""), + "spawn mode should be flipped to inline: {migrated}" + ); + // The rest of the file is untouched. + assert!(migrated.contains("centered = true")); + assert!(migrated.contains("swarm_max_concurrent_agents = 32")); + let parsed: Config = toml::from_str(&migrated).expect("migrated config parses"); + assert_eq!(parsed.agents.swarm_spawn_mode, SwarmSpawnMode::Inline); + + // Marker written: a later explicit "visible" survives future launches. + std::fs::write( + &config_path, + "[agents]\nswarm_spawn_mode = \"visible\"\n", + ) + .expect("write config"); + assert!( + !Config::migrate_legacy_swarm_spawn_mode_once(), + "migration must run at most once" + ); + let content = std::fs::read_to_string(&config_path).expect("read config"); + assert!(content.contains("swarm_spawn_mode = \"visible\"")); + + restore_env_var("JCODE_HOME", prev_home); +} + +#[test] +fn migrate_legacy_swarm_spawn_mode_noops_without_visible_value() { + let _guard = crate::storage::lock_test_env(); + let prev_home = std::env::var_os("JCODE_HOME"); + let dir = tempfile::TempDir::new().expect("tempdir"); + crate::env::set_var("JCODE_HOME", dir.path()); + + // No config file at all: no migration, but the marker is written. + assert!(!Config::migrate_legacy_swarm_spawn_mode_once()); + assert!( + dir.path() + .join("migrations") + .join("swarm-spawn-mode-inline") + .exists(), + "marker should be written even when there is nothing to migrate" + ); + + // Explicit non-visible values are never rewritten (marker already set, + // but check the matcher too with a fresh home). + let dir2 = tempfile::TempDir::new().expect("tempdir"); + crate::env::set_var("JCODE_HOME", dir2.path()); + let config_path = dir2.path().join("config.toml"); + std::fs::write(&config_path, "[agents]\nswarm_spawn_mode = \"headless\"\n") + .expect("write config"); + assert!(!Config::migrate_legacy_swarm_spawn_mode_once()); + let content = std::fs::read_to_string(&config_path).expect("read config"); + assert!(content.contains("swarm_spawn_mode = \"headless\"")); + + restore_env_var("JCODE_HOME", prev_home); +} diff --git a/src/cli/dispatch.rs b/src/cli/dispatch.rs index 2bb72f2990..b3030268c0 100644 --- a/src/cli/dispatch.rs +++ b/src/cli/dispatch.rs @@ -24,6 +24,12 @@ use provider_init::ProviderChoice; pub(crate) async fn run_main(mut args: Args) -> Result<()> { resolve_resume_arg(&mut args)?; + // One-time config migration: users whose config.toml still carries the old + // baked-in `swarm_spawn_mode = "visible"` default get flipped to the + // current `inline` default. Cheap (single file read, marker-gated), and it + // must run before the config cache is first populated. + crate::config::Config::migrate_legacy_swarm_spawn_mode_once(); + if let Some(profile_name) = args .provider_profile .as_deref() From 204862a2b99fee37e9bfc22fb63400155013247f Mon Sep 17 00:00:00 2001 From: jeremy <94247773+1jehuang@users.noreply.github.com> Date: Fri, 17 Jul 2026 15:25:34 -0700 Subject: [PATCH 0012/1688] feat(install): persist Windows user PATH from Git Bash installer + setup-friction eval The curl|sh installer on Windows (Git Bash) printed manual PATH instructions but never persisted the launcher dir on the user PATH, so 'jcode' was not found in new terminals. It now mirrors install.ps1: reads the user PATH via powershell.exe, dedupes stale jcode entries (case/trailing-slash-insensitive), prepends the canonical dir, and broadcasts WM_SETTINGCHANGE. Add scripts/setup_friction_eval.sh, a deterministic install/setup/ retention scorecard that runs the REAL install.sh in a sandbox with a mocked release endpoint and probes the result with REAL shells: A: fresh install resolves in bash -l/-i, sh -l, fish, zsh B: 3x reinstall leaves exactly one PATH stanza per rc file C: upgrade preserves ~/.jcode config/auth, keeps rollback binaries D: Windows PATH parity, incl. driving the Git Bash branch with a mocked powershell.exe (fails on the pre-fix installer) Wire it plus test_install_conversion.sh into CI as a setup-friction job. --- .github/workflows/ci.yml | 16 ++ scripts/install.sh | 55 ++++- scripts/setup_friction_eval.sh | 374 +++++++++++++++++++++++++++++++++ 3 files changed, 440 insertions(+), 5 deletions(-) create mode 100755 scripts/setup_friction_eval.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4bd8c40fdd..1caba36e17 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -390,6 +390,22 @@ jobs: - name: Check formatting run: cargo fmt --all -- --check + setup-friction: + name: Setup Friction Eval (Linux installer) + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + + - name: Install probe shells + run: sudo apt-get update && sudo apt-get install -y fish zsh + + - name: Installer conversion telemetry tests + run: bash scripts/test_install_conversion.sh + + - name: Setup friction scorecard + run: bash scripts/setup_friction_eval.sh + powershell-syntax: name: PowerShell Syntax runs-on: windows-latest diff --git a/scripts/install.sh b/scripts/install.sh index 06c7d77699..5f2c5fba2f 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -364,20 +364,65 @@ if [ "${JCODE_SKIP_SERVER_RELOAD:-}" != "1" ]; then fi if [ "$IS_WINDOWS" = true ]; then + INSTALL_STAGE="path_configuration" win_install_dir=$(cygpath -w "$INSTALL_DIR" 2>/dev/null || echo "$INSTALL_DIR") + + # Persist the launcher dir on the USER PATH so every future shell (PowerShell, + # cmd, Git Bash, Windows Terminal) finds jcode without manual setup. This is + # the Git Bash (`curl | sh`) counterpart of install.ps1's Set-JcodeUserPath: + # read the user PATH, drop stale jcode launcher entries (case- and trailing- + # slash-insensitive), prepend the canonical dir, and broadcast + # WM_SETTINGCHANGE so already-open apps can pick up the change. + win_path_persisted=false + _win_path_key() { printf '%s' "$1" | sed 's|[\\/]*$||' | tr '[:upper:]' '[:lower:]'; } + if command -v powershell.exe >/dev/null 2>&1; then + current_user_path=$(powershell.exe -NoProfile -NonInteractive -Command \ + "[Environment]::GetEnvironmentVariable('Path','User')" 2>/dev/null | tr -d '\r' || true) + target_key=$(_win_path_key "$win_install_dir") + new_user_path="$win_install_dir" + set -f + IFS=';' + for entry in $current_user_path; do + [ -n "$entry" ] || continue + [ "$(_win_path_key "$entry")" = "$target_key" ] && continue + new_user_path="$new_user_path;$entry" + done + unset IFS + set +f + if [ "$new_user_path" = "$current_user_path" ]; then + win_path_persisted=true + elif JCODE_NEW_USER_PATH="$new_user_path" powershell.exe -NoProfile -NonInteractive -Command \ + '[Environment]::SetEnvironmentVariable("Path", $env:JCODE_NEW_USER_PATH, "User")' >/dev/null 2>&1; then + win_path_persisted=true + # Broadcast WM_SETTINGCHANGE (0x001A) with the "Environment" lParam to + # HWND_BROADCAST so running shells learn about the new PATH. Best-effort. + powershell.exe -NoProfile -NonInteractive -Command ' + $sig = "[DllImport(\"user32.dll\", SetLastError = true, CharSet = CharSet.Auto)] public static extern IntPtr SendMessageTimeout(IntPtr hWnd, uint Msg, UIntPtr wParam, string lParam, uint fuFlags, uint uTimeout, out UIntPtr lpdwResult);" + $type = Add-Type -MemberDefinition $sig -Name "JcodeEnvBroadcast" -Namespace Win32 -PassThru + [UIntPtr]$result = [UIntPtr]::Zero + $type::SendMessageTimeout([IntPtr]0xffff, 0x001A, [UIntPtr]::Zero, "Environment", 2, 5000, [ref]$result) | Out-Null + ' >/dev/null 2>&1 || true + fi + fi + echo "" info "✅ jcode $VERSION installed successfully!" echo "" + if [ "$win_path_persisted" = true ]; then + info "Added $win_install_dir to your user PATH. New terminals will find jcode automatically." + fi if command -v jcode >/dev/null 2>&1; then info "Run 'jcode' to get started." else - echo " To start using jcode right now, run:" + echo " To start using jcode in THIS terminal right now, run:" echo "" printf ' \033[1;32mexport PATH="%s:$PATH" && jcode\033[0m\n' "$INSTALL_DIR" - echo "" - echo " To add jcode to PATH permanently (PowerShell):" - echo "" - printf ' \033[1;32m[Environment]::SetEnvironmentVariable("Path", "%s;" + [Environment]::GetEnvironmentVariable("Path", "User"), "User")\033[0m\n' "$win_install_dir" + if [ "$win_path_persisted" != true ]; then + echo "" + echo " To add jcode to PATH permanently (PowerShell):" + echo "" + printf ' \033[1;32m[Environment]::SetEnvironmentVariable("Path", "%s;" + [Environment]::GetEnvironmentVariable("Path", "User"), "User")\033[0m\n' "$win_install_dir" + fi fi else INSTALL_STAGE="path_configuration" diff --git a/scripts/setup_friction_eval.sh b/scripts/setup_friction_eval.sh new file mode 100755 index 0000000000..e980118f68 --- /dev/null +++ b/scripts/setup_friction_eval.sh @@ -0,0 +1,374 @@ +#!/usr/bin/env bash +# setup_friction_eval.sh - deterministic install / setup / retention friction +# scorecard. +# +# The TUI onboarding evaluator (onboarding_eval.rs) scores the in-app flow, but +# most first-run friction happens BEFORE the TUI: the installer, PATH +# persistence, and whether an upgrade quietly preserves the user's state. This +# script measures that surface deterministically, with no network and no real +# user data, by running the REAL scripts/install.sh inside a sandbox with a +# mocked release endpoint, then probing the result with REAL shells. +# +# Section A fresh-install PATH resolution - after one `curl | sh`-equivalent +# install, does `jcode` resolve in a brand-new login/interactive +# shell of every kind we claim to support (bash -l, bash -i, +# sh -l, fish, zsh)? This is the exact "it wasn't on my PATH" +# complaint, asked of the real rc files the installer wrote. +# Section B idempotency - three installs must leave exactly one PATH line +# per rc file (no duplicate exports piling up run after run). +# Section C retention - an upgrade must preserve ~/.jcode config and auth, +# keep both immutable version binaries (rollback stays possible), +# and the launcher must serve the new version. +# Section D Windows parity - static audit that the Git Bash installer path +# (install.sh) and the PowerShell installer (install.ps1) both +# persist the user PATH, dedupe stale entries, and broadcast +# WM_SETTINGCHANGE. Runtime Windows behavior is covered by +# scripts/test_windows_setup_evaluation.ps1 in CI; this section +# stops the two installers drifting apart on POSIX dev machines. +# +# Every case prints PASS/FAIL/SKIP with expected-vs-actual on failure. The +# composite is passed/(passed+failed); SKIPs (shell not installed) don't count +# against the score but are reported. Exits nonzero on any FAIL. +set -u + +repo_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd) +install_sh="$repo_dir/scripts/install.sh" +install_ps1="$repo_dir/scripts/install.ps1" + +work=$(mktemp -d) +trap 'rm -rf "$work"' EXIT + +passed=0 +failed=0 +skipped=0 +declare -a failures=() + +pass() { passed=$((passed + 1)); printf 'PASS %s\n' "$1"; } +skip() { skipped=$((skipped + 1)); printf 'SKIP %s (%s)\n' "$1" "$2"; } +fail() { + failed=$((failed + 1)) + failures+=("$1") + printf 'FAIL %s\n' "$1" + printf ' expected: %s\n' "$2" + printf ' actual: %s\n' "$3" +} + +check() { # check + local name="$1" expected="$2" actual="$3" status="$4" + if [ "$status" -eq 0 ]; then pass "$name"; else fail "$name" "$expected" "$actual"; fi +} + +# --------------------------------------------------------------------------- +# Sandbox: mocked release endpoint + tools, identical shape to +# test_install_conversion.sh so both exercise the same installer code paths. +# --------------------------------------------------------------------------- +mkdir -p "$work/bin" + +cat > "$work/bin/uname" <<'EOF' +#!/usr/bin/env bash +case "${1:-}" in + -s) printf '%s\n' "${EVAL_UNAME_S:-Linux}" ;; + -m) printf '%s\n' "${EVAL_UNAME_M:-x86_64}" ;; + *) printf '%s\n' "${EVAL_UNAME_S:-Linux}" ;; +esac +EOF + +cat > "$work/bin/curl" <<'EOF' +#!/usr/bin/env bash +output="" +url="" +while [ "$#" -gt 0 ]; do + case "$1" in + -o) output="$2"; shift 2 ;; + --data) shift 2 ;; + http*) url="$1"; shift ;; + *) shift ;; + esac +done +case "$url" in + *telemetry.jcode.sh*) ;; + *jcode.sh/releases/latest/version) printf 'v%s\n' "${EVAL_VERSION:-1.2.3}" ;; + *jcode.sh/releases/v*/download-bases) + printf 'https://github.com/1jehuang/jcode/releases/download/v%s\n' "${EVAL_VERSION:-1.2.3}" + ;; + *SHA256SUMS) + # Checksum of the deterministic fake archive written by the tar mock's + # sibling below (the literal bytes "fake archive"). + printf '8d57abb57a0dae3ff23c8f0df1f51951b7772822e0d560e860d6f68c24ef6d3d %s\n' \ + "${EVAL_CHECKSUM_ASSET:-jcode-linux-x86_64.tar.gz}" + ;; + *github.com*/releases/latest) + printf 'https://github.com/1jehuang/jcode/releases/tag/v%s' "${EVAL_VERSION:-1.2.3}" + ;; + *github.com*/releases/download/*) + [ -n "$output" ] || exit 2 + printf 'fake archive' > "$output" + ;; + *) exit 2 ;; +esac +EOF + +cat > "$work/bin/tar" <<'EOF' +#!/usr/bin/env bash +dest="" +while [ "$#" -gt 0 ]; do + case "$1" in + -C) dest="$2"; shift 2 ;; + *) shift ;; + esac +done +artifact="${EVAL_ARCHIVE_ARTIFACT:-jcode-linux-x86_64}" +cat > "$dest/$artifact" <&1 +} + +# Probe: does `jcode` resolve and run in a fresh shell of the given kind, with +# only the sandbox HOME's rc files to set it up? PATH starts minimal (no +# ~/.local/bin) so resolution can only come from what the installer wrote. +probe_shell() { # probe_shell + local home="$1"; shift + HOME="$home" \ + XDG_CONFIG_HOME="$home/.config" \ + ENV="$home/.profile" \ + PATH="/usr/bin:/bin" \ + "$@" 'command -v jcode >/dev/null 2>&1 && jcode --version' 2>/dev/null /dev/null || true) +[ "$ver" = "jcode 1.2.3" ]; check "launcher runs and reports the installed version" \ + "jcode 1.2.3" "${ver:-}" "$?" + +# The success message must not dead-end the user: either jcode is already +# resolvable or the copy explicitly says future shells will have it. +printf '%s' "$install_out" | grep -q "Run 'jcode' to get started\|Future terminal sessions will have jcode on PATH automatically" +check "install output gives a working next step (no dead end)" \ + "a 'run jcode' or 'future sessions' line" "neither line found in installer output" "$?" + +probe_case() { # probe_case