Conversation
* Apply suggestion from @coderabbitai[bot] * Update apps/desktop/src/index.ts Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * Update .scripts/electron-desktop-environment/concrete-environment-content/desktop-environment-content.ts Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> --------- Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Bumps [locutus](https://github.com/locutusjs/locutus) from 3.0.10 to 3.0.14. - [Release notes](https://github.com/locutusjs/locutus/releases) - [Changelog](https://github.com/locutusjs/locutus/blob/main/CHANGELOG.md) - [Commits](locutusjs/locutus@v3.0.10...v3.0.14) --- updated-dependencies: - dependency-name: locutus dependency-version: 3.0.14 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [undici](https://github.com/nodejs/undici) from 6.23.0 to 6.24.0. - [Release notes](https://github.com/nodejs/undici/releases) - [Commits](nodejs/undici@v6.23.0...v6.24.0) --- updated-dependencies: - dependency-name: undici dependency-version: 6.24.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
* fix: desktop timer stopped offline * fix: desktop timer offline handle * fix: remove logs * fix: change timelogid source
Bumps [undici](https://github.com/nodejs/undici) from 6.23.0 to 6.24.0. - [Release notes](https://github.com/nodejs/undici/releases) - [Commits](nodejs/undici@v6.23.0...v6.24.0) --- updated-dependencies: - dependency-name: undici dependency-version: 6.24.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…-3.0.14 chore(deps): bump locutus from 3.0.10 to 3.0.14
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
|
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/desktop-ui-lib/src/lib/time-tracker/time-tracker.service.ts">
<violation number="1" location="packages/desktop-ui-lib/src/lib/time-tracker/time-tracker.service.ts:585">
P2: The `try/catch` around `firstValueFrom(...)` is ineffective because `firstValueFrom` returns a Promise rather than throwing synchronously. The detailed error logging (with `requestBody`) in the `catch` block will never execute. Either make the callback `async` and `await` the result, or remove the redundant try/catch since `_serialized` already catches and logs errors.</violation>
</file>
<file name="apps/desktop/src/index.ts">
<violation number="1" location="apps/desktop/src/index.ts:351">
P1: This guard breaks local startup for configs that rely on existing environment/additional-config JWT secrets instead of `setupConfig.secret`. Fall back to the current `process.env` values before throwing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (!setupConfig.secret || !setupConfig.secret.jwt || !setupConfig.secret.refresh_token) { | ||
| throw new AppError('MAINSTRSERVER', new Error('JWT secrets are required for local server startup')); | ||
| } | ||
| process.env.JWT_SECRET = setupConfig.secret.jwt; | ||
| process.env.JWT_REFRESH_TOKEN_SECRET = setupConfig.secret.refresh_token; |
There was a problem hiding this comment.
P1: This guard breaks local startup for configs that rely on existing environment/additional-config JWT secrets instead of setupConfig.secret. Fall back to the current process.env values before throwing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/index.ts, line 351:
<comment>This guard breaks local startup for configs that rely on existing environment/additional-config JWT secrets instead of `setupConfig.secret`. Fall back to the current `process.env` values before throwing.</comment>
<file context>
@@ -347,8 +347,12 @@ async function startServer(setupConfig: DesktopSetupConfig, restart = false) {
- process.env.JWT_SECRET = setupConfig.secret?.jwt;
- process.env.JWT_REFRESH_TOKEN_SECRET = setupConfig.secret?.refresh_token;
+
+ if (!setupConfig.secret || !setupConfig.secret.jwt || !setupConfig.secret.refresh_token) {
+ throw new AppError('MAINSTRSERVER', new Error('JWT secrets are required for local server startup'));
+ }
</file context>
| if (!setupConfig.secret || !setupConfig.secret.jwt || !setupConfig.secret.refresh_token) { | |
| throw new AppError('MAINSTRSERVER', new Error('JWT secrets are required for local server startup')); | |
| } | |
| process.env.JWT_SECRET = setupConfig.secret.jwt; | |
| process.env.JWT_REFRESH_TOKEN_SECRET = setupConfig.secret.refresh_token; | |
| const jwtSecret = setupConfig.secret?.jwt || process.env.JWT_SECRET; | |
| const jwtRefreshTokenSecret = setupConfig.secret?.refresh_token || process.env.JWT_REFRESH_TOKEN_SECRET; | |
| if (!jwtSecret || !jwtRefreshTokenSecret) { | |
| throw new AppError('MAINSTRSERVER', new Error('JWT secrets are required for local server startup')); | |
| } | |
| process.env.JWT_SECRET = jwtSecret; | |
| process.env.JWT_REFRESH_TOKEN_SECRET = jwtRefreshTokenSecret; |
| this._loggerService.error<any>(`Error stopping timer: ${moment().format()}`, { error, requestBody: body }); | ||
| throw error; | ||
| } | ||
| return this._serialized('toggleApiStop', () => { |
There was a problem hiding this comment.
P2: The try/catch around firstValueFrom(...) is ineffective because firstValueFrom returns a Promise rather than throwing synchronously. The detailed error logging (with requestBody) in the catch block will never execute. Either make the callback async and await the result, or remove the redundant try/catch since _serialized already catches and logs errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-ui-lib/src/lib/time-tracker/time-tracker.service.ts, line 585:
<comment>The `try/catch` around `firstValueFrom(...)` is ineffective because `firstValueFrom` returns a Promise rather than throwing synchronously. The detailed error logging (with `requestBody`) in the `catch` block will never execute. Either make the callback `async` and `await` the result, or remove the redundant try/catch since `_serialized` already catches and logs errors.</comment>
<file context>
@@ -516,26 +582,38 @@ export class TimeTrackerService {
- this._loggerService.error<any>(`Error stopping timer: ${moment().format()}`, { error, requestBody: body });
- throw error;
- }
+ return this._serialized('toggleApiStop', () => {
+ try {
+ return firstValueFrom(this.http.post<ITimeLog>(API_URL, body, options));
</file context>
Greptile SummaryThis PR merges multiple fixes targeting the desktop app's offline-sync reliability and environment code-generation security. The core changes are: (1) a promise-chain mutex in
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant SQ as SequenceQueue
participant TTS as TimeTrackerService
participant Mutex as _timerMutex
participant API as Server API
participant IPC as Electron IPC
Note over SQ: Offline sync begins
SQ->>TTS: toggleApiStart(timer)
TTS->>Mutex: _serialized('toggleApiStart', fn)
Mutex->>API: POST /timer/start
API-->>Mutex: TimeLog response
Mutex-->>TTS: result
TTS-->>SQ: latest
SQ->>SQ: new TimeSlotQueue(timeLogId)
SQ->>SQ: Process time slot intervals
SQ->>TTS: toggleApiStop(timer) or updateTimeLog()
TTS->>Mutex: _serialized('toggleApiStop', fn)
Note over Mutex: Waits for previous op to complete
Mutex->>API: POST /timer/stop
API-->>Mutex: TimeLog response
Mutex-->>TTS: result
TTS-->>SQ: latest
SQ->>IPC: await UPDATE_SYNCED_TIMER
Note over SQ: Direct await (no asapScheduler)
IPC-->>SQ: DB updated
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return this._serialized('toggleApiStop', () => { | ||
| try { | ||
| return firstValueFrom(this.http.post<ITimeLog>(API_URL, body, options)); | ||
| } catch (error) { | ||
| this._loggerService.error<any>(`Error stopping timer: ${moment().format()}`, { error, requestBody: body }); | ||
| throw error; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Dead try/catch — never catches async rejections
The try/catch block wraps firstValueFrom(this.http.post(...)), which returns a Promise. Synchronous try/catch cannot catch rejected Promises — the catch block will never execute. The error logging on line 589 is unreachable code.
This is a pre-existing issue (the old code had the same pattern), but now that the call is wrapped in _serialized — which does properly await the result — this inner try/catch is pure dead code. Consider either removing it or converting the lambda to async and adding await:
| return this._serialized('toggleApiStop', () => { | |
| try { | |
| return firstValueFrom(this.http.post<ITimeLog>(API_URL, body, options)); | |
| } catch (error) { | |
| this._loggerService.error<any>(`Error stopping timer: ${moment().format()}`, { error, requestBody: body }); | |
| throw error; | |
| } | |
| }); | |
| return this._serialized('toggleApiStop', async () => { | |
| try { | |
| return await firstValueFrom(this.http.post<ITimeLog>(API_URL, body, options)); | |
| } catch (error) { | |
| this._loggerService.error<any>(`Error stopping timer: ${moment().format()}`, { error, requestBody: body }); | |
| throw error; | |
| } | |
| }); |
|
View your CI Pipeline Execution ↗ for commit 1c6889b
☁️ Nx Cloud last updated this comment at |
3 participants
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by cubic
Fixes desktop timer offline sync issues by serializing timer API calls and enforcing ordered local updates, preventing race conditions and corrupted time logs. Also hardens local server startup and escapes JWT secrets in generated env files.
Bug Fixes
toggleApiStart,toggleApiStop,updateTimeLog, andaddTimeLogvia a mutex with a per-op timeout to prevent overlapping requests and cascade deletions during offline/online transitions.asapSchedulerupdate with awaited IPC update in sequence queue to preserve write order; passtimeLogIdtoTimeSlotQueueso timeslots link to the correct log.updateTimeLog/addTimeLogwhenorganizationId/tenantIdare missing; support optional fields and includeorganizationContactId.escapeForSingleQuoteto handle backslashes, quotes, and newlines.Dependencies
undicito 6.24.0 andlocutusto 3.0.14; updatecaniuse-liteandnode-abiinyarn.lock.Written for commit 1c6889b. Summary will update on new commits.