fix: allow cancel/delete while server is connecting#176
Conversation
Normalize stored server URLs back into editable host/port fields so Edit Server no longer re-saves values like ircs://host:6697:6697. Keep the loading overlay non-blocking, clear global connecting state when a connecting server is deleted or disconnected, and avoid sending QUIT before a socket is actually open.\n\nAlso adds focused regressions for URL normalization, safe disconnect during CONNECTING, and deleting a server while the UI is still in a connecting state.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 42 minutes and 38 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces centralized server connection URL helpers ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/store/index.ts (1)
2419-2470:⚠️ Potential issue | 🟡 MinorStale
connectingServerIdon reconnect failure.
reconnectServernow eagerly setsconnectingServerId: serverId(line 2421), but neitherconnect()'s internal catch (lines 1121-1147 only clearsisConnecting) norreconnectServer's own catch (lines 2463-2469) resetsconnectingServerId. After a failed reconnect the store is left withisConnecting: falsebutconnectingServerId: <failed-id>, which is inconsistent and can mislead consumers that gate UI onconnectingServerIdalone.🛠️ Proposed fix — clear global connecting flags in the reconnect catch
} catch (error) { console.error(`Failed to reconnect to server ${serverId}`, error); // Update server state back to disconnected set((state) => ({ + isConnecting: + state.connectingServerId === serverId ? false : state.isConnecting, + connectingServerId: + state.connectingServerId === serverId ? null : state.connectingServerId, servers: state.servers.map((s) => s.id === serverId ? { ...s, connectionState: "disconnected" as const } : s, ), })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/index.ts` around lines 2419 - 2470, reconnectServer eagerly sets connectingServerId but its catch block doesn't clear it, leaving the store inconsistent after a failed reconnect; update the reconnectServer catch to reset the global connecting flags by calling set to set isConnecting: false and connectingServerId: null (or undefined per store convention) and also ensure the target server's connectionState is set to "disconnected" (same pattern used elsewhere), so use the existing set((state) => ({ servers: state.servers.map(...), isConnecting: false, connectingServerId: null })) to restore a consistent state after failure and avoid relying solely on connect()'s internal catch.
🧹 Nitpick comments (4)
src/lib/irc/IRCClient.ts (1)
713-728: Use the standardWebSocket.CONNECTING/WebSocket.OPENconstants for consistency.Logic is correct — QUIT is gated on OPEN, and
close()is safely no-op'd for CLOSING/CLOSED. The rest of this file already usesWebSocket.OPENdirectly (e.g.,sendRawat line 887,startWebSocketPingat line 901), so inliningconst CONNECTING = 0; const OPEN = 1;is inconsistent and slightly obscures intent.♻️ Proposed refactor
disconnect(serverId: string, quitMessage?: string): void { const socket = this.sockets.get(serverId); if (socket) { - const CONNECTING = 0; - const OPEN = 1; - - if (socket.readyState === OPEN) { + if (socket.readyState === WebSocket.OPEN) { const message = quitMessage || "ObsidianIRC - Bringing IRC to the future"; socket.send(`QUIT :${message}`); } - if (socket.readyState === CONNECTING || socket.readyState === OPEN) { + if ( + socket.readyState === WebSocket.CONNECTING || + socket.readyState === WebSocket.OPEN + ) { socket.close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/irc/IRCClient.ts` around lines 713 - 728, Replace the numeric magic constants in disconnect by using the standard WebSocket enums: change the local CONNECTING/OPEN (used to test socket.readyState in disconnect(serverId: string, quitMessage?: string)) to WebSocket.CONNECTING and WebSocket.OPEN so the readyState checks are consistent with other usages (e.g., sendRaw and startWebSocketPing) and make intent clear; ensure WebSocket is referenced as the global/available symbol when updating the readyState comparisons and gates around QUIT/send/close.src/lib/serverConnectionUrl.ts (2)
96-110: Minor: silent empty-string return can mask caller bugs; consider throwing or logging.If
hostInputever resolves to an empty host (e.g. input like"://"or a whitespace-only string that bypasses caller validation),buildServerConnectionUrlreturns""and the caller will pass""straight intoconnect(...). Today both modals validateserverHost.trim()before calling, so this is defensive only, but a silent empty return is easy to misuse from future callers. Either throw a descriptive error or fall back to${scheme}://${hostInput.trim()}:${port}so the failure mode is loud.Also note: passing
options.useWebSocketas thefallbackUseWebSocketargument here is effectively a no-op since onlyhostis destructured from the result — passingfalse(or simplyundefined) would make the intent clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/serverConnectionUrl.ts` around lines 96 - 110, The function building the URL currently returns an empty string silently when host is blank; change buildServerConnectionUrl so that after computing trimmedHost (from getServerConnectionFields(...)) it throws a descriptive Error (e.g. "Invalid server host: empty after trimming") instead of returning "" to make caller failures loud; also stop passing options.useWebSocket as the fallbackUseWebSocket argument to getServerConnectionFields (pass false or undefined) since only host is used, and keep the rest of the logic (scheme computation using options.isTauri and options.useWebSocket and returning `${scheme}://${trimmedHost}:${port}`) unchanged.
28-42: Minor:parseHostWithOptionalPortdoesn't recognize IPv6 or malformed multi-colon inputs.The regex
^([^:/?#\s]+):(\d+)$only matches a singlehost:portform, so:
[::1]:6697→ returns{ host: "[::1]:6697" }(no port extracted).host:6697:6697pasted without a scheme → returns{ host: "host:6697:6697" }.The PR's motivating malformed case (
ircs://host:6697:6697) is handled upstream byparseIrcUrl, so this is unlikely to hit in practice today. But if IPv6 endpoints or scheme-less pasted strings are plausible user input, consider extending the regex to handle a bracketed IPv6 literal and/or stripping only the last:<digits>segment as the port.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/serverConnectionUrl.ts` around lines 28 - 42, parseHostWithOptionalPort currently only handles single host:port forms and fails for bracketed IPv6 or multi-colon inputs; update parseHostWithOptionalPort to first check for a bracketed IPv6 literal (e.g., /^\[(.+)\]:(\d+)$/) and extract host and port accordingly, and otherwise fallback to a safe "take last :digits as port" strategy (split on last ':' and if the tail is all digits treat it as port, else treat the whole trimmed string as host). Reference parseHostWithOptionalPort and note parseIrcUrl handles other malformed scheme cases; ensure the extracted host preserves IPv6 brackets when present and that non-numeric trailing segments do not get misinterpreted as ports.tests/lib/serverConnectionUrl.test.ts (1)
7-63: Recommended: add a regression test for the malformedircs://host:6697:6697input that motivated this PR.The PR description specifically calls out that a stored
ircs://host:6697:6697was being re-saved unchanged. A targeted test asserting thatgetServerConnectionFields(and/orbuildServerConnectionUrl) normalizes such input back to a clean{host, port}/ircs://host:6697would lock in the fix and protect against future changes toparseIrcUrl.🧪 Suggested additional test
it("always builds wss endpoints on web", () => { expect( buildServerConnectionUrl("ircs://irc.h4ks.com:6697", 443, { isTauri: false, useWebSocket: false, }), ).toBe("wss://irc.h4ks.com:443"); }); + + it("normalizes malformed ircs urls with a duplicated port segment", () => { + const fields = getServerConnectionFields( + "ircs://irc.h4ks.com:6697:6697", + "6697", + false, + ); + expect(fields.host).toBe("irc.h4ks.com"); + expect(fields.port).toBe("6697"); + + expect( + buildServerConnectionUrl("ircs://irc.h4ks.com:6697:6697", 6697, { + isTauri: true, + useWebSocket: false, + }), + ).toBe("ircs://irc.h4ks.com:6697"); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lib/serverConnectionUrl.test.ts` around lines 7 - 63, Add a regression test that covers the malformed input "ircs://host:6697:6697": call getServerConnectionFields("ircs://host:6697:6697", defaultPort, false) and assert it returns { host: "host", port: "6697", useWebSocket: false }, and also call buildServerConnectionUrl("ircs://host:6697:6697", 6697, { isTauri: true, useWebSocket: false }) (and/or the web variant) and assert it normalizes to "ircs://host:6697"; this ensures getServerConnectionFields and buildServerConnectionUrl properly strip duplicated ports and produce the cleaned host/port and URL.
🤖 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/components/ui/EditServerModal.tsx`:
- Around line 27-33: The code is intentionally passing a hardcoded false for the
useWebSocket fallback because persisted ServerConfig has no useWebSocket field;
add a one-line clarifying comment next to the initialConnectionFields call (or
above getServerConnectionFields usage) noting that legacy bare hostnames (no
scheme) default to ircs/WSS behavior and that this false fallback will cause
Tauri legacy hosts like "irc.example.com" to show the WSS checkbox unchecked and
be rewritten to ircs:// on save. Reference getServerConnectionFields,
initialConnectionFields, and ServerConfig in the comment so future readers
understand this is an intentional legacy-handling choice.
---
Outside diff comments:
In `@src/store/index.ts`:
- Around line 2419-2470: reconnectServer eagerly sets connectingServerId but its
catch block doesn't clear it, leaving the store inconsistent after a failed
reconnect; update the reconnectServer catch to reset the global connecting flags
by calling set to set isConnecting: false and connectingServerId: null (or
undefined per store convention) and also ensure the target server's
connectionState is set to "disconnected" (same pattern used elsewhere), so use
the existing set((state) => ({ servers: state.servers.map(...), isConnecting:
false, connectingServerId: null })) to restore a consistent state after failure
and avoid relying solely on connect()'s internal catch.
---
Nitpick comments:
In `@src/lib/irc/IRCClient.ts`:
- Around line 713-728: Replace the numeric magic constants in disconnect by
using the standard WebSocket enums: change the local CONNECTING/OPEN (used to
test socket.readyState in disconnect(serverId: string, quitMessage?: string)) to
WebSocket.CONNECTING and WebSocket.OPEN so the readyState checks are consistent
with other usages (e.g., sendRaw and startWebSocketPing) and make intent clear;
ensure WebSocket is referenced as the global/available symbol when updating the
readyState comparisons and gates around QUIT/send/close.
In `@src/lib/serverConnectionUrl.ts`:
- Around line 96-110: The function building the URL currently returns an empty
string silently when host is blank; change buildServerConnectionUrl so that
after computing trimmedHost (from getServerConnectionFields(...)) it throws a
descriptive Error (e.g. "Invalid server host: empty after trimming") instead of
returning "" to make caller failures loud; also stop passing
options.useWebSocket as the fallbackUseWebSocket argument to
getServerConnectionFields (pass false or undefined) since only host is used, and
keep the rest of the logic (scheme computation using options.isTauri and
options.useWebSocket and returning `${scheme}://${trimmedHost}:${port}`)
unchanged.
- Around line 28-42: parseHostWithOptionalPort currently only handles single
host:port forms and fails for bracketed IPv6 or multi-colon inputs; update
parseHostWithOptionalPort to first check for a bracketed IPv6 literal (e.g.,
/^\[(.+)\]:(\d+)$/) and extract host and port accordingly, and otherwise
fallback to a safe "take last :digits as port" strategy (split on last ':' and
if the tail is all digits treat it as port, else treat the whole trimmed string
as host). Reference parseHostWithOptionalPort and note parseIrcUrl handles other
malformed scheme cases; ensure the extracted host preserves IPv6 brackets when
present and that non-numeric trailing segments do not get misinterpreted as
ports.
In `@tests/lib/serverConnectionUrl.test.ts`:
- Around line 7-63: Add a regression test that covers the malformed input
"ircs://host:6697:6697": call getServerConnectionFields("ircs://host:6697:6697",
defaultPort, false) and assert it returns { host: "host", port: "6697",
useWebSocket: false }, and also call
buildServerConnectionUrl("ircs://host:6697:6697", 6697, { isTauri: true,
useWebSocket: false }) (and/or the web variant) and assert it normalizes to
"ircs://host:6697"; this ensures getServerConnectionFields and
buildServerConnectionUrl properly strip duplicated ports and produce the cleaned
host/port and URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdfdf5b1-2258-4a3a-9e96-ff5b87d19020
📒 Files selected for processing (9)
src/components/ui/AddServerModal.tsxsrc/components/ui/EditServerModal.tsxsrc/components/ui/LoadingOverlay.tsxsrc/lib/irc/IRCClient.tssrc/lib/serverConnectionUrl.tssrc/store/index.tstests/lib/ircClient.test.tstests/lib/serverConnectionUrl.test.tstests/store/serverConnectionFlow.test.ts
| const initialConnectionFields = getServerConnectionFields( | ||
| serverConfig?.host || server?.host || "", | ||
| serverConfig?.port?.toString() || | ||
| server?.port?.toString() || | ||
| (isTauri() ? "6697" : "443"), | ||
| false, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether ServerConfig carries any useWebSocket / websocket-related flag that could serve as a better fallback.
rg -nP -C2 '\buseWebSocket\b|\bwebSocket\b|\bwss\b' --type=ts -g '!tests/**' -g '!**/*.test.*'
echo '---'
ast-grep --pattern 'interface ServerConfig {
$$$
}'Repository: ObsidianIRC/ObsidianIRC
Length of output: 15586
🏁 Script executed:
cat -n src/components/ui/EditServerModal.tsx | head -50Repository: ObsidianIRC/ObsidianIRC
Length of output: 2231
🏁 Script executed:
# Find getServerConnectionFields implementation and its signature
rg -n 'getServerConnectionFields|function getServerConnectionFields' --type=tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 1061
🏁 Script executed:
# Check what type serverConfig is in EditServerModal (props interface)
ast-grep --pattern 'interface $_EditServerModalProps {
$$$
}'Repository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
# Also check if there's a type annotation for the serverConfig parameter
rg -B5 -A5 'function EditServerModal' src/components/ui/EditServerModal.tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
cat -n src/lib/serverConnectionUrl.ts | head -100Repository: ObsidianIRC/ObsidianIRC
Length of output: 2927
🏁 Script executed:
# Look at the full getServerConnectionFields function
sed -n '44,90p' src/lib/serverConnectionUrl.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 1233
🏁 Script executed:
# Check if AddServerModal uses a different type for prefillServerDetails
sed -n '1,50p' src/components/ui/AddServerModal.tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 2040
🏁 Script executed:
# Look at the store types to understand the relationship
cat -n src/store/types.ts | head -30Repository: ObsidianIRC/ObsidianIRC
Length of output: 968
Add a clarifying comment for legacy config handling: ServerConfig has no useWebSocket field.
Since ServerConfig (the persisted type) lacks a useWebSocket field, fallbackUseWebSocket cannot reference it. The hardcoded false fallback will affect bare-hostname legacy configs (those without a scheme in the host field): a Tauri user with a legacy stored host like "irc.example.com" will see the WSS checkbox unchecked after opening Edit, and saving will rewrite the URL to ircs://…. Configs saved after the refactor will have schemes and round-trip correctly. Add a one-line comment (e.g., // Legacy bare hostnames default to ircs or similar) to document this intentional behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/EditServerModal.tsx` around lines 27 - 33, The code is
intentionally passing a hardcoded false for the useWebSocket fallback because
persisted ServerConfig has no useWebSocket field; add a one-line clarifying
comment next to the initialConnectionFields call (or above
getServerConnectionFields usage) noting that legacy bare hostnames (no scheme)
default to ircs/WSS behavior and that this false fallback will cause Tauri
legacy hosts like "irc.example.com" to show the WSS checkbox unchecked and be
rewritten to ircs:// on save. Reference getServerConnectionFields,
initialConnectionFields, and ServerConfig in the comment so future readers
understand this is an intentional legacy-handling choice.
1 participant
Summary
This PR fixes the case where a misconfigured server can get the UI stuck in a global connecting/loading state that makes it awkward to edit, disconnect, or delete from the sidebar.
What this changes
host/portfields in Add/Edit Serverircs://host:6697:6697isConnecting/connectingServerIdstate when a connecting or reconnecting server is deleted/disconnectedQUITbefore the socket is actually open when canceling a still-connecting serverWhy this matters
The broken flow is especially painful when a server was saved with the wrong transport/endpoint format: the client can enter a loading state, but the user cannot cleanly cancel from the sidebar because the UI still behaves as if the connection is globally busy.
This PR turns that into a recoverable flow:
Tests
Added focused regressions for:
CONNECTINGSummary by CodeRabbit
New Features
Bug Fixes