Skip to content

perf(tui): reuse storage + catalog manager across refreshes#33

Merged
kevinelliott merged 1 commit intomainfrom
perf/tui-reuse-storage
Apr 21, 2026
Merged

perf(tui): reuse storage + catalog manager across refreshes#33
kevinelliott merged 1 commit intomainfrom
perf/tui-reuse-storage

Conversation

@kevinelliott
Copy link
Copy Markdown
Owner

Summary

The TUI was opening a fresh `*storage.Store` on every `loadDataWithRefresh` call — every `r` keypress re-ran `sql.Open`, all 8 SQLite migrations, and a `catalog.Manager` construction. The helper and CLI already share their Store for the process lifetime; the TUI is the long-running surface where this reuse matters most.

Change

  • `Run()` opens the Store once, passes it into `New()`, and defers `Close` after `Program.Run` returns.
  • `Model` now holds `store storage.Store` and `catMgr *catalog.Manager`.
  • `loadDataWithRefresh` reads from those shared fields instead of rebuilding.
  • Detector + `installer.Manager` are still per-refresh — they're cheap.

Expected effect

A typical refresh drops from ~80–120ms of SQLite warm-up (sql.Open + 8 `CREATE TABLE IF NOT EXISTS` migrations + catalog manager instantiation) to ~2ms before detection starts. The saved time lands in the refresh latency users see when they hit `r`.

API

`tui.New` gains a `storage.Store` parameter. Only caller is `tui.Run` in the same file; no external callers exist.

Test plan

  • `go test ./internal/tui/... -race` green
  • `make lint` clean
  • Manual: `agentmgr tui` launches, `r` refreshes are visibly snappier, quitting closes cleanly

🤖 Generated with Claude Code

