Skip to content

Re-tune onboarding model recommendations for the base-model tiers#505

Merged
FuJacob merged 1 commit into
mainfrom
feat/onboarding-recommender-retune
Jun 1, 2026
Merged

Re-tune onboarding model recommendations for the base-model tiers#505
FuJacob merged 1 commit into
mainfrom
feat/onboarding-recommender-retune

Conversation

@FuJacob
Copy link
Copy Markdown
Owner

@FuJacob FuJacob commented Jun 1, 2026

Summary

The onboarding recommender's warning copy and memory thresholds were sized for the old ~3–5 GB instruct models. The base-model tiers are much smaller — quick 0.8 GB, everyday 1.4 GB, powerful 2.6 GB — so the Powerful disable floor of 10 GB wrongly excluded stock 8 GB Macs that can comfortably run a 2.6 GB model. This lowers that floor to 8 GB and derives the "~X GB" sizes in the copy from the catalog so they can't drift.

Validation

xcodebuild -project Cotabby.xcodeproj -scheme Cotabby -destination 'platform=macOS' \
  build-for-testing -derivedDataPath build/DerivedData
# ** TEST BUILD SUCCEEDED **

swiftlint lint --quiet
# 0 violations
  • OnboardingTemplateRecommenderTests updated to pin the new boundary: Powerful is disabled below 8 GB (tested at 6 GB) and allowed-but-warned at exactly 8 GB. Other cases (12/16/32 GB, Apple Intelligence, Quick, recommendation defaults) are unchanged.

Linked issues

None.

Risk / rollout notes

  • Behavior change (onboarding only). A stock 8 GB Mac now sees Powerful as available-with-a-warning instead of disabled. Apple Intelligence tiers and the Quick/Everyday recommendation logic are unchanged.
  • The warning strings now read their size from DownloadableRuntimeModel.approximateSizeLabel, so they track the catalog automatically; no hardcoded sizes remain in the recommender.

Greptile Summary

This PR lowers the Powerful-tier memory disable floor from 10 GB to 8 GB (matching the new ~2.6 GB base-model GGUF vs. the old ~5 GB instruct model), and replaces hardcoded model-size strings in warning copy with a live catalog lookup via modelSizeLabel(for:).

Confidence Score: 4/5

Safe to merge; the change is narrowly scoped to onboarding display logic with no data-persistence or auth surface touched.

The threshold change and catalog-driven copy are both straightforward, and the updated tests correctly pin the new 8 GB boundary. The one residual hardcoded value ("16 GB" in the warn-path string) could drift from powerfulWarnBelowGigabytes if that constant is later adjusted, and the "local" fallback in modelSizeLabel produces awkward copy — neither is a functional regression on the current code path.

The warn-path string in OnboardingTemplateRecommender.swift line 76 still contains a literal "16 GB" that the PR description implied was removed.

Important Files Changed

Filename Overview
Cotabby/Support/OnboardingTemplateRecommender.swift Lowers powerfulDisableBelowGigabytes from 10 to 8, allowing 8 GB Macs to use the Powerful tier with a warning; replaces hardcoded model size strings with modelSizeLabel(for:) reading from the catalog. One hardcoded memory value ("16 GB") remains in the warn-path copy and can drift from the powerfulWarnBelowGigabytes constant.
CotabbyTests/OnboardingTemplateRecommenderTests.swift Updates the disable-floor test probe from 8 GB to 6 GB and the warn-floor test probe from 12 GB to 8 GB, accurately pinning the new boundary decisions. All other cases unchanged.

Fix All in Codex Fix All in Claude Code

Reviews (1): Last reviewed commit: "Re-tune onboarding model recommendations..." | Re-trigger Greptile

Greptile also left 2 inline comments on this PR.

The recommender's warning copy and memory thresholds were sized for the
old ~3-5 GB instruct models. The base-model tiers are much smaller (quick
0.8 GB, everyday 1.4 GB, powerful 2.6 GB), so the Powerful disable floor
of 10 GB wrongly excluded stock 8 GB Macs that can comfortably run a
2.6 GB model.

Lower the Powerful disable floor to 8 GB (only sub-8 GB, effectively
pre-Apple-Silicon, Macs are now excluded) and derive the '~X GB' sizes in
the warning copy from the model catalog via approximateSizeLabel, so the
copy stays accurate as the catalog changes instead of repeating stale
hardcoded numbers. Warn ceilings are unchanged.
warning = "Needs more memory than this Mac has (uses a \(modelSizeLabel(for: template)) model)."
} else if gigabytes < powerfulWarnBelowGigabytes {
warning = "Uses a ~5 GB model; may run slowly with less than 16 GB of memory."
warning = "Uses a \(modelSizeLabel(for: template)) model; may run slowly with less than 16 GB of memory."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 The model size is now derived from the catalog, but the memory ceiling "16 GB" is still hardcoded in this warning string. The PR description claims "no hardcoded sizes remain in the recommender," yet this literal can drift if powerfulWarnBelowGigabytes is ever adjusted. Deriving it from the constant keeps the copy and the logic in sync.

Suggested change
warning = "Uses a \(modelSizeLabel(for: template)) model; may run slowly with less than 16 GB of memory."
warning = "Uses a \(modelSizeLabel(for: template)) model; may run slowly with less than \(Int(powerfulWarnBelowGigabytes)) GB of memory."

Fix in Codex Fix in Claude Code

/// in sync with the actual model instead of a hardcoded number. Falls back to a generic phrase if
/// the filename is missing from the catalog.
private static func modelSizeLabel(for template: OnboardingTemplate) -> String {
downloadableModel(filename: template.openSourceModelFilename)?.approximateSizeLabel ?? "local"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 The fallback string "local" produces grammatically awkward warning copy — e.g. "Uses a local model, which may run slowly…" — if a template's openSourceModelFilename is absent from the catalog. A phrase that completes the sentence naturally would be less confusing to users who might see this in edge cases.

Suggested change
downloadableModel(filename: template.openSourceModelFilename)?.approximateSizeLabel ?? "local"
downloadableModel(filename: template.openSourceModelFilename)?.approximateSizeLabel ?? "large"

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Codex Fix in Claude Code

@FuJacob FuJacob merged commit 66749e0 into main Jun 1, 2026
4 checks passed
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.

1 participant