Skip to content

fix(settings): disable thinking by default and fix phi model detection#355

Open
dishit-wednesday wants to merge 1 commit into
mainfrom
disable-thinking-by-default
Open

fix(settings): disable thinking by default and fix phi model detection#355
dishit-wednesday wants to merge 1 commit into
mainfrom
disable-thinking-by-default

Conversation

@dishit-wednesday

@dishit-wednesday dishit-wednesday commented May 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Thinking mode was on by default for all models that support it, increasing TTFT for new users before they've opted in. New users now start with thinking off and can enable it in settings when needed — better UX especially for vision models where thinking adds latency without clear benefit on first use
  • Dolphin models (dolphin-mistral, dolphin-llama etc.) were incorrectly blocked because 'dolphin'.includes('phi') is true. Replaced with /\bphi[-_]?\d/i so only actual Phi variants (phi-3, phi3, phi-4) are matched

Impact

  • New installs: thinking off by default, toggle still works
  • Existing users: no change, persisted store value takes precedence
  • Dolphin models: now loadable

- Flip thinkingEnabled default to false for new installs; existing
  users unaffected via persisted store
- Fix isPhiModel regex to use word-boundary + digit pattern so dolphin
  models are no longer incorrectly blocked as Phi models

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@sonarqubecloud

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request updates the isPhiModel utility to use a more precise regular expression for identifying Phi models, preventing false positives like 'dolphin'. It also sets the default value of thinkingEnabled to false in the app settings. Feedback was provided to remove redundant toLowerCase() calls in the isPhiModel function, as the new regular expression already handles case-insensitivity.

Comment on lines 72 to +75
const name = modelName.toLowerCase();
const id = modelId.toLowerCase();
return name.includes('phi') || id.includes('phi');
const phiPattern = /\bphi[-_]?\d/i;
return phiPattern.test(name) || phiPattern.test(id);

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.

medium

The toLowerCase() calls are redundant because the regular expression already uses the case-insensitive flag (/i). Removing these calls avoids unnecessary string allocations and simplifies the function. Additionally, the new regex correctly fixes the issue where models like 'dolphin' were being incorrectly blocked because they contained the substring 'phi' (as in 'dol-phi-n'). Note that since modelId is guaranteed to be set in this context, no additional existence guards are required.

Suggested change
const name = modelName.toLowerCase();
const id = modelId.toLowerCase();
return name.includes('phi') || id.includes('phi');
const phiPattern = /\bphi[-_]?\d/i;
return phiPattern.test(name) || phiPattern.test(id);
const phiPattern = /\bphi[-_]?\d/i;
return phiPattern.test(modelName) || phiPattern.test(modelId);
References
  1. Do not add redundant validation guards for fields in DownloadEntry (such as modelId) as these are guaranteed to be set and validated during the creation of the entry.

@codecov

codecov Bot commented May 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.06%. Comparing base (5a8589f) to head (88a47cd).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #355      +/-   ##
==========================================
- Coverage   83.07%   83.06%   -0.01%     
==========================================
  Files         231      231              
  Lines       11930    11931       +1     
  Branches     3272     3272              
==========================================
  Hits         9911     9911              
  Misses       1166     1166              
- Partials      853      854       +1     
Files with missing lines Coverage Δ
src/screens/ModelsScreen/utils.ts 96.66% <100.00%> (+0.05%) ⬆️
src/stores/appStore.ts 83.52% <ø> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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