The TUI was opening a fresh *storage.Store on every loadDataWithRefresh
call — which meant every \`r\` keypress re-ran sql.Open, all 8 SQLite
migrations, and a catalog.Manager construction. The helper and CLI
already share their Store for the process lifetime; the TUI is the
long-running one where this matters most.

Run() now opens the Store once, passes it into New(), and defers Close
after Program.Run returns. The Model holds both *storage.Store and
*catalog.Manager; loadDataWithRefresh reads from them instead of
rebuilding. Detector + installer.Manager are still per-refresh —
they're cheap.

Effect: typical refresh drops from ~80–120ms of SQLite warm-up before
detection starts to ~2ms — the saved time appears in the refresh
latency users see when they hit \`r\`.

No behavior change; the New signature adds a store parameter but the
only call site is Run() in the same file (no external callers of
tui.New today).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 21, 2026 18:56
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves TUI refresh performance by reusing long-lived storage and catalog resources across refreshes, aligning the TUI with the existing “open once per process” pattern used by other entry points.

Changes:

  • Open and initialize the SQLite storage.Store once in Run() and defer Close() until the TUI exits.
  • Extend the TUI Model to retain a shared store and *catalog.Manager across refreshes.
  • Update loadDataWithRefresh to reuse the shared store/catMgr instead of rebuilding them each refresh.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/tui/app.go
Comment on lines +47 to +48
// Store + Manager on every refresh (the old pattern re-ran 8 SQLite
// migrations per `r` keypress).
Comment thread internal/tui/app.go
Comment on lines +147 to +151
// New constructs the TUI Model. Callers are responsible for opening the
// Store (so Close can be deferred in the caller's scope after Program.Run
// returns) and passing it in. The Model reuses that Store across every
// refresh, avoiding the sql.Open + migrate churn the old per-refresh
// pattern incurred.
@kevinelliott kevinelliott merged commit e6897be into main Apr 21, 2026
19 checks passed
@kevinelliott kevinelliott deleted the perf/tui-reuse-storage branch April 21, 2026 19:04
kevinelliott added a commit that referenced this pull request Apr 24, 2026
Toolchain + dependency refresh. Minimum Go bumped to 1.25 (forced by
x/sync v0.20.0), golangci-lint migrated to v2.11.4, 5 deps bumped
plus 3 patch-level ones from v1.2.0-era. Also captures post-v1.2.0
polish: TUI storage reuse perf win (#33), doctor catalog-source
breakdown (#32), catalog ctx-scoped logger (#31), README refresh (#30).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kevinelliott added a commit that referenced this pull request Apr 25, 2026
* fix: address Copilot review feedback — correctness fixes

Seven real-bug fixes called out across review comments on merged PRs:

- pkg/platform/platform.go: resetLookPathCache() no longer reassigns
  the package-level sync.Map (unsafe vs concurrent readers). Uses
  Range+Delete instead.
- pkg/installer/providers/brew.go: GetLatestVersion no longer caches
  context-canceled / deadline-exceeded errors. A caller with a short
  ctx would have poisoned the 5m TTL for every later caller.
- pkg/installer/providers/progress.go: the progress writer is now
  wrapped in a locking writer so concurrent Write calls from
  cmd.Stdout and cmd.Stderr (os/exec writes those from separate
  goroutines) don't interleave bytes mid-call.
- pkg/catalog/embed.go: EmbeddedJSON() now returns a copy of the
  backing bytes. Previously callers could mutate the package-level
  []byte, corrupting the baseline for every future Get().
- pkg/catalog/manager.go: loadEmbedded now also probes
  /usr/share/agentmgr/catalog.json — where the goreleaser nfpm
  config installs the catalog for .deb/.rpm users. Previously that
  path was silently ignored at runtime even though goreleaser wrote
  the file there at install time.
- internal/cli/doctor.go: Catalog Sources section updated with the
  matching /usr/share/agentmgr row so `doctor` output mirrors the
  loader's probe order.
- internal/cli/agent.go: removed the --all (-a) and --check-updates
  flags from `agent list`. Both bound to local vars that were never
  read anywhere, so the flags did nothing; users setting them got a
  silent no-op instead of a useful behavior.

Supersedes the corresponding Copilot annotations on PRs #19, #28,
#29, #33, plus the dead-flag note on the agent list command.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore: address remaining Copilot review nits (docs + tests)

Batch of small review-comment fixes — documentation accuracy, test
hardening, and gosec G104 on grpc teardown:

- pkg/api/grpc/server.go: explicit \`_ = s.listener.Close()\` in Stop()
  and ForceStop() with a comment noting the close error is not
  actionable during teardown (silences gosec G104).
- pkg/catalog/manager.go: Refresh doc now explicitly notes that
  singleflight does not merge contexts and the first caller's ctx
  governs the shared fetch — a caller with a short deadline can
  cancel the in-flight work for all shared callers.
- pkg/installer/providers/progress.go: WithProgressWriter doc clarifies
  why passing nil is a no-op (asymmetry lets middleware attach a writer
  safely without downstream code accidentally clobbering it).
- internal/systray/cli_uninstall_darwin.go: file is darwin-only via
  //go:build tag, so platform.ID() is always Darwin at runtime. Removed
  the dead Linux and Windows branches (the filepath/os/platform imports
  they needed are gone too).
- CHANGELOG.md: "(Removed in [Unreleased].)" was a broken shortcut
  reference link — promoted to "(Removed in v1.2.0.)".
- README.md: feature bullet uses the full "agentmgr agent list" command
  prefix, consistent with everywhere else in the doc.
- internal/cli/output/spinner_test.go: non-TTY test replaced the
  30ms sleep with a direct assertion on s.isTTY. Start() is a no-op
  when isTTY is false, so sleeping was measuring nothing.
- internal/cli/output/table_test.go: ANSI-alignment loop now actually
  asserts inter-column padding instead of discarding each line.
- internal/systray/handlers_test.go: mustParseVersion panics on parse
  error so test-setup bugs surface at the fixture, not downstream.
  TestDialogTracking uses bare *exec.Cmd values (trackDialog only
  checks pointer identity) to avoid the PATH lookup for \`true\` on
  Windows CI.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(cli): drop "all" flag assertion from list; assert removal

The --all / -a and --check-updates flags were removed from
\`agent list\` in the parent commit because both bound to unused
local vars. Drop the existence assertion for "all" and add an
anti-regression check that asserts both flags are ABSENT.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants