Slice 4/6: dashboard features (Update config, Support recovery) + backup + decrypt - #261
Merged
Conversation
The group header already says Daemon, so the per-item word was redundant: 'Disable daemon' -> 'Disable', 'Restart daemon' -> 'Restart', 'Re-enable daemon' -> 'Re-enable', 'Install daemon' -> 'Install', 'Daemon status' -> 'Status'. Labels only; action values, dispatch, and the daemon result-screen titles (Daemon installed/disabled/restart/status, set in dashboard.go) are unchanged, so navigation + waitScreen in the driver tests stay valid.
The group header already says Backup; shorten the item to just 'Backup' (label only, action/dispatch unchanged), consistent with the other groups' terse command names.
…ix flaky hangs The Charm driver-test session seam created a bubbletea program per test but never tore it down: the seam cleanup only restored the newAgeSetupSession var. A leftover program's event-loop/renderer goroutines kept running and intermittently stalled a LATER test's RunTask/waitScreen (the flaky 60s driver-test timeouts that looked like 'badly written tests'). Close the session in the seam cleanup (Quit + block until the goroutines exit) and cancel its context, so every test starts from a clean slate. Verified: the Daemon|Dashboard batch runs green x5 and the full cmd/proxsave suite passes x3 under -shuffle (random order, the best stress for state leaks).
… description Label 'New encryption key' -> 'New key'; description 'reset the AGE recipients and run the key setup' -> 'create new encryption AGE key'. Label/description only; action and dispatch unchanged.
…nes like install Picking Backup in the dashboard used to close the session and run the backup as raw CLI. Now it streams the real backup run inside the graphical session, reusing the SAME bricks as the install finalization: - dashboard ActionBackup keeps the session alive and stashes it (stashDashboardSession) like the other flow actions, instead of closing it; - runBackupMode branches on dashboardHandoffPending(): the dashboard path runs runBackupStreamed (new backup_stream.go) which adopts the session, presents components.RunStreamTask, captures the default + bootstrap loggers via logging.CaptureConsole/NewLineWriter so the backup's [ts] LEVEL lines stream on screen, then shows buildBackupOutcomePrompt (shared renderInstallBanner 'Backup completed/failed' + Files/ Size/Duration/Archive/Local/Secondary/Cloud stat lines) below, and closes after Continue; - Esc cancels the run (taskCtx threaded into the backup ctx). The backup CORE (runBackupModeSteps / orch.RunGoBackup) is untouched - the graphical path only wraps it. CLI --backup, bare non-interactive, cron, and daemon never stash a session (dashboardHandoffPending()==false), so they run byte-identically to today; maybeHandoffManualBackup runs in both branches. A backupStreamSteps seam makes runBackupStreamed driver-testable. Built via a 1-builder -> 3-refuter workflow: all HOLD (LOW findings only). build/vet/test + -shuffle isolation + leaf/no-tview green.
…ack, selection) The backup run and install finalization streamed inside the bubbletea ALTSCREEN, which loses the terminal's native colors, scrollback, and text-selection - painful for a long backup and for copying the log for support. Switch them to an INLINE (non-altscreen) stream that emits each log line via tea.Println, so lines land in the terminal's native scrollback WITH colors and are selectable, while a live status line (spinner -> outcome) stays pinned below. Reusable bricks: - shell: Config.Inline -> rootModel.View sets AltScreen=false + MouseMode=None (cell-motion mouse steals native selection); StartInline + StartInlineForTestWithOutput. Altscreen sessions unchanged (Inline default false). - components: inlineStreamTask + RunStreamTaskInline - each StreamLineMsg returns tea.Println(line) (nothing retained), View is just the status line. The altscreen StreamTask/RunStreamTask (no remaining callers) removed. - logging: NewLineWriterRaw (keeps ANSI) + CaptureConsoleWithColor (colored bootstrap mirror), shared body; debug still filtered at the standard level. - exit-code fix: shared exitCodeSeverity(exitCode, logger) extracted from finalSummaryColor; the backup outcome banner now classifies by severity like the CLI (exit 1/ExitGenericError = yellow 'Backup completed with warnings', not red 'failed'; interrupted = magenta). The PROCESS exit code is unchanged (display-only), so it matches the CLI exactly. theme.InterruptedText (magenta) added. Consumers: dashboard Backup tears down its altscreen session then runs the backup in a fresh inline session (teardownDashboardSessionForInline; teardown-before-capture so restore returns to stdout); install closes the wizard altscreen then runs the finalization inline. CLI --backup / cron / daemon are byte-identical (never stash -> plain runBackupModeSteps). runDashboardUpgrade got a TODO to be rebuilt the same way. Built via a 3-builder -> 3-refuter workflow: all HOLD (0 findings). build/vet + UI/logging/cmd tests + -shuffle isolation + leaf/no-tview green.
…iewport (not raw scrollback) The previous inline tea.Println approach put the run lines in the terminal's RAW scrollback ABOVE the frame: scrolling exposed the whole prior CLI and the run was not contained in the graphics. Replace it with a CONTAINED, scrollable, COLORED viewport panel inside the altscreen frame (modeled on the existing Pager / bubbles viewport): the streamed lines accumulate in a bounded box, up/down/pgup/pgdown/home/end/wheel scroll INSIDE the box (never the terminal), auto-follow the tail while running (scrolling up stops follow), colors preserved, styled outcome below. - Revert the inline shell support (Config.Inline / StartInline / router AltScreen toggle) - altscreen is unconditional again with cell-motion mouse. - StreamTask/RunStreamTask rebuilt on viewport.Model (bounded ring 5000 lines, 'showing last N' when dropped, scroll% readout). sanitizeStreamLine: color-preserving line sanitizer (keeps SGR escapes, flattens control chars) so ANSI colors survive into the viewport. - Backup adopts the dashboard altscreen session again; install finalization runs on the wizard altscreen session; both via components.RunStreamTask with NewLineWriterRaw + CaptureConsoleWithColor (colored lines). - KEPT from the prior commit: exit-code fix (exitCodeSeverity -> the backup banner shows exit 1 as yellow 'Backup completed with warnings' matching the CLI, process exit code unchanged) + the color-capture helpers. CLI --backup / cron / daemon byte-identical (no stash -> plain runBackupModeSteps). runDashboardUpgrade keeps its TODO to be rebuilt the same way. Built via a 2-builder -> 2-refuter workflow: HOLD (2 LOW: stale comments fixed; upgrade TODO). build/vet + UI/logging/cmd + -shuffle isolation + leaf/no-tview green.
…st rendering) Harden the streamed viewport panel against the observed live-terminal garble (old lines sticking at the bottom over the footer): enable viewport.SoftWrap so long lines wrap INSIDE the box instead of using a horizontal-scroll offset (which renders unpredictably in the frame), and move vp.SetContent into View() AFTER SetWidth/SetHeight so the content is always measured/wrapped against the current width (never a stale or zero width carried over from an Update). The component View is proven to return exactly the requested height with no over-wide rows (unit-verified); these changes remove two rendering variables that can misalign the frame.
Native mouse text-selection is not available inside the altscreen viewport, so add a 'c' key to the StreamTask panel that copies the ENTIRE log to the system clipboard via tea.SetClipboard (OSC52), ANSI-stripped so the paste is clean plain text for a support request. A transient '✓ log copied' note appears in the header and clears on the next keypress; the help hint gains 'c copy log'.
…ction spacers The graphical panel only captured the loggers, so the run's fmt.Println() blank spacer lines between sections (11+ sites, raw os.Stdout) were lost and the sections ran together. Route BOTH the loggers AND raw os.Stdout through a single pipe into the stream (captureRunOutput): everything the run prints - colored logger lines AND the blank spacer rows - now flows into the panel IN ORDER, matching the CLI. bubbletea renders to its own saved fd, so redirecting the os.Stdout variable never touches the altscreen (same trick defaultUpgradeMuteStdio uses). StreamTask now KEEPS blank lines (they are the spacers). The install finalization uses the same helper (its temp permissions logger writes to os.Stdout, now captured, so it passes nil). CLI/cron/daemon unchanged (captureRunOutput only runs on the adopted graphical path). Tests: blank lines retained; build/vet + components + cmd + -shuffle + leaf/no-tview green.
…erver MAC Address line
The Healthchecks portal magic-link ("Monitoring portal (set your password): <link>") was printed inside the
Phase-7 notifications block. Move it to the backup epilogue so it prints immediately AFTER the Server MAC
Address line (both CLI and graphics share this epilogue), matching the requested ordering.
- HealthchecksChannel.Notify no longer displays the link: it captures/mints it and STORES the RAW link on
stats.HealthcheckLink (the mint-failure 'portal link unavailable' note stays in the notifications section).
- New sole display boundary logMonitoringPortalLink (cmd/proxsave/runtime_helpers.go), called right after
logServerIdentityValues in runConfiguredBackup: sanitizes the raw link via serverbot.SanitizeLoginURL and
prints the same wording; never registers a secret, fail-closed on nil/empty/hostile links.
Epilogue order: ... Server MAC Address -> Monitoring portal (set your password) -> [heap] -> exit status.
Not added to the non-backup main_identity.go path. Tests: channel tests assert the link is stored on stats
(not printed); new cmd/proxsave test proves sanitize-away (raw-space/javascript:/control-char), valid-link
display, and nil/empty no-op. Built via builder->2 refuters: HOLD (1 LOW stale telegram.go comment, fixed).
build/vet + orchestrator + cmd + notify + -shuffle + no-tview green.
…so the panel border aligns The console "Exit status: <glyph> STATE (code=N)" line used notify.GetStatusEmoji, whose WARNING glyph is the emoji-presentation "⚠️ " (U+26A0 + U+FE0F). lipgloss measures it as width 2 but the terminal renders it width 1, so that one line ran a column short and pushed the framed graphical panel's right border out of line (a stray '|' beside the Exit status row). Every other line in the run output uses plain text checkmarks (U+2713), so this line was the sole outlier. Add consoleStatusGlyph (text: ok checkmark / warning sign / cross, all width 1, terminal-stable) and use it for the Exit status line - fixing the border and matching the rest of the output. Note: notify.GetStatusEmoji stays for email/telegram/webhook payloads, where emoji are wanted. Test locks that the glyph carries no U+FE0F.
…op the password hint The end-of-run portal link line read "Monitoring portal (set your password): <link>". Rename it to "Healthchecks Portal: <link>" and remove the set-your-password instruction. Display logic, ordering (right after the Server MAC Address line), and the sanitize/never-a-secret discipline are unchanged. Updated the epilogue test wording and the channel's no-display assertions + doc comment to match.
…come The CLI footer prints a 'WARNINGS/ERRORS DURING RUN (warnings=N errors=M)' summary before the final box, but the graphical run outcome only showed the banner + stats. Add the same recap to buildBackupOutcomePrompt via appendRunIssueSummary: a colored count header (yellow for warnings-only, red once any error was logged) followed by the captured '[ts] LEVEL msg' issue lines, read from the SAME default logger the CLI footer uses so the counts and lines match exactly. Shown only when the run logged issues (including a failed run with no stats); the list is capped at 10 with a '... and N more (scroll up to review)' note so a noisy run cannot overflow the outcome block (the full list stays scrollable in the panel above). Tests: recap present with header+lines when issues logged, absent on a clean run; existing outcome tests unchanged.
Rename the end-of-run log line from "✓ Go backup orchestration completed" to "✓ Backup completed".
…r ID Telegram, Healthchecks link) Extend buildBackupOutcomePrompt so the graphical run recap states more of what the CLI/notifications carry, reusing existing data (no new counters): - Files line: 'Files: N collected - K missing' (K = st.FilesMissing, the SAME field the notifications report), keeping ' (M failed)' when FilesFailed > 0; missing is always shown (yellow when > 0). - 'Log: <path>' after the Archive line, from runLogPath() = logger.GetLogFilePath() with a LOG_FILE env fallback (the run log is closed during the backup's log-management phase before the outcome is built). - 'Server ID Telegram: <st.ServerID>' (the Telegram/relay pairing id) and 'Healthchecks link: <link>' after the storage lines, each shown only when non-empty. The link is sanitized through serverbot.SanitizeLoginURL (the same sole-display discipline as logMonitoringPortalLink): raw/hostile links are stripped, never printed, never registered as a secret. Tests: new Files format (missing 0 and > 0), Log via LOG_FILE env fallback, Server ID Telegram present/absent, Healthchecks link present(clean)/absent(empty)/hostile(space + javascript: stripped). Built via builder -> 2 refuters: HOLD (0 real findings). build/vet + cmd + -shuffle + no-tview green.
PBS logged a per-collection summary (newPBSFinalizeBricks: 'PBS collection summary:' + Files collected / not found at Info, non-zero Files failed at Warning, skipped / bytes at Debug) but PVE had no equivalent, so a PVE run never surfaced the collection counts in the flow at standard level. Add newPVEFinalizeSummaryBricks (brick brickPVEFinalizeSummary, 'Finalize PVE collection state') as the LAST brick of newPVERecipe - the same position newPBSFinalizeBricks holds in newPBSRecipe. The summary body is byte-identical to the PBS one (same lines, log levels, indentation), only the 'PVE'/'PBS' header word differs, so the two stay consistent. Updated TestNewPVERecipeOrder for the new trailing brick.
…ror it in the graphical recap The inline statistics block was console-only (the log file is closed before it runs). Make it DEBUG-only and show it in the graphical outcome recap instead: - logBackupStatistics early-returns when the level is below Debug, so a standard run emits nothing (block AND its blank spacers) - no orphan blanks; a debug run still prints the full block. All its lines (incl. the former Files-failed Warning, now carried by the PVE/PBS collection summary) are logging.Debug; logCompressionRatio and logBackupArtifactPaths (only reachable past the guard) are Debug too. - buildBackupOutcomePrompt now appends the SAME block verbatim (appendBackupStatsBlock), after the existing recap lines and before the warnings/errors recap. Identical strings, conditionals and formatters (formatBytes, formatDuration, and a shared compressionRatioText used by both the log and the recap so they cannot drift), only theme-styled instead of logged. The existing recap lines (Files collected - missing (failed), Size, Duration, Archive, Log, storage, Server ID Telegram, Healthchecks link) are UNCHANGED; duplicates are intentional for now. Built via builder -> 2 refuters: HOLD (0 findings). Tests: recap now carries the block + still the old lines; debug-gating proven (absent at Info, present at Debug). build/vet + cmd + -shuffle + no-tview green.
Per the requested cleanup of the two recap sections: - Lower block (the mirrored statistics): drop the '=== Backup Statistics ===' header line; the stats lines now follow directly. - Upper block (the initial recap): remove the Size, Duration, Archive and Local lines (Size/Duration/Archive are still carried by the stats block below; Local is dropped). Files, Log, Secondary/Cloud, Server ID Telegram and Healthchecks link stay; the warnings/errors recap still follows. Dropped the now-unused time + internal/backup imports. Tests updated: Success asserts the trimmed upper block + headerless stats block; Warning/Failure assert the Local line is gone. build/vet + cmd + no-tview green.
Both install paths wrote the binary 0755 and then relied on the security check to reconcile it: verifyBinaryIntegrity requires the executable to be root:root 0700, so with AUTO_FIX_PERMISSIONS on (default) it silently chmod'd 755 -> 700 on every run, and with auto-fix off it emitted a recurring "Executable ... should have permissions 700 (current 755)" warning. Writing 0755-then-fixing-to-700 is pointless churn. Install 0700 up front instead: - upgrade.go installBinary: create the temp target 0700 + explicit Chmod (guards against a leftover .tmp from a prior failed upgrade, whose perms O_TRUNC would otherwise preserve). - install.sh: chmod 700 instead of chmod +x after the mv. - upgrade_helpers_test.go: assert the installed binary is 0700. The binary runs as root and reaches the age identity/keys; owner-only rwx matches the other sensitive paths (secure account, identity/, identity/age) already enforced at 0700.
…lock Remove the lower block's 'Files collected: N' line and move the upper recap's 'Files: N collected - K missing (M failed)' line down to take its place (as the block's first line). The upper recap no longer carries a Files line; the lower block now leads with collected + missing + failed. The separate 'Files failed: M' line in the lower block is dropped too - the moved line already carries '(M failed)', so failed is not shown twice. The rest of the block (Directories/Data/Archive size/Compression/Duration/paths) still mirrors the debug-only log block. Tests updated accordingly. build/vet + cmd + no-tview green.
…t-0700 requirement Supersedes the earlier decision to force the binary to root:root 0700 (commit "install the proxsave binary as root-only 0700"): 0755 is the conventional executable mode and what FHS/package-managed installs (e.g. a hand-built RPM shipping /usr/bin/proxsave) use, so demanding exactly 0700 produced a spurious "should have permissions 700 (current 755)" warning and, with AUTO_FIX_PERMISSIONS on, churned the binary back to 0700 on every run (fighting the package manager). For an executable, integrity means root-owned and not writable by anyone but root; the exact owner mode is irrelevant (the binary is public compiled code, and proxsave needs root at runtime anyway). So: - verifyBinaryIntegrity now enforces root:root ownership + a dedicated ensureExecutableOwnerWriteOnly guard that flags/fixes ONLY a group/other- writable binary (perm & 0o022), clearing just those bits (perm &^ 0o022) and never widening the mode. Both 0755 and 0700 pass; 0777/0775 are corrected. - installers ship the conventional 0755 (install.sh chmod 755; upgrade.go installBinary back to 0o755). - tests: installBinary asserts 0755; the dry-run guard test uses a genuinely group/other-writable 0777 fixture; added positive coverage for the fix path (AUTO_FIX on -> 0755) and the warn-only path (AUTO_FIX off). Key/config/backup paths keep their strict 0700/0600 checks. Docs already recommend 0755 for build/proxsave, so no doc change needed.
…the block bottom - Bundle case now shows 'Bundle contents' before 'Bundle path' (inverted). A base (non-bundle) run is unchanged and still adapts: it shows 'Archive path' + manifest/checksum, no 'Bundle contents' line. - The remaining upper-recap lines (Log, Secondary/Cloud storage status, Server ID Telegram, Healthchecks link) move to the BOTTOM of the stats block, so the outcome is a single block: Files ... paths, then Log/storage/ identity, then the warnings/errors recap.
…-install The dashboard item sets args.Install=true (identical to the CLI --install flag) but was labelled 'Reconfigure', a different name than the known command. Rename it to 'Install' and note '(--install)' in the description so the dashboard and CLI use the same vocabulary. Only the display label changes; the ActionReconfigure enum + dispatch are unchanged.
Add an ActionNewInstall menu item, 'New install', right under 'Install' in the Maintenance group. The dashboard dispatch sets args.NewInstall=true (the CLI --new-install flag), routing to runNewInstallMode -> runNewInstall, which already confirms the destructive base-dir wipe itself (confirmNewInstallCharm) before resetting - so no extra dashboard confirmation is needed. Description states it wipes the install directory (keeps build/env/ identity). Updated the menu row-order test and every dashboard/daemon navigation test whose hardcoded down-count shifted by one now that a selectable row was inserted after 'Install'.
… (red) The decrypt workflow surfaced its three non-fatal conditions (backup scan failed, no backups found, no encrypted backups) via ui.ShowError -> a RED error Notice, even though the code logs them as logger.Warning and the flow just skips the source and continues. That is a cosmetic mismatch: an empty-state is not an error. Add a ShowWarning to the workflow UI (charm -> NoticeWarning yellow, cli -> stdout like ShowMessage) and route the three conditions through it, so the notice colour matches the severity like every other screen. Genuine errors (e.g. network preflight rollback) keep ShowError. Test mocks gain ShowWarning.
… Notice The decrypt workflow surfaced its non-fatal outcomes via components.Notice (ShowWarning/ShowError), a DIFFERENT component than the daemon/check/audit result screens, which all use a styled Selector with a 'Status: <colored keyword>' + Subtle explanation prompt (showDaemonResultScreen / buildDaemonResultPrompt). That is a graphical inconsistency. Add renderWorkflowStatusLevel + buildWorkflowStatusPrompt (byte-identical to the daemon renderers) and a ShowStatusResult method that draws the SAME 'Status:' selector; route the three decrypt outcomes (SCAN FAILED / NO BACKUPS FOUND / NO ENCRYPTED BACKUPS, Warn level, yellow) through it. Replaces the ShowWarning added earlier (wrong component). ShowError stays for the genuine network-preflight failure. Built via builder -> 2 refuters: HOLD (0 findings, pattern parity verified). build/vet + orchestrator + no-tview green.
…ector Audit found 4 more outcome screens still using components.Notice instead of the styled 'Status: <colored keyword>' selector every check/daemon result screen uses. Converted them (reusing showDaemonResultScreen / ShowStatusResult, no new renderer): - dashboard 'X not configured' (diagnostic result) -> Status: warn NOT CONFIGURED - decrypt success -> Status: ok DECRYPT COMPLETE (now matches the decrypt failures) - workflow ShowError (network preflight failure) -> Status: error <UPPER title> - install 'Encryption ready' -> Status: ok ENCRYPTION READY The ShowError interface + call site and the plain ShowMessage Info notices (NIC repair / full restore / rollback) are untouched. Tests updated (waitScreen titles + a waitOutput helper). Built via builder -> 2 refuters: HOLD (0 real findings). build/vet + orchestrator + cmd + no-tview green.
…sted
After the decrypt flow shows its graceful 'Status:' empty-state screen (e.g. 'NO ENCRYPTED BACKUPS') and the user
dismisses it, the exhausted-sources path returned a plain fmt.Errorf("no usable backup sources available") which
main surfaced as a redundant '[ts] ERROR ERROR: no usable backup sources available' line in the CLI - the user
already saw the outcome on screen. Add a sentinel ErrDecryptNoBackups and handle it in runDecryptOnlyMode like
ErrDecryptAborted: a terse Info line + clean ExitSuccess, no ERROR. The error message string is unchanged, so the
existing 'no usable backup sources' test assertions still hold. Pre-scan 'no backup paths configured' stays a real
error.
The previous change turned the redundant ERROR into an INFO line; the ask was to REMOVE it. The user already saw the graceful 'Status:' empty-state screen, so runDecryptOnlyMode now exits cleanly (ExitSuccess) with NO log line at all for ErrDecryptNoBackups.
…p it in CLI --decrypt Gate the ErrDecryptNoBackups clean-exit on dashboardIsBareInvocation(): only a bare (interactive dashboard) run skips the log line, because the user already saw the graceful 'Status:' empty-state screen there. A CLI --decrypt execution falls through to the original 'ERROR: no usable backup sources available' + ExitGenericError, so its CLI-execution log lines are left untouched.
…mmands Fully remove the very old legacy config-migration commands and everything exclusive to them (multi-agent mapped): - flags: EnvMigration, EnvMigrationDry, and the --old-env / LegacyEnvPath they alone consumed (internal/cli/args.go) - mode handlers + dispatch + exclusivity entries (main_config_modes.go, main_modes.go) - command impl cmd/proxsave/env_migration.go (+ test) - deleted - internal/config/migration.go (EnvMigrationSummary, MigrateLegacyEnv, PlanLegacyEnvMigration + all helpers) (+ test) - deleted; these had ZERO non-env-migration callers (verified) - help text (main_footer.go, install.go, config_helpers.go) + the config.go comment - ALL doc citations: deleted docs/MIGRATION_GUIDE.md + docs/BACKUP_ENV_MAPPING.md; scrubbed README/DEVELOPER_GUIDE/ EXAMPLES/TROUBLESHOOTING/INSTALL/CLI_REFERENCE (sections, table rows, links, TOC entries) KEPT (shared, still used elsewhere): parseEnvFile, blockValueKeys, BackupCephConfig parsing, resolveInstallConfigPath, and the captureStdout test helper (extracted to cmd/proxsave/testhelpers_test.go, used by 6 other test files). Built via a multi-agent map -> remove -> 2-refuter workflow. build/vet + cmd/cli/config tests + no-tview green; zero residual env-migration references in code or docs.
Surface --cleanup-guards in the dashboard as an in-session action under a new Recovery section (just above Exit). It runs result-only, like the check screens: no streaming, only the styled "Status:" outcome. Two-step: selecting it first runs a DRY RUN and shows its "Status:" preview with Apply / Cancel; only on Apply does it run the cleanup for real and show the final outcome, then loops back to the menu. The captured cleanup log is classified into Warn/DRY RUN, Ok/DONE, Ok/NOTHING TO CLEAN, Warn/PENDING, or Error/FAILED (the root-required error lands on a red FAILED screen). It reuses buildDaemonResultPrompt/ showDaemonResultScreen verbatim so it can never disagree visually with the daemon/ check result screens, and sets no flag (runs entirely in-session). Placed in a Recovery section so only Exit shifts one row (nav-count churn minimized). Tests: outcome classification, log-prefix stripping, and driver tests for the two-step Apply and the Cancel-skips-apply paths.
The first step is now a read-only CHECK, not a step labelled "DRY RUN": - nothing to unlock -> GREEN "Clean", action is Check (re-scan), NO Apply; - guards present -> YELLOW "Found", action is Apply. Apply then runs the real cleanup and shows DONE / PENDING / FAILED. "DRY RUN" never appears in the UI anymore. To classify Clean vs Found (and the apply outcome) without parsing logs, add orchestrator.CleanupMountGuardsReport returning a structured GuardCleanupReport (guard-dir present, bind guards, immutable flags, remaining/pending, dir removed). CleanupMountGuards keeps its error-only signature (wraps the report variant), so every existing caller/test is untouched. The dashboard builds clean, dry-run-free explanations from the report. Tests: check wording + pluralization, apply classification, and driver tests for Found->Apply, Clean->Back, Clean->re-Check, and Found->Cancel.
…ly brick Add "Update config" to the dashboard (below Updates), surfacing --upgrade-config as a two-step, result-only action: a read-only CHECK (--upgrade-config-dry-run) classifies "Up to date" (green, action Check) vs "Update available" (yellow, action Apply); Apply runs the real merge (--upgrade-config: backup + rollback) and shows the outcome. The two-step check -> Clean/Found -> Apply flow was identical to Cleanup guards, so extract it into ONE reusable brick, runDashboardCheckApply, that owns only the shared flow and screens; each feature supplies its own check/apply/describe logic. Cleanup guards is refactored onto the brick with byte-identical behaviour (its driver tests pass unchanged bar the +1 row shift). No monolith: the brick is a small flow helper, feature logic stays in each feature file. Menu: Update config sits just below Updates, shifting the later rows one down (nav-count tests updated accordingly). Tests: plan/apply wording + driver tests for Update available -> Apply, Up to date -> Back, Up to date -> re-Check, and Update available -> Cancel.
Remove the standalone "Update config" menu row and surface it as a third button inside Updates: the Upgrade screen now offers Check upgrade / Check config / Back. "Check config" runs the exact same two-step config-update flow (runDashboardUpdateConfig) as before; only its entry point moved, from a menu row to a button under Updates. Dropping the menu row restores the pre-existing navigation-count layout (the +1 shift from adding the row is reverted). The Upgrade-screen test's Back item moved from 2nd to 3rd (Check config sits between), and the Update config driver tests now reach the flow via Updates -> Check config. The shared check/apply brick and the config flow are unchanged.
…ckup note from Apply Put a newline after "Found N ... to add." so the check explanation reads on two lines, and remove "(a backup is saved first)" from the Apply button description since the line above already states it.
…pply screen The two-step brick's Found screen used "Cancel" for its go-back action while every other screen (including its own Clean branch) uses "Back". Make the secondary "Back" everywhere for consistency; applies to both Cleanup guards and Update config via the shared brick.
… config -> Upgrade config Align the dashboard labels with the CLI ground truth (--upgrade / --upgrade-config): the menu row "Updates" becomes "Upgrade" (matching the screen title and buttons), and the config flow's screen title "Update config" becomes "Upgrade config" (matching --upgrade-config). Driver test waitScreen titles updated to match.
…nside the binary screen Entering Upgrade now shows a chooser (Check upgrade / Check config / Back), and Check upgrade opens the binary upgrade screen on its own (Check upgrade / Run upgrade / Back) with no config button bolted on. Check config opens the config upgrade flow. This fixes the oddity where pressing Check upgrade re-rendered a screen that still carried Check config. runDashboardUpgradeMenu is the new chooser; runDashboardUpgrade drops the config item and is reached from it. The menu dispatch points at the chooser. Tests: new chooser test; the binary-screen test's Back reverts to the 2nd item; the config driver tests reach the flow via the chooser.
Add short descriptions to the Upgrade chooser's Check upgrade and Check config buttons so the two are distinguishable at a glance (binary release vs config template keys).
After the check, the Update available screen now lists the missing keys that Apply would add (one per line, under "Keys to add:"), capped at 12 with "… and N more" so a large template bump can't push the menu items off-screen.
… (Edit / Wipe)
Replace the two menu rows ("Install", "New install") with a single "Install" row that opens
an in-session chooser: "Edit install" -> the --install flow, "Wipe install" -> the
--new-install flow, Back -> menu. Only the dashboard labels are new; the CLI flags are
unchanged. The chooser resolves to the existing ActionReconfigure / ActionNewInstall, which
fall through to the exact same flag dispatch as before.
Dropping a menu row shifts the later rows up one (nav-count tests adjusted). The two install
dispatches are now covered by dedicated chooser driver tests (Edit/Wipe/Back) instead of the
fall-through table.
A standalone dry-run must not touch the backup-outcome monitor: it is a test, not a real backup. The post-install audit runs `proxsave --dry-run` as a subprocess to harvest "set KEY=false" hints and exits 1 on warnings; without a gate, maybeHandoffManualBackup handed that off to the daemon, which pinged the backup healthcheck with a phantom exit=1 (a false failure), and left the backup check looking fresher than the last real run. Gate maybeHandoffManualBackup on opts.dryRun. Add TestMaybeHandoffSkipsDryRun (probe never consulted, nothing written).
…de/config/guards/daemon) Make the check screens behave like Daemon status: run the check immediately on entry and label the re-run button "Re-check". - Check upgrade (runDashboardUpgrade): auto-run the release check on entry (no more "NOT CHECKED" pre-state); the button is "Re-check" (no update) or "Run upgrade". - Daemon status: rename the "Check" button to "Re-check". - The two-step brick's Clean/Up-to-date branch: rename its primary "Check" to "Re-check" (covers both Cleanup guards and Upgrade config, which already auto-check on entry). Tests updated: the upgrade-screen test no longer waits for NOT CHECKED / a manual Check press; the chooser test stubs the check (the binary screen now auto-checks on entry).
…check button In the dashboard (backToMenu=true) the Telegram-pairing and Healthchecks screens now run their check automatically on entry -- like Daemon status -- instead of waiting for a manual Check press, and the button reads "Re-check". The installer path (backToMenu=false) is unchanged: the first check stays manual (button "Check"). Both screens are shared with the install wizard: the verify logic moves to the top of the loop, guarded by a pendingCheck flag (initialized to backToMenu); the Check action just sets pendingCheck. The two healthcheck dashboard tests stub the check seam so the on-entry check is deterministic (no network).
…amed support backup Add a "Support" row (recovery group) that reproduces the --support intro graphically: a consent step (the DEBUG log — which may contain sensitive data, incl. this server's MAC — is emailed to github-support@tis24.it, and a GitHub issue must already be open), the GitHub nickname, the issue id (#1234), and a final confirm. On confirm it arms support mode (args.Support + the collected meta + args.SupportMetaProvided) and falls through to the SAME handoff as Backup, so the run streams in-graphics identically -- support is just a backup with a debug+email wrapper (main_runtime + main_defers), so the streamed run brick is reused with zero duplication. handleSupportIntro skips the stdin RunIntro when SupportMetaProvided is set (the dashboard already collected everything), so it never prompts over the graphical run. The form is a seam (dashboardRunSupportForm) so the dispatch is unit-tested; validateSupportIssue mirrors the CLI validation. Menu nav: Support sits between Cleanup guards and Exit (only Exit shifts).
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tis24dev, your pull request is larger than the review limit of 150000 diff characters
📝 WalkthroughWalkthroughThis PR adds dashboard-driven workflows, streamed backup output, structured status screens, and cleanup reporting; removes legacy environment migration; updates logging, healthcheck-link handling, decryption outcomes, stream rendering, and executable permission validation; and adds broad integration and unit test coverage. ChangesBackup streaming and dashboard workflows
Legacy migration removal
Workflow, backend, and security updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
internal/ui/components/streamtask_test.go (1)
73-94: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover non-SGR terminal escapes in this sanitizer test.
BEL coverage does not verify the primary terminal-injection boundary. Add cases for OSC 52 and CSI cursor/mode sequences, asserting they are removed while SGR remains.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ui/components/streamtask_test.go` around lines 73 - 94, Extend TestStreamScreenKeepsANSIInRing with input containing OSC 52 and CSI cursor/mode escape sequences, then assert those sequences are absent from scr.lines while the existing SGR escapes and text remain preserved. Keep the current BEL assertion and use representative terminal-injection cases covering both OSC and CSI sanitization.internal/logging/capture_test.go (1)
238-246: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the colored bootstrap mirror emits ANSI.
These checks pass even if
CaptureConsoleWithColorcreates an uncolored mirror. Assert that the bootstrap-captured line contains an escape sequence.Proposed test assertion
if !containsAll(captured[1], "INFO", "world") { t.Fatalf("second captured line %q missing INFO/world", captured[1]) } + if !strings.Contains(captured[1], "\x1b[") { + t.Fatalf("colored bootstrap line missing ANSI: %q", captured[1]) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/logging/capture_test.go` around lines 238 - 246, Update the assertions in the captured-line test to verify that the bootstrap mirror output includes an ANSI escape sequence, in addition to the existing INFO and message checks. Apply this to the relevant captured line produced by CaptureConsoleWithColor, preserving the current content assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/proxsave/backup_stream.go`:
- Around line 155-169: Move the runLogPath() block that renders the “Log:” entry
outside the res.supportStats != nil condition so it runs for all outcomes,
including initialization failures. Keep the backup statistics rendering
conditional while preserving the existing path fallback and formatting.
In `@cmd/proxsave/dashboard_cleanup_guards.go`:
- Around line 86-90: Update describeGuardApply to avoid claiming remaining
guards are caused by a live mount in every failure case. Use the
GuardCleanupReport fields, including GuardsRemaining and ImmutablePending, to
distinguish live-mount cleanup from other pending conditions, and provide a
generic or appropriately tailored message when unmounting will not resolve the
issue.
In `@cmd/proxsave/dashboard_support.go`:
- Around line 117-121: Update the issue validation around strconv.Atoi to accept
only positive decimal digit IDs: reject signs, zero, and any non-digit
characters after the leading '#'. Preserve the existing validation errors and
valid inputs such as `#1234`.
In `@internal/orchestrator/decrypt_workflow_ui.go`:
- Line 79: Propagate abort errors from each ui.ShowStatusResult call in
internal/orchestrator/decrypt_workflow_ui.go at lines 79, 93, and 105 by
checking the returned error and immediately returning nil, err when non-nil; do
not discard these errors so the decrypt workflow aborts instead of continuing or
returning ErrDecryptNoBackups.
In `@internal/ui/components/sanitize.go`:
- Around line 49-55: Update sanitizeStreamLine around ansi.DecodeSequence to
detect zero-width tab and newline tokens before the width check, writing them
unchanged instead of dropping them. Preserve existing handling for printable
sequences and SGR escapes, and add regression coverage confirming tabs and
newlines remain in the sanitized output.
In `@internal/ui/components/streamtask.go`:
- Around line 117-122: Update the stream-task history truncation logic around
t.lines and t.dropped so that when follow is false, the viewport offset is
reduced by the number of discarded oldest lines (or equivalent stable ring
coordinates are maintained). Preserve the currently viewed line while enforcing
streamLineCap, while leaving follow mode behavior unchanged.
- Around line 243-248: Optimize the render path around the viewport content
update in the stream task component by avoiding strings.Join and SetContent when
t.lines has not changed. Add content caching or dirty tracking tied to line
mutations, while preserving the current-width measurement and follow-mode
GotoBottom behavior whenever content is actually refreshed.
- Around line 233-239: Update the body-height reservation around the outcome
handling in the stream task view to count the outcome’s soft-wrapped rows, not
only explicit newlines. Use the available terminal width and an ANSI-aware
measurement/wrapping helper, and apply the same calculation in the related block
around the outcome rendering so narrow terminals reserve enough space for the
final status.
---
Nitpick comments:
In `@internal/logging/capture_test.go`:
- Around line 238-246: Update the assertions in the captured-line test to verify
that the bootstrap mirror output includes an ANSI escape sequence, in addition
to the existing INFO and message checks. Apply this to the relevant captured
line produced by CaptureConsoleWithColor, preserving the current content
assertions.
In `@internal/ui/components/streamtask_test.go`:
- Around line 73-94: Extend TestStreamScreenKeepsANSIInRing with input
containing OSC 52 and CSI cursor/mode escape sequences, then assert those
sequences are absent from scr.lines while the existing SGR escapes and text
remain preserved. Keep the current BEL assertion and use representative
terminal-injection cases covering both OSC and CSI sanitization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13e22dcd-e128-4ef2-bf84-72e49186de9f
📒 Files selected for processing (86)
cmd/proxsave/backup_execution.gocmd/proxsave/backup_execution_test.gocmd/proxsave/backup_exit_glyph_test.gocmd/proxsave/backup_healthcheck.gocmd/proxsave/backup_healthcheck_test.gocmd/proxsave/backup_mode.gocmd/proxsave/backup_stream.gocmd/proxsave/backup_stream_test.gocmd/proxsave/config_helpers.gocmd/proxsave/dashboard.gocmd/proxsave/dashboard_check_apply.gocmd/proxsave/dashboard_cleanup_guards.gocmd/proxsave/dashboard_cleanup_guards_test.gocmd/proxsave/dashboard_support.gocmd/proxsave/dashboard_support_test.gocmd/proxsave/dashboard_test.gocmd/proxsave/dashboard_update_config.gocmd/proxsave/dashboard_update_config_test.gocmd/proxsave/dashboard_upgrade.gocmd/proxsave/dashboard_upgrade_test.gocmd/proxsave/env_migration.gocmd/proxsave/env_migration_test.gocmd/proxsave/install.gocmd/proxsave/install_finalize_stream_test.gocmd/proxsave/install_outcome.gocmd/proxsave/install_tui.gocmd/proxsave/main.gocmd/proxsave/main_config_modes.gocmd/proxsave/main_footer.gocmd/proxsave/main_footer_test.gocmd/proxsave/main_modes.gocmd/proxsave/main_restore_decrypt.gocmd/proxsave/main_support.gocmd/proxsave/monitoring_portal_link_test.gocmd/proxsave/newkey_charm_test.gocmd/proxsave/runtime_helpers.gocmd/proxsave/testhelpers_test.gocmd/proxsave/upgrade.gocmd/proxsave/upgrade_helpers_test.godocs/BACKUP_ENV_MAPPING.mddocs/CLI_REFERENCE.mddocs/DEVELOPER_GUIDE.mddocs/EXAMPLES.mddocs/INSTALL.mddocs/MIGRATION_GUIDE.mddocs/README.mddocs/TROUBLESHOOTING.mdinstall.shinternal/backup/collector_bricks.gointernal/backup/collector_bricks_pve.gointernal/backup/collector_bricks_pve_finalize.gointernal/backup/collector_bricks_test.gointernal/cli/args.gointernal/cli/args_test.gointernal/config/config.gointernal/config/migration.gointernal/config/migration_test.gointernal/config/scheduler_healthcheck_test.gointernal/logging/capture.gointernal/logging/capture_test.gointernal/notify/telegram.gointernal/orchestrator/decrypt.gointernal/orchestrator/decrypt_charm_e2e_test.gointernal/orchestrator/decrypt_tui.gointernal/orchestrator/decrypt_workflow_ui.gointernal/orchestrator/decrypt_workflow_ui_test.gointernal/orchestrator/guards_cleanup.gointernal/orchestrator/healthcheck_section.gointernal/orchestrator/healthcheck_section_test.gointernal/orchestrator/restore_workflow_ui_helpers_test.gointernal/orchestrator/workflow_ui.gointernal/orchestrator/workflow_ui_charm.gointernal/orchestrator/workflow_ui_cli.gointernal/orchestrator/workflow_ui_status.gointernal/security/security.gointernal/security/security_timeout_test.gointernal/ui/components/sanitize.gointernal/ui/components/streamtask.gointernal/ui/components/streamtask_test.gointernal/ui/flows/install/healthcheck.gointernal/ui/flows/install/healthcheck_test.gointernal/ui/flows/install/telegram.gointernal/ui/flows/menu/menu.gointernal/ui/flows/menu/menu_test.gointernal/ui/shell/router_test.gointernal/ui/theme/theme.go
💤 Files with no reviewable changes (15)
- docs/BACKUP_ENV_MAPPING.md
- cmd/proxsave/main.go
- internal/config/migration.go
- docs/README.md
- cmd/proxsave/env_migration_test.go
- cmd/proxsave/env_migration.go
- docs/TROUBLESHOOTING.md
- docs/MIGRATION_GUIDE.md
- cmd/proxsave/install.go
- cmd/proxsave/main_config_modes.go
- docs/DEVELOPER_GUIDE.md
- docs/EXAMPLES.md
- internal/config/scheduler_healthcheck_test.go
- internal/config/migration_test.go
- docs/INSTALL.md
Comment on lines
+155
to
+169
| if st := res.supportStats; st != nil { | ||
| // The backup-statistics block (headerless); the log block is now debug-only. | ||
| b.WriteString("\n") | ||
| appendBackupStatsBlock(&b, st) | ||
|
|
||
| // Appended at the BOTTOM of the block: the run log path, then storage status, | ||
| // then centralized-mode identity. | ||
| // | ||
| // The run log file is CLOSED during the log-management phase before this | ||
| // outcome is built, so GetLogFilePath may be "" by now; runLogPath falls back | ||
| // to the LOG_FILE the runtime exports at startup. | ||
| if lp := runLogPath(); lp != "" { | ||
| b.WriteString("\n") | ||
| b.WriteString(theme.Text.Render("Log: " + lp)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Show the run log path when statistics are unavailable.
Log: is nested under res.supportStats != nil, so early configuration or initialization failures omit the diagnostic log path. Move the runLogPath() block outside this condition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/proxsave/backup_stream.go` around lines 155 - 169, Move the runLogPath()
block that renders the “Log:” entry outside the res.supportStats != nil
condition so it runs for all outcomes, including initialization failures. Keep
the backup statistics rendering conditional while preserving the existing path
fallback and formatting.
Comment on lines
+86
to
+90
| func describeGuardApply(r orchestrator.GuardCleanupReport) string { | ||
| if guardApplyClean(r) { | ||
| return "Removed the restore mount guards — the storage is unlocked." | ||
| } | ||
| return "Some guards are still in place (hidden under a live mount). Unmount the datastore and run Cleanup guards again once it is offline." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid attributing every pending cleanup to a live mount.
This branch also covers GuardsRemaining == -1 and ImmutablePending > 0, where unmounting the datastore may not address the problem. Use a generic message or tailor it to the report fields.
Proposed fix
- return "Some guards are still in place (hidden under a live mount). Unmount the datastore and run Cleanup guards again once it is offline."
+ return "Some guards remain or could not be verified. Unmount the datastore, check immutable flags, and run Cleanup guards again."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func describeGuardApply(r orchestrator.GuardCleanupReport) string { | |
| if guardApplyClean(r) { | |
| return "Removed the restore mount guards — the storage is unlocked." | |
| } | |
| return "Some guards are still in place (hidden under a live mount). Unmount the datastore and run Cleanup guards again once it is offline." | |
| func describeGuardApply(r orchestrator.GuardCleanupReport) string { | |
| if guardApplyClean(r) { | |
| return "Removed the restore mount guards — the storage is unlocked." | |
| } | |
| return "Some guards remain or could not be verified. Unmount the datastore, check immutable flags, and run Cleanup guards again." | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/proxsave/dashboard_cleanup_guards.go` around lines 86 - 90, Update
describeGuardApply to avoid claiming remaining guards are caused by a live mount
in every failure case. Use the GuardCleanupReport fields, including
GuardsRemaining and ImmutablePending, to distinguish live-mount cleanup from
other pending conditions, and provide a generic or appropriately tailored
message when unmounting will not resolve the issue.
Comment on lines
+117
to
+121
| if !strings.HasPrefix(issue, "#") || len(issue) < 2 { | ||
| return fmt.Errorf("issue must start with '#' and a numeric id, e.g. #1234") | ||
| } | ||
| if _, err := strconv.Atoi(issue[1:]); err != nil { | ||
| return fmt.Errorf("issue must be #<number>, e.g. #1234") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject signed and zero issue IDs.
strconv.Atoi accepts #-1, #+1, and #0, although GitHub issue IDs must be positive decimal digits.
Proposed fix
- if _, err := strconv.Atoi(issue[1:]); err != nil {
+ if id, err := strconv.ParseUint(issue[1:], 10, 64); err != nil || id == 0 {
return fmt.Errorf("issue must be #<number>, e.g. `#1234`")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if !strings.HasPrefix(issue, "#") || len(issue) < 2 { | |
| return fmt.Errorf("issue must start with '#' and a numeric id, e.g. #1234") | |
| } | |
| if _, err := strconv.Atoi(issue[1:]); err != nil { | |
| return fmt.Errorf("issue must be #<number>, e.g. #1234") | |
| if !strings.HasPrefix(issue, "#") || len(issue) < 2 { | |
| return fmt.Errorf("issue must start with '#' and a numeric id, e.g. `#1234`") | |
| } | |
| if id, err := strconv.ParseUint(issue[1:], 10, 64); err != nil || id == 0 { | |
| return fmt.Errorf("issue must be #<number>, e.g. `#1234`") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/proxsave/dashboard_support.go` around lines 117 - 121, Update the issue
validation around strconv.Atoi to accept only positive decimal digit IDs: reject
signs, zero, and any non-digit characters after the leading '#'. Preserve the
existing validation errors and valid inputs such as `#1234`.
| if scanErr != nil { | ||
| logger.Warning("Failed to inspect %s: %v", option.Path, scanErr) | ||
| _ = ui.ShowError(ctx, "Backup scan failed", fmt.Sprintf("Failed to inspect %s: %v", option.Path, scanErr)) | ||
| _ = ui.ShowStatusResult(ctx, "Decrypt", HealthcheckSetupLevelWarn, "SCAN FAILED", fmt.Sprintf("Failed to inspect %s: %v", option.Path, scanErr)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate abort errors from ui.ShowStatusResult.
By assigning the result of ui.ShowStatusResult to _, abort signals (such as pressing ctrl+c or the terminal closing) are swallowed. This causes the loop to incorrectly continue to the next option or incorrectly return ErrDecryptNoBackups, instead of immediately aborting the workflow.
internal/orchestrator/decrypt_workflow_ui.go#L79-L79: wrap theui.ShowStatusResultcall in anif err := ...; err != nil { return nil, err }block.internal/orchestrator/decrypt_workflow_ui.go#L93-L93: apply the same error check.internal/orchestrator/decrypt_workflow_ui.go#L105-L105: apply the same error check.
📍 Affects 1 file
internal/orchestrator/decrypt_workflow_ui.go#L79-L79(this comment)internal/orchestrator/decrypt_workflow_ui.go#L93-L93internal/orchestrator/decrypt_workflow_ui.go#L105-L105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/orchestrator/decrypt_workflow_ui.go` at line 79, Propagate abort
errors from each ui.ShowStatusResult call in
internal/orchestrator/decrypt_workflow_ui.go at lines 79, 93, and 105 by
checking the returned error and immediately returning nil, err when non-nil; do
not discard these errors so the decrypt workflow aborts instead of continuing or
returning ErrDecryptNoBackups.
Comment on lines
+49
to
+55
| seq, width, n, newState := ansi.DecodeSequence(s, state, nil) | ||
| if width > 0 { | ||
| // A printable grapheme cluster: strip control runes only. | ||
| b.WriteString(stripStreamText(seq)) | ||
| } else if isSGR(seq) { | ||
| // A color/style escape (ESC[...m): keep it verbatim. | ||
| b.WriteString(seq) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline internal/ui/components/sanitize.go --view expanded || true
printf '\n== Relevant lines ==\n'
cat -n internal/ui/components/sanitize.go | sed -n '1,220p'
printf '\n== Search for DecodeSequence usage ==\n'
rg -n "DecodeSequence|stripStreamText|isSGR|WriteByte\\(' '\\)|\\t|\\n" internal/ui/components/sanitize.goRepository: tis24dev/proxsave
Length of output: 4117
🏁 Script executed:
python3 - <<'PY'
import os, pathlib, re, sys, json
p = pathlib.Path("internal/ui/components/sanitize.go")
text = p.read_text()
print("contains tab branch:", "seq == \"\\n\" || seq == \"\\t\"" in text)
print("contains stripStreamText:", "stripStreamText" in text)
print("contains isSGR:", "isSGR" in text)
PYRepository: tis24dev/proxsave
Length of output: 234
🏁 Script executed:
set -euo pipefail
printf '\n== Locate ansi.DecodeSequence source ==\n'
go env GOPATH GOMODCACHE 2>/dev/null || true
modcache="$(go env GOMODCACHE 2>/dev/null || true)"
if [ -n "${modcache:-}" ] && [ -d "$modcache" ]; then
rg -n "func DecodeSequence|DecodeSequence\\(" "$modcache" -g 'decode*.go' -g '*.go' | head -n 50
fi
printf '\n== Search module cache for charmbracelet/x/ansi ==\n'
if [ -n "${modcache:-}" ] && [ -d "$modcache" ]; then
fd -a 'ansi' "$modcache" | head -n 50
fi
printf '\n== Search repository for sanitizeStreamLine call sites ==\n'
rg -n "sanitizeStreamLine\\(" .
printf '\n== Search for tests around sanitizeStreamLine ==\n'
rg -n "sanitizeStreamLine|sanitizeLine|stripStreamText|DecodeSequence" internal -g '*_test.go'Repository: tis24dev/proxsave
Length of output: 3842
🏁 Script executed:
set -euo pipefail
modcache="$(go env GOMODCACHE)"
printf '\n== DecodeSequence implementation ==\n'
sed -n '70,170p' "$modcache/github.com/charmbracelet/x/ansi@v0.11.7/parser_decode.go"
printf '\n== DecodeSequence tests mentioning control chars ==\n'
rg -n "\\\\n|\\\\t|control|newline|tab|width" "$modcache/github.com/charmbracelet/x/ansi@v0.11.7/parser_decode_test.go" -A 3 -B 3
printf '\n== Repository sanitizer tests ==\n'
cat -n internal/ui/components/sanitize_test.go | sed -n '1,220p'Repository: tis24dev/proxsave
Length of output: 11829
🏁 Script executed:
set -euo pipefail
modcache="$(go env GOMODCACHE)"
printf '\n== Remaining decodeSequence control branches ==\n'
sed -n '170,280p' "$modcache/github.com/charmbracelet/x/ansi@v0.11.7/parser_decode.go"
printf '\n== Explicit newline/tab cases in ansi module ==\n'
rg -n "\\\\n|\\\\t|TAB|LF|newline|carriage return|control" "$modcache/github.com/charmbracelet/x/ansi@v0.11.7" -g '*.go' -A 2 -B 2
printf '\n== Any tests for control bytes in DecodeSequence ==\n'
rg -n "0x09|0x0a|\\\t|\\\n|tab|newline" "$modcache/github.com/charmbracelet/x/ansi@v0.11.7/parser_decode_test.go" -A 2 -B 2Repository: tis24dev/proxsave
Length of output: 50373
Preserve tabs and newlines in sanitizeStreamLine internal/ui/components/sanitize.go:49-55
ansi.DecodeSequence returns \n and \t as zero-width control tokens, so they bypass stripStreamText and get dropped; "a\tb" becomes "ab". Handle these tokens before the width check and add regression coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/ui/components/sanitize.go` around lines 49 - 55, Update
sanitizeStreamLine around ansi.DecodeSequence to detect zero-width tab and
newline tokens before the width check, writing them unchanged instead of
dropping them. Preserve existing handling for printable sequences and SGR
escapes, and add regression coverage confirming tabs and newlines remain in the
sanitized output.
Comment on lines
+117
to
+122
| t.lines = append(t.lines, sanitizeStreamLine(msg.Line)) | ||
| if len(t.lines) > streamLineCap { | ||
| // Drop the oldest lines beyond the cap (bounded ring) and | ||
| // remember it so the panel can note the truncation. | ||
| t.lines = t.lines[len(t.lines)-streamLineCap:] | ||
| t.dropped = true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the viewed line when truncating history.
With follow == false, removing the oldest line while retaining the same viewport offset advances the visible content on every new message. Decrement the offset by the dropped count, or use stable ring coordinates, so manual review is not gradually yanked forward.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/ui/components/streamtask.go` around lines 117 - 122, Update the
stream-task history truncation logic around t.lines and t.dropped so that when
follow is false, the viewport offset is reduced by the number of discarded
oldest lines (or equivalent stable ring coordinates are maintained). Preserve
the currently viewed line while enforcing streamLineCap, while leaving follow
mode behavior unchanged.
Comment on lines
+233
to
+239
| reserved := lipglossCount(headerStr) + 1 /*rule*/ + 1 /*scroll row*/ | ||
| if outcome != "" { | ||
| reserved += lipglossCount(outcome) + 1 /*rule below panel*/ | ||
| } | ||
| bodyH := height - reserved | ||
| if bodyH < 1 { | ||
| bodyH = 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Account for soft-wrapped rows when reserving outcome space.
lipglossCount only counts explicit newlines. Long outcome lines wrap at terminal width, leaving bodyH oversized and potentially clipping the final status on narrow terminals. Make the row calculation width- and ANSI-aware.
Also applies to: 276-282
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/ui/components/streamtask.go` around lines 233 - 239, Update the
body-height reservation around the outcome handling in the stream task view to
count the outcome’s soft-wrapped rows, not only explicit newlines. Use the
available terminal width and an ANSI-aware measurement/wrapping helper, and
apply the same calculation in the related block around the outcome rendering so
narrow terminals reserve enough space for the final status.
Comment on lines
+243
to
+248
| // Set the content HERE, after sizing, so it is always measured/soft-wrapped | ||
| // against the CURRENT width (never a stale or zero width from an earlier | ||
| // Update), and re-pin to the bottom while following. | ||
| t.vp.SetContent(strings.Join(t.lines, "\n")) | ||
| if t.follow { | ||
| t.vp.GotoBottom() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Avoid rebuilding the full 5,000-line viewport on every render.
View joins and resets all retained lines for spinner ticks, keypresses, and stream updates—even when history is unchanged. Cache/dirty-track viewport content or batch updates to prevent long, high-volume runs from becoming progressively sluggish.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/ui/components/streamtask.go` around lines 243 - 248, Optimize the
render path around the viewport content update in the stream task component by
avoiding strings.Join and SetContent when t.lines has not changed. Add content
caching or dirty tracking tied to line mutations, while preserving the
current-width measurement and follow-mode GotoBottom behavior whenever content
is actually refreshed.
This was referenced Jul 31, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Slice 4 of 6 — progressive rebuild of dev (base=dev), cherry-picked checkpoint range, tree matches
a71ab67,go build ./...verified. ~86 files.Content: dashboard Update config + shared two-step check/apply brick, Support (recovery) graphical form + streamed support backup, backup refinements, decrypt fixes.
Merges into dev (no release/tag). Trigger review with
@coderabbitai review.Summary by CodeRabbit
New Features
Bug Fixes
Removed
backup.envmigration commands and related documentation are no longer available.Greptile Summary
This PR expands the dashboard backup and recovery workflows. The main changes are:
Confidence Score: 4/5
The streamed dashboard backup path needs a panic-cleanup fix before merging.
internal/ui/components/streamtask.go
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant User participant Dashboard participant BackupMode participant StreamTask participant BackupRun User->>Dashboard: Choose Backup or Support Dashboard->>Dashboard: Keep and stash session Dashboard->>BackupMode: Continue normal dispatch BackupMode->>StreamTask: Adopt session and start stream StreamTask->>BackupRun: Run backup with streaming emit BackupRun-->>StreamTask: Log lines and outcome StreamTask-->>User: Show stream and result User->>StreamTask: Continue StreamTask-->>BackupMode: Return result BackupMode->>Dashboard: Close session%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant User participant Dashboard participant BackupMode participant StreamTask participant BackupRun User->>Dashboard: Choose Backup or Support Dashboard->>Dashboard: Keep and stash session Dashboard->>BackupMode: Continue normal dispatch BackupMode->>StreamTask: Adopt session and start stream StreamTask->>BackupRun: Run backup with streaming emit BackupRun-->>StreamTask: Log lines and outcome StreamTask-->>User: Show stream and result User->>StreamTask: Continue StreamTask-->>BackupMode: Return result BackupMode->>Dashboard: Close sessionComments Outside Diff (1)
internal/ui/components/streamtask.go, line 298-304 (link)When the streamed backup body panics, this goroutine exits before sending
doneorStreamDoneMsg. Because the dashboard backup path now keeps the session open while the backup runs, the caller can block waiting for completion and the terminal can be left in the alternate-screen TUI state instead of returning to the normal terminal.Reviews (1): Last reviewed commit: "feat(dashboard): add Support (recovery) ..." | Re-trigger Greptile