Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a4cb18fcf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR focuses on expanding Vitest coverage around core logic, storage, and Google Fit integration while also tightening the project’s Node/tooling configuration (Node version, pnpm/lockfile updates, and a Rollup override).
Changes:
- Added/expanded unit tests for
StorageService, core timer logic edge cases, andGoogleFitServicebehaviors (token expiry, init/connect, error paths). - Updated Node/tooling configuration via
package.jsonengines and pnpm lockfile changes (including a Rollup override). - Added an
.nvmrcto guide local Node selection.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ts/storage.test.ts | Adds settings persistence tests (but currently introduces an order-dependent/flaky test). |
| src/ts/logic.test.ts | Adds edge-case tests for getCurrentRound and getCountdownBeep. |
| src/ts/google-fit.test.ts | Adds constructor/init/connect/error-path coverage and reduces noisy stderr in some tests. |
| pnpm-workspace.yaml | Introduces a Rollup override configuration. |
| pnpm-lock.yaml | Updates lockfile to reflect overrides and dependency changes. |
| package.json | Adds Node engine requirement and modifies dependencies. |
| .nvmrc | Introduces Node major version pinning for local dev. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 8 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2bbe10d47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const newService = new GoogleFitService(); | ||
| expect(newService.accessToken).toBeNull(); | ||
| expect(localStorage.getItem('google_fit_token')).toBeNull(); | ||
| logSpy.mockRestore(); |
There was a problem hiding this comment.
logSpy.mockRestore() (and similar console spies later in this file) is called after assertions; if an assertion throws, the spy won't be restored and can leak into subsequent tests. Prefer wrapping the spy usage in try/finally, or add an afterEach that calls vi.restoreAllMocks() so the original console methods are always restored.
| const newService = new GoogleFitService(); | |
| expect(newService.accessToken).toBeNull(); | |
| expect(localStorage.getItem('google_fit_token')).toBeNull(); | |
| logSpy.mockRestore(); | |
| try { | |
| const newService = new GoogleFitService(); | |
| expect(newService.accessToken).toBeNull(); | |
| expect(localStorage.getItem('google_fit_token')).toBeNull(); | |
| } finally { | |
| logSpy.mockRestore(); | |
| } |
| "packageManager": "pnpm@10.30.3", | ||
| "pnpm": { | ||
| "overrides": { | ||
| "rollup@>=4.0.0 <4.59.0": ">=4.59.0" |
There was a problem hiding this comment.
The pnpm override value ">=4.59.0" is open-ended, which can make installs non-reproducible (it may float to newer major versions) and can unexpectedly change behavior in CI/local dev. Consider pinning to a specific version (e.g., 4.59.0) or a bounded range (e.g., ^4.59.0) depending on your upgrade policy.
| "rollup@>=4.0.0 <4.59.0": ">=4.59.0" | |
| "rollup@>=4.0.0 <4.59.0": "^4.59.0" |
| await new Promise((resolve, reject) => { | ||
| request.onsuccess = resolve; | ||
| request.onerror = reject; |
There was a problem hiding this comment.
In this Promise wrapper around indexedDB.deleteDatabase, request.onerror = reject will reject with the raw event rather than the underlying request.error, which makes failures harder to diagnose. Consider rejecting with request.error (or a new Error that includes request.error?.message) so test failures are actionable.
| await new Promise((resolve, reject) => { | |
| request.onsuccess = resolve; | |
| request.onerror = reject; | |
| await new Promise<void>((resolve, reject) => { | |
| request.onsuccess = () => { | |
| resolve(); | |
| }; | |
| request.onerror = () => { | |
| const error = request.error; | |
| if (error) { | |
| reject(error); | |
| } else { | |
| reject(new Error(`IndexedDB deleteDatabase("${DB_NAME}") failed for an unknown reason.`)); | |
| } | |
| }; |
Silenced noisy test output and increased overall coverage to 90.5%. Set up .nvmrc for Node v24 LTS latest (Krypton) and configured Git auto-upstream.