fix: Catch local API server various errors

[Vanalite]

Describe Your Changes

Add various error catching during the start of Local API server

• Catch occupied port
• Catch model file path invalid

Fixes Issues

• Closes bug: Local API Server failed to start after removing a previously selected imported model. #6546

Test plan

Port occupation error catch

Model path error catch

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

Improves error handling for Local API server by catching port conflicts and model path errors, with added tests and UI feedback.

• Error Handling:
  • In proxy.rs, added error handling for port binding failures in start_server().
  • In local-api-server.tsx, added error handling for port conflicts and invalid model paths in toggleAPIServer().
• Testing:
  • In useLocalApiServer.test.ts, added tests for port conflict error messages and API key validation.
  • Added tests for CORS error handling and timeout configurations.
• UI Feedback:
  • In local-api-server.tsx, added toast notifications for specific errors like port occupation and model path issues.

^{This description was created by for 2eef4af. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to ecae0f5 in 2 minutes and 0 seconds. Click for details.

• Reviewed 84 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. src-tauri/src/core/server/proxy.rs:752

• Draft comment:
  Good use of Server::try_bind to catch binding errors. Consider adding contextual information (e.g. with anyhow's context) when returning the error for easier debugging.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

2. web-app/src/routes/settings/local-api-server.tsx:187

• Draft comment:
  In the catch block, consider using 'instanceof Error' for type-safe extraction of the error message instead of checking 'message' in error. This improves robustness when dealing with unknown error types.
• 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 current code is actually more flexible than using instanceof Error since it can handle any object with a message property, not just Error instances. In a TypeScript/JavaScript environment, especially when dealing with errors from various sources (server, model loading, etc.), errors may not always be proper Error instances. The current approach is more defensive and handles a wider range of error formats. The suggestion isn't necessarily wrong - using instanceof Error would be more type-safe. However, it might be too restrictive and could miss valid error messages from non-Error objects. The current implementation is actually a better pattern for this specific use case where errors could come from multiple sources and in various formats. The code needs to be flexible rather than strictly type-safe. The comment should be deleted. The current error handling approach is more appropriate for this context where flexibility is more important than strict type safety.

3. web-app/src/routes/settings/local-api-server.tsx:236

• Draft comment:
  The getButtonText function refers to 'isServerRunning' which is declared later (line 256). Although function execution order ensures it's defined by render time, reordering these declarations for clarity would improve code readability.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

Workflow ID: wflow_pGggCIttHstzIwcD

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

[github-actions]

Barecheck - Code coverage report

Total: 30.53%

Your code coverage diff: -0.03% ▾

Uncovered files and lines

File	Lines
web-app/src/routes/settings/local-api-server.tsx	1-25, 28-30, 32-36, 38, 40-56, 58-64, 66-78, 80-82, 84-92, 95, 97-106, 109-114, 117-127, 129-130, 132, 134, 136-140, 142-146, 148, 150-155, 157-158, 161-166, 169-171, 173-191, 194-196, 199-203, 205-214, 216-234, 236-246, 248-254, 256, 258-266, 268-284, 286-289, 292-301, 303-307, 309-310, 313-327, 329-330, 333-338, 340-364, 366-375, 377-382, 384-385, 388-399, 401-412, 414-419, 421

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed 68d318a in 2 minutes and 45 seconds. Click for details.

• Reviewed 179 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. web-app/src/hooks/__tests__/useLocalApiServer.test.ts:447

• Draft comment:
  No test covers the 'model file path invalid' error mentioned in the PR description; please add a case for it.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment references something from the PR description which I don't have access to. Without seeing the PR description or the actual implementation code, I can't verify if this is a real issue or if testing this scenario is even relevant. The comment is speculative since it's based on information outside the diff. Additionally, asking for test coverage based solely on PR description text violates the rule about not making assumptions about PR descriptions. Maybe there really is an important error case that needs testing. Without seeing the PR description or implementation, I could be wrong about dismissing this. Even if there is a real error case to test, comments should be self-contained and based on the code changes shown, not external documentation. The reviewer should point to specific code that needs testing. Delete this comment as it relies on external context from the PR description and violates the rule about not making assumptions about PR descriptions.

2. web-app/src/hooks/__tests__/useLocalApiServer.test.ts:536

• Draft comment:
  Missing a regression test for issue #6546 (failure to start the server after a previously selected imported model is removed).
• Reason this comment was not posted:
  Comment was on unchanged code.

3. web-app/src/hooks/__tests__/useLocalApiServer.test.ts:591

• Draft comment:
  Add a newline at the end of the file to comply with standard file formatting conventions.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_qbOEYUuhI8vOSFRM

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

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 2eef4af in 1 minute and 48 seconds. Click for details.

• Reviewed 254 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. web-app/src/hooks/__tests__/useLocalApiServer.test.ts:431

• Draft comment:
  The PR description mentions catching errors for an invalid model file path, but no test covers that scenario. Consider adding a test case to simulate and verify the handling of an invalid model file path.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. web-app/src/hooks/__tests__/useLocalApiServer.test.ts:666

• Draft comment:
  Ensure the file ends with a newline to conform with best practices.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

3. web-app/src/hooks/__tests__/useLocalApiServer.test.ts:467

• Draft comment:
  The use of toString() on apiKey is redundant since apiKey is already a string. Consider removing the toString() call.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_StgCRCUx6f00u7Lh

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

[Minh141120]

I am trying to test the case Model path error catch where I import and chat with the model first, and move the model to a different location. but I'm not seeing the popup msg error? But when I navigate back to the Chat UI I can see the error on the console log here.

This is because:

isModelSupported doesn't work because the file doesn't exist anymore. But it is loaded in memory if it has been started before.
and Jan API server doesn't need isModelSupported indicator icon

[louis-jan]

LGTM

[Minh141120]

LGTM!