fix(threads): remove runaway nThreads migration and show resolved Au…#313
Conversation
…to count migratePersistedState was resetting any explicit nThreads=4 back to Auto on every app open, silently overriding the user's choice. Deleted the unguarded migration block. Also resolves the Auto sentinel to its actual hardware thread count (cores <= 4 ? cores : floor(cores * 0.8)) so the settings slider shows "Auto (4)" instead of just "Auto".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the application to display the resolved thread count when the CPU Threads setting is set to 'Auto'. It introduces state management and an effect within the useTextGenerationAdvanced hook to fetch this value from the hardware service, along with corresponding updates to the test suite. Review feedback focuses on optimizing performance and reliability by caching the static hardware information, implementing a cancellation flag for the asynchronous state update to prevent memory leaks, and fixing a logic inconsistency between the display label and the slider's fallback values.
| import { useAppStore } from '../stores'; | ||
| import { CacheType, INFERENCE_BACKENDS } from '../types'; | ||
| import { hardwareService } from '../services/hardware'; | ||
|
|
There was a problem hiding this comment.
To avoid a visible flicker from "Auto" to "Auto (N)" on every mount of the settings screen, and to prevent redundant asynchronous calls to fetch the hardware core count (which is static for the device), consider caching the resolved thread count in a module-level variable.
| let cachedRecommendedThreads: number | null = null; | |
| const [resolvedThreadCount, setResolvedThreadCount] = useState<number | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| if (settings?.nThreads !== 0) return; | ||
| hardwareService.getRecommendedThreadCount().then(setResolvedThreadCount); | ||
| }, [settings?.nThreads]); |
There was a problem hiding this comment.
Initialize the state with the cached value and update the cache once the recommended thread count is resolved. To prevent state updates on unmounted components or race conditions from stale renders, use a cancellation flag within the useEffect as per repository guidelines. The cache update itself can remain 'fire-and-forget' to ensure the value is available for future mounts even if the current one is cancelled.
const [resolvedThreadCount, setResolvedThreadCount] = useState<number | null>(cachedRecommendedThreads);
useEffect(() => {
let isCancelled = false;
if (settings?.nThreads !== 0 || resolvedThreadCount !== null) return;
hardwareService.getRecommendedThreadCount().then(count => {
cachedRecommendedThreads = count;
if (isCancelled) return;
setResolvedThreadCount(count);
});
return () => {
isCancelled = true;
};
}, [settings?.nThreads, resolvedThreadCount]);References
- When handling asynchronous operations in useEffect that update state, ensure to use a cancellation flag to prevent state updates on unmounted components.
- A 'fire-and-forget' asynchronous call is an acceptable pattern for a best-effort fallback to update a cache.
| const cpuThreadsDisplayValue = settings?.nThreads === 0 ? 'Auto' : String(settings?.nThreads ?? 6); | ||
| const cpuThreadsDisplayValue = settings?.nThreads === 0 | ||
| ? (resolvedThreadCount != null ? `Auto (${resolvedThreadCount})` : 'Auto') | ||
| : String(settings?.nThreads ?? 6); |
There was a problem hiding this comment.
The fallback value 6 is inconsistent with the fallback logic used for cpuThreadsSliderValue (which defaults to 1). If settings.nThreads is missing or invalid, the UI will show "6" while the slider remains at the minimum position of "1". Using cpuThreadsSliderValue as the fallback ensures the label and slider stay in sync.
| : String(settings?.nThreads ?? 6); | |
| : String(cpuThreadsSliderValue); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #313 +/- ##
==========================================
+ Coverage 83.85% 83.89% +0.03%
==========================================
Files 224 224
Lines 11478 11481 +3
Branches 3145 3146 +1
==========================================
+ Hits 9625 9632 +7
+ Misses 1070 1069 -1
+ Partials 783 780 -3
🚀 New features to boost your workflow:
|
When nThreads is undefined, the display value fell back to "6" while the slider sat at 1. Both now use cpuThreadsSliderValue as the source of truth, keeping label and slider in sync. Co-authored-by: Dishit <hanmadishit74@gmail.com>
|
1 participant
…to count
migratePersistedState was resetting any explicit nThreads=4 back to Auto on every app open, silently overriding the user's choice. Deleted the unguarded migration block.
Also resolves the Auto sentinel to its actual hardware thread count (cores <= 4 ? cores : floor(cores * 0.8)) so the settings slider shows "Auto (4)" instead of just "Auto".
Summary
Type of Change
Screenshots / Screen Recordings
Android
iOS
Checklist
General
Testing
npm test)React Native Specific
project.pbxproj)SPACING/TYPOGRAPHYconstants from the themeuseThemedStylespattern (not inline or staticStyleSheet.create)FlatList/FlashList(not.map()insideScrollView)Performance & Models
/vs\\)Security
Related Issues
Additional Notes