feat: add proxy auto-select and proxies select menu! #389
Conversation
WalkthroughThe changes introduce a comprehensive proxy management and auto-selection system. New modules and UI components enable users to add, edit, and remove proxy servers, visualize their latency, and select proxies manually or via automatic selection based on real-time network measurements. Core logic for measuring latency and selecting the optimal proxy is implemented, and the UI is updated to support these features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (ServersList/ProxiesList)
participant ProxyAutoSelect
participant PingProxy
participant Network
User->>UI (ProxiesList): Add/Edit/Remove Proxy
UI (ProxiesList)->>ProxyAutoSelect: Update proxy list state
User->>UI (ServersList): Select "Auto-select" proxy
UI (ServersList)->>ProxyAutoSelect: selectBestProxy(proxies)
ProxyAutoSelect->>PingProxy: pingProxyServer(proxyUrl)
PingProxy->>Network: WebSocket ping/pong
Network-->>PingProxy: Latency/response
PingProxy-->>ProxyAutoSelect: Latency result
ProxyAutoSelect-->>UI (ServersList): Update ping status
ProxyAutoSelect-->>UI (ServersList): Return best proxy
UI (ServersList)-->>User: Show selected/best proxy and latency
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/react/ProxiesList.tsx (2)
32-32: Improve ID generation for better uniqueness.Using
Math.random().toString(36).slice(2)for ID generation could theoretically produce duplicates, though unlikely. Consider using a more robust approach.- id: Math.random().toString(36).slice(2), + id: `${Date.now()}-${Math.random().toString(36).slice(2)}`,Or consider using
crypto.randomUUID()if available:- id: Math.random().toString(36).slice(2), + id: crypto.randomUUID?.() ?? `${Date.now()}-${Math.random().toString(36).slice(2)}`,
21-35: Consider adding URL validation.The
addProxyfunction doesn't validate the entered proxy URL format. Consider adding basic validation to ensure the URL is properly formatted.const addProxy = async () => { const result = await showInputsModal('Add Proxy', { url: { type: 'text', label: 'Proxy URL', placeholder: 'wss://your-proxy.com' } }) if (!result) return + // Basic URL validation + try { + new URL(result.url) + } catch { + // Could show an error notification here + return + } proxiesState.proxies.push({ id: Math.random().toString(36).slice(2), url: result.url }) }src/react/SelectOption.tsx (1)
200-247: LGTM! Well-implemented select component with minor accessibility suggestions.The
SimpleSelectOptioncomponent is well-structured with proper TypeScript typing and clean logic flow. Consider adding accessibility improvements:<div style={{ cursor: 'pointer', color: value ? 'white' : 'gray', userSelect: 'none' }} + role="button" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + // trigger the same logic as onClick + } + }} onClick={() => {This would make the component accessible via keyboard navigation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/core/pingProxy.ts(1 hunks)src/core/proxyAutoSelect.ts(1 hunks)src/index.ts(2 hunks)src/react/ProxiesList.tsx(1 hunks)src/react/Select.tsx(3 hunks)src/react/SelectOption.tsx(1 hunks)src/react/ServersList.tsx(5 hunks)src/react/appStorageProvider.ts(1 hunks)src/reactUi.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.ts`: You may use the global variable `bot` directly in any file under ...
src/**/*.ts: You may use the global variablebotdirectly in any file under thesrc/directory (e.g.,src/mineflayer/playerState.ts).
Insrc/code, you may use the global variableappViewerfromsrc/appViewer.tsdirectly. Do not importappVieweror usewindow.appViewer; use the globalappViewervariable as-is.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/vars-usage.mdc)
List of files the instruction was applied to:
src/react/appStorageProvider.tssrc/core/pingProxy.tssrc/index.tssrc/core/proxyAutoSelect.ts
🧠 Learnings (3)
src/reactUi.tsx (1)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Some other global variables that can be used without window prefixes are listed in src/globals.d.ts.
src/react/ProxiesList.tsx (1)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
src/index.ts (1)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Some other global variables that can be used without window prefixes are listed in src/globals.d.ts.
🧬 Code Graph Analysis (2)
src/react/ProxiesList.tsx (1)
src/react/SelectOption.tsx (1)
useSnapshot(101-198)
src/core/proxyAutoSelect.ts (1)
src/core/pingProxy.ts (1)
pingProxyServer(71-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (14)
src/react/appStorageProvider.ts (1)
18-18: LGTM! Clean interface extension.The addition of the optional
isAutoSelectboolean property properly extends the proxy data structure to support the auto-selection feature.src/reactUi.tsx (2)
69-69: LGTM! Proper component import.The import follows the established pattern for React components in this file.
254-254: LGTM! Proper component integration.The
ProxiesListcomponent is correctly placed inside theRobustPortalalongside other modal components, following the established UI pattern.src/react/Select.tsx (2)
22-22: LGTM! Well-typed optional prop.The
renderOptionprop is properly typed as an optional function that takes anOptionStorageand returns a React node.
51-56: LGTM! Proper implementation with fallback.The implementation correctly uses the custom
renderOptionfunction when provided, with a sensible fallback to the defaultoption.labelrendering.src/index.ts (2)
100-101: LGTM! Clean imports for new functionality.The imports are correctly structured and follow the existing pattern in the file.
310-311: Good refactoring of proxy parsing logic.Moving the proxy parsing inside the conditional block after auto-selection is logical and efficient. The code maintains the same functionality while supporting the new auto-selection feature.
src/core/proxyAutoSelect.ts (2)
5-19: Well-designed state interface with clear typing.The interface clearly defines the proxy ping state structure with appropriate types for status tracking and error handling.
86-91: Clean cancellation implementation.The cancellation logic is properly implemented with appropriate cleanup.
src/react/ServersList.tsx (3)
1-1: Clean import organization supporting new proxy features.All necessary dependencies are properly imported for the enhanced proxy functionality.
Also applies to: 3-3, 5-5, 10-10, 14-14
25-30: Excellent integration of auto-selection logic in getCurrentProxy.The conditional logic properly handles auto-selection mode by returning
undefinedto delegate proxy choice to the connection logic.
169-188: Well-designed proxy selection UI with comprehensive options.The UI provides excellent user experience with auto-select, manual selection, and management options. The state management properly handles the
isAutoSelectflag.src/core/pingProxy.ts (2)
71-97: Robust error handling in pingProxyServer function.The function has good error handling and proper resource cleanup. The abort signal integration is well-implemented.
8-25: Ensure robust WebSocket URL construction and endpoint validation
- Replace manual string-based protocol/path manipulation with the URL API and explicit error handling:
async connect () { return new Promise<void>((resolve, reject) => { - // Convert http(s):// to ws(s):// - let wsUrl = this.serverUrl.replace(/^http/, 'ws') + '/api/vm/net/ping' - if (!wsUrl.startsWith('ws')) { - wsUrl = 'wss://' + wsUrl - } + let wsUrl: string + try { + const url = new URL(this.serverUrl) + url.protocol = url.protocol === 'https:' ? 'wss:' : 'ws:' + url.pathname = '/api/vm/net/ping' + wsUrl = url.toString() + } catch { + return reject(new Error(`Invalid server URL: ${this.serverUrl}`)) + } this.ws = new WebSocket(wsUrl)
- No references to
/api/vm/net/pingwere found elsewhere in the repo. Please verify that all target proxy servers actually expose this endpoint—or adjust the path if necessary.
| // This would be in a separate state file in a real implementation | ||
| export const proxiesState = proxy({ | ||
| proxies: [] as Array<{ id: string, url: string }> | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider integrating with existing proxy storage system.
This component creates its own separate proxiesState instead of integrating with the existing appStorage.proxiesData from src/react/appStorageProvider.ts. This could lead to data synchronization issues between the proxy management UI and the actual proxy selection logic.
Consider using the existing storage system:
-// This would be in a separate state file in a real implementation
-export const proxiesState = proxy({
- proxies: [] as Array<{ id: string, url: string }>
-})
+import { appStorage } from '../appStorageProvider'Then use appStorage.proxiesData throughout the component to ensure data consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // This would be in a separate state file in a real implementation | |
| export const proxiesState = proxy({ | |
| proxies: [] as Array<{ id: string, url: string }> | |
| }) | |
| import { appStorage } from '../appStorageProvider' |
🤖 Prompt for AI Agents
In src/react/ProxiesList.tsx around lines 10 to 13, the code defines a new
proxiesState which duplicates proxy data management. Instead, import and use the
existing appStorage.proxiesData from src/react/appStorageProvider.ts to maintain
a single source of truth. Replace proxiesState references with
appStorage.proxiesData throughout the component to ensure data consistency and
avoid synchronization issues.
| if (appStorage.proxiesData?.isAutoSelect && appStorage.proxiesData.proxies.length > 0) { | ||
| setLoadingScreenStatus('Selecting best proxy...') | ||
| const bestProxy = await selectBestProxy(appStorage.proxiesData.proxies) | ||
| if (bestProxy) { | ||
| connectOptions.proxy = bestProxy | ||
| } else { | ||
| let message = 'Failed to find a working proxy.' | ||
| if (navigator.onLine) { | ||
| message += '\n\nPlease check your internet connection and try again.' | ||
| } else { | ||
| message += '\nWe tried these proxies but none of them worked, try opening any of these urls in your browser:' | ||
| message += `\n${appStorage.proxiesData.proxies.join(', ')}` | ||
| } | ||
| setLoadingScreenStatus(message, true) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify error handling and user experience for proxy selection failures.
The auto-selection logic is well-implemented, but there are a few considerations:
- The error message construction could be improved for better user guidance
- The
navigator.onLinecheck may not be reliable in all environments
Consider this improvement for better error messaging:
- let message = 'Failed to find a working proxy.'
- if (navigator.onLine) {
- message += '\n\nPlease check your internet connection and try again.'
- } else {
- message += '\nWe tried these proxies but none of them worked, try opening any of these urls in your browser:'
- message += `\n${appStorage.proxiesData.proxies.join(', ')}`
- }
+ let message = 'Failed to find a working proxy.'
+ if (!navigator.onLine) {
+ message += '\n\nYour device appears to be offline. Please check your internet connection.'
+ } else {
+ message += '\n\nTry manually testing these proxy URLs in your browser:'
+ message += `\n${appStorage.proxiesData.proxies.join('\n')}`
+ }Could you verify if navigator.onLine works reliably across all target browsers and environments?
🏁 Script executed:
#!/bin/bash
# Search for other usages of navigator.onLine to understand browser support requirements
rg "navigator\.onLine" --type ts --type js -A 2 -B 2Length of output: 319
Improve offline detection and error messaging for proxy selection
The navigator.onLine API is known to be unreliable (e.g. some browsers always report “online”). Instead of relying solely on it, add an active connectivity check before choosing which error message to show, and update the messaging accordingly.
• File: src/index.ts, lines 292–308
• Replace the navigator.onLine branch with something like:
- let message = 'Failed to find a working proxy.'
- if (!navigator.onLine) {
- message += '\n\nYour device appears to be offline. Please check your internet connection.'
- } else {
- message += '\n\nTry manually testing these proxy URLs in your browser:'
- message += `\n${appStorage.proxiesData.proxies.join('\n')}`
- }
+ let message = 'Failed to find a working proxy.'
+ // Perform a lightweight network check rather than relying on navigator.onLine
+ const isConnected = await fetch('https://example.com/healthcheck', {
+ method: 'HEAD',
+ cache: 'no-store',
+ })
+ .then(() => true)
+ .catch(() => false)
+
+ if (!isConnected) {
+ message += '\n\nUnable to reach the internet. Please check your network connection and try again.'
+ } else {
+ message += '\n\nAll tested proxies failed. You can try opening these URLs directly in your browser:'
+ message += `\n${appStorage.proxiesData.proxies.join('\n')}`
+ }This approach gives more accurate connectivity detection across browsers. You may want to extract the health-check URL or logic into a reusable helper.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/index.ts around lines 292 to 308, the use of navigator.onLine for offline
detection is unreliable. Replace the navigator.onLine check with an active
connectivity test by performing a lightweight fetch request to a known reliable
URL to confirm internet access. Based on the fetch result, update the error
message accordingly. Extract this connectivity check into a reusable helper
function for clarity and reuse.
| // Use Promise.race to get the first successful response | ||
| const results = await Promise.race([ | ||
| // Wait for first successful ping | ||
| Promise.any(pingPromises.map(async p => p.then(r => r && { type: 'success' as const, data: r }))), | ||
| // Or wait for all to fail | ||
| Promise.all(pingPromises).then(results => { | ||
| if (results.every(r => r === null)) { | ||
| return { type: 'all-failed' as const } | ||
| } | ||
| return null | ||
| }) | ||
| ]) | ||
|
|
||
| if (!results || results.type === 'all-failed') { | ||
| return null | ||
| } | ||
|
|
||
| return results.type === 'success' ? results.data.proxy : null | ||
| } finally { | ||
| currentPingAbortController = null | ||
| proxyPingState.checkStarted = false | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Promise.race logic and add timeout protection.
The current Promise.race logic is complex and could be simplified. Also, there's no timeout protection for the ping operations.
- // Use Promise.race to get the first successful response
- const results = await Promise.race([
- // Wait for first successful ping
- Promise.any(pingPromises.map(async p => p.then(r => r && { type: 'success' as const, data: r }))),
- // Or wait for all to fail
- Promise.all(pingPromises).then(results => {
- if (results.every(r => r === null)) {
- return { type: 'all-failed' as const }
- }
- return null
- })
- ])
-
- if (!results || results.type === 'all-failed') {
- return null
- }
-
- return results.type === 'success' ? results.data.proxy : null
+ // Wait for first successful ping or all to complete
+ try {
+ const firstSuccess = await Promise.any(
+ pingPromises.map(async p => {
+ const result = await p
+ if (result) return result
+ throw new Error('Ping failed')
+ })
+ )
+ return firstSuccess.proxy
+ } catch {
+ // All pings failed
+ return null
+ }Also consider adding a timeout wrapper:
const PING_TIMEOUT = 10000 // 10 seconds
const pingWithTimeout = (promise: Promise<any>) =>
Promise.race([
promise,
new Promise((_, reject) =>
setTimeout(() => reject(new Error('Ping timeout')), PING_TIMEOUT)
)
])🤖 Prompt for AI Agents
In src/core/proxyAutoSelect.ts around lines 62 to 84, simplify the Promise.race
logic by removing the nested Promise.any and Promise.all combination and instead
use a single Promise.race with ping promises wrapped in a timeout. Implement a
pingWithTimeout helper that wraps each ping promise with a timeout rejection
after 10 seconds, then race these wrapped promises to get the first successful
ping or timeout. This will both simplify the logic and add timeout protection to
the ping operations.
| export async function selectBestProxy (proxies: string[]): Promise<string | null> { | ||
| if (proxyPingState.checkStarted) { | ||
| cancelProxyPinging() | ||
| } | ||
| proxyPingState.checkStarted = true | ||
|
|
||
| // Cancel any ongoing pings | ||
| if (currentPingAbortController) { | ||
| currentPingAbortController.abort() | ||
| } | ||
| currentPingAbortController = new AbortController() | ||
| const abortController = currentPingAbortController // Store in local const to satisfy TypeScript | ||
|
|
||
| // Reset ping states | ||
| for (const proxy of proxies) { | ||
| proxyPingState.proxyStatus[proxy] = { status: 'checking' } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address potential race condition in checkStarted flag.
The current implementation has a potential race condition where checkStarted is set to true before checking for existing operations.
export async function selectBestProxy (proxies: string[]): Promise<string | null> {
- if (proxyPingState.checkStarted) {
- cancelProxyPinging()
- }
- proxyPingState.checkStarted = true
+ // Cancel any ongoing pings first
+ if (proxyPingState.checkStarted) {
+ cancelProxyPinging()
+ }
- // Cancel any ongoing pings
if (currentPingAbortController) {
currentPingAbortController.abort()
}
currentPingAbortController = new AbortController()
const abortController = currentPingAbortController // Store in local const to satisfy TypeScript
+
+ proxyPingState.checkStarted = true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function selectBestProxy (proxies: string[]): Promise<string | null> { | |
| if (proxyPingState.checkStarted) { | |
| cancelProxyPinging() | |
| } | |
| proxyPingState.checkStarted = true | |
| // Cancel any ongoing pings | |
| if (currentPingAbortController) { | |
| currentPingAbortController.abort() | |
| } | |
| currentPingAbortController = new AbortController() | |
| const abortController = currentPingAbortController // Store in local const to satisfy TypeScript | |
| // Reset ping states | |
| for (const proxy of proxies) { | |
| proxyPingState.proxyStatus[proxy] = { status: 'checking' } | |
| } | |
| export async function selectBestProxy (proxies: string[]): Promise<string | null> { | |
| // Cancel any ongoing pings first | |
| if (proxyPingState.checkStarted) { | |
| cancelProxyPinging() | |
| } | |
| if (currentPingAbortController) { | |
| currentPingAbortController.abort() | |
| } | |
| currentPingAbortController = new AbortController() | |
| const abortController = currentPingAbortController // Store in local const to satisfy TypeScript | |
| proxyPingState.checkStarted = true | |
| // Reset ping states | |
| for (const proxy of proxies) { | |
| proxyPingState.proxyStatus[proxy] = { status: 'checking' } | |
| } | |
| // …rest of function… | |
| } |
🤖 Prompt for AI Agents
In src/core/proxyAutoSelect.ts around lines 23 to 40, the checkStarted flag is
set to true before confirming no existing operations are running, causing a
potential race condition. To fix this, move the assignment of checkStarted =
true to after cancelProxyPinging() completes and any ongoing pings are aborted,
ensuring no concurrent operations proceed simultaneously.
| // Create a promise for each proxy | ||
| const pingPromises = proxies.map(async (proxy) => { | ||
| if (proxy.startsWith(':')) { | ||
| proxy = `${location.protocol}//${location.hostname}${proxy}` | ||
| } | ||
| try { | ||
| const result = await pingProxyServer(proxy, abortController.signal) | ||
| if (result.success) { | ||
| proxyPingState.proxyStatus[proxy] = { status: 'success', latency: result.latency } | ||
| return { proxy, latency: result.latency } | ||
| } else { | ||
| proxyPingState.proxyStatus[proxy] = { status: 'error', error: result.error } | ||
| return null | ||
| } | ||
| } catch (err) { | ||
| proxyPingState.proxyStatus[proxy] = { status: 'error', error: String(err) } | ||
| return null | ||
| } | ||
| }) |
There was a problem hiding this comment.
Potential mutation of loop variable causing incorrect proxy tracking.
There's a critical bug where the proxy variable is mutated within the loop, which will cause incorrect tracking in proxyPingState.proxyStatus.
const pingPromises = proxies.map(async (proxy) => {
+ let normalizedProxy = proxy
if (proxy.startsWith(':')) {
- proxy = `${location.protocol}//${location.hostname}${proxy}`
+ normalizedProxy = `${location.protocol}//${location.hostname}${proxy}`
}
try {
- const result = await pingProxyServer(proxy, abortController.signal)
+ const result = await pingProxyServer(normalizedProxy, abortController.signal)
if (result.success) {
- proxyPingState.proxyStatus[proxy] = { status: 'success', latency: result.latency }
- return { proxy, latency: result.latency }
+ proxyPingState.proxyStatus[proxy] = { status: 'success', latency: result.latency }
+ return { proxy: normalizedProxy, latency: result.latency }
} else {
proxyPingState.proxyStatus[proxy] = { status: 'error', error: result.error }
return null
}
} catch (err) {
proxyPingState.proxyStatus[proxy] = { status: 'error', error: String(err) }
return null
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create a promise for each proxy | |
| const pingPromises = proxies.map(async (proxy) => { | |
| if (proxy.startsWith(':')) { | |
| proxy = `${location.protocol}//${location.hostname}${proxy}` | |
| } | |
| try { | |
| const result = await pingProxyServer(proxy, abortController.signal) | |
| if (result.success) { | |
| proxyPingState.proxyStatus[proxy] = { status: 'success', latency: result.latency } | |
| return { proxy, latency: result.latency } | |
| } else { | |
| proxyPingState.proxyStatus[proxy] = { status: 'error', error: result.error } | |
| return null | |
| } | |
| } catch (err) { | |
| proxyPingState.proxyStatus[proxy] = { status: 'error', error: String(err) } | |
| return null | |
| } | |
| }) | |
| // Create a promise for each proxy | |
| const pingPromises = proxies.map(async (proxy) => { | |
| let normalizedProxy = proxy | |
| if (proxy.startsWith(':')) { | |
| normalizedProxy = `${location.protocol}//${location.hostname}${proxy}` | |
| } | |
| try { | |
| const result = await pingProxyServer(normalizedProxy, abortController.signal) | |
| if (result.success) { | |
| proxyPingState.proxyStatus[proxy] = { status: 'success', latency: result.latency } | |
| return { proxy: normalizedProxy, latency: result.latency } | |
| } else { | |
| proxyPingState.proxyStatus[proxy] = { status: 'error', error: result.error } | |
| return null | |
| } | |
| } catch (err) { | |
| proxyPingState.proxyStatus[proxy] = { status: 'error', error: String(err) } | |
| return null | |
| } | |
| }) |
🤖 Prompt for AI Agents
In src/core/proxyAutoSelect.ts between lines 42 and 60, the loop variable
'proxy' is mutated inside the map callback, which causes incorrect keys in
proxyPingState.proxyStatus. To fix this, create a new variable to hold the
potentially modified proxy URL instead of reassigning 'proxy'. Use this new
variable for all subsequent operations and assignments to ensure the original
loop variable remains unchanged.
| const ProxyPingStatus = ({ proxy }: { proxy: string }) => { | ||
| const pingState = useSnapshot(proxyPingState).proxyStatus[proxy] | ||
| useEffect(() => { | ||
| if (!proxyPingState.checkStarted) { | ||
| void selectBestProxy(appStorage.proxiesData?.proxies ?? []) | ||
| } | ||
| }, []) | ||
|
|
||
| if (!pingState) return null | ||
|
|
||
| let color = 'yellow' | ||
| let text = '...' | ||
|
|
||
| if (pingState.status === 'success') { | ||
| color = 'limegreen' | ||
| text = `${pingState.latency}ms` | ||
| } else if (pingState.status === 'error') { | ||
| color = 'red' | ||
| text = 'err' | ||
| } | ||
|
|
||
| return ( | ||
| <div style={{ display: 'flex', alignItems: 'center', gap: '4px', marginLeft: '4px' }}> | ||
| <div style={{ | ||
| width: '8px', | ||
| height: '8px', | ||
| borderRadius: '50%', | ||
| backgroundColor: color | ||
| }} /> | ||
| <span style={{ fontSize: '12px', color: 'lightgray' }}>{text}</span> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ProxyPingStatus component has potential performance and lifecycle issues.
The component has good visual design but some concerns:
- The useEffect runs on every render without proper dependencies
- Potential memory leak if component unmounts during ping operation
- Missing error boundary protection
const ProxyPingStatus = ({ proxy }: { proxy: string }) => {
const pingState = useSnapshot(proxyPingState).proxyStatus[proxy]
+ const hasStartedRef = useRef(false)
+
useEffect(() => {
- if (!proxyPingState.checkStarted) {
+ if (!proxyPingState.checkStarted && !hasStartedRef.current) {
+ hasStartedRef.current = true
void selectBestProxy(appStorage.proxiesData?.proxies ?? [])
}
- }, [])
+ }, []) // Dependencies are correct since we only want this to run once
if (!pingState) return nullConsider adding error boundary protection and cleanup:
useEffect(() => {
return () => {
// Cleanup on unmount
if (proxyPingState.checkStarted) {
// Consider if we should cancel here or let it continue
}
}
}, [])🤖 Prompt for AI Agents
In src/react/ServersList.tsx between lines 45 and 77, the ProxyPingStatus
component's useEffect lacks dependencies, causing it to run only once but
potentially missing updates; add proper dependencies to ensure it runs when
needed. Also, implement a cleanup function inside useEffect to handle component
unmounting, preventing memory leaks by canceling or ignoring ongoing ping
operations if applicable. Finally, wrap the component or its usage in an error
boundary to catch and handle runtime errors gracefully.
| export async function monitorLatency () { | ||
| const monitor = new LatencyMonitor('https://your-server.com') | ||
|
|
||
| try { | ||
| await monitor.connect() | ||
|
|
||
| // Single measurement | ||
| const latency = await monitor.measureLatency() | ||
|
|
||
| // Or continuous monitoring | ||
| setInterval(async () => { | ||
| try { | ||
| const latency = await monitor.measureLatency() | ||
| console.log('Current latency:', latency) | ||
| } catch (error) { | ||
| console.error('Error measuring latency:', error) | ||
| } | ||
| }, 5000) // Check every 5 seconds | ||
|
|
||
| } catch (error) { | ||
| console.error('Failed to connect:', error) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or improve the example monitorLatency function.
The monitorLatency function appears to be example code with a hardcoded URL and should not be in production code.
Either remove this function entirely if it's just an example, or improve it for actual use:
-export async function monitorLatency () {
- const monitor = new LatencyMonitor('https://your-server.com')
+export async function monitorLatency (serverUrl: string, options: {
+ interval?: number
+ onLatency?: (latency: any) => void
+ onError?: (error: any) => void
+} = {}) {
+ const { interval = 5000, onLatency = console.log, onError = console.error } = options
+ const monitor = new LatencyMonitor(serverUrl)
try {
await monitor.connect()
- // Single measurement
- const latency = await monitor.measureLatency()
-
- // Or continuous monitoring
setInterval(async () => {
try {
const latency = await monitor.measureLatency()
- console.log('Current latency:', latency)
+ onLatency(latency)
} catch (error) {
- console.error('Error measuring latency:', error)
+ onError(error)
}
- }, 5000) // Check every 5 seconds
+ }, interval)
} catch (error) {
- console.error('Failed to connect:', error)
+ onError(error)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function monitorLatency () { | |
| const monitor = new LatencyMonitor('https://your-server.com') | |
| try { | |
| await monitor.connect() | |
| // Single measurement | |
| const latency = await monitor.measureLatency() | |
| // Or continuous monitoring | |
| setInterval(async () => { | |
| try { | |
| const latency = await monitor.measureLatency() | |
| console.log('Current latency:', latency) | |
| } catch (error) { | |
| console.error('Error measuring latency:', error) | |
| } | |
| }, 5000) // Check every 5 seconds | |
| } catch (error) { | |
| console.error('Failed to connect:', error) | |
| } | |
| } | |
| export async function monitorLatency ( | |
| serverUrl: string, | |
| options: { | |
| interval?: number | |
| onLatency?: (latency: any) => void | |
| onError?: (error: any) => void | |
| } = {} | |
| ) { | |
| const { interval = 5000, onLatency = console.log, onError = console.error } = options | |
| const monitor = new LatencyMonitor(serverUrl) | |
| try { | |
| await monitor.connect() | |
| setInterval(async () => { | |
| try { | |
| const latency = await monitor.measureLatency() | |
| onLatency(latency) | |
| } catch (error) { | |
| onError(error) | |
| } | |
| }, interval) | |
| } catch (error) { | |
| onError(error) | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/core/pingProxy.ts between lines 99 and 121, the monitorLatency function
uses a hardcoded URL and serves as example code, which is not suitable for
production. Remove this function entirely if it is only for demonstration, or
refactor it to accept the server URL as a parameter and handle errors
appropriately to make it reusable and production-ready.
| async measureLatency (): Promise<{ | ||
| roundTripTime: number; | ||
| serverProcessingTime: number; | ||
| networkLatency: number; | ||
| }> { | ||
| return new Promise((resolve, reject) => { | ||
| if (!this.isConnected) { | ||
| reject(new Error('Not connected')) | ||
| return | ||
| } | ||
|
|
||
| const pingId = Date.now().toString() | ||
| const startTime = performance.now() | ||
|
|
||
| const handler = (event: MessageEvent) => { | ||
| if (typeof event.data === 'string' && event.data.startsWith('pong:')) { | ||
| const [_, receivedPingId, serverProcessingTime] = event.data.split(':') | ||
|
|
||
| if (receivedPingId === pingId) { | ||
| this.ws?.removeEventListener('message', handler) | ||
| const roundTripTime = performance.now() - startTime | ||
|
|
||
| resolve({ | ||
| roundTripTime, | ||
| serverProcessingTime: parseFloat(serverProcessingTime), | ||
| networkLatency: roundTripTime - parseFloat(serverProcessingTime) | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| this.ws?.addEventListener('message', handler) | ||
| this.ws?.send('ping:' + pingId) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve latency measurement robustness and cleanup.
The latency measurement has good logic but needs better error handling and cleanup:
- No timeout protection for ping responses
- Event listener not removed on error paths
- Potential memory leak with orphaned event listeners
async measureLatency (): Promise<{
roundTripTime: number;
serverProcessingTime: number;
networkLatency: number;
}> {
- return new Promise((resolve, reject) => {
+ return new Promise((resolve, reject) => {
+ const PING_TIMEOUT = 5000 // 5 seconds
+ let timeoutId: NodeJS.Timeout
if (!this.isConnected) {
reject(new Error('Not connected'))
return
}
const pingId = Date.now().toString()
const startTime = performance.now()
+ const cleanup = () => {
+ if (timeoutId) clearTimeout(timeoutId)
+ this.ws?.removeEventListener('message', handler)
+ }
+
const handler = (event: MessageEvent) => {
if (typeof event.data === 'string' && event.data.startsWith('pong:')) {
const [_, receivedPingId, serverProcessingTime] = event.data.split(':')
if (receivedPingId === pingId) {
- this.ws?.removeEventListener('message', handler)
+ cleanup()
const roundTripTime = performance.now() - startTime
resolve({
roundTripTime,
serverProcessingTime: parseFloat(serverProcessingTime),
networkLatency: roundTripTime - parseFloat(serverProcessingTime)
})
}
}
}
+
+ timeoutId = setTimeout(() => {
+ cleanup()
+ reject(new Error('Ping timeout'))
+ }, PING_TIMEOUT)
this.ws?.addEventListener('message', handler)
this.ws?.send('ping:' + pingId)
})
}🤖 Prompt for AI Agents
In src/core/pingProxy.ts between lines 27 and 61, the measureLatency method
lacks timeout handling and proper cleanup of event listeners on error or
timeout. To fix this, add a timeout mechanism that rejects the promise if no
pong response is received within a reasonable time, and ensure the message event
listener is removed both on successful response and on timeout or error to
prevent memory leaks.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes