Cache TUI model registry - #496
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses #494 by avoiding repeated reconstruction of the curated model registry during TUI rendering, caching the registry per TUI model instance and reusing it for context-window resolution and vision capability checks.
Changes:
- Add a cached
modelCatalogregistry field to the TUImodeland initialize it once innewModel. - Switch context-window and vision checks to consult the cached registry rather than calling
modelregistry.DefaultRegistry()repeatedly. - Add regression tests and a benchmark to validate/quantify cached vs uncached lookup behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/picker_test.go | Updates context-window test setup to use the model’s cached registry field. |
| internal/tui/model.go | Adds cached registry field and initializes it during model construction; wires registry into default usage tracker creation. |
| internal/tui/model_catalog.go | Uses cached registry for modelContextWindow resolution instead of rebuilding the registry each call. |
| internal/tui/model_catalog_test.go | Adds regression tests ensuring cached-registry behavior plus a benchmark for lookup cost. |
| internal/tui/image_attach.go | Uses cached registry for vision checks instead of rebuilding the registry each call. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe TUI model now caches the curated model registry at construction and reuses it for context-window and vision-capability checks. The render path no longer loads the default registry on each call, and tests plus a benchmark cover the updated lookup flow. ChangesCached model catalog for TUI lookups
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/model_catalog.go (1)
64-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPanic on
modelCatalogErrunconditionally
newModelonly surfacesDefaultRegistry()failures whenusageTracker == nil; if a tracker is supplied, the constructor can cache a zero-value catalog and silently lose curated context-window and vision lookups for the whole TUI session.internal/tui/model.goshould panic onmodelCatalogErrregardless ofUsageTracker.🤖 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/model_catalog.go` around lines 64 - 95, The constructor path for the TUI model catalog is swallowing modelCatalogErr when a UsageTracker is present, which can leave modelCatalog in a zero-value state for the whole session. Update newModel in internal/tui/model.go so it always fails fast on DefaultRegistry()/catalog initialization errors, regardless of whether usageTracker is nil, and make sure modelContextWindow still relies on a valid catalog for curated lookups.
🤖 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/model.go`:
- Around line 665-671: The registry load failure in modelCatalog initialization
is only handled when usageTracker is nil, which can silently cache an empty
registry when a tracker is pre-supplied. Update the modelCatalog/modelCatalogErr
handling in model.go so DefaultRegistry errors are checked and surfaced
unconditionally before any UsageTracker branching, ensuring m.modelCatalog never
stores a zero-value Registry and preserving correct behavior for
modelContextWindow and modelSupportsVisionTUI.
---
Outside diff comments:
In `@internal/tui/model_catalog.go`:
- Around line 64-95: The constructor path for the TUI model catalog is
swallowing modelCatalogErr when a UsageTracker is present, which can leave
modelCatalog in a zero-value state for the whole session. Update newModel in
internal/tui/model.go so it always fails fast on DefaultRegistry()/catalog
initialization errors, regardless of whether usageTracker is nil, and make sure
modelContextWindow still relies on a valid catalog for curated lookups.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f7e939b-3cba-44a4-adbb-aa57ef180d0b
📒 Files selected for processing (5)
internal/tui/image_attach.gointernal/tui/model.gointernal/tui/model_catalog.gointernal/tui/model_catalog_test.gointernal/tui/picker_test.go
3323b3e to
2e8a848
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found one process blocker that needs a maintainer decision before this is ready.
Findings
- [P2] Resolve the approved-issue requirement for this community PR
CONTRIBUTING.md:19
This PR links#494, but the linked issue currently only has theenhancementlabel, while the contribution policy says community PRs must be tied to an existing issue that has already been reviewed and marked withissue-approved. Please get the linked issue marked as approved, or have a maintainer explicitly confirm that this PR falls under the team-member/internal-work exception, before proceeding with the code changes.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Good fix. modelContextWindow runs a couple of times per render frame (header, context gauge, sidebar), and on main each call rebuilt the whole registry — validate 13 entries, compile the regexes, deep-clone everything — which the codebase itself warns against (session_controls.go:156: "MUST NOT be called from the render path"). Caching the catalog once in newModel and reading it on lookup is the right minimal fix, and reusing that same registry for the usage tracker instead of building a second one is a nice touch.
I checked the thing that worried me — staleness after a /model switch — and it's fine: the cache is the whole immutable catalog, not a memoized per-model answer, so Resolve(m.modelName) still resolves the new model correctly. Discovered/Ollama models stay in their own live maps, untouched. No race either — built eagerly in newModel, only ever read after, and the registry methods clone on read.
Bonus: the vision reorder actually fixes a latent bug on main where the name heuristic could short-circuit true before the discovered list was consulted (a discovered text-only gpt-5-* would wrongly report vision support). There's a regression test for it.
CodeRabbit's one real catch — the registry error getting swallowed when a UsageTracker is injected — is handled at HEAD; the DefaultRegistry() error now panics unconditionally before the tracker branch, so the cached registry can't be a zero value.
Only non-code thing: #494 has enhancement but not issue-approved — your call. Approving.
Fixes #494
Summary
Summary by CodeRabbit
Performance
Bug Fixes
Tests