fix: replace broken plugin-http with custom Rust HTTP command for all API requests#969
Open
Me-Priyank wants to merge 5 commits intoCircuitVerse:mainfrom
Open
fix: replace broken plugin-http with custom Rust HTTP command for all API requests#969Me-Priyank wants to merge 5 commits intoCircuitVerse:mainfrom
Me-Priyank wants to merge 5 commits intoCircuitVerse:mainfrom
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
WalkthroughThis PR adds Tauri-side HTTP support and a unified API layer: upgrades Tauri JS and Rust deps, adds Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/simulator/src/data/save.js (1)
434-462:⚠️ Potential issue | 🟡 MinorProject creation: no error handling for non-ok responses.
The
.then()block only handlesresponse.okand silently ignores non-ok responses. The update path (Line 501) properly shows an error message for non-ok responses. This should be consistent:Suggested addition
.then((response) => { if (response.ok) { showMessage( `We have Created a new project: ${projectName} in our servers.` ) - let loadingIcon = document.querySelector('.loadingIcon') loadingIcon.style.transition = 'opacity 0.2s'; loadingIcon.style.opacity = '0'; - localStorage.removeItem('recover') const responseJson = response.json() responseJson.then((data) => { UpdateProjectDetail(data) }) + } else { + showMessage( + "There was an error, we couldn't create the project" + ) + let loadingIcon = document.querySelector('.loadingIcon') + loadingIcon.style.transition = 'opacity 0.2s'; + loadingIcon.style.opacity = '0'; } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/src/data/save.js` around lines 434 - 462, The POST to apiFetch currently only acts when response.ok and silently ignores other statuses; update the promise chain in the apiFetch POST call so non-ok responses are handled: after receiving response, if !response.ok parse response.json() (or response.text() as fallback) and call showMessage with a clear error (and hide/reset the loadingIcon and not call UpdateProjectDetail), and only call UpdateProjectDetail when response.ok; reference the existing apiFetch call, the response variable, UpdateProjectDetail, showMessage, and the loadingIcon handling so the error branch mirrors the update path's error reporting.
🧹 Nitpick comments (10)
src/simulator/src/data/save.js (1)
376-378: DoublegetAuthToken()invocation.Same pattern flagged in
api.tsreview — consider using theauthHeaders()helper:Suggested change
const headers = { 'Content-Type': 'application/json', - ...(getAuthToken() ? { Authorization: `Token ${getAuthToken()}` } : {}), + ...authHeaders(), }This requires adding
authHeadersto the import on Line 19.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/src/data/save.js` around lines 376 - 378, The headers object in save.js currently calls getAuthToken() twice (const headers = { 'Content-Type': 'application/json', ...(getAuthToken() ? { Authorization: `Token ${getAuthToken()}` } : {}), }) — replace this with the authHeaders() helper to avoid duplicate calls: import authHeaders at the top (add to the existing imports where getAuthToken is) and construct headers by merging Content-Type with authHeaders() (e.g., { 'Content-Type': 'application/json', ...authHeaders() }) so Authorization is only resolved once via the authHeaders helper.src/utils/api.ts (1)
37-40:authHeaders()helper is defined but unused across the codebase.Every caller manually duplicates the pattern:
...(getAuthToken() ? { Authorization: `Token ${getAuthToken()}` } : {})This calls
getAuthToken()twice per usage. TheauthHeaders()function already encapsulates this logic. Consider using it in callers (e.g.,save.js,simulatorHandler.vue,UpdateProjectDetail.vue) to reduce duplication and the double-call overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/api.ts` around lines 37 - 40, The codebase duplicates the auth header construction and calls getAuthToken() twice; replace those manual patterns with the existing helper authHeaders() to avoid duplication and double token retrieval. Locate callers in save.js, simulatorHandler.vue, UpdateProjectDetail.vue (and any similar spots) that use ...(getAuthToken() ? { Authorization: `Token ${getAuthToken()}` } : {}) and swap them to spread authHeaders() instead, removing the extra getAuthToken() calls and relying on the authHeaders() function for a single-token retrieval and consistent header format.src/pages/simulatorHandler.vue (2)
36-61:checkEditAccessmixes.then()chaining withasyncfunction.The function is declared
asyncbut uses.then()instead ofawait. This means errors from the.then()chain won't be caught by any surrounding try/catch, and the returned promise resolves before the.then()callbacks complete. Consider usingawaitfor consistency withgetLoginDatabelow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/simulatorHandler.vue` around lines 36 - 61, The checkEditAccess block uses .then() chaining inside an async context; replace it with await and a try/catch so errors are propagated and handled consistently. Await the apiFetch call, use const res = await apiFetch(...), then if (res.ok) await res.json() and call authStore.setUserInfo(...), set (window as any).isUserLoggedIn = true and isLoading.value = false; handle res.status === 401/403/404 branches similarly (set hasAccess.value/isLoading.value or redirect) and ensure isLoading.value is cleared in all paths; wrap the whole flow in try/catch to log or handle unexpected errors (and set isLoading.value = false) and continue to use getAuthToken() and window.logixProjectId as in the original.
14-17: Remove unusedgetTokenfunction.The exported
getTokenfunction (lines 14-17) is not used anywhere—neither within this file nor imported by any other file. The codebase usesgetAuthTokenfrom#/utils/apiinstead. Remove it to reduce dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/simulatorHandler.vue` around lines 14 - 17, Remove the dead exported function getToken from this file: delete the entire getToken(name: string) { ... } declaration and its export so it is no longer part of the module surface; ensure there are no remaining references to getToken in this file after removal and rely on the existing getAuthToken from `#/utils/api` where needed.src-tauri/capabilities/default.json (1)
14-24: Overly broad URL allowlist if the http plugin is still in use.The previous config restricted
http:allow-fetchto specific API endpoints (login, signup). Now it allowshttps://circuitverse.org/**(all paths) andhttps://api.imgur.com/**. If the http plugin's fetch is still reachable from the frontend, this widens the attack surface — any JS running in the webview could fetch arbitrary paths on these hosts.If the custom Rust
http_requestcommand has fully replaced@tauri-apps/plugin-httpusage, these capability entries (includinghttp:defaulton Line 11) are dead config and should be removed for clarity and defense-in-depth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/capabilities/default.json` around lines 14 - 24, The allowlist in the capabilities JSON currently includes broad entries ("https://circuitverse.org/**", "https://api.imgur.com/**") and enables "http:default", which widens the attack surface if the `@tauri-apps/plugin-http` fetch is still reachable; remove the dead HTTP capability entries (remove "http:default" and the wide "allow" URL patterns) or narrow them to the exact endpoints previously used (e.g., specific login/signup API paths) and/or delete the entire HTTP section if the custom Rust command `http_request` has fully replaced plugin-http so the frontend cannot call plugin fetches.src/simulator/src/Verilog2CV.js (1)
122-139: Redundant DOM queries — use the already-fetched reference.Each guard fetches the element, checks it, then fetches it again. Use the variable you already have.
♻️ Example fix (apply same pattern to all similar blocks)
const code_window = document.getElementById('code-window') - if (code_window) - document.getElementById('code-window').style.display = 'block' + if (code_window) code_window.style.display = 'block' const elementPanel = document.querySelector('.elementPanel') - if (elementPanel) - document.querySelector('.elementPanel').style.display = 'none' + if (elementPanel) elementPanel.style.display = 'none'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/src/Verilog2CV.js` around lines 122 - 139, The code performs redundant DOM queries by checking variables you just assigned (e.g., elementPanel, timingDiagramPanel, quickBtn, verilogEditorPanel, code_window) but then calling document.querySelector/getElementById again inside the if-block; update each block to use the already-fetched variable instead of re-querying the DOM — for example, replace subsequent document.querySelector('.elementPanel').style.display = 'none' with elementPanel.style.display = 'none', and do the same for timingDiagramPanel, quickBtn, verilogEditorPanel and the code_window case so each if uses the local reference.src-tauri/src/lib.rs (2)
40-57: Response body read astext()is appropriate for JSON APIs but will silently corrupt binary responses.This is fine for the current use case (JSON API endpoints). Just be aware that if
apiFetchis ever used for binary downloads (e.g., images), the body will be lossy. A comment noting this limitation would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/lib.rs` around lines 40 - 57, The response body is currently read with response.text() (resp_body) which will corrupt binary payloads; update the apiFetch handling by either (a) adding a clear comment above the resp_body/read block noting that response.text() is only suitable for text/JSON APIs and will be lossy for binary downloads, or (b) implement conditional handling in the function that checks response.headers().get("content-type") and uses response.bytes() for non-text/binary types and response.text() for JSON/text; reference the resp_body variable and the response.text() call so the maintainer can find and update the code.
20-20: A newreqwest::Clientis created on every invocation — no connection pooling or timeout.
reqwest::Client::new()per call discards connection pools and TLS sessions. More importantly, there is no timeout configured, so a misbehaving server can cause the request to hang indefinitely — the very issue this PR aims to fix.Consider using a shared client with a timeout:
♻️ Proposed fix using a static client with timeout
+use std::sync::LazyLock; +use std::time::Duration; + +static HTTP_CLIENT: LazyLock<reqwest::Client> = LazyLock::new(|| { + reqwest::Client::builder() + .timeout(Duration::from_secs(30)) + .build() + .expect("failed to build HTTP client") +}); + #[tauri::command] async fn http_request( method: String, url: String, headers: Vec<(String, String)>, body: Option<String>, ) -> Result<HttpResponse, String> { - let client = reqwest::Client::new(); - - let mut req = match method.to_uppercase().as_str() { - "GET" => client.get(&url), + let mut req = match method.to_uppercase().as_str() { + "GET" => HTTP_CLIENT.get(&url), - "POST" => client.post(&url), + "POST" => HTTP_CLIENT.post(&url), - "PATCH" => client.patch(&url), + "PATCH" => HTTP_CLIENT.patch(&url), - "PUT" => client.put(&url), + "PUT" => HTTP_CLIENT.put(&url), - "DELETE" => client.delete(&url), + "DELETE" => HTTP_CLIENT.delete(&url), - "HEAD" => client.head(&url), + "HEAD" => HTTP_CLIENT.head(&url), other => return Err(format!("Unsupported HTTP method: {}", other)), };Note:
LazyLockis stable since Rust 1.80. Your MSRV is 1.77.2, so useonce_cell::sync::Lazyinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/lib.rs` at line 20, Replace the per-call reqwest::Client instantiation with a shared, static client that has a sensible timeout; specifically, stop calling reqwest::Client::new() inside the handler and instead create a once_cell::sync::Lazy static (e.g., STATIC_CLIENT) that builds reqwest::Client::builder().timeout(Duration::from_secs(...)).build().expect(...) so connection pooling and TLS sessions are reused and requests cannot hang indefinitely; update uses of the local client variable to reference STATIC_CLIENT and add the necessary use imports (once_cell::sync::Lazy and std::time::Duration).src/simulator/src/setup.js (1)
107-107:getAuthToken()is called twice — cache the result.This is a minor inefficiency repeated across multiple files in this PR (here,
ReportIssue.vue,save.js). Store the token in a local variable to avoid redundantlocalStoragereads.♻️ Suggested fix
+ const token = getAuthToken() const response = await apiFetch( `/api/v1/projects/${projectId}/circuit_data`, { method: 'GET', headers: { Accept: 'application/json', - ...(getAuthToken() ? { Authorization: `Token ${getAuthToken()}` } : {}), + ...(token ? { Authorization: `Token ${token}` } : {}), }, } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/src/setup.js` at line 107, getAuthToken() is being invoked twice when building headers; cache its return value in a local variable and reuse it. Replace repeated calls like ...(getAuthToken() ? { Authorization: `Token ${getAuthToken()}` } : {}) with a two-step pattern: call const token = getAuthToken(); then use ...(token ? { Authorization: `Token ${token}` } : {}) so you avoid redundant localStorage reads; apply the same change wherever getAuthToken() is duplicated (e.g., ReportIssue.vue, save.js, setup.js).src-tauri/Cargo.toml (1)
28-28: Remove unusedtauri-plugin-httpdependency.
tauri-plugin-http(line 25 in Cargo.toml) and its initialization inlib.rs(line 70) are no longer needed. The frontend has no active imports from@tauri-apps/plugin-http—only a commented-out reference inUserMenu.vue—and the newreqwest-based custom command fully supersedes it. Removing the plugin would reduce binary size and attack surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/Cargo.toml` at line 28, Remove the unused tauri-plugin-http dependency and its initialization: delete the tauri-plugin-http entry from Cargo.toml and remove the plugin initialization call in src-tauri/src/lib.rs (the code around the previous plugin init at line ~70), and also ensure there are no active imports in the frontend (only a commented reference in UserMenu.vue) so the new reqwest-based custom command remains the sole implementation; after removal, run a cargo build to verify no references to tauri_plugin_http remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 25: Remove the unused Tauri HTTP plugin and related permissions: delete
the "@tauri-apps/plugin-http" entry from package.json, remove the
tauri-plugin-http = "2" dependency from src-tauri/Cargo.toml, and remove the
.plugin(tauri_plugin_http::init()) call in src-tauri/src/lib.rs so the plugin is
no longer initialized; also remove the "http:default" and "http:allow-fetch"
entries from src-tauri/capabilities/default.json since your custom http_request
implementation uses reqwest directly.
In `@src/utils/api.ts`:
- Around line 53-57: apiFetch currently always calls apiUrl(input) which
prepends the base URL and breaks when callers pass a full URL; modify apiFetch
to detect if input is already an absolute URL (e.g., starts with "http://" or
"https://") and, in that case, use input as-is, otherwise call apiUrl(input);
update references to apiFetch and apiUrl only—no callers need changing.
- Around line 76-77: The current assignment const body: string | null =
init?.body != null ? String(init.body) : null silently corrupts non-string
RequestInit.body types; update the implementation in the function that declares
body so it either (A) narrows the input type to accept only string | null for
init.body (change the function signature/types accordingly), or (B) add a
runtime guard that checks the runtime type of init.body and only JSON.stringify
or String() when appropriate (e.g., primitive/string or plain object), otherwise
pass through FormData/Blob/ArrayBuffer/URLSearchParams/ReadableStream unchanged
and ensure the fetch call uses that preserved value; reference the variable body
and the init parameter when making this change.
---
Outside diff comments:
In `@src/simulator/src/data/save.js`:
- Around line 434-462: The POST to apiFetch currently only acts when response.ok
and silently ignores other statuses; update the promise chain in the apiFetch
POST call so non-ok responses are handled: after receiving response, if
!response.ok parse response.json() (or response.text() as fallback) and call
showMessage with a clear error (and hide/reset the loadingIcon and not call
UpdateProjectDetail), and only call UpdateProjectDetail when response.ok;
reference the existing apiFetch call, the response variable,
UpdateProjectDetail, showMessage, and the loadingIcon handling so the error
branch mirrors the update path's error reporting.
---
Duplicate comments:
In `@src-tauri/src/lib.rs`:
- Line 70: The tauri_plugin_http::init() plugin is still being registered but
may be redundant since the new http_request command handles frontend HTTP
traffic; remove the call to tauri_plugin_http::init() from the Tauri builder
(search for the .plugin(tauri_plugin_http::init()) invocation) unless you have
other code depending on that plugin, or alternatively gate its registration
behind a feature flag/config check; ensure the http_request command
implementation (the command handler named http_request) fully covers any
functionality previously provided by the plugin before deletion.
In `@src/components/ReportIssue/ReportIssue.vue`:
- Line 227: Duplicate calls to getAuthToken() are being made when building the
headers in ReportIssue.vue; capture the token once into a local variable (e.g.,
const token = getAuthToken()) and reuse that variable when conditionally adding
the Authorization header and anywhere else that currently calls getAuthToken(),
so replace the inline ternary ...( getAuthToken() ? { Authorization: `Token
${getAuthToken()}` } : {} ) with a single-token-based conditional using the
local token variable.
---
Nitpick comments:
In `@src-tauri/capabilities/default.json`:
- Around line 14-24: The allowlist in the capabilities JSON currently includes
broad entries ("https://circuitverse.org/**", "https://api.imgur.com/**") and
enables "http:default", which widens the attack surface if the
`@tauri-apps/plugin-http` fetch is still reachable; remove the dead HTTP
capability entries (remove "http:default" and the wide "allow" URL patterns) or
narrow them to the exact endpoints previously used (e.g., specific login/signup
API paths) and/or delete the entire HTTP section if the custom Rust command
`http_request` has fully replaced plugin-http so the frontend cannot call plugin
fetches.
In `@src-tauri/Cargo.toml`:
- Line 28: Remove the unused tauri-plugin-http dependency and its
initialization: delete the tauri-plugin-http entry from Cargo.toml and remove
the plugin initialization call in src-tauri/src/lib.rs (the code around the
previous plugin init at line ~70), and also ensure there are no active imports
in the frontend (only a commented reference in UserMenu.vue) so the new
reqwest-based custom command remains the sole implementation; after removal, run
a cargo build to verify no references to tauri_plugin_http remain.
In `@src-tauri/src/lib.rs`:
- Around line 40-57: The response body is currently read with response.text()
(resp_body) which will corrupt binary payloads; update the apiFetch handling by
either (a) adding a clear comment above the resp_body/read block noting that
response.text() is only suitable for text/JSON APIs and will be lossy for binary
downloads, or (b) implement conditional handling in the function that checks
response.headers().get("content-type") and uses response.bytes() for
non-text/binary types and response.text() for JSON/text; reference the resp_body
variable and the response.text() call so the maintainer can find and update the
code.
- Line 20: Replace the per-call reqwest::Client instantiation with a shared,
static client that has a sensible timeout; specifically, stop calling
reqwest::Client::new() inside the handler and instead create a
once_cell::sync::Lazy static (e.g., STATIC_CLIENT) that builds
reqwest::Client::builder().timeout(Duration::from_secs(...)).build().expect(...)
so connection pooling and TLS sessions are reused and requests cannot hang
indefinitely; update uses of the local client variable to reference
STATIC_CLIENT and add the necessary use imports (once_cell::sync::Lazy and
std::time::Duration).
In `@src/pages/simulatorHandler.vue`:
- Around line 36-61: The checkEditAccess block uses .then() chaining inside an
async context; replace it with await and a try/catch so errors are propagated
and handled consistently. Await the apiFetch call, use const res = await
apiFetch(...), then if (res.ok) await res.json() and call
authStore.setUserInfo(...), set (window as any).isUserLoggedIn = true and
isLoading.value = false; handle res.status === 401/403/404 branches similarly
(set hasAccess.value/isLoading.value or redirect) and ensure isLoading.value is
cleared in all paths; wrap the whole flow in try/catch to log or handle
unexpected errors (and set isLoading.value = false) and continue to use
getAuthToken() and window.logixProjectId as in the original.
- Around line 14-17: Remove the dead exported function getToken from this file:
delete the entire getToken(name: string) { ... } declaration and its export so
it is no longer part of the module surface; ensure there are no remaining
references to getToken in this file after removal and rely on the existing
getAuthToken from `#/utils/api` where needed.
In `@src/simulator/src/data/save.js`:
- Around line 376-378: The headers object in save.js currently calls
getAuthToken() twice (const headers = { 'Content-Type': 'application/json',
...(getAuthToken() ? { Authorization: `Token ${getAuthToken()}` } : {}), }) —
replace this with the authHeaders() helper to avoid duplicate calls: import
authHeaders at the top (add to the existing imports where getAuthToken is) and
construct headers by merging Content-Type with authHeaders() (e.g., {
'Content-Type': 'application/json', ...authHeaders() }) so Authorization is only
resolved once via the authHeaders helper.
In `@src/simulator/src/setup.js`:
- Line 107: getAuthToken() is being invoked twice when building headers; cache
its return value in a local variable and reuse it. Replace repeated calls like
...(getAuthToken() ? { Authorization: `Token ${getAuthToken()}` } : {}) with a
two-step pattern: call const token = getAuthToken(); then use ...(token ? {
Authorization: `Token ${token}` } : {}) so you avoid redundant localStorage
reads; apply the same change wherever getAuthToken() is duplicated (e.g.,
ReportIssue.vue, save.js, setup.js).
In `@src/simulator/src/Verilog2CV.js`:
- Around line 122-139: The code performs redundant DOM queries by checking
variables you just assigned (e.g., elementPanel, timingDiagramPanel, quickBtn,
verilogEditorPanel, code_window) but then calling
document.querySelector/getElementById again inside the if-block; update each
block to use the already-fetched variable instead of re-querying the DOM — for
example, replace subsequent
document.querySelector('.elementPanel').style.display = 'none' with
elementPanel.style.display = 'none', and do the same for timingDiagramPanel,
quickBtn, verilogEditorPanel and the code_window case so each if uses the local
reference.
In `@src/utils/api.ts`:
- Around line 37-40: The codebase duplicates the auth header construction and
calls getAuthToken() twice; replace those manual patterns with the existing
helper authHeaders() to avoid duplication and double token retrieval. Locate
callers in save.js, simulatorHandler.vue, UpdateProjectDetail.vue (and any
similar spots) that use ...(getAuthToken() ? { Authorization: `Token
${getAuthToken()}` } : {}) and swap them to spread authHeaders() instead,
removing the extra getAuthToken() calls and relying on the authHeaders()
function for a single-token retrieval and consistent header format.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/utils/api.ts (2)
13-17: Hardcoded production URL prevents local/staging development in Tauri.
CV_PRODUCTION_URLis baked tohttps://circuitverse.org. When developing or testing the Tauri app against a local or staging backend, there's no way to override this without editing source. Consider reading from an environment variable (e.g.,import.meta.env.VITE_API_BASE_URL) with a production fallback.Suggested change
-const CV_PRODUCTION_URL = "https://circuitverse.org"; +const CV_PRODUCTION_URL = + import.meta.env.VITE_CV_API_BASE_URL ?? "https://circuitverse.org"; export function getApiBaseUrl(): string { return isTauri() ? CV_PRODUCTION_URL : ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/api.ts` around lines 13 - 17, The code currently hardcodes CV_PRODUCTION_URL and always returns it when isTauri() is true; change getApiBaseUrl to read an environment override (e.g., import.meta.env.VITE_API_BASE_URL) and use that when present, falling back to CV_PRODUCTION_URL otherwise. Update the CV_PRODUCTION_URL constant to remain the production fallback, call isTauri() in getApiBaseUrl as before, and ensure the function returns an empty string for non-Tauri contexts; reference CV_PRODUCTION_URL, getApiBaseUrl, and isTauri() when making the edits.
23-37:getAuthTokenretrieves from two sources with no expiry/validation — consider documenting the priority.
localStorage.cv_tokenis checked first, thendocument.cookiecvt. If both exist with different values, localStorage always wins. This is fine if intentional, but worth a brief inline comment explaining the precedence for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/api.ts` around lines 23 - 37, getAuthToken currently prefers localStorage.getItem("cv_token") over the cookie named "cvt" (checked via document.cookie) without stating that precedence; add a brief inline comment in the getAuthToken function clarifying that localStorage.cv_token takes priority over the cvt cookie (and note that no expiry/validation is performed), and reference the isTauri() branch so readers understand the cookie check is skipped in Tauri environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/api.ts`:
- Line 1: Run the project formatter (e.g., oxfmt --write or the repo-configured
formatter) on this file to fix the CI formatting error; specifically fix the
malformed file-level comment starting with "/**" at the top of the file (ensure
the block comment is properly closed and the file has correct
indentation/trailing newline) and re-run the formatter so the file passes CI.
---
Duplicate comments:
In `@src/utils/api.ts`:
- Around line 55-59: apiFetch currently calls apiUrl(input) which always
prepends getApiBaseUrl(), causing absolute URLs passed into apiFetch to become
malformed; modify apiFetch (or apiUrl) to detect if input is already an absolute
URL (e.g., starts with "http://" or "https://" or new URL parse success) and, in
that case, skip prepending the base and use the input as-is. Update apiFetch to
validate the input string, call apiUrl only for relative paths, and ensure
behavior of apiUrl/getApiBaseUrl remains unchanged for relative inputs.
- Around line 78-79: The current conversion const body: string | null =
init?.body != null ? String(init.body) : null silently corrupts non-string
BodyInit types; update the logic to preserve valid BodyInit values or narrow the
accepted type: check init?.body's runtime type (typeof init.body === 'string')
and use it directly, else if init.body is an instance of FormData, Blob,
ArrayBuffer, URLSearchParams, ReadableStream, or ArrayBufferView then assign it
to body (typed as BodyInit | null) rather than String(...); alternatively,
change the function signature to accept body: BodyInit | null and remove the
String() coercion so symbols like init?.body and the const body variable keep
correct types.
---
Nitpick comments:
In `@src/utils/api.ts`:
- Around line 13-17: The code currently hardcodes CV_PRODUCTION_URL and always
returns it when isTauri() is true; change getApiBaseUrl to read an environment
override (e.g., import.meta.env.VITE_API_BASE_URL) and use that when present,
falling back to CV_PRODUCTION_URL otherwise. Update the CV_PRODUCTION_URL
constant to remain the production fallback, call isTauri() in getApiBaseUrl as
before, and ensure the function returns an empty string for non-Tauri contexts;
reference CV_PRODUCTION_URL, getApiBaseUrl, and isTauri() when making the edits.
- Around line 23-37: getAuthToken currently prefers
localStorage.getItem("cv_token") over the cookie named "cvt" (checked via
document.cookie) without stating that precedence; add a brief inline comment in
the getAuthToken function clarifying that localStorage.cv_token takes priority
over the cvt cookie (and note that no expiry/validation is performed), and
reference the isTauri() branch so readers understand the cookie check is skipped
in Tauri environments.
- Format api.ts and project.ts with Prettier (oxfmt-compatible settings) - Upgrade Tauri ecosystem to latest 2.x: tauri 2.10.2, plugin-http 2.5.7, plugin-fs 2.4.5 - Upgrade npm packages to match: @tauri-apps/api ^2.10.1, cli ^2.10.0 - Revert Cargo.toml version pins to open semver to prevent transitive dep conflicts
Contributor
Author
|
@tachyons @Nihal4777 Implemented the whole backend architecture fix, all backend API calls and cross-platform builds now work correctly. CI is also green |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
60-60:⚠️ Potential issue | 🟡 Minor
cross-env ^10.1.0is incompatible with Node 18.
cross-env10.x requires Node ≥ 20. This repo runs Node 18 per CI configuration; upgrade to Node 20+ or downgrade tocross-env ^7.0.3. Based on learnings, "In Node 18 environments (e.g., this repo cv-frontend-vue), pin cross-env to 7.x (e.g., ^7.0.3) because cross-env 10.x requires Node >= 20."🛠️ Proposed fix
- "cross-env": "^10.1.0", + "cross-env": "^7.0.3",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 60, The package.json currently pins "cross-env" to ^10.1.0 which is incompatible with Node 18; update package.json to pin "cross-env" to a 7.x release (e.g., ^7.0.3) or alternatively upgrade the CI and local Node runtime to Node 20+; after changing the "cross-env" entry in package.json, run your package manager (npm/yarn/pnpm) to regenerate the lockfile and verify scripts using cross-env still work, and if you choose Node upgrade, adjust CI configuration to use Node 20+ instead of modifying the dependency.
🧹 Nitpick comments (2)
src/simulator/src/data/project.ts (1)
39-79: Delete the commented-out jQuery block rather than leaving it as dead code.Lines 39–79 contain the old jQuery dialog implementation wrapped in
/* ... */. Keeping large commented-out blocks inflates the file and confuses future readers. The replacement behaviour (simulatorStore.dialogBox.open_project_dialog = true) is already in place above the comment.♻️ Proposed change
simulatorStore.dialogBox.open_project_dialog = true; - /* - $('#openProjectDialog').empty() - const projectList = JSON.parse(localStorage.getItem('projectList')) - let flag = true - for (id in projectList) { - flag = false - $('#openProjectDialog').append( - `<label class="option custom-radio">...` - ) - } - ... - */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/src/data/project.ts` around lines 39 - 79, Remove the large commented-out jQuery block (the /* ... */ section that contains the old $('#openProjectDialog') dialog logic) since the new flow uses simulatorStore.dialogBox.open_project_dialog; delete the dead code to reduce clutter and ensure there are no remaining references to the old handlers (e.g., deleteOfflineProject, Open_offline_btn) elsewhere before committing.src/utils/api.ts (1)
3-9:isTauri()is evaluated twice perapiFetchcall — consider caching.
apiUrl(input)at line 54 already callsgetApiBaseUrl()→isTauri(), and theif (isTauri())guard at line 57 evaluates it a second time. While the overhead is trivial now, caching the result as a module-level constant avoids the double DOM inspection and makes the intent clearer.♻️ Proposed refactor
-export function isTauri(): boolean { - return ( - typeof window !== "undefined" && - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - (!!(window as any).__TAURI_INTERNALS__ || !!(window as any).__TAURI__) - ); -} +// eslint-disable-next-line `@typescript-eslint/no-explicit-any` +export const isTauri: boolean = + typeof window !== "undefined" && + (!!(window as any).__TAURI_INTERNALS__ || !!(window as any).__TAURI__);And update all call sites from
isTauri()toisTauri.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/api.ts` around lines 3 - 9, The isTauri() function is being invoked twice during apiFetch (once via apiUrl → getApiBaseUrl and again in the apiFetch guard); make a module-level cached boolean (e.g., const isTauri = isTauri(); rename the original function if needed) or compute a single cached value at module initialization and replace all runtime calls to isTauri() in apiUrl, getApiBaseUrl, and apiFetch with that cached identifier to avoid repeated window inspection and clarify intent; update all call sites to reference the cached isTauri symbol instead of calling the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@package.json`:
- Line 60: The package.json currently pins "cross-env" to ^10.1.0 which is
incompatible with Node 18; update package.json to pin "cross-env" to a 7.x
release (e.g., ^7.0.3) or alternatively upgrade the CI and local Node runtime to
Node 20+; after changing the "cross-env" entry in package.json, run your package
manager (npm/yarn/pnpm) to regenerate the lockfile and verify scripts using
cross-env still work, and if you choose Node upgrade, adjust CI configuration to
use Node 20+ instead of modifying the dependency.
---
Duplicate comments:
In `@package.json`:
- Line 25: The `@tauri-apps/plugin-http` JS package is unused because API calls go
through invoke('http_request') via `@tauri-apps/api/core`; remove the dependency
from package.json (delete "@tauri-apps/plugin-http": "^2.5.7"), remove the
corresponding tauri_plugin_http::init() call in your Rust
bootstrap/initialization code, and drop the http:* capability entries from your
Tauri config/manifest to keep permissions and Cargo/Rust code consistent with
the JS dependencies.
In `@src/utils/api.ts`:
- Line 54: The call const url = apiUrl(input) will double-prefix absolute URLs
in Tauri because apiUrl always prepends the base; update apiUrl (or replace this
call) to detect absolute URLs (e.g., check for a scheme like
/^([a-z][a-z0-9+.-]*:)?\/\//i or startsWith('http://')/('https://')) and return
the input unchanged when absolute, otherwise prepend the base; ensure the
behavior is covered where apiUrl is used so callers passing full URLs (Tauri
case) are not re-prefixed.
- Line 73: The current assignment for the variable body uses String(init.body)
which corrupts non-string body types; change the logic that sets body (the const
body variable reading from init) to preserve FormData, Blob, ArrayBuffer,
ArrayBufferView (ArrayBuffer.isView), URLSearchParams, and ReadableStream as-is,
only convert to a string for true string values or JSON.stringify plain objects,
and keep null when init.body is null/undefined; implement this by replacing
String(init.body) with a type-checking conditional (e.g., check typeof init.body
=== 'string', instanceof FormData/Blob/URLSearchParams, ArrayBuffer.isView,
ArrayBuffer, or ReadableStream) and fallback to JSON.stringify for plain
objects.
---
Nitpick comments:
In `@src/simulator/src/data/project.ts`:
- Around line 39-79: Remove the large commented-out jQuery block (the /* ... */
section that contains the old $('#openProjectDialog') dialog logic) since the
new flow uses simulatorStore.dialogBox.open_project_dialog; delete the dead code
to reduce clutter and ensure there are no remaining references to the old
handlers (e.g., deleteOfflineProject, Open_offline_btn) elsewhere before
committing.
In `@src/utils/api.ts`:
- Around line 3-9: The isTauri() function is being invoked twice during apiFetch
(once via apiUrl → getApiBaseUrl and again in the apiFetch guard); make a
module-level cached boolean (e.g., const isTauri = isTauri(); rename the
original function if needed) or compute a single cached value at module
initialization and replace all runtime calls to isTauri() in apiUrl,
getApiBaseUrl, and apiFetch with that cached identifier to avoid repeated window
inspection and clarify intent; update all call sites to reference the cached
isTauri symbol instead of calling the function.
Contributor
Author
|
@tachyons @Nihal4777 ping ... |
Contributor
|
Tested locally, working fine for me. Ready for review @tachyons @ThatDeparted2061 |
Contributor
Author
|
@tachyons PTAL |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/simulator/src/data/save.js (1)
432-460:⚠️ Potential issue | 🟠 MajorEnsure loader cleanup and user feedback on all failure paths.
In the create/update request flows, failed requests can leave the loading indicator visible and provide no user-facing error (especially create flow). This causes a stuck UX on network/API failures.
Robust error/finally handling pattern
apiFetch('/api/v1/projects', { method: 'POST', headers, body: JSON.stringify({ data, image: imageData, name: projectName, }), }) - .then((response) => { - if (response.ok) { + .then(async (response) => { + if (response.ok) { showMessage( `We have Created a new project: ${projectName} in our servers.` ) - - let loadingIcon = document.querySelector('.loadingIcon') - loadingIcon.style.transition = 'opacity 0.2s'; - loadingIcon.style.opacity = '0'; - localStorage.removeItem('recover') - const responseJson = response.json() - responseJson.then((data) => { - UpdateProjectDetail(data) - }) + const data = await response.json() + UpdateProjectDetail(data) + } else { + showMessage("There was an error, we couldn't save to our servers") } }) .catch((error) => { console.error('Error:', error) + showMessage("There was an error, we couldn't save to our servers") + }) + .finally(() => { + const loadingIcon = document.querySelector('.loadingIcon') + loadingIcon.style.transition = 'opacity 0.2s' + loadingIcon.style.opacity = '0' })Also applies to: 499-526
🧹 Nitpick comments (2)
src/simulator/src/data/project.ts (1)
209-211: Avoid silent localhost fallback in desktop redirects.At Line 211, falling back to
http://localhost:4000can create hard-to-diagnose failures in packaged builds. Prefer failing fast (or showing an explicit error) whengetApiBaseUrl()is unavailable.Suggested hardening
- const baseUrl = - window.location.origin !== "null" - ? window.location.origin - : getApiBaseUrl() || "http://localhost:4000"; + const baseUrl = + window.location.origin !== "null" + ? window.location.origin + : getApiBaseUrl(); + if (!baseUrl) { + showError("API base URL is not configured for desktop mode."); + return; + }src/simulator/src/data/save.js (1)
374-377: Resolve auth token once before building headers.At Line 376, calling
getAuthToken()twice is unnecessary and can produce inconsistent values if storage changes mid-execution.Small cleanup
- const headers = { - 'Content-Type': 'application/json', - ...(getAuthToken() ? { Authorization: `Token ${getAuthToken()}` } : {}), - } + const authToken = getAuthToken() + const headers = { + 'Content-Type': 'application/json', + ...(authToken ? { Authorization: `Token ${authToken}` } : {}), + }
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #968
Describe the changes you have made in this PR -
This PR resolves a critical architecture-level failure in the Tauri desktop application where all HTTP API requests (POST/PATCH/PUT) were hanging indefinitely. This was caused by a fundamental version incompatibility in the streaming layer between the core Tauri API and the HTTP plugin.
Core Fix: Decoupled Rust Networking Layer
I have replaced the broken JavaScript-based
@tauri-apps/plugin-httpimplementation with a custom Rust command (http_request) that interfaces directly withreqwest. This change:Specific Functional Fixes:
POST /auth/loginandPOST /auth/signup.localStorage.cv_tokenduring signOut(). Previously, sessions would "resurrect" after an app restart due to stale tokens.POST /simulator/verilogcvendpoint to correctly transmit the Verilog code payload and parse the response, fixing the "infinite spinner" bug.POST /projectsandPATCH /projects/update_circuitto handle large JSON payloads reliably without hanging.Code Quality Improvements:
window.fetch(web) andinvoke('http_request')(Tauri) based on the runtime environment.console.logstatements and standardized error reporting viaconsole.erroronly on failures.Screenshots of the UI changes (If any) -
1. Auth fix
auth-fix.mp4
2. Verilog Request Successful
verilog-fix.mp4
Note: Verilog nodes misalignment will be fixed after this PR merge : 831
3. Project Saving Successfully
project-save-update.mp4
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
1. Root Cause Analysis
I traced the "hanging request" issue to the interaction between
@tauri-apps/api@2.3.0(core) and@tauri-apps/plugin-http@2.4.4(plugin). The plugin attempts to stream the response body using aReadableStreambacked by a TauriChannel. However, theChannel.onmessageimplementation in API v2.3.0 does not correctly signal the "close" event expected by the plugin's stream reader. This causesawait response.json()to wait forever for a stream termination that never arrives.2. The Solution: "Rust-First" Networking
Instead of downgrading libraries or waiting for upstream fixes, I architected a Rust-first networking layer:
Rust Side (src-tauri/src/lib.rs):
I implemented an async command http_request using
reqwest. It acceptsmethod,url,headers, andbody(optional string). It executes the request and returns a HttpResponse struct containing the status, headers, and the full response body as aString.Why String? Passing the full body as a string via
invokeis simple, reliable, and avoids the complexity and overhead of streaming for standard API payloads.TypeScript Side (src/utils/api.ts):
I created a unified apiFetch wrapper.
globalThis.fetch(), preserving standard browser behavior.invoke('http_request', ...)dynamically. It then reconstructs a standard Response object from the result, so consumer code (likeauthStore, save.js) doesn't need to know it's running on Tauri.await import('@tauri-apps/api/core')inside the Tauri-only block to ensure the web build remains lean and free of Tauri dependencies.3. Handling Edge Cases
window.location.origincould be"null"in Tauri. Added a fallback to getApiBaseUrl().Vec<(String, String)>. I added logic in api.ts to normalize Headers,string[][], andRecord<string, string>into this format.unwrap()calls in Rust in favor ofmap_errto ensure the app never panics, even if the network fails.Checklist before requesting a review
Summary by CodeRabbit
New Features
Bug Fixes
Chores