feat: add model load error handling to improve UX #5802
Merged
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.
Caution
Changes requested ❌
Reviewed everything up to 159f1e0 in 2 minutes and 52 seconds. Click for details.
- Reviewed
275lines of code in5files - 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/ErrorMessage.tsx:29
- Draft comment:
The fragment (<></>) wrapping the spans inside theis redundant. You can return the spans directly.
- Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. web-app/src/containers/dialogs/LoadModelErrorDialog.tsx:19
- Draft comment:
Similar to ErrorMessage, consider handling potential errors from navigator.clipboard.writeText (e.g. using try/catch) in handleCopy. - Reason this comment was not posted:
Marked as duplicate.
3. web-app/src/hooks/useChat.ts:406
- Draft comment:
Consider logging the error details (e.g. using console.error) before calling setModelLoadError, to aid debugging. - 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% The code already handles errors appropriately by extracting the message and passing it to setModelLoadError. While logging could help with debugging, the error is already being handled and displayed to the user through setModelLoadError. The removal of console.error appears intentional as part of consolidating error handling. Adding back logging would be a matter of preference rather than fixing a clear issue. Error logging can be valuable for debugging production issues. The current code only shows errors to the user but doesn't preserve them for debugging. While logging could help debugging, this appears to be an intentional change to consolidate error handling through setModelLoadError. There's no clear evidence that additional logging is required or that its removal was a mistake. The comment should be deleted as it suggests reverting an intentional change without strong evidence that the change was incorrect.
4. web-app/src/routes/__root.tsx:101
- Draft comment:
Ensure that LoadModelErrorDialog’s placement within the root layout matches your UI/UX expectations, particularly with respect to stacking with other dialogs. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_zqMaJeUTJdGiURXJ
You can customize 
Barecheck - Code coverage reportTotal: 35.11%Your code coverage diff: -0.24% ▾ Uncovered files and lines
|
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 3eb82f2 in 2 minutes and 3 seconds. Click for details.
- Reviewed
742lines of code in3files - 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. extensions/llamacpp-extension/src/util.ts:26
- Draft comment:
Consider validating that 'state' exists in the parsed config before accessing properties to avoid runtime 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 has proper error handling through try-catch. TypeScript provides compile-time type checking. An additional runtime check would be redundant since any undefined access would be caught by the existing try-catch. The function returns null on any error, which is a clean way to handle invalid configurations. The comment raises a valid concern about defensive programming. Runtime type checking can catch issues that TypeScript can't catch at compile time. The existing try-catch block already provides sufficient runtime protection, making an additional check redundant. The code handles errors gracefully by returning null. The comment should be deleted as the code already has appropriate error handling through try-catch and TypeScript type checking.
2. src-tauri/src/core/utils/download.rs:109
- Draft comment:
The HEAD request handling is noted as buggy; consider revisiting the logic for obtaining file size. - 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. src-tauri/src/core/utils/download.rs:69
- Draft comment:
Using std::fs::remove_file in an async context blocks the thread; consider using tokio::fs::remove_file instead. - 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. src-tauri/src/core/utils/download.rs:249
- Draft comment:
Avoid using unwrap() on app.emit; handle potential errors to prevent runtime panics. - 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_F8pO4x1dnok7TSAq
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed eac8166 in 1 minute and 38 seconds. Click for details.
- Reviewed
34lines 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. web-app/src/hooks/useChat.ts:367
- Draft comment:
Ensure stopModel is called with the correct provider; changing 'cortex' to 'llamacpp' now aligns with the conditional check. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. web-app/src/services/mcp.ts:26
- Draft comment:
Switching from '||' to '??' changes fallback behavior: an empty string will not be replaced and will throw on JSON.parse. Confirm that getMcpConfigs doesn't return empty strings. - 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 raises a theoretical issue about empty strings, but analyzing the code shows this isn't possible here. Line 25 ensures configString is either the API result or '{}'. The second ?? on line 26 is actually redundant since configString can never be null/undefined at that point. The change from || to ?? is safe here. I could be wrong about the API behavior - maybe getMcpConfigs() could return an empty string explicitly. Even if getMcpConfigs() returns an empty string, line 25's ?? '{}' would catch that case, making the concern moot. The comment should be deleted because the code structure already prevents empty string issues through the first ?? operator on line 25.
Workflow ID: wflow_iMBIFA0muwjqgo4z
You can customize
louis-jan
force-pushed
the
feat/model-load-error-handling
branch
from
eac8166 to
ecc318f
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ecc318f in 1 minute and 40 seconds. Click for details.
- Reviewed
297lines of code in3files - 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. src-tauri/src/core/utils/download.rs:648
- Draft comment:
Whitespace and formatting changes appear non-functional. Ensure these changes are consistent with our coding style. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src-tauri/src/core/utils/download.rs:640
- Draft comment:
The serialization test 'test_proxy_config_serialization' was removed. Confirm this removal is intentional and test coverage is maintained. - 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/hooks/useChat.ts:369
- Draft comment:
stopModel now uses 'llamacpp' instead of 'cortex'. Verify this change correctly aligns with the intended model provider. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%The comment is asking the PR author to verify if the change aligns with the intended model provider, which violates the rule against asking for confirmation of intention. However, it does point out a specific change in the code, which could be useful if rephrased to suggest checking the change rather than asking for verification.
4. web-app/src/hooks/useChat.ts:406
- Draft comment:
Error handling now calls setModelLoadError on exception. Confirm that setModelLoadError properly displays errors in the UI. - 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.
5. web-app/src/services/mcp.ts:26
- Draft comment:
Switching from '||' to '??' in JSON.parse may produce a different fallback behavior if getMcpConfigs returns an empty string. Confirm that the API will not return an empty string to avoid JSON.parse("") 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 comment misses that configString is already protected by ?? '{}' on line 25. If getMcpConfigs returns an empty string, it would pass through the ?? operator (since empty string is not nullish), but that's actually fine - if the API returns an empty string, that's a valid input to JSON.parse. The real protection against invalid JSON is already handled by the first ?? operator. Could there be edge cases where an empty string response is actually indicative of an API error that we should handle differently? No - if empty string was meant to be treated as an error, the API should return null/undefined instead. Empty string is a valid response that should be parsed as-is. The comment should be deleted. The code is already properly handling null/undefined cases, and empty strings are valid JSON parse inputs.
Workflow ID: wflow_ggp9BC4tL1HQxmFH
You can customize
louis-jan
force-pushed
the
feat/model-load-error-handling
branch
from
ecc318f to
cea400e
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed cea400e in 1 minute and 26 seconds. Click for details.
- Reviewed
284lines 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. src-tauri/src/core/utils/download.rs:82
- Draft comment:
Several whitespace-only changes are present. Consider separating formatting changes from functional changes to keep the diff focused. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src-tauri/src/core/utils/download.rs:650
- Draft comment:
The test 'test_proxy_config_serialization' has been removed. Confirm that its coverage is no longer needed or is tested elsewhere. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that the coverage of a removed test is no longer needed or is tested elsewhere. This falls under asking the author to confirm their intention or to ensure something, which is against the rules.
3. web-app/src/hooks/useChat.ts:369
- Draft comment:
Provider string corrected from 'cortex' to 'llamacpp' in stopModel call. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, stating what was changed without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest any improvements.
4. web-app/src/hooks/useChat.ts:64
- Draft comment:
Integration of the new error hook 'setModelLoadError' enhances UX. Ensure the LoadModelErrorDialog component properly consumes this state. - 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_z0D4UaxSiX0AlTyj
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.
Changes
fail to load or start
recovery options
Recording.2025-07-17.221433.mp4
Test plan
Important
Improved model loading error handling with new error dialog and state management in the web app.
useChat.tsby settingmodelLoadErroron error.LoadModelErrorDialog.tsxto display model load errors with copy functionality.LoadModelErrorDialogin__root.tsxto show error dialogs globally.ErrorMessage.tsxcomponent for displaying error messages with copy option.LoadModelErrorDialog.tsxprovides error details and recovery options.useModelLoad.tsto manage model load error state.useChat.tsto usesetModelLoadErrorfor error handling.download.rs.This description was created by
for cea400e. You can customize this summary. It will automatically update as commits are pushed.