Pool monitor sidebar and persistent pool cache#204
Conversation
github-actions
bot
added
commands
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/tui/workspace/views/pool_monitor.go">
<violation number="1" location="internal/tui/workspace/views/pool_monitor.go:310">
P2: Off-by-one in FetchComplete padding: `-2` should be `-1`. There's only one space separator between `latStr` and `ageStr`, so subtracting 2 makes each FetchComplete row 1 column short of the target width.</violation>
</file>
<file name="internal/tui/workspace/data/mutation.go">
<violation number="1" location="internal/tui/workspace/data/mutation.go:191">
P2: Disk I/O (`cache.Save`) is performed while holding `mp.mu.Lock()`, which will block all pool readers/writers during the write. The `Apply` path correctly captures the cache reference and performs the save outside the lock. This should follow the same pattern: capture `cache` under the lock, then save after unlocking.</violation>
</file>
<file name="internal/tui/workspace/data/pool.go">
<violation number="1" location="internal/tui/workspace/data/pool.go:205">
P2: `cache.Save` performs disk I/O (temp file write + rename) while holding `p.mu` write lock, blocking all pool readers including `Get()` on the render path. Consider capturing the values needed for the save, releasing the lock, then writing to disk outside the critical section.</violation>
</file>
<file name="internal/tui/workspace/workspace.go">
<violation number="1" location="internal/tui/workspace/workspace.go:1067">
P1: Pool monitor's `Init()` is never called. When the pool monitor is first created via `poolMonitorFactory()`, the function returns `nil` without calling `w.poolMonitor.Init()`. This means the view won't execute its startup commands (tickers, initial data fetches, etc.). Compare with `openSidebarPanel()` which correctly calls `w.stampCmd(w.sidebarView.Init())`.</violation>
</file>
<file name="internal/tui/workspace/data/pool_cache.go">
<violation number="1" location="internal/tui/workspace/data/pool_cache.go:85">
P1: Path traversal: key sanitization in `path()` doesn't handle `\` or `..` components. On Windows, a key like `..\..\sensitive` passes through the replacer unchanged, and `filepath.Join` resolves the `..` segments, writing/reading outside the cache directory. Consider using `filepath.Base` on the sanitized key (to strip directory components) or also replacing `\` and rejecting/cleaning `..`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
This PR adds a disk-backed “stale-while-revalidate” pool cache so the TUI can warm-boot into real screens immediately, and replaces the old bottom metrics panel with an interactive right-sidebar pool monitor (toggled with backtick) that shows pool status plus recent fetch events.
Changes:
- Add
PoolCacheand wire it throughHubso pools persist/load snapshots from~/.cache/basecamp/pools/. - Replace the bottom
MetricsPanelwith a right sidebarPoolMonitorview (with focus/scroll/expand + activity feed). - Extend telemetry (
PoolStatus.Fetching,CachedFetchedAt,RecentEvents, mutation telemetry) to support the monitor and accurate age display.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/workspace/workspace.go | Adds pool-monitor sidebar plumbing, layout geometry, and toggle/focus handling; removes metrics panel usage. |
| internal/tui/workspace/views/pool_monitor.go | New interactive right-sidebar view rendering pool table + activity feed. |
| internal/tui/workspace/data/pool_cache.go | New disk-backed JSON cache with atomic write (temp + rename). |
| internal/tui/workspace/data/pool_cache_test.go | Tests for save/load, atomic writes, overwrite, and key sanitization. |
| internal/tui/workspace/data/pool.go | Adds cache integration and exposes cached fetched-at for accurate “age” display. |
| internal/tui/workspace/data/mutation.go | Preserves cached baseline age across optimistic mutations; records fetch telemetry and saves reconciled data to cache. |
| internal/tui/workspace/data/metrics.go | Extends PoolStatus and adds RecentEvents accessor for the monitor feed. |
| internal/tui/workspace/data/hub.go | Adds cacheDir param to NewHub and initializes a shared PoolCache. |
| internal/tui/workspace/data/hub_global.go | Wires SetCache(h.cache) into global-scope pools. |
| internal/tui/workspace/data/hub_test.go | Updates tests for new NewHub(multi, cacheDir) signature. |
| internal/tui/workspace/data/pool_test.go | Adds warm-boot age preservation test via disk cache. |
| internal/tui/workspace/data/mutation_test.go | Adds tests for telemetry events, cache writes, and cached-age preservation across rollbacks/mutations. |
| internal/tui/workspace/session.go | Passes app cache dir into Hub so caching is enabled in real sessions. |
| internal/tui/workspace/workspace_test.go | Updates Hub constructor usage for tests. |
| internal/tui/workspace/keymap.go | Renames the backtick hint from “metrics” to “pool monitor”. |
| internal/tui/workspace/chrome/metrics_panel.go | Removes the old bottom metrics panel implementation. |
| internal/commands/tui.go | Passes a pool-monitor factory into workspace.New and constructs the monitor from Hub metrics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a8dc46819
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
All review findings addressed across two commits: 242e6fe — Fix cross-account cache leak and resize focus loss
18e6853 — Address review feedback: alignment, locking, safety, focus
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/tui/workspace/views/pool_monitor.go">
<violation number="1" location="internal/tui/workspace/views/pool_monitor.go:391">
P2: `%.0f` rounds instead of truncating, so durations just under a boundary produce contradictory output (e.g., 23h 31m → "24h" despite being less than 24 hours). Use integer truncation for consistency with the guard conditions.</violation>
<violation number="2" location="internal/tui/workspace/views/pool_monitor.go:393">
P2: Same `%.0f` rounding issue in the days branch — use integer truncation to avoid showing a day count that hasn't fully elapsed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Disk-backed PoolCache seeds pools as Stale on startup so the TUI boots into real screens while background refresh runs. Cache writes are synchronous and atomic (temp+rename). Pools track cachedFetchedAt for accurate age display, preserved through optimistic mutation rollbacks. Also extends metrics/telemetry: PoolStatus exposes Fetching and CachedFetchedAt, MutatingPool.Apply records fetch events, and RecentEvents provides ring-buffer access for the upcoming pool monitor.
Interactive, focusable PoolMonitor view in a 38-column right sidebar (toggled with backtick). Two sections: pool table with cursor navigation and expandable detail rows, and an activity feed showing fetch events with latency and age. Auto-hides when terminal is too narrow. Replaces the former 8-line MetricsPanel bottom strip with a richer display that surfaces all pool telemetry: Apdex score, fetch state indicators, per-pool stats, and the full event ring buffer.
People pool key now includes account ID (e.g. "people:abc123") so switching accounts cannot serve cached data from a different account. When the pool monitor is focused and a window resize makes it too narrow to display, focus returns to the main view so polling and input resume correctly.
Pool monitor: - Fixed-width right-aligned columns for state/age and latency/age so values align in neat columns instead of jagged right-edge. - ANSI-aware truncation via charmbracelet/x/ansi.Truncate instead of rune-slicing that could corrupt escape sequences. - Expanded state keyed by pool key (not slice index) so it stays stable when pool list changes. Data layer: - Move cache.Save outside mutex in Pool.Fetch and MutatingPool.Fetch to avoid blocking readers during disk I/O. - Harden pool_cache.path() against path traversal: sanitize backslash and "..", apply filepath.Base to strip directory components. - Use filepath.Join for portable path construction in NewHub. Workspace: - Call poolMonitor.Init() on first open (was silently dropped). - Guard focus transition on poolMonitorActive() so toggle can't focus a monitor that isn't renderable at the current width. - Capture returned View from all pool monitor Update() calls to support immutable/value-type view implementations.
"Pools 1.00" → "Pools apdex 1.00" so the number has context.
- Capture returned View from BlurMsg when focusing pool monitor - Move disk I/O outside mutex in SetCache (load before lock) - Use formatDuration for activity feed latency so >=1s fetches render as "10s" not "10000ms", staying within the 6-char column
- Clear pool monitor keyboard focus in navigate/goBack/goToDepth/switchAccount so the panel forwarding block doesn't eat keys after navigation - Replace view.SetSize with relayout() in navigation functions so the combined layout doesn't exceed terminal width when the monitor is open - Esc pops panel focus (monitor or sidebar) before back-navigating - Tab cycles through all active panels (main → sidebar → monitor → main) in both normal and inputActive modes - Backtick toggles pool monitor during text input capture - Number keys routed after panel forwarding so focused panels consume them - Duplicate-navigation guards on Hey/MyStuff/Activity in both code paths
Use a fixed noon timestamp instead of time.Now() so consecutive test messages never straddle a day boundary when CI runs near midnight UTC.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func truncate(s string, maxLen int) string { | ||
| if len(s) <= maxLen { | ||
| return s | ||
| } | ||
| return s[:maxLen-1] + "…" |
There was a problem hiding this comment.
truncate() slices by byte length (s[:maxLen-1]) which can split multi-byte UTF-8 characters and produce invalid text in the UI (e.g., in error Detail strings). Prefer rune-aware truncation (or ansi-aware truncation) to avoid corrupting output.
| } | ||
| feedHeight := v.height - tableHeight - 1 // -1 for divider |
There was a problem hiding this comment.
PoolMonitor.View enforces a minimum tableHeight of 4, but for very small heights this can make the rendered output exceed v.height because feedHeight is clamped to 0 without compensating. Clamp tableHeight to available height (e.g., max/min based on v.height) so total rendered lines never exceed v.height.
| } | |
| feedHeight := v.height - tableHeight - 1 // -1 for divider | |
| } | |
| // Ensure the table plus divider never exceeds the available height. | |
| maxTableHeight := v.height - 1 // leave 1 line for the divider/feed boundary | |
| if maxTableHeight < 1 { | |
| maxTableHeight = 1 | |
| } | |
| if tableHeight > maxTableHeight { | |
| tableHeight = maxTableHeight | |
| } | |
| feedHeight := v.height - tableHeight - 1 // -1 for divider | |
| if feedHeight < 0 { | |
| feedHeight = 0 | |
| } |
| if cache != nil { | ||
| if err := cache.Save(key, remoteData, time.Now()); err != nil { | ||
| log.Printf("pool cache save %s: %v", key, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Cache save failures are logged on every Apply/Fetch (log.Printf). In a persistent failure mode (unwritable cache dir), this can produce repeated stderr output while the TUI is running. Consider reusing a shared failure flag/rate limiter (possibly in PoolCache) so this only logs/surfaces once per session.
| updated, _ := w.poolMonitor.Update(BlurMsg{}) | ||
| w.poolMonitor = updated | ||
| if view := w.router.Current(); view != nil { | ||
| updated, cmd := view.Update(FocusMsg{}) | ||
| w.replaceCurrentView(updated) | ||
| return w, w.stampCmd(cmd) |
There was a problem hiding this comment.
When resizing causes the pool monitor to become inactive, the BlurMsg update’s returned cmd is discarded (updated, _ := w.poolMonitor.Update(...)). If the monitor view ever emits cleanup/stop commands on blur (which tests now allow), they’ll never run. Capture the blur cmd and batch it with the main view’s Focus cmd (and stamp both).
| updated, _ := w.poolMonitor.Update(BlurMsg{}) | |
| w.poolMonitor = updated | |
| if view := w.router.Current(); view != nil { | |
| updated, cmd := view.Update(FocusMsg{}) | |
| w.replaceCurrentView(updated) | |
| return w, w.stampCmd(cmd) | |
| updated, blurCmd := w.poolMonitor.Update(BlurMsg{}) | |
| w.poolMonitor = updated | |
| if view := w.router.Current(); view != nil { | |
| updated, focusCmd := view.Update(FocusMsg{}) | |
| w.replaceCurrentView(updated) | |
| return w, tea.Batch( | |
| w.stampCmd(blurCmd), | |
| w.stampCmd(focusCmd), | |
| ) |
| vDividerStr := strings.TrimRight(strings.Repeat("│\n", w.viewHeight()), "\n") | ||
| vDivider := lipgloss.NewStyle(). | ||
| Foreground(theme.Border). | ||
| Height(w.viewHeight()). | ||
| Render(strings.TrimRight(strings.Repeat("│\n", w.viewHeight()), "\n")) | ||
| mainContent := "" | ||
| if view := w.router.Current(); view != nil { | ||
| mainContent = view.View() | ||
| Render(vDividerStr) |
There was a problem hiding this comment.
The vertical divider string/style is constructed unconditionally in the main rendering path, even when neither sidebar nor pool monitor is active, which adds avoidable allocation/work on every render. Consider building vDividerStr/vDivider only when at least one side panel is actually being rendered.
Summary
~/.cache/basecamp/pools/. Cache writes are atomic (temp+rename), performed outside locks. Pools trackcachedFetchedAtfor accurate age display, preserved through optimistic mutation rollbacks.PoolStatusexposesFetchingandCachedFetchedAt,MutatingPool.Applyrecords fetch events,RecentEventsprovides ring-buffer access.Geometry
Test plan
make checkpasses (lint, vet, unit tests, 263 e2e tests)~indicator on pools mid-fetch, state colors correct~/.cache/basecamp/pools/contains JSON files after fetches