[codex] default desktop chat transport to websocket#121
Open
zouyonghe wants to merge 8 commits intoAstrBotDevs:mainfrom
Open
[codex] default desktop chat transport to websocket#121zouyonghe wants to merge 8 commits intoAstrBotDevs:mainfrom
zouyonghe wants to merge 8 commits intoAstrBotDevs:mainfrom
Conversation
* fix: add backend startup heartbeat liveness probe * fix: tighten startup heartbeat validation * refactor: centralize startup heartbeat metadata * fix: surface heartbeat invalidation sooner * fix: harden startup heartbeat parsing * fix: warn on stop-time heartbeat failures * refactor: simplify startup heartbeat control flow * refactor: flatten readiness heartbeat helpers * refactor: clarify heartbeat helper responsibilities * docs: clarify startup heartbeat path coupling * fix: harden startup heartbeat coordination * fix: make startup heartbeat checks monotonic * fix: clean up heartbeat test and exit handling
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a default chat transport mode setting in the desktop bridge. It adds constants for the storage key and the 'websocket' mode, implements a function to ensure this default is set in local storage if not already present, and integrates this check into the bootstrap process. I have no feedback to provide as there are no review comments.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
ensureDefaultChatTransportMode, consider explicitly checkingstorage.getItem(CHAT_TRANSPORT_MODE_STORAGE_KEY) !== nullinstead of a truthy check so that an intentionally stored empty string or falsy value is still respected as an existing preference.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ensureDefaultChatTransportMode`, consider explicitly checking `storage.getItem(CHAT_TRANSPORT_MODE_STORAGE_KEY) !== null` instead of a truthy check so that an intentionally stored empty string or falsy value is still respected as an existing preference.
## Individual Comments
### Comment 1
<location path="src-tauri/src/bridge_bootstrap.js" line_range="704-707" />
<code_context>
+ try {
+ const storage = window.localStorage;
+ if (!storage) return;
+ if (storage.getItem(CHAT_TRANSPORT_MODE_STORAGE_KEY)) return;
+ storage.setItem(
+ CHAT_TRANSPORT_MODE_STORAGE_KEY,
</code_context>
<issue_to_address>
**suggestion:** Guard should distinguish between `null` and other falsy stored values to avoid unintended overrides.
Because this relies on truthiness, valid stored values like `""` or `"0"` will be treated as missing and overwritten with the default. If you only want to set the default when the key is absent, check explicitly for `null`, e.g. `if (storage.getItem(CHAT_TRANSPORT_MODE_STORAGE_KEY) !== null) return;`.
```suggestion
const storage = window.localStorage;
if (!storage) return;
if (storage.getItem(CHAT_TRANSPORT_MODE_STORAGE_KEY) !== null) return;
storage.setItem(
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
ensureDefaultChatTransportModefunction silently swallows all errors; consider at least logging unexpected exceptions or narrowing the try/catch to make debugging startup/localStorage issues easier. - The
CHAT_TRANSPORT_MODE_STORAGE_KEYandCHAT_TRANSPORT_MODE_WEBSOCKETconstants are hard-coded here; if the ChatUI or upstream code defines equivalent values, it may be safer to centralize or reference them to avoid divergence if the key name or mode string changes later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ensureDefaultChatTransportMode` function silently swallows all errors; consider at least logging unexpected exceptions or narrowing the try/catch to make debugging startup/localStorage issues easier.
- The `CHAT_TRANSPORT_MODE_STORAGE_KEY` and `CHAT_TRANSPORT_MODE_WEBSOCKET` constants are hard-coded here; if the ChatUI or upstream code defines equivalent values, it may be safer to centralize or reference them to avoid divergence if the key name or mode string changes later.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Member
Author
|
Addressed in
Validation used for this update:
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
chat.transportModekey andwebsocketvalue are now hard-coded both indesktop-bridge-expectations.mjsandbridge_bootstrap.js; consider centralizing these constants (or at least deriving one from the other) to avoid subtle drift if the storage contract changes in the future. - In
ensureDefaultChatTransportMode, you might want to defensively check fortypeof window !== 'undefined'before accessingwindow.localStorageso this bootstrap module fails more gracefully if ever imported or executed in a non-browser-like context (e.g., certain tests or tooling).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `chat.transportMode` key and `websocket` value are now hard-coded both in `desktop-bridge-expectations.mjs` and `bridge_bootstrap.js`; consider centralizing these constants (or at least deriving one from the other) to avoid subtle drift if the storage contract changes in the future.
- In `ensureDefaultChatTransportMode`, you might want to defensively check for `typeof window !== 'undefined'` before accessing `window.localStorage` so this bootstrap module fails more gracefully if ever imported or executed in a non-browser-like context (e.g., certain tests or tooling).Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
Addressed in
Validation for this update:
|
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
desktop_bridge_bootstrap_script, theevent_nameparameter is effectively only honored on the first call because of theOnceLock, so either remove the parameter or document/enforce that it must be constant to avoid future misuse where different event names are silently ignored. - The new
desktop-bridge-expectationshints still hard-codelocalStorage["chat.transportMode"]and"websocket"while the actual key/value now come fromdesktop_bridge_chat_transport_contract.json; consider reading the contract for the hint strings as well to avoid divergence if the contract changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `desktop_bridge_bootstrap_script`, the `event_name` parameter is effectively only honored on the first call because of the `OnceLock`, so either remove the parameter or document/enforce that it must be constant to avoid future misuse where different event names are silently ignored.
- The new `desktop-bridge-expectations` hints still hard-code `localStorage["chat.transportMode"]` and `"websocket"` while the actual key/value now come from `desktop_bridge_chat_transport_contract.json`; consider reading the contract for the hint strings as well to avoid divergence if the contract changes.
## Individual Comments
### Comment 1
<location path="src-tauri/src/bridge_bootstrap.js" line_range="713-714" />
<code_context>
} catch {}
};
+ const ensureDefaultChatTransportMode = () => {
+ const storage = window.localStorage;
+ if (!storage) return;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Accessing `window.localStorage` can throw before the try/catch blocks run.
In some environments (e.g., storage disabled), reading `window.localStorage` can itself throw, skipping your existing `try/catch` around `getItem`/`setItem` and potentially aborting bootstrap. Please either wrap the `window.localStorage` access in its own `try/catch` or move it into the existing `try` blocks so failures are handled consistently and only produce a logged warning.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/bridge/desktop.rs" line_range="22-27" />
<code_context>
+ websocket_value: String,
+}
+
+fn desktop_bridge_chat_transport_contract() -> &'static DesktopBridgeChatTransportContract {
+ DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT.get_or_init(|| {
+ serde_json::from_str(DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT_TEMPLATE)
+ .expect("desktop bridge chat transport contract must be valid JSON")
+ })
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The Rust-side contract loading doesn’t enforce the non-empty-string constraint that the JS tooling validates.
Node validates `desktop_bridge_chat_transport_contract.json` to ensure `storageKey` and `websocketValue` are non-empty, but this loader only checks that the JSON parses. If the file is modified or generated outside the Node path, empty keys/values could be injected into the bootstrap script without any Rust-side guard. Please add equivalent non-empty checks here (e.g., `assert!(!storage_key.is_empty() && !websocket_value.is_empty())`) so contracts fail fast when invalid.
```suggestion
fn desktop_bridge_chat_transport_contract() -> &'static DesktopBridgeChatTransportContract {
DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT.get_or_init(|| {
let contract: DesktopBridgeChatTransportContract =
serde_json::from_str(DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT_TEMPLATE)
.expect("desktop bridge chat transport contract must be valid JSON");
assert!(
!contract.storage_key.is_empty(),
"desktop bridge chat transport contract storageKey must be non-empty"
);
assert!(
!contract.websocket_value.is_empty(),
"desktop bridge chat transport contract websocketValue must be non-empty"
);
contract
})
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+713
to
+714
| const ensureDefaultChatTransportMode = () => { | ||
| const storage = window.localStorage; |
There was a problem hiding this comment.
issue (bug_risk): Accessing window.localStorage can throw before the try/catch blocks run.
In some environments (e.g., storage disabled), reading window.localStorage can itself throw, skipping your existing try/catch around getItem/setItem and potentially aborting bootstrap. Please either wrap the window.localStorage access in its own try/catch or move it into the existing try blocks so failures are handled consistently and only produce a logged warning.
Comment on lines
+22
to
+27
| fn desktop_bridge_chat_transport_contract() -> &'static DesktopBridgeChatTransportContract { | ||
| DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT.get_or_init(|| { | ||
| serde_json::from_str(DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT_TEMPLATE) | ||
| .expect("desktop bridge chat transport contract must be valid JSON") | ||
| }) | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): The Rust-side contract loading doesn’t enforce the non-empty-string constraint that the JS tooling validates.
Node validates desktop_bridge_chat_transport_contract.json to ensure storageKey and websocketValue are non-empty, but this loader only checks that the JSON parses. If the file is modified or generated outside the Node path, empty keys/values could be injected into the bootstrap script without any Rust-side guard. Please add equivalent non-empty checks here (e.g., assert!(!storage_key.is_empty() && !websocket_value.is_empty())) so contracts fail fast when invalid.
Suggested change
| fn desktop_bridge_chat_transport_contract() -> &'static DesktopBridgeChatTransportContract { | |
| DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT.get_or_init(|| { | |
| serde_json::from_str(DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT_TEMPLATE) | |
| .expect("desktop bridge chat transport contract must be valid JSON") | |
| }) | |
| } | |
| fn desktop_bridge_chat_transport_contract() -> &'static DesktopBridgeChatTransportContract { | |
| DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT.get_or_init(|| { | |
| let contract: DesktopBridgeChatTransportContract = | |
| serde_json::from_str(DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT_TEMPLATE) | |
| .expect("desktop bridge chat transport contract must be valid JSON"); | |
| assert!( | |
| !contract.storage_key.is_empty(), | |
| "desktop bridge chat transport contract storageKey must be non-empty" | |
| ); | |
| assert!( | |
| !contract.websocket_value.is_empty(), | |
| "desktop bridge chat transport contract websocketValue must be non-empty" | |
| ); | |
| contract | |
| }) | |
| } |
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
1 participant
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.
Summary
This change makes the desktop shell default new chat sessions to the websocket transport instead of SSE.
Issue #120 reported that desktop chat felt much slower than direct OpenAI streaming. The desktop shell loads the packaged AstrBot ChatUI, which currently defaults to SSE unless
chat.transportModeis already present in local storage. On the upstream side, the SSE chat route adds a fixedawait asyncio.sleep(0.05)after each streamed payload, so new desktop installs inherit that slower transport by default even though the same ChatUI already supports websocket.This patch keeps the change scoped to desktop startup. The injected desktop bridge now seeds
localStorage["chat.transportMode"] = "websocket"only when the key is missing, so existing users keep their chosen setting while fresh desktop installs start on the lower-latency websocket path.Validation
node --check src-tauri/src/bridge_bootstrap.jscargo test --manifest-path src-tauri/Cargo.tomlSummary by Sourcery
Default desktop chat sessions to the websocket transport by seeding a shared, contract-driven chat transport preference into local storage during desktop startup.
New Features:
Enhancements:
Tests: