perf(tui): reuse storage + catalog manager across refreshes#33
Merged
kevinelliott merged 1 commit intomainfrom Apr 21, 2026
Merged
perf(tui): reuse storage + catalog manager across refreshes#33kevinelliott merged 1 commit intomainfrom
kevinelliott merged 1 commit intomainfrom
Conversation
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>
There was a problem hiding this comment.
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.Storeonce inRun()and deferClose()until the TUI exits. - Extend the TUI
Modelto retain a sharedstoreand*catalog.Manageracross refreshes. - Update
loadDataWithRefreshto reuse the sharedstore/catMgrinstead of rebuilding them each refresh.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+47
to
+48
| // Store + Manager on every refresh (the old pattern re-ran 8 SQLite | ||
| // migrations per `r` keypress). |
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
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>
5 tasks
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>
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
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
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
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
🤖 Generated with Claude Code