feat: Enhance Llama.cpp backend management with persistence #5886
+675
−384
Conversation
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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to d773b52 in 2 minutes and 36 seconds. Click for details.
- Reviewed
400lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/src/index.ts:217
- Draft comment:
Using localeCompare to sort version strings may lead to incorrect ordering for semantic versions (e.g. '1.10.0' vs '1.2.0'). Consider using a proper semver parser or comparator. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a valid technical concern - localeCompare() does lexicographical comparison which can give incorrect results for semantic versions. For example, '1.10.0' would be considered less than '1.2.0' because '1' is compared to '1', then '1' is compared to '2'. However, looking at the broader context, this is used for comparing backend versions which may not follow semver format. The comment is speculative ("may lead to") and doesn't provide evidence that this is actually causing problems. The comment identifies a real technical issue with string comparison of version numbers. If the versions do follow semver format, this could cause the wrong version to be selected as "latest". While technically correct, the comment is speculative and there's no evidence that the backend versions follow semver format or that this is causing actual problems. The current code may be intentionally using lexicographical comparison. The comment should be deleted because it is speculative and makes assumptions about the version format without evidence. If version comparison becomes a real issue, it can be addressed when there is concrete evidence of problems.
2. extensions/llamacpp-extension/src/index.ts:312
- Draft comment:
When splitting the saved backend setting, ensure the string contains '/' to avoid undefined values. Consider adding a format check before calling split. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The suggestion is technically correct - splitting a string that doesn't contain '/' could lead to undefined values. However, the code already handles this safely by checking if backendType exists before using it. Additionally, savedBackendSetting comes from a controlled source (the settings) where the format is enforced. The suggestion would add defensive programming but doesn't fix any actual issue. I could be wrong about the source of savedBackendSetting - maybe it could come from user input or storage where the format isn't guaranteed. The current code would still handle it safely but might be less clear about the intent. Looking at the code more carefully, savedBackendSetting comes from the settings system where the format is strictly controlled. The value can only be selected from a predefined list of options that all follow the version/backend format. The existing null check is sufficient. Delete the comment. While technically correct, the suggestion is unnecessary since the code already handles the case safely and the input format is guaranteed by the settings system.
3. extensions/llamacpp-extension/src/index.ts:719
- Draft comment:
In onSettingUpdate, the asynchronous closure calling ensureBackendReady is not awaited, meaning any errors will be lost. Consider handling promise rejections. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. extensions/llamacpp-extension/src/index.ts:619
- Draft comment:
The removeOldBackend cleanup relies on directory naming conventions. Ensure that version names and backend directory structures are strictly controlled to avoid accidental deletions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure that version names and directory structures are controlled, which is similar to asking them to double-check or verify something. This violates the rule against asking the author to ensure behavior is intended or to double-check things.
5. extensions/llamacpp-extension/src/index.ts:180
- Draft comment:
The localStorage access in get/set/clearStoredBackendType is wrapped in try/catch. Ensure that localStorage is available in the runtime environment (e.g. if running in non-browser contexts). - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_NOzXxB6d41g6V8pM
You can customize 
urmauur
approved these changes
Jul 24, 2025
|
LGTM |
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed d62006b in 1 minute and 42 seconds. Click for details.
- Reviewed
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/src/index.ts:889
- Draft comment:
Consider using a consistent, namespaced key (e.g., 'llamacpp_models_migrated') instead of 'cortex_models_migrated' to avoid confusion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While consistent namespacing is generally good practice, this is a one-time migration flag that will be set once and never used again. The actual name of the key doesn't impact functionality. The comment is suggesting a style improvement rather than fixing a bug or serious issue. Given the temporary nature of this migration flag, the naming is not critically important. I could be undervaluing the importance of consistent namespacing. Poor naming conventions could lead to key collisions or confusion for other developers. The risk of key collision is very low since this is a one-time migration flag. The temporary nature of the flag makes extensive namespacing unnecessary overhead. The comment should be deleted as it suggests a minor style improvement for a temporary migration flag that doesn't meaningfully impact the code's functionality or maintainability.
Workflow ID: wflow_Z3jmGJPVYjGaSRby
You can customize
qnixsynapse
force-pushed
the
fix/update_version_only
branch
from
d62006b to
cb68f41
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed cb68f41 in 1 minute and 27 seconds. Click for details.
- Reviewed
104lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/src/index.ts:312
- Draft comment:
Good improvement: the change checks the current stored backend value before updating it, which avoids unnecessary writes. Consider extracting this repeated conditional update into a helper function to reduce code duplication. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. extensions/llamacpp-extension/src/index.ts:365
- Draft comment:
Nice addition: updating the UI settings immediately to reflect the new backend configuration improves clarity. The logging clearly communicates the effective backend. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. extensions/llamacpp-extension/src/index.ts:719
- Draft comment:
Consistent conditional update: the onSettingUpdate handler now only updates the localStorage if the backend differs. This aligns well with the previous changes and helps preserve user preferences. Great job resolving the bug! - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_3PaVNlpxlh7Ou1A4
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5603794 in 1 minute and 16 seconds. Click for details.
- Reviewed
62lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/routes/settings/providers/$providerName.tsx:39
- Draft comment:
Removed EngineManager import is fine if it's no longer needed. Ensure that any engine-specific settings for llamacpp get handled elsewhere. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. web-app/src/routes/settings/providers/$providerName.tsx:82
- Draft comment:
Local state for settings and its useEffect were removed. This simplifies the component, but confirm that provider.settings always contains up-to-date data, especially for llamacpp. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. web-app/src/routes/settings/providers/$providerName.tsx:274
- Draft comment:
Mapping directly over provider?.settings is concise. Consider defaulting to an empty array (e.g., (provider?.settings || [])) to avoid runtime errors if settings is undefined. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_FZmYHSbaYLfQM1DW
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 01ada08 in 1 minute and 55 seconds. Click for details.
- Reviewed
273lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/settings.json:59
- Draft comment:
Removed 'Context Size' (ctx_size) setting. Ensure that any legacy configurations or references in the code are updated/migrated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that legacy configurations or references are updated, which violates the rule against asking the author to ensure things are tested or reviewed. It doesn't provide a specific suggestion or point out a specific issue.
2. extensions/llamacpp-extension/settings.json:116
- Draft comment:
Removed 'GPU Layers' (n_gpu_layers) setting. Confirm that this removal doesn’t affect any backend logic relying on GPU layer configuration. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. web-app/src/routes/settings/providers/$providerName.tsx:88
- Draft comment:
The 'needsBackendConfig' check is effective in determining if backend configuration is missing. Review if additional falsy values should be handled for 'version_backend'. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. web-app/src/routes/settings/providers/$providerName.tsx:124
- Draft comment:
Auto-refresh useEffect for backend configuration (every 3 seconds) is implemented correctly. Verify that this polling frequency is optimal without overloading the system. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify the polling frequency, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue with the code. Therefore, it should be removed.
5. web-app/src/routes/settings/providers/$providerName.tsx:323
- Draft comment:
The inline onChange handler for DynamicControllerSetting properly updates settings and resets related fields when 'version_backend' changes. Consider refactoring this callback into a separate helper for better readability if the logic expands. - Reason this comment was not posted:
Comment was on unchanged code.
6. web-app/src/routes/settings/providers/$providerName.tsx:1
- Draft comment:
Disabling 'react-hooks/exhaustive-deps' at the top may mask dependency issues in hooks. Ensure all dependencies are correctly managed and consider re-enabling the lint rule once confirmed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment identifies a real issue (missing dependencies in hooks), it's not specific enough about what needs to be fixed. It's more of a general warning without actionable guidance. The comment suggests "consider re-enabling once confirmed" but doesn't explain what needs to be confirmed or how to fix the dependency issues. The comment points to a real code quality issue that could cause bugs. Missing dependencies in useEffect hooks can lead to stale closures and unexpected behavior. However, the comment doesn't provide specific guidance on which hooks need fixing or how to fix them. It's more of a general warning that doesn't help the developer take action. Delete the comment. While it points to a real issue, it's not specific or actionable enough. A better comment would point out specific hooks that need fixing and suggest how to fix them.
Workflow ID: wflow_L5V0QxqS4pf6Qd3P
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a601434 in 1 minute and 48 seconds. Click for details.
- Reviewed
26lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/settings.json:60
- Draft comment:
Changing the key from 'context_shift' to 'ctx_shift' is consistent with the UI code. Consider adding migration logic or documentation for users with existing settings to avoid losing their configuration. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a valid concern - renaming a configuration key could break existing user settings. However, this falls into the category of speculative comments ("If users have existing settings..."). We don't know if this is actually a problem or if migration logic already exists elsewhere. The comment is making assumptions about the codebase without strong evidence. I might be underestimating the impact of breaking changes on user configuration. Configuration migration is an important consideration. While configuration migration is important, without seeing the full context of how settings are loaded and migrated, this comment is speculative and could be incorrect if migration logic already exists elsewhere. The comment should be deleted as it makes assumptions without strong evidence and is speculative about potential issues rather than pointing out a definite problem.
2. web-app/src/hooks/useChat.ts:185
- Draft comment:
The toggleOnContextShifting function directly accesses the setting using the new key without checking if it exists. Add a check for a valid index and verify that the property name ('controller_props') matches the setting object structure (which in JSON uses 'controllerProps'). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_078RI9k50rGU04o2
You can customize
This commit introduces significant improvements to how the Llama.cpp extension manages and updates its backend installations, focusing on user preference persistence and smarter auto-updates. Key changes include: * **Persistent Backend Type Preference:** The extension now stores the user's preferred backend type (e.g., `cuda`, `cpu`, `metal`) in `localStorage`. This ensures that even after updates or restarts, the system attempts to use the user's previously selected backend type, if available. * **Intelligent Auto-Update:** The auto-update mechanism has been refined to prioritize updating to the **latest version of the *currently selected backend type*** rather than always defaulting to the "best available" backend (which might change). This respects user choice while keeping the chosen backend type up-to-date. * **Improved Initial Installation/Configuration:** For fresh installations or cases where the `version_backend` setting is invalid, the system now intelligently determines and installs the best available backend, then persists its type. * **Refined Old Backend Cleanup:** The `removeOldBackends` function has been renamed to `removeOldBackend` and modified to specifically clean up *older versions of the currently selected backend type*, preventing the accumulation of unnecessary files while preserving other backend types the user might switch to. * **Robust Local Storage Handling:** New private methods (`getStoredBackendType`, `setStoredBackendType`, `clearStoredBackendType`) are introduced to safely interact with `localStorage`, including error handling for potential `localStorage` access issues. * **Version Filtering Utility:** A new utility `findLatestVersionForBackend` helps in identifying the latest available version for a specific backend type from a list of supported backends. These changes provide a more stable, user-friendly, and maintainable backend management experience for the Llama.cpp extension. Fixes: #5883
This commit refines the Llama.cpp extension's backend management by: * **Optimizing `localStorage` Writes:** The system now only writes the backend type preference to `localStorage` if the new value is different from the currently stored one. This reduces unnecessary `localStorage` operations. * **Ensuring UI Consistency on Initial Setup:** When a fresh installation or an invalid backend configuration is detected, the UI settings are now explicitly updated to reflect the newly determined `effectiveBackendString`, ensuring the displayed setting matches the active configuration. These changes improve performance by reducing redundant storage operations and enhance user experience by maintaining UI synchronization with the backend state.
This reverts commit ce6af62.
louis-jan
force-pushed
the
fix/update_version_only
branch
from
93ebca8 to
8daf0fa
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 93ebca8 in 2 minutes and 30 seconds. Click for details.
- Reviewed
85lines of code in4files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/ModelSetting.tsx:71
- Draft comment:
Removal of updateModel: Ensure that relying solely on updateProvider for state persistence is intended. The conversion logic for settings values (to number) is no longer applied, so verify that this behavior is acceptable. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. web-app/src/hooks/useModelProvider.ts:77
- Draft comment:
When mapping provider.models to compute updatedModels, consider defaulting to an empty array if provider.models is undefined to avoid potential errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code already handles undefined provider.models safely through optional chaining. If provider.models is undefined, the ?. operator will return undefined and no mapping will occur. The suggestion would change the behavior to map over an empty array instead, but this isn't necessarily better or required for safety. The current code's behavior is intentional and safe. The suggestion could make the code more explicit about handling undefined cases. There might be places elsewhere in the codebase expecting an array rather than undefined. The current code is already safe and the change in behavior isn't clearly beneficial. The code's context suggests undefined is an acceptable return value, as seen in the ternary on line 87 where either updatedModels or mergedModels is used. The comment should be deleted as it suggests a change that isn't necessary for correctness or safety, and the current code already handles the undefined case appropriately.
3. web-app/src/routes/threads/$threadId.tsx:209
- Draft comment:
debouncedScroll is created without a delay parameter. Specify an explicit debounce delay to ensure proper throttling of scroll events. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. web-app/src/routes/threads/$threadId.tsx:245
- Draft comment:
Both an onScroll prop and an addEventListener for scroll (using debouncedScroll) are attached. Confirm that having duplicate scroll event handlers is intentional to avoid redundant updates. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_Cjk3u6sWZasDckEp
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed ffd3b21 in 2 minutes and 43 seconds. Click for details.
- Reviewed
531lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/routes/hub/index.tsx:587
- Draft comment:
Each virtualized item container should be absolutely positioned using virtualItem.start (e.g. style={{ position: 'absolute', top: virtualItem.start, left: 0, width: '100%' }}) to correctly position items within the relative wrapper. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%This comment is purely informative, providing guidance on how to position virtualized item containers. It doesn't suggest a change, ask for confirmation, or point out a potential issue. It seems to be more of a best practice or guideline rather than a specific actionable comment.
2. web-app/src/routes/hub/index.tsx:411
- Draft comment:
Define helper variables (e.g. isDownloading and isLastStep) before they are used inside handleJoyrideCallback to improve clarity and avoid potential temporal dead zone issues. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. web-app/src/routes/hub/index.tsx:222
- Draft comment:
Review the estimated size value (35) used in useVirtualizer. Ensure it accurately reflects the actual rendered card height to prevent layout discrepancies during scrolling. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. web-app/src/routes/hub/index.tsx:609
- Draft comment:
There appears to be an extra space in the className string ('capitalize sm:max-w-none'). Consider removing the additional space for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_7GuGB8cwLEkjoCW3
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 81ce2dc in 51 seconds. Click for details.
- Reviewed
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/routes/hub/index.tsx:591
- Draft comment:
Good removal of the debug console.log. Keeping production code clean improves performance and security. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_EKEU3xWdQ3kewBHx
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed e6e29f0 in 1 minute and 2 seconds. Click for details.
- Reviewed
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/routes/settings/providers/$providerName.tsx:391
- Draft comment:
The added className hides the 'device' setting. Consider conditionally rendering the CardItem (or filtering it out) if it's not needed to avoid mounting hidden elements. Also, since similar logic exists in DynamicControllerSetting (line 316), it might be beneficial to refactor this logic for clarity and consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_nkAqvcwThWLuQFfU
You can customize
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.
This commit introduces significant improvements to how the Llama.cpp extension manages and updates its backend installations, focusing on user preference persistence and smarter auto-updates.
Key changes include:
cuda,cpu,metal) inlocalStorage. This ensures that even after updates or restarts, the system attempts to use the user's previously selected backend type, if available.version_backendsetting is invalid, the system now intelligently determines and installs the best available backend, then persists its type.removeOldBackendsfunction has been renamed toremoveOldBackendand modified to specifically clean up older versions of the currently selected backend type, preventing the accumulation of unnecessary files while preserving other backend types the user might switch to.getStoredBackendType,setStoredBackendType,clearStoredBackendType) are introduced to safely interact withlocalStorage, including error handling for potentiallocalStorageaccess issues.findLatestVersionForBackendhelps in identifying the latest available version for a specific backend type from a list of supported backends.These changes provide a more stable, user-friendly, and maintainable backend management experience for the Llama.cpp extension.
Fixes: #5883
Important
Enhances Llama.cpp extension with persistent backend preferences, smarter updates, and improved installation.
localStoragefor persistence across updates/restarts inindex.ts.index.ts.version_backendis invalid inindex.ts.removeOldBackendstoremoveOldBackendand modifies it to clean up older versions of the selected backend type inindex.ts.getStoredBackendType,setStoredBackendType, andclearStoredBackendTypeforlocalStorageinteractions inindex.ts.findLatestVersionForBackendto identify the latest version for a specific backend type inindex.ts.context_shifttoctx_shiftinsettings.jsonanduseChat.ts.n_gpu_layersandctx_sizesettings fromsettings.json.@tanstack/react-virtualdependency inpackage.json.This description was created by
for e6e29f0. You can customize this summary. It will automatically update as commits are pushed.