Skip to content

Conversation

@urmauur
Copy link
Member

@urmauur urmauur commented Sep 11, 2025

Describe Your Changes

This pull request introduces a small UI improvement to the "Optimize Settings" section for the llamacpp provider in the ModelSetting component. The main change is the addition of an "experimental" label next to the section heading to clarify the feature's status to users.

UI enhancement:

  • Added an "experimental" badge next to the "Optimize Settings" heading in the llamacpp provider section of ModelSetting.tsx to indicate the feature is experimental.

Fixes Issues

Screenshot 2025-09-11 at 13 09 17
  • Closes #
  • Closes #

Self Checklist

  • Added relevant comments, esp in complex areas
  • Updated docs (for bug fixes / features)
  • Created issues for follow-up changes or refactoring needed

Important

Enhance UI with 'experimental' label for llamacpp settings and refactor noOffloadMmproj to offloadMmproj across codebase.

  • UI Enhancement:
    • Added 'experimental' badge to 'Optimize Settings' in ModelSetting.tsx for llamacpp provider.
  • Code Refactoring:
    • Renamed noOffloadMmproj to offloadMmproj in index.ts, default.ts, and types.ts.
    • Updated logic in planModelLoad() in index.ts to reflect the rename.
  • Functionality:
    • Adjusted planModelLoad() in default.ts to handle offloadMmproj logic.
    • Updated ModelPlan interface in types.ts to include offloadMmproj.

This description was created by Ellipsis for 19aa15f. You can customize this summary. It will automatically update as commits are pushed.

@urmauur urmauur requested a review from qnixsynapse September 11, 2025 06:12
@urmauur urmauur requested a review from Minh141120 September 11, 2025 06:12
@urmauur urmauur moved this to Needs Review in Jan Sep 11, 2025
@urmauur urmauur added this to the v0.6.10 milestone Sep 11, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to 791563e in 1 minute and 3 seconds. Click for details.
  • Reviewed 28 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft 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:246
  • Draft comment:
    Good use of a flex container for placing the experimental badge next to the heading. Please ensure the translation key 'mcp-servers:experimental' exists in all locales or includes a fallback.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50% None
2. web-app/src/containers/ModelSetting.tsx:248
  • Draft comment:
    Consider adding ARIA attributes (e.g., aria-label) to the experimental badge for improved accessibility.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50% None

Workflow ID: wflow_c61C8lfdEahgyDcj

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 11, 2025

@github-actions
Copy link
Contributor

github-actions bot commented Sep 11, 2025

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed bc29046 in 1 minute and 11 seconds. Click for details.
  • Reviewed 42 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft 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:49
  • Draft comment:
    Good update: passing mmproj_path as optional in planModelLoad. Verify downstream null/undefined handling.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. web-app/src/containers/ModelSetting.tsx:252
  • Draft comment:
    New experimental badge added; ensure the translation key 'mcp-servers:experimental' exists and styling is consistent.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
3. web-app/src/services/models/types.ts:139
  • Draft comment:
    planModelLoad signature now accepts an optional mmprojPath; ensure related docs and call sites are updated accordingly.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_b5Yqv1uZCM11CNgZ

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

The flag `noOffloadMmproj` was misleading – it actually indicates when the mmproj file **is** offloaded to VRAM. Renaming it to `offloadMmproj` clarifies its purpose and aligns the naming with the surrounding code.

Additionally, the `planModelLoad` signature has been reordered to place `mmprojPath` before `requestedCtx`, improving readability and making the optional parameters more intuitive. All related logic, calculations, and log messages have been updated to use the new flag name.
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed abd0cbe in 2 minutes and 55 seconds. Click for details.
  • Reviewed 84 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft 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:79
  • Draft comment:
    Renamed property in ModelPlan: changed from 'noOffloadMmproj' to 'offloadMmproj'. Ensure all consumers and docs are updated to reflect this breaking change.
  • 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.
2. extensions/llamacpp-extension/src/index.ts:2277
  • Draft comment:
    In the GPU memory adjustment loop, the 'estimatedGPUUsage' is computed once and then 'newEstimate' is calculated inside the loop. Consider updating the main usage variable (or clarifying the break logic) so that the loop condition uses the updated estimate.
  • 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. extensions/llamacpp-extension/src/index.ts:2050
  • Draft comment:
    Verify parameter order in planModelLoad: the second parameter is now mmprojPath (optional) and the third requestedCtx. Ensure all callers use the new ordering.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_VQ2wegamYTcavHLm

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed 14c7fc0 in 2 minutes and 22 seconds. Click for details.
  • Reviewed 80 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft 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:258
  • Draft comment:
    UI Enhancement: An 'experimental' badge has been added to the Optimize Settings heading. Ensure the translation key 'mcp-servers:experimental' exists in your localization files.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. web-app/src/services/models/default.ts:519
  • Draft comment:
    Reordered parameters in planModelLoad call (now passing mmprojPath before requestedCtx). Confirm that engine implementations support this new parameter order.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 40% <= threshold 50% The comment is asking the author to confirm that the engine implementations support the new parameter order. This is a request for confirmation, which is against the rules. However, it does point out a specific change in parameter order, which could be useful for the author to consider. The comment could be rephrased to suggest checking compatibility without directly asking for confirmation.
