-
Notifications
You must be signed in to change notification settings - Fork 2.1k
refactor: clean up unused hardware apis #6089
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 ab44fae in 2 minutes and 12 seconds. Click for details.
- Reviewed
1465
lines of code in15
files - Skipped
1
files when reviewing. - Skipped posting
15
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. .github/workflows/template-tauri-build-linux-x64.yml:107
- Draft comment:
Removed vulkan file mapping from deb config. Confirm that GPU detection now fully relies on llama.cpp. - 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 comment makes a speculative connection between removing vulkan.so and GPU detection implementation. Without seeing the actual GPU detection code or llama.cpp integration, we can't verify this claim. The comment also asks for confirmation, which violates our rules about not asking authors to verify things. The comment could be correct about the relationship between vulkan and GPU detection, but we don't have enough context to verify this. We're making assumptions about the codebase architecture. Even if the technical observation about vulkan removal is accurate, the comment asks for confirmation and makes speculative claims about implementation details we can't verify from this diff alone. Delete this comment because it asks for confirmation and makes claims about implementation details that can't be verified from the visible changes alone.
2. Makefile:33
- Draft comment:
Removed 'yarn download:lib' calls in dev/test/build targets. Verify no downstream dependency remains. - 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 that there are no downstream dependencies remaining after removing 'yarn download:lib' calls. This falls under asking the author to double-check things, which is against the rules.
3. package.json:21
- Draft comment:
Removed the 'download:lib' script. Ensure that any dependent logic is refactored accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to ensure that dependent logic is refactored, which is similar to asking them to ensure the change is tested or verified. This violates the rule against asking the author to ensure things are done.
4. scripts/download-lib.mjs:1
- Draft comment:
Entire file deleted; confirm that legacy GPU library download is no longer needed. - 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. src-tauri/src/core/hardware.rs:143
- Draft comment:
Legacy GPU detection code (AMD, NVIDIA, Vulkan) removed. Verify that llama.cpp now handles all GPU detection. - 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 that a specific functionality (GPU detection) is now handled by a different part of the code (llama.cpp). This falls under asking the author to ensure behavior is intended, which is against the rules.
6. src-tauri/src/core/hardware/amd.rs:1
- Draft comment:
File deleted: AMD GPU detection module removed. Confirm that this functionality is fully migrated. - 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.
7. src-tauri/src/core/hardware/nvidia.rs:1
- Draft comment:
File deleted: NVIDIA GPU detection module removed. Ensure llama.cpp covers these cases. - 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.
8. src-tauri/src/core/hardware/vulkan.rs:1
- Draft comment:
File deleted: Vulkan GPU detection module removed; aligns with new centralized detection. - 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.
9. src-tauri/tauri.bundle.windows.nsis.template:696
- Draft comment:
Removed inclusion of vulkan-1.dll from installer. Confirm that it’s unused post-refactor. - Reason this comment was not posted:
Comment looked like it was already resolved.
10. src-tauri/tauri.linux.conf.json:10
- Draft comment:
Removed reference to 'libvulkan.so' from the deb files section. This now matches the updated GPU detection. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, describing a change made in the code without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest any improvements.
11. testRunner.js:1
- Draft comment:
Deleted testRunner.js. Confirm that your testing framework is configured without this custom runner. - 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.
12. web-app/src/hooks/__tests__/useHardware.test.ts:1
- Draft comment:
Removed GPU-specific tests and references. Ensure that hardware tests now align with the updated store. - 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 hardware tests align with the updated store after removing GPU-specific tests and references. This is a request for confirmation and testing, which violates the rules.
13. web-app/src/hooks/useHardware.ts:60
- Draft comment:
Removed GPU-related state and methods. Confirm that no UI components still rely on GPU data. - 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 comment violates several rules: 1) It asks for verification/confirmation which is discouraged 2) It's speculative about potential UI issues 3) It requires knowledge of UI components outside this file. However, it did help me notice the lingering gpuLoading state that should probably be removed. The comment did help identify an actual issue with incomplete GPU removal. Maybe keeping it would help catch this oversight? While the comment led to finding an issue, the comment itself isn't making that specific point - it's just asking for general verification which violates our rules. The comment should be deleted as it asks for verification and requires external context. A better comment would specifically point out the remaining gpuLoading state.
14. web-app/src/routes/hub/index.tsx:504
- Draft comment:
Adjusted loading condition to 'loading && !filteredModels.length' to display available data during load. Verify that this condition meets UX requirements. - 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 that the condition meets UX requirements, which is against the rules. It does not provide a specific suggestion or point out a potential issue with the code itself.
15. web-app/src/routes/system-monitor.tsx:30
- Draft comment:
Polling system usage every 5 seconds using setInterval is acceptable; ensure getSystemUsage handles errors gracefully. - 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_jD1RmNAVlOtM2o97
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this 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 2569999 in 1 minute and 7 seconds. Click for details.
- Reviewed
21
lines of code in2
files - Skipped
1
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. Cargo.toml:2
- Draft comment:
Consider adding a trailing newline at the end of the file for best practices. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. Makefile:44
- Draft comment:
Verify that 'cargo test' runs in the correct context; if tests should be scoped to a specific crate, consider specifying the manifest path. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_im2ESeX0myrGG4Yd
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this 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 b8f5fd5 in 1 minute and 44 seconds. Click for details.
- Reviewed
69
lines of code in3
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. Makefile:44
- Draft comment:
Added 'yarn copy:assets:tauri' in the test target: ensure this command is idempotent and is needed to properly copy Tauri assets during tests. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. src-tauri/src/core/mcp.rs:1007
- Draft comment:
Refactored test setup now creates mcp_config.json in the app data folder and ensures parent directories exist. This improves isolation; consider handling potential failures in to_str() instead of unwrapping in production code (though acceptable in tests). - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. src-tauri/src/core/utils/download.rs:695
- Draft comment:
In the SOCKS proxy test, the expected error is now correctly asserted since reqwest does not support SOCKS proxies. The fallback test with an HTTP proxy ensures proper behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, explaining the changes made in the test without suggesting any action or improvement. It doesn't align with the rules for useful comments.
Workflow ID: wflow_kXpp9YGs2wFVODrs
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this 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 cafd874 in 1 minute and 12 seconds. Click for details.
- Reviewed
12
lines of code in1
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. Makefile:45
- Draft comment:
Verify if 'yarn build:icon' is intended in the test target. Typically, asset/icon building is part of the build process rather than testing, so ensure this ordering is deliberate. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_Jj7lbFLsyPTYE1HJ
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this 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 3366d26 in 1 minute and 10 seconds. Click for details.
- Reviewed
13
lines of code in1
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. .github/workflows/jan-linter-and-test.yml:137
- Draft comment:
The conditional selection for Windows runners was removed in favor of a fixed 'windows-latest'. Confirm that this change meets all environments (especially for forked PRs) and that tests behave as expected. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_mOvgV4Q5O4IHb0On
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this 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 dc82fd6 in 1 minute and 46 seconds. Click for details.
- Reviewed
56
lines of code in1
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/utils/extensions/inference_llamacpp_extension/server.rs:999
- Draft comment:
Refactored the call from 'tempfile::tempdir()' to 'tempdir()' for consistency with the imported API. This makes the code cleaner. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:1005
- Draft comment:
Updated the error message for directory creation to better match the purpose (handling uncommon characters) rather than specifically 'non-ASCII'. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:1007
- Draft comment:
Replaced the strict ASCII check with a match block that validates the existence of the generated short path and that it differs from the original. This is a more flexible approach across different system configurations. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_th7krSQfPWCo2z7N
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this 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 b0e7030 in 1 minute and 44 seconds. Click for details.
- Reviewed
12
lines of code in1
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. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:807
- Draft comment:
Removed the #[cfg(windows)] attribute beforeuse tempfile;
. This change unconditionally imports tempfile in tests. Confirm that this is intended so tests can run on all platforms. If tempfile is only needed for Windows-related tests, consider conditionally importing it to avoid unused warnings on other platforms. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_oSYINezVwhRv644Z
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This reverts commit b0e7030.
There was a problem hiding this 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 bdec0af in 1 minute and 20 seconds. Click for details.
- Reviewed
13
lines of code in1
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. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:998
- Draft comment:
Explicitly calling tempfile::tempdir() clarifies the source. Consider importing it at the top if used frequently. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_icbTcXp62Co2XbAC
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this 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 9285714 in 2 minutes and 36 seconds. Click for details.
- Reviewed
60
lines of code in2
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/fs.rs:182
- Draft comment:
Test cleanup: replacing .unwrap() with 'let _ =' ignores errors from fs::remove_dir_all. Consider logging or handling errors to ease debugging cleanup failures. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. src-tauri/src/core/fs.rs:242
- Draft comment:
Similarly, in this test the error from directory removal is ignored. If cleanup issues matter, consider at least logging the error. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. src-tauri/src/core/threads.rs:565
- Draft comment:
Test cleanup in threads: ignoring errors on fs::remove_dir_all might hide cleanup failures. If failing cleanup is significant, consider logging the error. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_oGMX3eyMD23nOEAY
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this 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 0b7477e in 2 minutes and 7 seconds. Click for details.
- Reviewed
13
lines of code in1
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. src-tauri/Cargo.toml:62
- Draft comment:
Ensure the 'nix' dependency is still needed after removing GPU detection code. If it's no longer used, consider removing it to avoid unused dependencies. - 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 violates several rules. It starts with "Ensure..." which is a red flag. It's speculative ("if it's no longer used"). It asks the author to verify something rather than making a concrete assertion. The diff actually shows the dependency being intentionally reorganized, not removed or changed. The dependency could actually be unused - just because it was moved doesn't mean it's needed. Maybe the comment has merit? Even if the dependency is unused, this comment's phrasing makes it non-actionable. If there was strong evidence the dependency was unused, a direct "This dependency appears unused and should be removed" would be better. The comment should be deleted as it's speculative, asks for verification rather than making a concrete assertion, and the diff shows intentional dependency organization rather than removal.
Workflow ID: wflow_HJL6zIBzOvoHJlq4
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this 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 fc7d8a7 in 1 minute and 10 seconds. Click for details.
- Reviewed
13
lines of code in1
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. src-tauri/src/core/utils/mod.rs:50
- Draft comment:
Switching to create_dir_all ensures any missing intermediate directories in thread_dir are created. This aligns with ensure_data_dirs and avoids failures for nested thread IDs. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, explaining the reason for usingcreate_dir_all
. It doesn't provide a suggestion or ask for confirmation about the code change. According to the rules, purely informative comments should be removed.
Workflow ID: wflow_aLbIuKKxIf3VfCfd
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this 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 157fd72 in 2 minutes and 6 seconds. Click for details.
- Reviewed
74
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
15
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/Cargo.toml:16
- Draft comment:
Updated 'tauri-build' to 2.3.1. Verify compatibility and review any breaking changes compared to 2.0.2. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. src-tauri/Cargo.toml:22
- Draft comment:
Upgraded 'tauri' from 2.5.0 to 2.7.0. Ensure that no deprecated APIs or behavioral changes affect the app. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. src-tauri/Cargo.toml:25
- Draft comment:
Updated 'tauri-plugin-log' from 2.0.0-rc to 2.6.0. Confirm that logging configuration and usage align with the new version. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. src-tauri/Cargo.toml:26
- Draft comment:
Upgraded 'tauri-plugin-shell' to 2.3.0; double-check shell command behaviors remain consistent. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. src-tauri/Cargo.toml:27
- Draft comment:
Updated 'tauri-plugin-os' to 2.3.0. Verify that OS-specific functionalities work as expected. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. src-tauri/Cargo.toml:28
- Draft comment:
Bumped 'tauri-plugin-opener' to 2.4.0. Confirm there are no API changes affecting its usage. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
7. src-tauri/Cargo.toml:29
- Draft comment:
Updated 'flate2' from 1.0 to 1.1. Ensure that compression/decompression behavior remains compatible. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
8. src-tauri/Cargo.toml:44
- Draft comment:
Major version jump for 'uuid' from 1.7 to 1.17. Review any potential API changes or migration steps needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is about a major version change in a dependency, which is related to dependencies. The rules specify not to comment on pure dependency changes or library versions that are not recognized. Therefore, this comment should be removed.
9. src-tauri/Cargo.toml:47
- Draft comment:
Minor update of 'tokio-util' from 0.7.14 to 0.7.16. Verify that async functionality relying on it is unaffected. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
10. src-tauri/Cargo.toml:49
- Draft comment:
Updated 'tauri-plugin-dialog' to 2.3.2. Check that dialog behavior and any related configurations remain consistent. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
11. src-tauri/Cargo.toml:60
- Draft comment:
Bumped 'libloading' from 0.8.7 to 0.8.8. This minor update should be safe but verify if any dynamic loading behavior is affected. - Reason this comment was not posted:
Confidence changes required:10%
<= threshold50%
None
12. src-tauri/Cargo.toml:63
- Draft comment:
Changed platform condition from 'cfg(not(windows))' to 'cfg(unix)'. Confirm that this change targets all intended non-Windows environments. - 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 their intention regarding a change in platform condition. This violates the rule against asking the author to confirm their intention or to ensure the behavior is intended. The comment does not provide a specific suggestion or point out a clear issue with the code.
13. src-tauri/Cargo.toml:67
- Draft comment:
Updated 'libc' to 0.2.174. This minor bump is likely safe but worth a quick check on platform-specific builds. - Reason this comment was not posted:
Confidence changes required:10%
<= threshold50%
None
14. src-tauri/Cargo.toml:72
- Draft comment:
Bumped 'once_cell' from 1.18 to 1.21. Verify that any usage of once_cell remains compatible with the updated version. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
15. src-tauri/Cargo.toml:73
- Draft comment:
Upgraded 'tauri-plugin-single-instance' to 2.3.2. Ensure the deep-link feature and singleton guard work as expected. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_VCQ5zZf5njkRDk0a
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This reverts commit 3245b63.
There was a problem hiding this 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 3245b63 in 2 minutes and 0 seconds. Click for details.
- Reviewed
130
lines of code in2
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. src-tauri/Cargo.toml:75
- Draft comment:
Consider removing the commented-out dev-dependency 'tempfile' entirely if it's no longer used instead of leaving it commented out. - 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 reasonable code quality suggestion - commented out code is generally considered bad practice and should either be restored or removed entirely. The comment is clear and actionable. However, it's a very minor issue and doesn't affect functionality. We need to consider if this rises to the level of importance that warrants a PR comment. The comment is about code style rather than functionality. Maybe the author has a reason for keeping it commented out as a reminder for future development. While there might be reasons to keep commented code, it's generally not best practice in version controlled codebases since the history is preserved in git. The suggestion to remove it entirely is valid. While this is a valid suggestion, it's an extremely minor code quality issue that doesn't warrant interrupting the PR review process. The comment should be removed.
2. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:997
- Draft comment:
The test 'test_path_with_uncommon_dir_names' and related tempfile imports are commented out. If the underlying hardware API is deprecated, remove this dead code to keep the codebase clean. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_v7kipX1v3RxeZ528
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this 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 0e4a0e8 in 2 minutes and 31 seconds. Click for details.
- Reviewed
74
lines of code in2
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. src-tauri/Cargo.toml:69
- Draft comment:
Removal of the Windows dev-dependency on 'tempfile' appears safe in this refactor since the related test is updated. Ensure that any tests previously relying on 'tempfile' now use an alternative approach. - 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. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:996
- Draft comment:
In the Windows branch of test 'test_path_with_uncommon_dir_names', a hardcoded path (C:\Windows32...) is used without creating the directory. This may cause the test to fail if the path doesn’t exist. Consider using a temporary directory or explicitly creating the directory. - 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% Looking at the code, the get_short_path() function only needs to attempt to convert a path string - it doesn't actually need the path to exist on disk. The function calls Windows API GetShortPathNameW() which can generate a short path name for hypothetical paths. The test is just verifying the string conversion behavior, not file system operations. I could be wrong about the Windows API behavior - maybe GetShortPathNameW() does require the path to exist. I should double check the Windows API documentation. Looking at the Windows API docs, GetShortPathNameW() can generate short paths for non-existent paths. The existence of the directory is not required for this test's purpose of verifying path string conversion. The comment should be deleted. The test is correctly written to verify the path string conversion behavior, and does not require the directory to actually exist.
Workflow ID: wflow_85PLJt4hR9nos86i
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Describe Your Changes
This PR addresses two key improvements:
Changes
Important
Refactor to remove unused GPU detection code and improve Hub loading state, with updates to workflows and build configurations.
hardware.rs
and related files, as GPU detection is now handled byllama.cpp
.index.tsx
to display available data during loading instead of waiting for new data.jan-linter-and-test.yml
to run onwindows-latest
fortest-on-windows-pr
job.template-tauri-build-linux-x64.yml
to remove Vulkan library from Debian package files.download-lib.mjs
script and related Makefile entries.Cargo.toml
.useHardware.test.ts
and other test files.system-monitor.tsx
to fetch devices usinguseLlamacppDevices
.This description was created by
for 0e4a0e8. You can customize this summary. It will automatically update as commits are pushed.