Add optimal syncing - #36
Conversation
WalkthroughThis update introduces a robust WebSocket-based synchronization mechanism for site time tracking and deep work hours in an Electron application. The changes add a dedicated WebSocket server process, client logic for efficient delta updates, and lifecycle management to ensure reliability across app and system events. The build and packaging configurations are enhanced to support native modules and multiple entry points. Several scripts and dependencies are added or updated to automate builds, handle Electron native module rebuilding, and support WebSocket communication. The preload and renderer layers are updated to enable secure IPC for user data, and new configuration files are introduced to streamline the build process for main, preload, and renderer processes. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant Preload
participant Main
participant WS_Server
participant Backend
Renderer->>Preload: window.electron.sendUserToBackend(user)
Preload->>Main: IPC 'user-data' with user
Main->>Main: Log received user data
Renderer->>Main: updateSiteTimeTracker()
Main->>OptiSyncClient: optiSync.sendUpdates(trackers, deepWorkHours)
OptiSyncClient->>WS_Server: WebSocket SYNC_UPDATE (delta)
WS_Server->>Backend: HTTP POST /api/v1/activity/persist
Backend-->>WS_Server: HTTP 2xx (success)
WS_Server->>OptiSyncClient: WebSocket SYNC_ACK
Suggested labels
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
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:
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: 9
🧹 Nitpick comments (8)
src/wsServer.ts (1)
6-10: Implement actual message handling logic.The current implementation only logs messages from the parent process without taking any action. This should be replaced with actual message handling logic to control the server's behavior.
// Handle process messages if needed process.on('message', (message) => { // Handle any messages from the parent process - console.log('Received message:', message) + log.info('Received message from parent process:', message) + + if (typeof message === 'object' && message !== null) { + // Handle different message types + switch (message.type) { + case 'shutdown': + log.info('Shutdown command received') + process.exit(0) + break; + case 'restart': + log.info('Restart command received') + // Implement restart logic here + break; + default: + log.warn('Unknown message type:', message.type) + } + } })src/productivityUtils.ts (1)
166-166: Consider making the sync operation asynchronous.The current implementation synchronously calls the sync operation, which could potentially impact performance if the sync process is slow or the network is unreliable.
- optiSync.sendUpdates(timeTrackers, getDeepWorkHours()); + // Run sync operation asynchronously to avoid blocking + Promise.resolve().then(() => { + try { + optiSync.sendUpdates(timeTrackers, getDeepWorkHours()); + } catch (error) { + log.error('Failed to sync updates:', error); + } + });Alternative implementation if the
sendUpdatesmethod already returns a Promise:- optiSync.sendUpdates(timeTrackers, getDeepWorkHours()); + optiSync.sendUpdates(timeTrackers, getDeepWorkHours()) + .catch(error => { + log.error('Failed to sync updates:', error); + });src/server.ts (2)
35-41: Return negative acknowledgement to client on failureWhen the forward fails you only log an error; the client remains unaware and may discard its buffered data.
Send aSYNC_NACKor an error message so the client can retry or fall back to HTTP.- log.error('OptiSync Server: Failed to forward to Railway', response.status) + const errorMsg = { + type: 'SYNC_NACK', + data: { status: response.status } + } + if (ws.readyState === ws.OPEN) ws.send(JSON.stringify(errorMsg)) + log.error('OptiSync Server: Failed to forward to Railway', response.status)
42-45: Include stack trace in error logs for easier debugging
log.error('Error processing message', err)only prints the generic object.
Useerr instanceof Error ? err.stack : errto capture the full trace.vite.main.config.ts (1)
19-26: Consider extracting the growingexternallistEvery added dependency must be duplicated here. Rollup allows a function:
external: (id) => builtinModules.includes(id) || /^node:/.test(id) || /^(electron|ws|conf|electron-store)$/.test(id)
This keeps the list maintainable as more modules are introduced.src/optiSync.ts (1)
25-33: Reconnect strategy grows unbounded—cap the delay
setTimeout(() => this.connect(), 5000 * this.reconnectAttempts)yields 25 s after 5 tries but keeps growing if the socket opens then closes again.
ResetreconnectAttemptson every successful connection and failed attempt cap the delay:- setTimeout(() => this.connect(), 5000 * this.reconnectAttempts) + const delay = Math.min(30_000, 5_000 * this.reconnectAttempts) + setTimeout(() => this.connect(), delay)src/main.ts (2)
729-738: Variable shadowing hides module‑leveldeepWorkHours
const deepWorkHours = getDeepWorkHours()shadows the outerdeepWorkHoursobject, which may confuse readers and tooling.
Rename the inner constant, e.g.currentHours.
472-475: Ensure graceful shutdown of persistence timersInside
before-quityou stop the WebSocket server but not thepersistenceInterval; residual async work can prevent Electron from exiting or log unhandled rejections.app.on('before-quit', () => { isQuitting = true stopWebSocketServer() + stopPersistenceInterval() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
package.json(5 hunks)postcss.config.js(1 hunks)src/main.ts(7 hunks)src/optiSync.ts(1 hunks)src/productivityUtils.ts(2 hunks)src/server.ts(1 hunks)src/wsServer.ts(1 hunks)vite.main.config.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/productivityUtils.ts (2)
src/optiSync.ts (1)
optiSync(66-66)src/main.ts (1)
getDeepWorkHours(481-484)
src/server.ts (1)
src/renderer/src/config.ts (1)
API_BASE_URL(3-3)
src/main.ts (1)
src/types.ts (4)
SiteTimeTracker(29-36)User(2-8)StoreSchema(10-28)DeepWorkHours(38-46)
🔇 Additional comments (6)
postcss.config.js (1)
1-1: LGTM - Valid module format change.The switch from ES module syntax to CommonJS module syntax is appropriate and aligns with the build system updates.
src/productivityUtils.ts (1)
11-12: LGTM!These imports provide the necessary functionality for syncing time tracking data.
package.json (3)
11-12: LGTM!The updated lint command and new build script for handling multiple Vite configurations are appropriate improvements.
30-30: LGTM!Adding WebSocket type definitions is appropriate for the new WebSocket functionality.
60-61: LGTM!The added dependencies for WebSocket functionality and HTTP fetching are appropriate for the new synchronization features.
Also applies to: 71-72, 82-86
src/main.ts (1)
98-103:join(__dirname, 'wsServer.js')likely breaks in production buildAfter Vite bundles code to
.vite/build, runtime files live insideapp.asar.
Usepath.resolve(__dirname, 'wsServer.js')and ensure the file is included in thefilessection ofpackage.json/extraFilesof electron‑builder.
| import { startServer } from './server' | ||
|
|
||
| // Start the WebSocket server | ||
| startServer() | ||
|
|
||
| // Handle process messages if needed | ||
| process.on('message', (message) => { | ||
| // Handle any messages from the parent process | ||
| console.log('Received message:', message) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and proper logging for WebSocket server startup.
The WebSocket server lacks error handling and proper shutdown logic. Using console.log also bypasses the electron-log system used elsewhere in the application.
import { startServer } from './server'
+import log from 'electron-log/node.js'
// Start the WebSocket server
-startServer()
+try {
+ startServer()
+ log.info('WebSocket server started successfully')
+} catch (error) {
+ log.error('Failed to start WebSocket server:', error)
+}
// Handle process messages if needed
process.on('message', (message) => {
// Handle any messages from the parent process
- console.log('Received message:', message)
+ log.info('Received message from parent process:', message)
})
+// Handle graceful shutdown
+process.on('SIGINT', () => {
+ log.info('Shutting down WebSocket server...')
+ process.exit(0)
+})📝 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.
| import { startServer } from './server' | |
| // Start the WebSocket server | |
| startServer() | |
| // Handle process messages if needed | |
| process.on('message', (message) => { | |
| // Handle any messages from the parent process | |
| console.log('Received message:', message) | |
| }) | |
| import { startServer } from './server' | |
| import log from 'electron-log/node.js' | |
| // Start the WebSocket server | |
| try { | |
| startServer() | |
| log.info('WebSocket server started successfully') | |
| } catch (error) { | |
| log.error('Failed to start WebSocket server:', error) | |
| } | |
| // Handle process messages if needed | |
| process.on('message', (message) => { | |
| // Handle any messages from the parent process | |
| log.info('Received message from parent process:', message) | |
| }) | |
| // Handle graceful shutdown | |
| process.on('SIGINT', () => { | |
| log.info('Shutting down WebSocket server...') | |
| process.exit(0) | |
| }) |
| } | ||
| timeTrackers.push(tracker) | ||
| } | ||
| optiSync.sendUpdates(timeTrackers, getDeepWorkHours()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling around sync operations.
The sync operation might fail for various reasons (e.g., network issues, server unavailability), and currently there's no error handling.
- optiSync.sendUpdates(timeTrackers, getDeepWorkHours());
+ try {
+ optiSync.sendUpdates(timeTrackers, getDeepWorkHours());
+ } catch (error) {
+ log.error('Failed to sync updates:', error);
+ // Consider adding a retry mechanism or queueing system for failed sync attempts
+ }📝 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.
| optiSync.sendUpdates(timeTrackers, getDeepWorkHours()); | |
| try { | |
| optiSync.sendUpdates(timeTrackers, getDeepWorkHours()); | |
| } catch (error) { | |
| log.error('Failed to sync updates:', error); | |
| // Consider adding a retry mechanism or queueing system for failed sync attempts | |
| } |
| "world-countries": "^5.1.0", | ||
| "ws": "^8.18.1" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify security implications of WebSocket implementation.
While the WebSocket dependencies are appropriate, it's important to ensure that the implementation follows security best practices.
🌐 Web query:
What are the security best practices for WebSocket servers in Electron applications?
💡 Result:
Security Best Practices for WebSocket Servers in Electron Applications
Ensuring secure WebSocket communication in Electron applications requires careful consideration of both WebSocket-specific and Electron-specific security risks. Below are best practices derived from current, reputable sources.
Use Secure WebSocket Protocol (WSS)
- Always use
wss://(WebSocket Secure) instead ofws://to encrypt data in transit, mitigating risks such as man-in-the-middle attacks[1][2][4][7].
Input and Output Validation
- Validate all incoming data from clients before processing it to prevent injection attacks (such as SQL and OS command injection)[1][2][7].
- Equally important, validate server-sent data on the client side. Never assign received content directly to the DOM or evaluate it as code; instead, treat all data as untrusted and parse JSON using
JSON.parse()[1][2][7].
Authentication and Authorization
- Require authentication before establishing a WebSocket connection. Do not rely on the WebSocket protocol itself for this; instead, authenticate users before initiating the handshake (e.g., by passing a session token or using secure cookies)[4][5][7][9].
- Implement authorization controls to ensure that only permitted users can perform sensitive actions or access restricted data via WebSockets[1][5][7].
Origin and CSRF Protection
- Validate the
Originheader on the server to allow connections only from trusted domains, preventing cross-site WebSocket hijacking[4]. - Use session-specific tokens (such as CSRF tokens) that must be included in connection requests, making it harder for attackers to forge WebSocket requests[4].
Rate Limiting and Resource Controls
- Apply rate limiting to limit the number of requests or messages per user/IP per time window. This helps mitigate Denial of Service (DoS) and brute-force attacks[2][4][5][7].
- Restrict the number of concurrent connections and enforce reasonable message size limits to prevent resource exhaustion attacks[4][5].
Avoid Dangerous Tunneling
- Do not tunnel arbitrary, sensitive TCP services (such as databases) through WebSockets. This can escalate attacks, especially in case of cross-site scripting (XSS) vulnerabilities, enabling attackers to pivot through your application[1][2][7].
Enforce Secure Electron Configuration
- In Electron, always disable
nodeIntegrationand enablecontextIsolationin renderer processes that display remote or untrusted content to prevent attackers from executing native Node.js code via web content[8][10]. - Define a strict Content Security Policy (CSP) to restrict what scripts can run and what resources can be loaded in your app[8].
- Enable process sandboxing, do not disable web security, and make use of Electron’s permission request handlers for any session loading remote content[8][10].
Patch and Test Regularly
- Keep Electron, dependencies, and your server libraries up-to-date to protect against known vulnerabilities[8][10].
- Regularly test your WebSocket endpoints and Electron configuration for misconfigurations and vulnerabilities using static analysis and pentesting tools[10].
Summary Table: WebSocket Security Measures for Electron Apps
| Security Measure | WebSocket | Electron |
|---|---|---|
| Use Secure Protocol | wss:// only |
- |
| Input/Output Validation | Validate all data | - |
| Authentication/Authorization | Before connection | - |
| Origin Header/CSRF Tokens | Check Origin, use tokens |
- |
| Rate Limiting/Resource Controls | Yes | - |
| Avoid Dangerous Tunneling | Yes | - |
| Disable Node Integration in Renderer | - | Yes |
| Enable Context Isolation | - | Yes |
| Strict Content Security Policy | - | Yes |
| Keep Dependencies Updated | Yes | Yes |
Blockquote:
"Always use the WebSocket Secure (wss) protocol instead of WebSocket (ws) to ensure data exchanged between the client and server is encrypted...Only allow authenticated users to open a WebSocket connection...On the server side, validate the origin of incoming WebSocket connections. Only allow connections from trusted domains by checking the Origin header. Reject connections from unexpected or unauthorized origins." [4]
Implementing these practices will help ensure the security of your WebSocket server within an Electron application, safeguarding your users and data against a wide range of common and advanced threats.
Citations:
- 1: https://devcenter.heroku.com/articles/websocket-security
- 2: https://www.brightsec.com/blog/websocket-security-top-vulnerabilities/
- 3: https://www.vaadata.com/blog/how-websockets-work-vulnerabilities-and-security-best-practices/
- 4: https://ably.com/topic/websocket-security
- 5: https://www.freecodecamp.org/news/how-to-secure-your-websocket-connections-d0be0996c556/
- 6: https://support.tmssoftware.com/t/strange-error-with-websockets-in-electron-app/13873
- 7: https://www.wallarm.com/what/websocket-security
- 8: https://electronjs.org/docs/latest/tutorial/security
- 9: https://ably.com/blog/websocket-authentication
- 10: https://www.cobalt.io/blog/common-misconfigurations-electron-apps-part-1
Review WebSocket security implementation
Please verify that your use of the ws dependency follows Electron and WebSocket security best practices:
- Use wss:// with a valid TLS certificate to encrypt all traffic.
- Authenticate clients before the WebSocket handshake (e.g., session tokens or secure cookies).
- Validate the Origin header against a trusted whitelist to prevent cross‐site hijacking.
- Sanitize and validate every incoming message; never execute or inject untrusted data.
- Enforce rate limits, maximum message sizes and connection limits to guard against DoS.
- In your Electron BrowserWindow: disable nodeIntegration and enable contextIsolation.
- Define a strict Content Security Policy for any loaded web content.
- Keep Electron, ws, and all related dependencies up‑to‑date and perform regular security audits.
Audit these locations:
- package.json (lines 85–86:
"ws": "^8.18.1") - Your WebSocket server setup in the main process (e.g.,
src/main.js) - Your client–side WebSocket handling in renderer scripts (e.g.,
src/renderer.js)
| const wss = new WebSocketServer({ port: 3001 }) | ||
| const API_BASE_URL = 'https://backend-production-5eec.up.railway.app' | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hard‑coding port & backend URL; read from env / shared config instead
Binding the server to port 3001 and duplicating the backend URL string couples runtime behaviour to the source and makes it difficult to run multiple instances, e2e tests or change environments (staging, dev, prod).
Reuse the existing API_BASE_URL from renderer/src/config.ts (or move it to a common config.ts) and read the port from process.env.PORT with a sensible default.
-const wss = new WebSocketServer({ port: 3001 })
-const API_BASE_URL = 'https://backend-production-5eec.up.railway.app'
+const PORT = Number(process.env.OPTI_SYNC_PORT ?? 3001)
+const wss = new WebSocketServer({ port: PORT })
+import { API_BASE_URL } from './config' // <- shared constantCommittable suggestion skipped: line range outside the PR's diff.
| date: dayjs().format('dddd'), | ||
| url: t.url.slice(0, 200), | ||
| title: t.title.slice(0, 100), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
date & today should be an ISO calendar date, not just weekday
dayjs().format('dddd') returns strings like "Wednesday".
This loses year/week context and will overwrite previous weeks’ data. Prefer an ISO date (YYYY‑MM‑DD) and let the backend derive weekday if necessary.
- date: dayjs().format('dddd'),
+ date: dayjs().format('YYYY-MM-DD'),
...
- today: dayjs().format('dddd'),
+ today: dayjs().format('YYYY-MM-DD'),📝 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.
| date: dayjs().format('dddd'), | |
| url: t.url.slice(0, 200), | |
| title: t.title.slice(0, 100), | |
| date: dayjs().format('YYYY-MM-DD'), | |
| url: t.url.slice(0, 200), | |
| title: t.title.slice(0, 100), |
| dailyData: trackers.map((t: any) => ({ | ||
| username: 'user', // Updated dynamically in main.ts | ||
| date: dayjs().format('dddd'), | ||
| url: t.url.slice(0, 200), | ||
| title: t.title.slice(0, 100), | ||
| timeSpent: t.timeSpent | ||
| })), | ||
| deepWorkHours, | ||
| today: dayjs().format('dddd'), | ||
| username: 'user' | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Do not hard‑code "user"; pass the real username from the client
The payload always transmits username: 'user', so data for every account will be merged under the same identifier on the server.
Either include username inside the SYNC_UPDATE message or fetch it from a secure process‑wide store and inject it here.
-username: 'user'
+username: msg.data.username // or another reliable sourceI've started inspecting src/server.ts to confirm how usernames are handled. Fetching the relevant lines…
🏁 Script executed:
#!/bin/bash
# Locate the mapping that hard-codes "user" and inspect surrounding context
rg -n "dailyData" -R src
sed -n '1,200p' src/server.ts | nl -baLength of output: 561
🏁 Script executed:
#!/bin/bash
# Locate server.ts in the repository
fd server.ts
# Show occurrences of hard‑coded username in src/
rg -n "username: 'user'" -C3 src
# Inspect where SYNC_UPDATE messages are handled
rg -n "SYNC_UPDATE" -C3 src
# Check for any use of msg.data.username or similar
rg -n "data.username" -C3 srcLength of output: 1692
Pass the real username through the SYNC_UPDATE payload
The server currently hard‑codes username: 'user' in two places, causing all accounts’ data to be merged under the same identifier. Since neither msg.data.username nor a secure store is used, you must propagate the actual username from the client.
• In src/server.ts (around lines 23–33), replace both hard‑coded fields:
- username: 'user', // Updated dynamically in main.ts
+ username: msg.data.username
…
- username: 'user'
+ username: msg.data.username• In src/optiSync.ts (where SYNC_UPDATE is sent), include the username in the payload:
this.ws.send(
JSON.stringify({
type: 'SYNC_UPDATE',
- data: { trackers: deltaTrackers, deepWorkHours }
+ data: { trackers: deltaTrackers, deepWorkHours, username: this.username }
})
)Ensure that this.username (or another reliable source) is set on the client before invoking SYNC_UPDATE.
📝 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.
| dailyData: trackers.map((t: any) => ({ | |
| username: 'user', // Updated dynamically in main.ts | |
| date: dayjs().format('dddd'), | |
| url: t.url.slice(0, 200), | |
| title: t.title.slice(0, 100), | |
| timeSpent: t.timeSpent | |
| })), | |
| deepWorkHours, | |
| today: dayjs().format('dddd'), | |
| username: 'user' | |
| }) | |
| dailyData: trackers.map((t: any) => ({ | |
| username: msg.data.username, | |
| date: dayjs().format('dddd'), | |
| url: t.url.slice(0, 200), | |
| title: t.title.slice(0, 100), | |
| timeSpent: t.timeSpent | |
| })), | |
| deepWorkHours, | |
| today: dayjs().format('dddd'), | |
| username: msg.data.username | |
| }) |
| sendUpdates(trackers: SiteTimeTracker[], deepWorkHours: DeepWorkHours) { | ||
| if (!this.ws || this.ws.readyState !== WebSocket.OPEN) return | ||
|
|
||
| // Compute delta: new or updated trackers | ||
| const deltaTrackers = trackers.filter((t) => { | ||
| const last = this.lastSyncTrackers.find((lt) => lt.title === t.title && lt.url === t.url) | ||
| return ( | ||
| !last || | ||
| last.timeSpent !== t.timeSpent || | ||
| last.lastActiveTimestamp !== t.lastActiveTimestamp | ||
| ) | ||
| }) | ||
|
|
||
| if (deltaTrackers.length > 0) { | ||
| this.ws.send( | ||
| JSON.stringify({ | ||
| type: 'SYNC_UPDATE', | ||
| data: { trackers: deltaTrackers, deepWorkHours } | ||
| }) | ||
| ) | ||
| this.lastSyncTrackers = [...trackers] // Update last synced state | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Queue updates while offline to avoid data loss
sendUpdates silently returns when ws is not open, dropping activity data accumulated during downtime.
Maintain an in‑memory queue and flush on reconnect.
| import { WebSocket } from 'ws' | ||
| import { SiteTimeTracker, DeepWorkHours } from './types' | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the global WebSocket in renderer instead of the ws package
The ws library is typically server‑side; bundling it into the renderer adds ~150 KB and native addons for optional deps.
Electron’s renderer & main contexts already provide the WHATWG/WebSocket implementation.
-import { WebSocket } from 'ws'
+// eslint-disable-next-line no-restricted-globals
+const WebSocketImpl: typeof WebSocket =
+ typeof WebSocket !== 'undefined' ? WebSocket : (await import('ws')).WebSocketCommittable suggestion skipped: line range outside the PR's diff.
| if (!persistenceInterval && !isSystemSuspended) { | ||
| persistenceInterval = setInterval( | ||
| async () => { | ||
| const today = dayjs().format('dddd') as keyof DeepWorkHours; // "Wednesday" | ||
| const username = user.username; | ||
| const deepWorkHours = getDeepWorkHours(); | ||
| const MIN_TIME_THRESHOLD = 10; | ||
| const today = dayjs().format('dddd') as keyof DeepWorkHours // "Wednesday" | ||
| const username = user.username | ||
| const deepWorkHours = getDeepWorkHours() | ||
| const MIN_TIME_THRESHOLD = 10 | ||
|
|
||
| const filteredTrackers = currentSiteTimeTrackers.filter( |
There was a problem hiding this comment.
Null‑safety: user may be null when accessing user.username
startPersistenceInterval runs on app.on('ready'), before the user necessarily logged in.
Accessing user.username throws. Guard or early‑return until a user is available.
- const username = user.username
+ if (!user) return
+ const username = user.username📝 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.
| if (!persistenceInterval && !isSystemSuspended) { | |
| persistenceInterval = setInterval( | |
| async () => { | |
| const today = dayjs().format('dddd') as keyof DeepWorkHours; // "Wednesday" | |
| const username = user.username; | |
| const deepWorkHours = getDeepWorkHours(); | |
| const MIN_TIME_THRESHOLD = 10; | |
| const today = dayjs().format('dddd') as keyof DeepWorkHours // "Wednesday" | |
| const username = user.username | |
| const deepWorkHours = getDeepWorkHours() | |
| const MIN_TIME_THRESHOLD = 10 | |
| const filteredTrackers = currentSiteTimeTrackers.filter( | |
| if (!persistenceInterval && !isSystemSuspended) { | |
| persistenceInterval = setInterval( | |
| async () => { | |
| const today = dayjs().format('dddd') as keyof DeepWorkHours // "Wednesday" | |
| if (!user) return | |
| const username = user.username | |
| const deepWorkHours = getDeepWorkHours() | |
| const MIN_TIME_THRESHOLD = 10 | |
| const filteredTrackers = currentSiteTimeTrackers.filter( |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
vite.main.config.ts (1)
1-51: 💡 Verification agent🧩 Analysis chain
This file appears to be obsolete and should be removed
The
forge.config.tsfile now referencesvite.main.config.mtsinstead of this file. Both files contain nearly identical configurations, with this one having more inline comments. To prevent confusion and maintenance issues, consider removing this file since it's being replaced by the .mts version.This file should be removed as it's been replaced by
vite.main.config.mts.
🏁 Script executed:
#!/bin/bash # Check if both files exist and compare their content echo "Checking if both configuration files exist..." if [[ -f "vite.main.config.ts" && -f "vite.main.config.mts" ]]; then echo "Both files exist. Comparing content..." # Compare content ignoring comments and whitespace diff -w <(grep -v "\/\/" vite.main.config.ts) <(grep -v "\/\/" vite.main.config.mts) echo "These files have similar functionality but forge.config.ts now references vite.main.config.mts" else echo "One or both files don't exist." fi # Check for references to these files in the codebase echo "Checking for references to vite.main.config.ts..." grep -r "vite.main.config.ts" --include="*.{ts,js,json}" . || echo "No references found" echo "Checking for references to vite.main.config.mts..." grep -r "vite.main.config.mts" --include="*.{ts,js,json}" . || echo "No references found"Length of output: 1083
Continuing verification to confirm file duplication and references:
🏁 Script executed:
#!/bin/bash set -e echo "1) Listing vite.main.config files:" fd -e ts -e mts vite.main.config || find . -maxdepth 2 -type f \( -name "vite.main.config.ts" -o -name "vite.main.config.mts" \) echo echo "2) Comparing content (ignoring comments/blanks):" grep -v '^\s*//' vite.main.config.ts | grep -v '^\s*$' > /tmp/config_ts.clean grep -v '^\s*//' vite.main.config.mts | grep -v '^\s*$' > /tmp/config_mts.clean if diff -w /tmp/config_ts.clean /tmp/config_mts.clean; then echo "→ No differences between configs" else echo "→ Detected differences" fi echo echo "3) Searching for references to both files:" rg "vite\.main\.config\.(ts|mts)" -g "*.{ts,mts,js,json}" || echo "→ No references found" echo echo "4) Inspecting forge.config.ts for any implicit references:" if [[ -f "forge.config.ts" ]]; then sed -n '1,200p' forge.config.ts else echo "→ forge.config.ts not found" fiLength of output: 4774
Remove duplicate config and update build scripts
vite.main.config.ts and vite.main.config.mts provide identical behavior (the only differences are inline comments), but your
forge.config.tsand your package.json build scripts point to different files. To clean this up:• In package.json (under the “build” script), replace references to
vite.main.config.tswithvite.main.config.mts."build": "vite build --config vite.main.config.ts && \ vite build --config vite.renderer.config.mts && \ vite build --config vite.preload.config.ts",should become:
"build": "vite build --config vite.main.config.mts && \ vite build --config vite.renderer.config.mts && \ vite build --config vite.preload.config.ts",• Once the build scripts are updated, remove
vite.main.config.tsfrom the repo to avoid confusion and maintenance overhead.
🧹 Nitpick comments (6)
vite.main.config.mts (1)
6-50: Well-structured configuration with good separation of concernsThe configuration properly sets up multiple entry points (main.ts and wsServer.ts) with appropriate externals for Node.js modules and WebSocket dependencies. The Node 20 target aligns well with modern Electron versions.
Consider the following enhancements:
- Add source maps for development builds to improve debugging
- Consider conditional emptyOutDir based on environment to avoid stale files
- Add more descriptive comments for excluded dependencies
export default defineConfig({ build: { target: 'node20', emptyOutDir: false, outDir: '.vite/build', + sourcemap: process.env.NODE_ENV === 'development' ? 'inline' : false, lib: { entry: { main: 'src/main.ts', wsServer: 'src/wsServer.ts' }, formats: ['cjs'] },vite.preload.config.ts (1)
4-10: Appropriate preload configuration addedGood addition of explicit configuration for the preload script, aligning with the Node 20 target used in other configurations.
Consider these optional enhancements:
- Explicitly specify the output format (e.g., 'cjs')
- Add external dependencies if the preload script uses any
- Consider source maps for development builds
export default defineConfig({ build: { target: 'node20', outDir: '.vite/build/preload', - emptyOutDir: false + emptyOutDir: false, + sourcemap: process.env.NODE_ENV === 'development' ? 'inline' : false, + lib: { + formats: ['cjs'] + } } })src/renderer/src/Login.tsx (2)
34-42: Remove debug logging code before production release.These lifecycle hooks are being used to log the Electron API object for debugging purposes. While useful during development, this verbose logging should be removed before shipping to production.
- onMount(() => { - console.log("window.electron:", window.electron); - console.log("Available methods:", Object.keys(window.electron || {})); - }); - - // Alternatively, you can use createEffect if you need reactivity - createEffect(() => { - console.log("window.electron (from createEffect):", window.electron); - });
66-66: Remove debug logging code before production release.This additional logging of the user object should be removed before shipping to production to avoid exposing sensitive user information in console logs.
- console.log('User is ', user)src/preload.ts (1)
40-46: Enhanced window.electron API with sendUserToBackend method.The exposed Electron API has been improved by adding a dedicated method for sending user data to the main process via IPC, which simplifies the renderer code and centralizes IPC communication logic.
However, consider adding type safety to the
userparameter:- sendUserToBackend: (user: any) => { + sendUserToBackend: (user: { id: string; username: string; [key: string]: any }) => {src/renderer/src/lib/utils.ts (1)
17-22: Good practice keeping old implementation as reference.Keeping the old implementation as a comment can help during the transition but should be removed once the new implementation is fully tested and stable.
- // export const sendUserToBackend = (user: User) => { - // console.log('Sending user data to backend:', user) - // const sanitizedUser = JSON.parse(JSON.stringify(user)) - // console.log('window is ', window) - // window?.electron.ipcRenderer.send('login-user', sanitizedUser) - // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
forge.config.ts(3 hunks)package.json(4 hunks)src/main.ts(9 hunks)src/preload.ts(3 hunks)src/renderer/src/Login.tsx(3 hunks)src/renderer/src/lib/utils.ts(2 hunks)src/server.ts(1 hunks)src/wsServer.ts(1 hunks)vite.main.config.mts(1 hunks)vite.main.config.ts(1 hunks)vite.preload.config.mts(1 hunks)vite.preload.config.ts(1 hunks)vite.renderer.config.mts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- vite.renderer.config.mts
- vite.preload.config.mts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/wsServer.ts
- src/server.ts
- package.json
- src/main.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/renderer/src/lib/utils.ts (1)
src/preload.ts (1)
ElectronAPI(6-21)
🔇 Additional comments (8)
forge.config.ts (4)
8-8: Good addition of AutoUnpackNativesPluginAdding this plugin is essential for handling native modules like 'ws', 'bufferutil', and 'utf-8-validate' that are used for WebSocket functionality.
62-63: Appropriate migration to .mts file extensionsUpdating the config file extensions from .ts to .mts reflects the proper use of ES modules in TypeScript, which aligns with modern JavaScript practices.
Also applies to: 67-68
19-20: Minor whitespace fix in osxSign.identityGood cleanup of extra whitespace.
74-75: Config file extension update for rendererConsistent update of the renderer config file extension.
src/renderer/src/Login.tsx (1)
2-2: Import statement updated to include lifecycle hooks.The import statement has been expanded to include
onMountandcreateEffectlifecycle hooks from 'solid-js', which will be used for debugging the Electron API exposure.src/preload.ts (1)
3-3: Import updated to include ipcRenderer.The import statement now includes
ipcRendererwhich is necessary for IPC communication between renderer and main processes.src/renderer/src/lib/utils.ts (2)
5-11: Improved type safety with global Window interface extension.Adding proper TypeScript type definitions for the
window.electronproperty improves type safety and developer experience by providing autocomplete and type checking for Electron API usage throughout the renderer code.
27-27: Updated to use the new preload-exposed API method.The implementation now correctly uses the dedicated
sendUserToBackendmethod exposed by the preload script instead of directly usingipcRenderer.send, which follows Electron's recommended security practices for context isolation.
| export const stopActivityMonitoring = () => { | ||
| console.log('Stopping activity monitoring') | ||
| window?.electron.ipcRenderer.send('logout-user') | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Inconsistent IPC pattern for stopActivityMonitoring.
While sendUserToBackend has been updated to use the new preload-exposed method, stopActivityMonitoring still uses the direct ipcRenderer.send approach. For consistency, this should also be updated to use a dedicated method exposed through the preload script.
export const stopActivityMonitoring = () => {
console.log('Stopping activity monitoring')
- window?.electron.ipcRenderer.send('logout-user')
+ window?.electron.logoutUser() // This method needs to be added to preload.ts
}🏁 Script executed:
#!/bin/bash
# Check for direct ipcRenderer usage across the renderer code
echo "Checking for direct ipcRenderer usage in renderer code:"
rg "ipcRenderer\.send" src/renderer/src/Length of output: 2097
🏁 Script executed:
#!/bin/bash
# Display contents of utils.ts to verify sendUserToBackend implementation
sed -n '1,200p' src/renderer/src/lib/utils.tsLength of output: 1537
🏁 Script executed:
#!/bin/bash
# Locate the ElectronAPI interface and check for sendUserToBackend/logoutUser methods
echo "🔍 Inspecting ElectronAPI in preload:"
rg -n "interface ElectronAPI" -A 20
echo -e "\n🔍 Checking for sendUserToBackend in preload:"
rg -n "sendUserToBackend" -A 5
echo -e "\n🔍 Checking for logoutUser in preload:"
rg -n "logoutUser" -A 5Length of output: 5850
Standardize IPC for stopping activity monitoring
We still have a direct ipcRenderer.send('logout-user') call in stopActivityMonitoring, whereas sendUserToBackend now uses a preload‐exposed API. Let’s align these patterns:
• src/preload.ts
– In your api object (and ElectronAPI interface), add a logoutUser method:
logoutUser: (): void => {
console.log('Preload: Logging out user')
ipcRenderer.send('logout-user')
},Ensure logoutUser is declared on ElectronAPI and exposed via contextBridge.exposeInMainWorld('electron', api).
• src/renderer/src/lib/utils.ts
– Update stopActivityMonitoring to use the new API:
export const stopActivityMonitoring = () => {
console.log('Stopping activity monitoring')
- window?.electron.ipcRenderer.send('logout-user')
+ window?.electron.logoutUser()
}This keeps all renderer‐to‐main communications encapsulated in the preload layer.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
48-55: Enhance Demo section with example logs
Right now the demo line is very brief. Consider including a short log snippet or screenshot so users can verify that OptiSync is running as expected. For example:#### Demo ```bash 2025-04-15T10:32:20.123Z [optiSync] Connected to ws://localhost:3001 2025-04-15T10:32:20.456Z [optiSync] Sent delta update: {"site":"github.com","duration":120}This makes the sync feature more tangible to newcomers. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between b82c0b8ab1f8722ee3f824a947199c06ad33ccfd and 6569914356009ee316ad5595776ae5aaac43e7b5. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `README.md` (1 hunks) </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| # deepFocus with OptiSync | ||
| Electron app with WebSocket sync for productivity tracking. | ||
| ## Setup | ||
| 1. `pnpm install` | ||
| 2. `pnpm run start` | ||
| ## Demo | ||
| Logs show WebSocket sync of SiteTimeTracker updates. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent heading level, branding, and missing Table of Contents entry
The new section is introduced as a top‐level H1 (# deepFocus with OptiSync) with lowercase “d” and isn’t linked in the existing Table of Contents. To maintain hierarchy and branding consistency, and to ensure discoverability, please:
- Rename to a level‑3 heading under “Getting Started” (e.g.
### DeepFocus with OptiSync), matching the project’sDeepFocuscasing. - Adjust sub‑headings to fit (e.g.
#### Setup,#### Demo). - Add a corresponding entry to the TOC.
Proposed diff:
@@ -16,6 +16,7 @@
- [Getting Started](#getting-started)
+- [DeepFocus with OptiSync](#deepfocus-with-optisync)
## Getting Started
@@ -48,8 +49,9 @@
-# deepFocus with OptiSync
-Electron app with WebSocket sync for productivity tracking.
+### DeepFocus with OptiSync
+Electron app with WebSocket synchronization for productivity tracking.
-## Setup
-1. `pnpm install`
-2. `pnpm run start`
+#### Setup
+```bash
+pnpm install
+pnpm run start
+```
-## Demo
-Logs show WebSocket sync of SiteTimeTracker updates.
+#### Demo
+Logs show WebSocket synchronization of SiteTimeTracker updates.1 participant
Description
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).Summary by CodeRabbit
New Features
Improvements
Developer Experience