Feat/implement phase c#2
Merged
Merged
Conversation
Adds internal/tui/panels/jobs.go implementing the Jobs panel that bridges jobs.Runner Event channels into Bubble Tea's update loop via a re-issuing SubscribeCmd pump. Also adds the charmbracelet/harmonica indirect dependency required by bubbles/progress.
Add dashboard_snapshot_test.go with two teatest golden-snapshot tests (Initial, FocusDumps). Generate testdata/*.golden via -update flag.
These are directly imported by the TUI (huh forms, teatest snapshots, termenv in the focus test) but were still listed as indirect. tidy reclassifies them; no version changes.
The dashboard owns error-overlay dismissal (intercepts esc/enter/q itself), so ErrorModel.Update never drove quitting — but its docstring claimed it 'quits the program' and the dead tea.Quit path was a trap for any future caller that routed input to it. Update is now an inert tea.Model stub that only absorbs the window-size hint; docstring reflects the real lifecycle.
Phase B+C both ship: status table C → Complete, warning + commands + features reflect the now-working interactive dashboard.
… broken profiles filter
…ect when dumps exist, type when empty)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR implements a full Bubble Tea TUI dashboard (Profiles, Dumps, Jobs), modal backup/restore forms, job event subscriptions and error overlay handling, keybindings and styles, CLI wiring to pass initialized deps into the TUI, and comprehensive unit + snapshot tests with golden files. ChangesPhase C Bubble Tea Dashboard Implementation
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (root.go)
participant TUI as TUI Run
participant Dashboard as Dashboard
participant Profiles as Profiles Panel
participant Dumps as Dumps Panel
participant Jobs as Jobs Panel
participant Forms as Backup/Restore Forms
participant App as app.Backup/app.Restore
CLI->>TUI: Run(deps)
TUI->>Dashboard: NewDashboard(deps) / Start
Dashboard->>Profiles: Init()
Dashboard->>Dumps: Init()
Dashboard->>Jobs: Init()
User->>Dashboard: KeyMsg (e.g., 'b' backup / 'r' restore / Tab)
Dashboard->>Forms: NewBackup/NewRestore(...)
Forms-->>Dashboard: Form completed -> BackupResult/RestoreResult
Dashboard->>App: app.Backup/profile or app.Restore(...) (runs)
App-->>Dashboard: JobChannelMsg (job events) or ErrorMsg
Dashboard->>Jobs: SubscribeCmd(job channel)
Jobs->>Dashboard: RefreshMsg on completion
Dashboard->>Dumps: Reload() after completion
Dashboard->>Dashboard: Render updated view or ErrorModel overlay
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/tui/panels/dumps.go (2)
64-71: 💤 Low valueSimplify
SetFocus—Focus()is called even when blurring.When
bis false the table is focused then immediately blurred. The end state is correct, but the unconditionalFocus()is misleading.♻️ Proposed tweak
func (p *Dumps) SetFocus(b bool) { p.focused = b - p.tbl.Focus() - if !b { + if b { + p.tbl.Focus() + } else { p.tbl.Blur() } }🤖 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/tui/panels/dumps.go` around lines 64 - 71, The SetFocus method currently always calls p.tbl.Focus() then conditionally p.tbl.Blur(), which unnecessarily focuses before blurring; update Dumps.SetFocus to set p.focused = b and call p.tbl.Focus() only when b is true, otherwise call p.tbl.Blur() so the table isn't focused when b is false and the behavior/readability matches intent.
82-98: 💤 Low value
Reloadsilently swallowsList()errors.A failed catalog read leaves the table empty with no signal to the user. Since
Reloadhas no return value, the dashboard can't surface this via its error overlay (e.g. after a completed job at dashboard.go Line 145). Consider returning the error (or a status flag) so callers can decide whether to display it.🤖 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/tui/panels/dumps.go` around lines 82 - 98, The Reload method currently swallows errors from p.deps.Dumps.List(), leaving the UI unaware; change Reload to return an error (i.e., func (p *Dumps) Reload() error), propagate and return the error if p.deps.Dumps.List() fails, and on success continue building rows and call p.tbl.SetRows(rows) then return nil; update callers to check the returned error and surface it via the dashboard overlay as needed.
🤖 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 `@internal/tui/dashboard_snapshot_test.go`:
- Around line 34-56: The snapshot test TestDashboard_FocusDumps_Snapshot is not
verifying a different focused state after sending the Tab key; ensure the test
deterministically captures a focused rendering by either (A) asserting the
dashboard model reports the Dumps panel focused before snapshot (e.g., inspect
the Dashboard instance returned by NewDashboard or tm.Model() for a FocusedPanel
/ IsFocused("dumps") property and fail if not) and waiting until that becomes
true, or (B) making the Dashboard's Render output include a deterministic focus
marker for Dumps (e.g., a distinct string or visual marker emitted when the
Dumps panel is focused) so the golden differs; update the test to perform the
explicit assertion/wait after tm.Send(tea.KeyMsg{Type: tea.KeyTab}) and before
reading tm.FinalOutput and keep the existing teatest.RequireEqualOutput call.
In `@internal/tui/modals/errormodal.go`:
- Around line 35-42: In ErrorModel.View guard against a nil m.err and clamp the
computed width before rendering: check m.err and use a safe fallback message
(e.g. "unknown error") when nil, compute an availableWidth variable from m.width
(e.g. available := m.width - 4) and clamp it to a sensible minimum (so you never
pass a negative or too-small value into min/Width), then call min with that
clamped availableWidth and use that when constructing the lipgloss box; update
references in ErrorModel.View (m.err, min call and Width(...)) accordingly.
In `@internal/tui/panels/jobs.go`:
- Around line 100-110: The View currently iterates the map p.jobs which is
non-deterministic and causes jumping rows; add a deterministic ordering by
tracking insertion order: add a slice field (e.g., jobOrder []string or []jobID)
to the same struct that holds p.jobs, append the job's ID to jobOrder inside the
absorb method when a new job is first seen, and then change View to iterate
jobOrder (looking up each job in p.jobs and skipping missing entries) instead of
ranging over the map; also ensure you remove IDs from jobOrder when jobs are
completed/removed to avoid stale entries.
In `@internal/tui/panels/profiles.go`:
- Around line 44-52: loadItems currently discards the error returned by
d.Profiles.Get(n), which can yield empty profile fields if a profile is removed
between List() and Get(); update loadItems to check the error from
d.Profiles.Get(n) and skip that entry (or log the error) instead of using the
zero-value ProfileConfig, ensuring only valid profileItem instances (created
from successful d.Profiles.Get calls) are appended to items; reference the
loadItems function, d.Profiles.Get, and profileItem when making the change.
---
Nitpick comments:
In `@internal/tui/panels/dumps.go`:
- Around line 64-71: The SetFocus method currently always calls p.tbl.Focus()
then conditionally p.tbl.Blur(), which unnecessarily focuses before blurring;
update Dumps.SetFocus to set p.focused = b and call p.tbl.Focus() only when b is
true, otherwise call p.tbl.Blur() so the table isn't focused when b is false and
the behavior/readability matches intent.
- Around line 82-98: The Reload method currently swallows errors from
p.deps.Dumps.List(), leaving the UI unaware; change Reload to return an error
(i.e., func (p *Dumps) Reload() error), propagate and return the error if
p.deps.Dumps.List() fails, and on success continue building rows and call
p.tbl.SetRows(rows) then return nil; update callers to check the returned error
and surface it via the dashboard overlay as needed.
🪄 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: 982ec39b-de9b-4d16-a65c-9453185ab092
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
README.mdgo.modinternal/cli/root.gointernal/tui/dashboard.gointernal/tui/dashboard_snapshot_test.gointernal/tui/dashboard_test.gointernal/tui/keys.gointernal/tui/modals/backup.gointernal/tui/modals/errormodal.gointernal/tui/modals/restore.gointernal/tui/msg.gointernal/tui/panel.gointernal/tui/panels/dumps.gointernal/tui/panels/jobs.gointernal/tui/panels/profiles.gointernal/tui/run.gointernal/tui/styles/styles.gointernal/tui/testdata/TestDashboard_FocusDumps_Snapshot.goldeninternal/tui/testdata/TestDashboard_Initial_Snapshot.golden
The focus snapshot's golden is byte-identical to the initial one (teatest renders off-TTY in ASCII, so the focus border-color difference is stripped), so it didn't actually verify focus moved. Add an explicit assertion on the final model's focusIdx (==1, Dumps) after Tab, and document why the golden can't prove focus. The snapshot remains a layout/content regression guard; focus movement is also visually verified in TestDashboard_Tab_MovesFocus.
NewError is exported, so a nil error could reach View → m.err.Error() panic; now falls back to 'unknown error'. And the dashboard doesn't forward a WindowSizeMsg to the overlay, so m.width is typically 0 → min(80, -4) = -4 passed to Width(); now clamps available width to a sensible minimum (20). Adds errormodal_test.go covering both edge cases (no prior test in modals).
Jobs.View ranged over the jobs map, so with >1 job the rows reordered randomly between frames. Track first-seen order in a new 'order []string' field appended in absorb, and iterate that in View (skipping any id without a backing view). Adds jobs_test.go asserting stable B/A/C ordering across updates. Skipped the review's 'remove IDs on completion' suggestion: the panel never removes jobs by design (completed/failed jobs keep their final state visible; jobsChannelClosedMsg is a documented no-op), so there are no stale entries to prune. Removal would land with a future 'clear completed' feature.
loadItems discarded the error from d.Profiles.Get(n) and appended a zero-value profileItem on failure. Now it skips the entry. Unreachable today (List and Get read the same in-memory map, single-threaded TUI) but removes a latent footgun if List/Get ever diverge. No test: the error path can't be triggered through the public Store API without contorting internals.
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.
Summary by CodeRabbit
New Features
Documentation
Tests