feat: Add ws auto reconnect #78
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several changes across multiple files, focusing on enhancing WebSocket functionality, modifying Docker workflows, and updating service monitoring structures. Key updates include a new WebSocket management file that implements automatic reconnection, modifications to the Docker metadata tagging logic, and the addition of new properties in service definitions. Additionally, the build script now dynamically sets the project version based on Git tags, and a new dependency has been added to the frontend package. Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (5)
frontend/src/routes/(dashboard)/+layout.svelte (1)
58-58: Remove commented console.log statementsClean up the code by removing the commented debug statements.
Also applies to: 66-66
frontend/src/routes/(dashboard)/services/(components)/data-table-toolbar.svelte (2)
27-29: Enhance property documentation with validation rules.While the new properties are well-typed, their documentation could be more descriptive about:
- Valid ranges for
expected_code- Format requirements for
expected_payload- Use cases for the
invertflagConsider expanding the JSDoc comments like this:
- * @property {boolean} invert - Invert the expected result. - * @property {number} expected_code - The expected HTTP status code. - * @property {string} expected_payload - The expected HTTP response body. + * @property {boolean} invert - When true, inverts the success condition (useful for negative testing). + * @property {number} expected_code - The expected HTTP status code (100-599). + * @property {string} expected_payload - Expected response body (supports exact match or regex pattern).
Line range hint
1-1: Add UI elements for new monitoring configuration options.The new fields (
invert,expected_code,expected_payload) are initialized but not exposed in the UI form. Users can't modify these monitoring parameters.Add form fields after the timeout input:
<div class="grid grid-cols-4 items-center gap-4"> <Label for="expected_code" class="text-right">Expected Status</Label> <Input id="expected_code" bind:value={newService.expected_code} placeholder="HTTP status code (e.g., 200)" type="number" min="100" max="599" class="col-span-3" /> </div> <div class="grid grid-cols-4 items-center gap-4"> <Label for="expected_payload" class="text-right">Expected Response</Label> <Input id="expected_payload" bind:value={newService.expected_payload} placeholder="Expected response body or regex" class="col-span-3" /> </div> <div class="grid grid-cols-4 items-center gap-4"> <Label for="invert" class="text-right">Invert Result</Label> <div class="col-span-3"> <input type="checkbox" id="invert" bind:checked={newService.invert} /> <span class="ml-2">Invert the success condition</span> </div> </div>frontend/src/lib/store/ws.js (2)
77-79: Enhance error handling inonErrorcallbackThe
onErrorcallback currently only logs errors to the console. Consider providing user feedback or implementing additional error handling to improve the user experience.You might display a toast notification to inform the user:
onError: (ws, err) => { console.log(err); + toast.error('WebSocket encountered an error.', { + description: err.message || 'An unknown error occurred.' + }); }This ensures that users are aware of errors without having to check the console.
55-76: Prevent multiple intervals inonDisconnectedhandlerIf the
onDisconnectedcallback is triggered multiple times before reconnection occurs, multiple intervals may be set, leading to unexpected behavior or memory leaks. Consider tracking the interval and clearing any existing interval before starting a new one.Modify the code to manage the interval:
+ let reconnectInterval; onDisconnected: () => { + if (reconnectInterval) { + clearInterval(reconnectInterval); + } toast.warning('Websocket disconnected!', { id: toastId, description: 'Reconnecting in 30 seconds...' // dismissible: false }); let now = Date.now(); - let interval = setInterval(() => { + reconnectInterval = setInterval(() => { toast.info(`Reconnecting in ${30 - Math.ceil((Date.now() - now) / 1000)} seconds...`, { id: toastId, description: '' }); if (Date.now() - now > 29000) { - clearInterval(interval); + clearInterval(reconnectInterval); + reconnectInterval = null; toast.loading('Reconnecting...', { id: toastId, duration: 3000 // dismissible: true }); } }, 500); },This change ensures that only one interval runs at a time, preventing potential memory leaks.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.github/workflows/docker.yml(1 hunks)crates/server/build.rs(1 hunks)frontend/package.json(1 hunks)frontend/src/lib/store/logs.js(0 hunks)frontend/src/lib/store/ws.js(1 hunks)frontend/src/routes/(dashboard)/+layout.svelte(3 hunks)frontend/src/routes/(dashboard)/services/(components)/data-table-toolbar.svelte(3 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/lib/store/logs.js
🔇 Additional comments (6)
crates/server/build.rs (2)
1-6: LGTM!
The import and existing migration tracking logic are appropriate.
8-18: Verify version propagation to Docker builds
Let's ensure the version is properly propagated to Docker builds.
✅ Verification successful
Let me gather more information about how the version is used in the codebase.
Version integration with Docker builds is properly handled
The version propagation is correctly implemented:
- The version from git tags is captured during build time via
build.rsand exposed asSTAMON_VERSION - The server logs the version on startup, falling back to
CARGO_PKG_VERSIONif git tag is not available - Docker builds use the official GitHub metadata action to generate appropriate version tags based on git tags
- The Docker workflow is triggered on tag pushes matching 'v*' pattern and generates semver-compatible tags
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how STAMON_VERSION is used in Docker builds
# Check Dockerfile for version usage
rg -l "STAMON_VERSION"
# Check GitHub workflow files for version handling
fd -t f . .github/workflows -x cat {} \; | rg -A 5 -B 5 "tags|version"
Length of output: 1196
Script:
#!/bin/bash
# Check how STAMON_VERSION is used in main.rs
rg -A 5 -B 5 "STAMON_VERSION" crates/server/src/main.rs
# Check Dockerfile content
fd Dockerfile -x cat {}
Length of output: 2394
frontend/package.json (1)
24-24: Verify @sveu/browser package security and stability
Let's verify the security and stability of the @sveu/browser package before proceeding.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the package's security and stability:
@sveu/browser package appears secure and stable for use
- Package is actively maintained with MIT license
- No security vulnerabilities found in GitHub Advisory Database
- Has minimal dependencies (only @sveu/shared)
- Package is stable with 35 versions released
- Published and maintained by a consistent maintainer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check package information, download stats, and security advisories
# Check NPM for package info and stats
echo "Package Info:"
npm view @sveu/browser
echo "\nWeekly Downloads:"
npm view @sveu/browser downloads
# Check for known vulnerabilities using npm audit
echo "\nVulnerability Check:"
npm audit @sveu/browser
# Check GitHub Advisory Database
echo "\nGitHub Security Advisories:"
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "@sveu/browser") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1631
frontend/src/routes/(dashboard)/+layout.svelte (3)
12-15: LGTM: New imports are appropriate
The added imports support the new WebSocket and data fetching functionality.
86-86: LGTM: Appropriate authentication check
The conditional rendering ensures the dashboard is only accessible to authenticated users.
47-53: WebSocket initialization looks good but could be more robust
The WebSocket initialization aligns with the PR objective of implementing auto-reconnect. The dynamic import and status check are appropriate.
Let's verify the WebSocket store implementation:
✅ Verification successful
WebSocket implementation is robust and well-implemented
The WebSocket store implementation is actually quite comprehensive with proper auto-reconnect functionality:
- Uses
@sveu/browserWebSocket wrapper with auto-reconnect configuration - Has proper error handling with user feedback via toasts
- Implements reconnection with 5 retries and 30-second delay
- Includes proper connection state management (connected, disconnected, error)
- Server-side implementation supports proper connection lifecycle and error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the WebSocket store implements reconnection logic
ast-grep --pattern 'class $_ {
$$$
reconnect() {
$$$
}
$$$
}'
Length of output: 75
Script:
#!/bin/bash
# Let's search for WebSocket-related code in the store directory
rg -A 10 "class.*WebSocket|class.*ws" src/lib/store/
# Also check for any reconnection-related code
rg -A 5 "reconnect|open.*WebSocket" src/lib/store/
Length of output: 217
Script:
#!/bin/bash
# Let's first find the correct path to the store directory
fd -t d "store"
# Then search for WebSocket-related files
fd -t f "ws"
# And look for WebSocket-related code in all files
rg -A 10 "class.*WebSocket|class.*ws"
Length of output: 1520
Script:
#!/bin/bash
# Let's examine the WebSocket implementation in the store
cat frontend/src/lib/store/ws.js
# Also check the Rust WebSocket implementation for reconnection logic
cat crates/server/src/ws.rs
Length of output: 7547
| // Set version env | ||
| let output = Command::new("git") | ||
| .args(["describe", "--tags"]) | ||
| .output() | ||
| .expect("git command not found."); | ||
| if output.status.success() { | ||
| println!( | ||
| "cargo:rustc-env=STAMON_VERSION={}", | ||
| String::from_utf8_lossy(&output.stdout) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve version detection robustness and error handling
The current implementation has several potential issues:
- It will panic if git isn't available
- No validation of tag format
- No trimming of whitespace/newlines from command output
- No fallback version for environments without git
Consider this more robust implementation:
- let output = Command::new("git")
- .args(["describe", "--tags"])
- .output()
- .expect("git command not found.");
- if output.status.success() {
- println!(
- "cargo:rustc-env=STAMON_VERSION={}",
- String::from_utf8_lossy(&output.stdout)
- );
- }
+ let version = Command::new("git")
+ .args(["describe", "--tags"])
+ .output()
+ .ok()
+ .and_then(|output| {
+ if output.status.success() {
+ String::from_utf8(output.stdout).ok()
+ } else {
+ None
+ }
+ })
+ .map(|v| v.trim().to_string())
+ .unwrap_or_else(|| String::from("0.0.0-unknown"));
+
+ println!("cargo:rustc-env=STAMON_VERSION={}", version);This implementation:
- Gracefully handles missing git command
- Trims whitespace and newlines
- Provides a fallback version
- Uses proper error handling instead of .expect()
📝 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.
| // Set version env | |
| let output = Command::new("git") | |
| .args(["describe", "--tags"]) | |
| .output() | |
| .expect("git command not found."); | |
| if output.status.success() { | |
| println!( | |
| "cargo:rustc-env=STAMON_VERSION={}", | |
| String::from_utf8_lossy(&output.stdout) | |
| ); | |
| } | |
| // Set version env | |
| let version = Command::new("git") | |
| .args(["describe", "--tags"]) | |
| .output() | |
| .ok() | |
| .and_then(|output| { | |
| if output.status.success() { | |
| String::from_utf8(output.stdout).ok() | |
| } else { | |
| None | |
| } | |
| }) | |
| .map(|v| v.trim().to_string()) | |
| .unwrap_or_else(|| String::from("0.0.0-unknown")); | |
| println!("cargo:rustc-env=STAMON_VERSION={}", version); |
| push: true |
There was a problem hiding this comment.
💡 Codebase verification
Docker workflow configuration needs attention
The workflow configuration has potential issues that need to be addressed:
- The DockerHub credentials are not properly configured:
DOCKERHUB_TOKENis missing from repository secretsDOCKERHUB_USERNAMEis referenced as a variable (vars) instead of a secret
- The Docker image is hardcoded to
k4htoo/stamonwhich might not be the intended repository
Please ensure:
- Add
DOCKERHUB_TOKENto repository secrets - Move
DOCKERHUB_USERNAMEfrom variables to secrets for better security - Verify if
k4htoo/stamonis the correct Docker repository
🔗 Analysis chain
LGTM! The push parameter is correctly configured.
The addition of push: true is necessary for publishing the Docker images to DockerHub and is properly formatted.
Let's verify the workflow configuration:
Let me gather more information about the Docker workflow configuration to ensure all required elements are properly set up.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the workflow file syntax and required secrets
# Check if the workflow file is valid YAML
yamllint .github/workflows/docker.yml 2>/dev/null || echo "Workflow file syntax is valid"
# Verify required secrets are configured
gh api repos/:owner/:repo/actions/secrets | jq -r '.secrets[] | select(.name=="DOCKERHUB_TOKEN") | .name' || echo "DOCKERHUB_TOKEN is not configured"
# Check if docker-bake.hcl exists and is valid
if [ -f docker-bake.hcl ]; then
echo "docker-bake.hcl exists"
cat docker-bake.hcl
else
echo "Warning: docker-bake.hcl not found"
fi
Length of output: 708
Script:
#!/bin/bash
# Check the complete workflow file content
cat .github/workflows/docker.yml
# Check if DOCKERHUB_USERNAME is referenced in the workflow
rg "DOCKERHUB_USERNAME" .github/workflows/docker.yml
# Check Docker metadata configuration
rg "docker/metadata-action" -A 5 .github/workflows/docker.yml
Length of output: 1564
| @@ -21,6 +21,7 @@ | |||
| "@sveltejs/adapter-static": "^3.0.1", | |||
| "@sveltejs/kit": "^2.0.0", | |||
| "@sveltejs/vite-plugin-svelte": "^3.0.0", | |||
| "@sveu/browser": "^1.0.1", | |||
There was a problem hiding this comment.
Move @sveu/browser to runtime dependencies
The @sveu/browser package is being used for WebSocket functionality which is needed at runtime. It should be moved from devDependencies to dependencies.
Apply this diff:
"devDependencies": {
"@fontsource/fira-mono": "^4.5.10",
"@neoconfetti/svelte": "^1.0.0",
"@playwright/test": "^1.28.1",
"@sveltejs/adapter-auto": "^3.0.0",
"@sveltejs/adapter-static": "^3.0.1",
"@sveltejs/kit": "^2.0.0",
"@sveltejs/vite-plugin-svelte": "^3.0.0",
- "@sveu/browser": "^1.0.1",
"@types/eslint": "^8.56.0",
...
},
"dependencies": {
+ "@sveu/browser": "^1.0.1",
"@unovis/svelte": "^1.4.1",
...
}📝 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.
| "@sveu/browser": "^1.0.1", | |
| "devDependencies": { | |
| "@fontsource/fira-mono": "^4.5.10", | |
| "@neoconfetti/svelte": "^1.0.0", | |
| "@playwright/test": "^1.28.1", | |
| "@sveltejs/adapter-auto": "^3.0.0", | |
| "@sveltejs/adapter-static": "^3.0.1", | |
| "@sveltejs/kit": "^2.0.0", | |
| "@sveltejs/vite-plugin-svelte": "^3.0.0", | |
| "@types/eslint": "^8.56.0", | |
| ... | |
| }, | |
| "dependencies": { | |
| "@sveu/browser": "^1.0.1", | |
| "@unovis/svelte": "^1.4.1", | |
| ... | |
| } |
| cfetch('/services').then(async (res) => { | ||
| if (res.ok) { | ||
| const data = await res.json(); | ||
| // console.log(data); | ||
| services.set(data.services); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and loading states for data fetching
The fetch calls should include:
- Error handling for failed requests
- Loading states for better UX
- Error states to show when data fetching fails
Consider refactoring to:
+ let loading = true;
+ let error = null;
onMount(async () => {
if ($user) {
const { ws } = await import('$lib/store/ws');
if (get(ws.status) === 'CLOSED') {
ws.open();
}
try {
- cfetch('/services').then(async (res) => {
- if (res.ok) {
- const data = await res.json();
- // console.log(data);
- services.set(data.services);
- }
- });
+ const [servicesRes, statsRes] = await Promise.all([
+ cfetch('/services'),
+ cfetch('/stats')
+ ]);
+
+ if (!servicesRes.ok) throw new Error('Failed to fetch services');
+ if (!statsRes.ok) throw new Error('Failed to fetch stats');
+
+ const servicesData = await servicesRes.json();
+ const statsData = await statsRes.json();
+
+ services.set(servicesData.services);
+ stats.set(statsData.stats);
+ } catch (e) {
+ error = e.message;
+ console.error('Failed to fetch dashboard data:', e);
+ } finally {
+ loading = false;
}
}
});Also applies to: 63-69
| timeout: 20, | ||
| invert: false, | ||
| expected_code: 2, | ||
| expected_payload: '' |
There was a problem hiding this comment.
Fix invalid default HTTP status code and add input validation.
There are two critical issues in the initialization:
- The default
expected_codeof 2 is invalid (HTTP status codes range from 100-599) - Missing input validation for these new fields in the form
Consider these fixes:
- timeout: 20,
- invert: false,
- expected_code: 2,
- expected_payload: ''
+ timeout: 20,
+ invert: false,
+ expected_code: 200, // Default to OK status
+ expected_payload: ''Also, add validation constraints to the form inputs:
<Input
id="expected_code"
bind:value={newService.expected_code}
type="number"
min="100"
max="599"
class="col-span-3"
required
/>| // fix data types | ||
| newService.retry = Number(newService.retry); | ||
| newService.retry_interval = Number(newService.retry_interval); | ||
| newService.interval = Number(newService.interval); | ||
| newService.timeout = Number(newService.timeout); | ||
| newService.expected_code = Number(newService.expected_code); |
There was a problem hiding this comment.
Improve type conversion safety and validation.
The current number conversions could result in invalid values (NaN) being sent to the server if the input is malformed.
Consider adding validation:
- // fix data types
- newService.retry = Number(newService.retry);
- newService.retry_interval = Number(newService.retry_interval);
- newService.interval = Number(newService.interval);
- newService.timeout = Number(newService.timeout);
- newService.expected_code = Number(newService.expected_code);
+ // Convert and validate numeric fields
+ const numericFields = {
+ retry: { value: newService.retry, min: 1 },
+ retry_interval: { value: newService.retry_interval, min: 1 },
+ interval: { value: newService.interval, min: 1 },
+ timeout: { value: newService.timeout, min: 1 },
+ expected_code: { value: newService.expected_code, min: 100, max: 599 }
+ };
+
+ for (const [field, config] of Object.entries(numericFields)) {
+ const num = Number(config.value);
+ if (isNaN(num) || num < config.min || (config.max && num > config.max)) {
+ throw new Error(`Invalid value for ${field}`);
+ }
+ newService[field] = num;
+ }📝 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.
| // fix data types | |
| newService.retry = Number(newService.retry); | |
| newService.retry_interval = Number(newService.retry_interval); | |
| newService.interval = Number(newService.interval); | |
| newService.timeout = Number(newService.timeout); | |
| newService.expected_code = Number(newService.expected_code); | |
| // Convert and validate numeric fields | |
| const numericFields = { | |
| retry: { value: newService.retry, min: 1 }, | |
| retry_interval: { value: newService.retry_interval, min: 1 }, | |
| interval: { value: newService.interval, min: 1 }, | |
| timeout: { value: newService.timeout, min: 1 }, | |
| expected_code: { value: newService.expected_code, min: 100, max: 599 } | |
| }; | |
| for (const [field, config] of Object.entries(numericFields)) { | |
| const num = Number(config.value); | |
| if (isNaN(num) || num < config.min || (config.max && num > config.max)) { | |
| throw new Error(`Invalid value for ${field}`); | |
| } | |
| newService[field] = num; | |
| } |
| toast[noti.level](noti.title, { | ||
| description: noti.message | ||
| }); |
There was a problem hiding this comment.
Validate noti.level before using it to prevent runtime errors
Currently, noti.level is used directly to access methods on the toast object. If noti.level contains an invalid value, this could result in runtime errors or unexpected behavior. It's important to validate noti.level to ensure it corresponds to a valid toast method.
Apply this diff to validate noti.level before usage:
// @ts-ignore
- toast[noti.level](noti.title, {
+ const validLevels = ['success', 'error', 'warning', 'info'];
+ const level = validLevels.includes(noti.level) ? noti.level : 'info';
+ toast[level](noti.title, {
description: noti.message
});This change ensures that noti.level is one of the valid toast methods, defaulting to 'info' if not. It also removes the need for // @ts-ignore, improving type safety.
📝 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.
| toast[noti.level](noti.title, { | |
| description: noti.message | |
| }); | |
| const validLevels = ['success', 'error', 'warning', 'info']; | |
| const level = validLevels.includes(noti.level) ? noti.level : 'info'; | |
| toast[level](noti.title, { | |
| description: noti.message | |
| }); |
| }; | ||
|
|
||
| // Create a new websocket | ||
| export const ws = websocket(dev ? 'ws://localhost:3000/ws' : `ws://${location.host}/ws`, options); |
There was a problem hiding this comment.
Use wss:// for secure WebSocket connections in production
In production, using 'ws://' for WebSocket connections may result in mixed content warnings or blocked connections when the application is served over HTTPS. To ensure secure communication and avoid these issues, use 'wss://' in production when the page is served over HTTPS.
Modify the code to dynamically select the appropriate protocol:
export const ws = websocket(
- dev ? 'ws://localhost:3000/ws' : `ws://${location.host}/ws`,
+ dev ? 'ws://localhost:3000/ws' : `${location.protocol.replace('http', 'ws')}//${location.host}/ws`,
options
);This change ensures that if the page is loaded over HTTPS, the WebSocket connection will use wss://.
📝 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 const ws = websocket(dev ? 'ws://localhost:3000/ws' : `ws://${location.host}/ws`, options); | |
| export const ws = websocket(dev ? 'ws://localhost:3000/ws' : `${location.protocol.replace('http', 'ws')}//${location.host}/ws`, options); |
Closes #63
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Dependencies
@sveu/browseras a new development dependency.