3. web-app/src/services/models/types.ts:138
  • Draft comment:
    Updated parameter order for planModelLoad in the interface to (modelPath, mmprojPath, requestedCtx). Verify that all implementations and usage align with this new signature.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify that all implementations and usage align with the new signature. This falls under the rule of not asking the author to double-check things or ensure behavior is intended. Therefore, this comment should be removed.

Workflow ID: wflow_me1QAbwzU11V6tkN

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Expose the new `mmproj_offload` option in the model settings UI and include it in the `ModelPlan` type. The component now collects the offload flag (`result.offloadMmproj`) and queues it with other setting updates to ensure a single atomic change, preventing race conditions when toggling this feature. This enables users to control MMProj offloading directly from the app.
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed 8f67f29 in 2 minutes and 8 seconds. Click for details.
  • Reviewed 33 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft 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/services/models/types.ts:88
  • Draft comment:
    Added 'offloadMmproj' in ModelPlan; ensure the UI uses the matching key name so that settings update and restart checks align.
  • 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% This comment violates several rules. It's asking to "ensure" something, which is explicitly called out as not useful in our rules. It's speculative about potential UI misalignment without concrete evidence. It's also not clearly requiring a code change - it's more of a reminder or warning. The type system would catch any mismatched property names automatically. Maybe this is a critical setting that needs special UI handling that isn't obvious from just the types file? Maybe there's important context about UI-backend coupling that I'm missing? Even if there are special UI considerations, the comment is still phrased as a verification request rather than a concrete issue. The TypeScript compiler would catch any property name mismatches. The comment should be deleted as it's a verification request that doesn't point out a concrete issue, and any property name mismatches would be caught by TypeScript.
2. web-app/src/services/models/types.ts:88
  • Draft comment:
    The new property name 'offloadMmproj' may be a typographical error or inconsistent in its abbreviation. Please confirm if this is spelled as intended or if it should follow the same capitalization convention as in 'noOffloadKVCache' (for example, 'offloadMMProj').
  • 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 comment suggests inconsistency with noOffloadKVCache, but looking at the full context, 'mmproj' is actually used consistently throughout the file. The casing appears to be intentional - 'mmproj' is treated as a single term while 'KVCache' is clearly two words. This seems to be an established pattern in the codebase. I could be wrong about the codebase's conventions - there might be other files where 'MMProj' is used instead. Even if other files use different casing, this file is internally consistent in its use of 'mmproj', and the comment would lead to inconsistency within this file. The comment should be deleted as it would actually introduce inconsistency into this file, where 'mmproj' is used consistently throughout.

Workflow ID: wflow_lCWbAEtEy24L4LEB

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Implemented `isAbsolutePath` helper to correctly identify POSIX, Windows drive‑letter, and UNC absolute paths. Updated `planModelLoad` to automatically resolve relative model and mmproj paths against the Jan data folder, enhancing usability for users supplying non‑absolute paths. Also refined minor formatting for readability.
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed 7c41408 in 2 minutes and 47 seconds. Click for details.
  • Reviewed 50 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft 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:2052
  • Draft comment:
    Typo: In the comment "Normalize back‑slashes to forward‑slashes first.", the dash in "back‑slashes" is a non-standard dash. Consider replacing it with a standard hyphen ("-") for consistency and clarity.
  • 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% While the comment is technically correct about the non-standard dash character, this is an extremely minor issue that doesn't affect functionality. The comment is in a code comment, not in actual code. The rules state not to make purely informative comments or comments that are obvious/unimportant. This seems to fall into that category. Could a non-standard dash character cause issues with text editors or source control systems? Could it affect code readability? Non-standard dashes in comments are very unlikely to cause any real issues. The code and comments are still perfectly readable and functional. This is an extremely minor stylistic issue. Delete this comment. While technically correct, it points out an extremely minor stylistic issue in a comment that doesn't affect functionality. This violates the rule about not making unimportant comments.
2. extensions/llamacpp-extension/src/index.ts:2056
  • Draft comment:
    Typo: In the comment "Drive‑letter Windows (C: or D:)" the dash in "Drive‑letter" is a non-standard dash. Consider using a regular hyphen ("-") for consistency.
  • 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% While the comment is technically correct about the non-standard dash character, this is an extremely minor issue that doesn't affect functionality. The dash is in a comment, not in code. Comments are for human readability and the non-standard dash doesn't significantly impact that. This feels like unnecessary nitpicking. Could inconsistent character encoding cause issues in some environments or version control systems? Could this be part of a broader style guide requirement? Even if there were encoding issues, they would only affect this comment line, not functionality. Style guides typically don't get this granular about comment formatting. The benefit of fixing this is extremely low compared to the overhead of making the change. Delete this comment. It's an extremely minor formatting issue in a comment that doesn't impact functionality or readability.

Workflow ID: wflow_mDVY55Kmmzf741CM

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 19aa15f in 1 minute and 26 seconds. Click for details.
  • Reviewed 20 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft 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/services/models/default.ts:533
  • Draft comment:
    Ensure consistent naming for the offload setting. Here the fallback returns use 'offloadMmproj' (camelCase), while provider settings use 'offload_mmproj' (snake_case). Consider standardizing the naming and adding a brief code comment on its purpose.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_KgLmFYSydvE78LLP

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@urmauur urmauur merged commit e709d20 into dev Sep 11, 2025
21 checks passed
@urmauur urmauur deleted the enhancement/experimental-label branch September 11, 2025 09:12
@github-project-automation github-project-automation bot moved this from Needs Review to QA in Jan Sep 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants