[WIP-#111] chore(deps): Major dependency upgrades (5/6 completed) #99
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a dependency upgrade strategy document, updates Node/Apollo-related CI workflows, bumps frontend Apollo client, refactors ExternalApiService into modular extraction/transform builders, changes progress endpoint defaults to env var, and converts task status representations in OpenAPI from numeric to string enums while inlining a team progress response. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend as Frontend (caller)
participant ExtAPI as ExternalApiService
participant HTTP as HTTP Fetch
participant Builders as Migration Builders
Frontend->>ExtAPI: fetchDataWithApiToken(token, oldDomain?)
note right of ExtAPI #e6f7ff: determine endpoint (env or default)
ExtAPI->>HTTP: GET {oldDomain}/v1/progress with token
HTTP-->>ExtAPI: response (raw JSON)
ExtAPI->>ExtAPI: extractApiData(raw JSON) → normalized shape
ExtAPI->>Builders: createMigrationData(normalized)
Builders-->>ExtAPI: ProgressData (taskCompletions, hideoutParts, ...)
ExtAPI-->>Frontend: ProgressData | null on error
note right of ExtAPI #fff4e6: on error → log status/statusText and truncated body
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
coderabbitai
bot
added
type:dependencies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
DEPENDENCY_UPGRADE_STRATEGY.md(1 hunks)frontend/package.json(1 hunks)frontend/src/plugins/apollo.ts(1 hunks)functions/package.json(1 hunks)package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
frontend/src/**/*.{vue,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Vue 3 Composition API syntax in frontend code
Files:
frontend/src/plugins/apollo.ts
frontend/src/**/*.{ts,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
frontend/src/**/*.{ts,vue}: Use absolute imports with @/ prefix in frontend
Group imports: framework/Vue first, then local imports
Remove unused imports regularly
Files:
frontend/src/plugins/apollo.ts
{frontend,functions}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
{frontend,functions}/**/*.ts: Avoid any types; prefer proper interfaces
Use proper null checking for optional values
Files:
frontend/src/plugins/apollo.ts
🧠 Learnings (1)
📚 Learning: 2025-10-17T08:18:39.260Z
Learnt from: CR
PR: tarkovtracker-org/TarkovTracker#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-17T08:18:39.260Z
Learning: Have Firebase CLI installed as dev dependency (firebase-tools)
Applied to files:
package.json
🪛 LanguageTool
DEPENDENCY_UPGRADE_STRATEGY.md
[style] ~7-~7: Consider removing “of” to be more concise
Context: ... ## ✅ Completed Upgrades (2025-02-15) All of the following upgrades have been successful...
(ALL_OF_THE)
🔇 Additional comments (7)
functions/package.json (1)
28-28: Minor version bumps look good.The graphql-request and eslint upgrades are minor/patch versions and should be safe.
Also applies to: 36-36
frontend/src/plugins/apollo.ts (1)
1-10: Excellent refactor to avoid React-specific dependencies.The switch from
createHttpLinktoHttpLinkconstructor with@apollo/client/coreimports successfully eliminates React/rehackt dependencies while maintaining identical functionality. This aligns with the documented strategy and the ~14KB bundle size reduction mentioned in the PR.package.json (1)
31-38: DevDependency upgrades align with the strategy.All version bumps are patch/minor updates. The Firebase upgrade to 12.4.0 correctly aligns with the frontend workspace upgrade documented in the strategy.
frontend/package.json (3)
33-33: Major version upgrades successfully implemented.The Firebase 12.x, uuid 13.x, @intlify/unplugin-vue-i18n 11.x, and jsdom 27.x upgrades are significant version jumps that align with the documented strategy. The fact that 192 tests pass and the dev server runs successfully provides strong validation.
Also applies to: 41-41, 50-50, 63-63
40-40: Based on the search results, I've identified a critical discrepancy. Let me verify one more important detail about the version requirements:Unnecessary explicit dependency—throttle-debounce should be a transitive dependency of @vue/apollo-composable.
@vue/apollo-composable uses throttle-debounce internally (imports
debounceandthrottlefrom 'throttle-debounce' in useQuery.ts), but the rg search found no direct imports of throttle-debounce in frontend code. The npm page shows throttle-debounce as one of @vue/apollo-composable's direct dependencies, which means it should be installed transitively when @vue/apollo-composable is installed.Verify whether frontend code directly uses throttle-debounce, or if adding it to package.json is unnecessary. If it's only needed by apollo-composable, remove it from frontend/package.json and let it be resolved as a transitive dependency.
41-41: Now let me verify uuid 13.x ESM requirements:No changes required—uuid 13.x ESM compatibility is correct.
Starting with uuid@12, CommonJS is no longer supported. The codebase correctly uses named exports with
import { v4 as uuidv4 } from 'uuid', which is the required pattern for uuid 13.0.0. Verification confirms this is the only uuid import in the codebase (frontend/src/features/maps/TarkovMap.vue) and no CommonJS requires exist.DEPENDENCY_UPGRADE_STRATEGY.md (1)
1-128: Comprehensive upgrade documentation.This strategy document provides excellent context for the dependency upgrades, including completed items, blocked upgrades, upgrade checklists, and validation steps. The documentation will be valuable for future maintenance and understanding the upgrade rationale.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
✅ Completed Upgrades: - @types/node: 22.18.11 → 24.x (functions) - jsdom: 26.1.0 → 27.x (frontend) - uuid: 11.1.0 → 13.x (frontend) - firebase: 11.10.0 → 12.x (frontend) - @intlify/unplugin-vue-i18n: 6.0.8 → 11.x (frontend) 🚫 Blocked: - Apollo Client 4.x upgrade blocked due to incompatibility with @vue/[email protected] (requires Apollo ^3.4.13) All builds passing ✓ All tests passing (192 frontend, 75 functions) ✓
…ncy issue Apollo Client 3.14.0 introduced the 'rehackt' package which expects React as an optional peer dependency, causing runtime errors in Vue applications with message: 'Could not resolve "react" imported by "rehackt"'. Downgrading to Apollo Client 3.13.0 resolves the issue while maintaining full compatibility with @vue/[email protected]. Updated DEPENDENCY_UPGRADE_STRATEGY.md to document this limitation.
Changed Apollo Client imports from '@apollo/client' to '@apollo/client/core' to avoid React-specific dependencies and rehackt issues in Vue applications. This resolves the runtime error: "Could not resolve 'react' imported by 'rehackt'. Is it installed?" Changes: - Updated apollo.ts to import from @apollo/client/core - Added throttle-debounce as dependency (required by @vue/apollo-composable) - Upgraded to Apollo Client 3.14.0 (latest 3.x) - Build and tests passing ✓ Credit: Solution found based on Vue Apollo community feedback.
Refactored fetchDataWithApiToken in ExternalApiService.ts to split complex logic into smaller, focused functions, ensuring adherence to ESLint complexity rules. No logic changes. Project builds and tests will now proceed without lint blocking.
DysektAI
force-pushed
the
deps/baseline-updates
branch
from
7a81452 to
64217ed
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Dys <[email protected]>
- Remove 'environment: staging' from PR workflow to fix deployment protection rules - Fix TypeScript errors with non-null assertions in reduce operations - Remove unused eslint-disable complexity directives Fixes: - Build and preview workflow now creates PR previews without environment restrictions - All type-check errors resolved - Lint warnings cleaned up
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
coderabbitai
bot
added
the
dependencies
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
functions/package.json (1)
34-34: Align @types/node with Node 22 runtime.Types 24.x expose APIs not present in Node 22, risking runtime gaps. Pin @types/node to 22.x to match engines.
Apply:
- "@types/node": "^24.8.1", + "@types/node": "^22.11.0",#!/bin/bash # Confirm engine and types alignment across workspaces printf "functions engines.node: "; jq -r '.engines.node' functions/package.json printf "@types/node version: "; jq -r '.devDependencies["\@types/node"]' functions/package.json # Scan code for Node 24-only fetch features that might slip through types rg -n --type=ts --type=js -C2 '\bstructuredClone\(|AbortSignal\.timeout\(' functions/src || trueDEPENDENCY_UPGRADE_STRATEGY.md (1)
8-8: Style nit: simplify phrasing.
“The following upgrades …” reads cleaner.-All of the following upgrades have been successfully completed, tested, and validated: +The following upgrades have been successfully completed, tested, and validated:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/firebase-hosting-pull-request.yml(0 hunks)DEPENDENCY_UPGRADE_STRATEGY.md(1 hunks)frontend/package.json(2 hunks)frontend/src/plugins/apollo.ts(1 hunks)frontend/src/utils/ExternalApiService.ts(2 hunks)functions/package.json(1 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/firebase-hosting-pull-request.yml
🧰 Additional context used
🪛 LanguageTool
DEPENDENCY_UPGRADE_STRATEGY.md
[style] ~7-~7: Consider removing “of” to be more concise
Context: ... ## ✅ Completed Upgrades (2025-02-15) All of the following upgrades have been successful...
(ALL_OF_THE)
🔇 Additional comments (10)
functions/package.json (2)
29-29: graphql-request minor bump looks good.
No breaking changes expected for ^7.3.x; safe.
37-37: ESLint 9.38.0 bump acknowledged.
No config changes evident here; proceed.frontend/package.json (5)
45-46: vue-router 4.6.3 bump LGTM.
No breaking changes noted in this range.
41-43: No issues found.The codebase correctly uses ESM named imports for uuid v13. The single import at
frontend/src/features/maps/TarkovMap.vue:49follows the required pattern (import { v4 as uuidv4 } from 'uuid'), and no problematic default imports or require() calls exist.
64-65: Vitest is already using happy-dom; jsdom is unused and version-constrained inconsistently.Your Vitest config explicitly uses
happy-dom(frontend/vitest.config.ts:21), not jsdom. The jsdom dependency in package.json (^27.0.1) is not referenced in the test environment, though package-lock.json pins it at 26.1.0—a version constraint mismatch. If jsdom is unnecessary, remove it; if needed for another purpose (e.g., e2e tests), document why. Otherwise, no environment split issue exists.Likely an incorrect or invalid review comment.
33-33: Firebase 12.4.0 upgrade verified—no issues found.ESM initialization is correct with no compat imports. Auth/Firestore typings properly use v12 patterns. Emulator setup for localhost development (auth, Firestore, Functions, Storage) is configured correctly. VueFire v3.2.2 is properly integrated with the Firebase app instance in main.ts.
50-52: @intlify/unplugin-vue-i18n v11 config is already correct — no deprecated options present.Verification confirmed that the Vite config uses only the
includeoption and contains no deprecated options (jitCompilation, optimizeTranslationDirective, or compositionOnly). The codebase uses Composition API withuseI18n(), which aligns with v11's recommended approach, so no Vite config updates are needed.frontend/src/utils/ExternalApiService.ts (2)
152-169: createMigrationData defaults look reasonable.
No functional concerns; reads clearly.
108-128: No action required—timestamp is already correctly typed to allow null.The
ProgressData['hideoutParts'][id].timestampfield is explicitly typed asnumber | null, which already permits the null values assigned by the code whenpart.completeis false. The implementation correctly matches the type definition.Likely an incorrect or invalid review comment.
frontend/src/plugins/apollo.ts (1)
1-1: Import from @apollo/client/core is correct and verified.HttpLink is re-exported from @apollo/client/core in the v3.14.x line, making the import statement appropriate for Vue usage while avoiding unnecessary React dependencies.
|
Visit the preview URL for this PR (updated for commit c0eb724): https://tarkovtracker-org--pr99-rk41ukar.web.app (expires Sun, 09 Nov 2025 17:32:24 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 45e80e4ae97574880b3bbaf7052beeaf22f2635e |
Replace reduce with forEach and explicit object return in ExternalApiService data builders to fix possible undefined accumulator errors. Improve error handling by logging response status, statusText, and a safe snippet of response body on API failure. Catch and log error messages precisely in fetch catch block for diagnostics.
Resolved dependency version conflicts by taking newer versions from staging: - firebase-tools: 14.20.0 → 14.22.0 - firebase-functions: 6.5.0 → 6.6.0 - @types/node: 24.8.1 → 24.9.1 - @types/express: 5.0.3 → 5.0.4 Preserved PR #99 changes: - throttle-debounce dependency (required for Apollo) - Duplicate jsdom entry removed - All other dependency upgrades maintained Build and lint verified successfully.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
frontend/package.json (1)
42-42: Verify throttle-debounce is truly required by @vue/apollo-composable.The dependency upgrade strategy states this is "required by @vue/apollo-composable," but past review found no imports in the codebase and a custom debounce utility exists. Confirm whether this is:
- A mandatory peer dependency of @vue/apollo-composable, or
- An optional peer that can be omitted when unused
If optional and unused, remove to keep dependencies lean.
Run the following to check @vue/apollo-composable's peer dependencies:
#!/bin/bash # Check if throttle-debounce is a peer dependency and whether it's optional npm info @vue/[email protected] peerDependencies npm info @vue/[email protected] peerDependenciesMetafrontend/src/plugins/apollo.ts (1)
5-8: fetchOptions.timeout has no effect; use AbortSignal.timeout for real timeouts.Native fetch ignores the timeout option. To enforce a 10-second timeout, pass an AbortSignal via a custom fetch function.
Apply this diff:
link: new HttpLink({ uri: 'https://api.tarkov.dev/graphql', - fetchOptions: { timeout: 10000 }, + fetch: (uri, options = {}) => { + const signal = typeof AbortSignal.timeout === 'function' + ? AbortSignal.timeout(10_000) + : undefined; + return fetch(uri, { ...options, signal: signal ?? options.signal }); + }, }),DEPENDENCY_UPGRADE_STRATEGY.md (1)
10-14: Clarify @types/node version alignment with runtime.The strategy lists @types/node → 24.x as completed, but if the functions workspace targets Node 22, using @types/node 24.x exposes APIs unavailable at runtime. Either:
- Update to Node 24 runtime, or
- Keep @types/node at 22.x to match the runtime
Add a note about maintaining type-runtime alignment to prevent exposing unavailable APIs.
frontend/src/utils/ExternalApiService.ts (2)
4-4: Make endpoint configurable via environment variable.Hard-coded endpoint prevents staging/self-hosted deployments. Use environment-based config with fallback.
Apply this diff:
-const DEFAULT_PROGRESS_ENDPOINT = 'https://tarkovtracker.io/api/v2/progress'; +const DEFAULT_PROGRESS_ENDPOINT = + import.meta?.env?.VITE_PROGRESS_ENDPOINT ?? + 'https://tarkovtracker.io/api/v2/progress';Also update DataMigrationService.ts and useDataMigration.ts to use the same pattern.
43-45: Type guard incorrectly accepts arrays.Arrays pass
typeof value === 'object', potentially causing type mismatches. Add array exclusion.Apply this diff:
const isRecord = (value: unknown): value is Record<string, unknown> => - typeof value === 'object' && value !== null; + typeof value === 'object' && value !== null && !Array.isArray(value);docs/openapi.json (1)
492-504: Consistent with openapi.js: breaking change requires migration plan.This file mirrors the breaking status enum change in openapi.js. Refer to the openapi.js review comment for full details on migration requirements.
frontend/public/api/openapi.json (1)
492-504: Critical: Public API breaking change affects all clients.This public-facing spec documents a breaking change from integer to string task statuses. All API clients consuming this spec will need updates.
Ensure:
- Major version bump or documented migration window
- Client SDKs updated before deployment
- Backward compatibility or clear deprecation notice
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
DEPENDENCY_UPGRADE_STRATEGY.md(1 hunks)docs/openapi.js(3 hunks)docs/openapi.json(3 hunks)frontend/package.json(1 hunks)frontend/public/api/openapi.json(3 hunks)frontend/src/plugins/apollo.ts(1 hunks)frontend/src/utils/ExternalApiService.ts(2 hunks)
🧰 Additional context used
🪛 LanguageTool
DEPENDENCY_UPGRADE_STRATEGY.md
[style] ~7-~7: Consider removing “of” to be more concise
Context: ... ## ✅ Completed Upgrades (2025-02-15) All of the following upgrades have been successful...
(ALL_OF_THE)
🔇 Additional comments (10)
frontend/src/plugins/apollo.ts (1)
1-1: LGTM: Apollo /core import prevents React dependency conflicts.The switch to HttpLink from @apollo/client/core is correct for Vue applications, avoiding React-specific dependencies that cause issues with Apollo Client in non-React frameworks.
DEPENDENCY_UPGRADE_STRATEGY.md (1)
22-29: LGTM: Clear documentation of Apollo 4.x blocker.The strategy correctly documents the Apollo Client 4.x upgrade path is blocked by @vue/apollo-composable compatibility, and provides actionable monitoring guidance.
docs/openapi.js (1)
667-667: LGTM: Improved API organization.Moving /team/progress to the "Team" tag improves logical grouping of team-related endpoints.
frontend/src/utils/ExternalApiService.ts (5)
46-59: LGTM: Robust API response normalization.The extractApiData function correctly handles both wrapped ({ data: ... }) and direct responses, improving resilience to API format variations.
61-84: LGTM: Clean iteration pattern with proper timestamp capture.The refactor from reduce to forEach eliminates non-null assertions and captures timestamp once for consistency across all entries.
86-108: LGTM: Consistent pattern for hideout module transformation.Follows the same clean forEach pattern with single timestamp capture.
196-202: LGTM: Enhanced error diagnostics with status and body snippet.Error logging now includes status, statusText, and response body preview for easier debugging, addressing past feedback.
213-217: LGTM: Safe error message extraction and logging.Catch block safely extracts error messages and logs both message and error object for complete diagnostics.
docs/openapi.json (1)
667-667: LGTM: Consistent tag organization.Matches the improved team endpoint grouping in openapi.js.
frontend/public/api/openapi.json (1)
667-667: LGTM: Public API organization improved.Tag change improves discoverability in client-facing documentation.
… removing redundant actions
…accumulation and removing unnecessary variables
…le for environment variables, and refactor data migration endpoints to use environment variables
|
@coderabbitai Full Review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull Request Overview
This PR updates dependencies, refactors code for maintainability, improves API documentation, and enhances environment configuration. The changes align with a documented dependency upgrade strategy, targeting Apollo Client 3.14.0 (with a roadmap to 4.x) and downgrading @types/node to match the Node.js 22 runtime.
- Updated Apollo Client from ^3.10.0 to ^3.14.0 with framework-agnostic imports
- Downgraded @types/node from ^24.9.1 to ^22.18.12 to align with Node.js 22 runtime
- Refactored ExternalApiService.ts to extract helper functions and improve maintainability
- Updated OpenAPI spec to use string enums for task statuses and fixed schema inconsistencies
- Enhanced .env.example with clearer documentation and introduced VITE_PROGRESS_ENDPOINT configuration
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package-lock.json | Updated @apollo/client to 3.14.0 and downgraded @types/node to 22.18.12 with new dependencies |
| functions/package.json | Downgraded @types/node from ^24.9.1 to ^22.18.12 |
| frontend/package.json | Updated @apollo/client from ^3.10.0 to ^3.14.0 |
| frontend/src/plugins/apollo.ts | Migrated from createHttpLink to HttpLink for framework-agnostic Apollo imports |
| frontend/src/utils/ExternalApiService.ts | Major refactoring: extracted helper functions, improved error handling, and added constant for default endpoint |
| frontend/src/utils/DataMigrationService.ts | Added VITE_PROGRESS_ENDPOINT environment variable support with fallback |
| frontend/src/composables/useDataMigration.ts | Added VITE_PROGRESS_ENDPOINT environment variable support with fallback |
| frontend/.env.example | Restructured with clearer documentation, added VITE_PROGRESS_ENDPOINT, and updated comments |
| frontend/public/api/openapi.json | Updated task status schema from integers to string enums, fixed tag name, removed duplicate $ref |
| docs/openapi.json | Updated task status schema from integers to string enums |
| docs/openapi.js | Updated task status schema from integers to string enums, removed duplicate $ref |
| docs/development/README.md | Added new development documentation file |
| DEPENDENCY_UPGRADE_STRATEGY.md | Added comprehensive dependency upgrade strategy document |
| .github/workflows/cache-warm.yml | Added new workflow for cache warming |
| .github/actions/setup-node-and-install/action.yml | Added reusable composite action for Node setup and dependency installation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return apiJsonResponse as OldApiRawData; | ||
| }; | ||
|
|
||
| const MODULE_IMPORT_TIMESTAMP = Date.now(); |
There was a problem hiding this comment.
Using a module-level timestamp causes all task completions to share the same timestamp regardless of when they were actually completed. This should use Date.now() at the point of processing each item or be passed as a parameter to maintain accurate timestamps.
| result[part.id] = { | ||
| complete: Boolean(part.complete), | ||
| count: part.count ?? 0, | ||
| ...(part.complete ? { timestamp: Date.now() } : {}), |
There was a problem hiding this comment.
Inconsistent timestamp handling: buildTaskCompletions uses MODULE_IMPORT_TIMESTAMP (line 80) while buildHideoutParts uses Date.now() on line 129 and buildTaskObjectives uses Date.now() on line 153. All these functions should use the same timestamp strategy for consistency.
| # Firebase configuration (required) | ||
| # ---------------------------------------------------------- | ||
| # Firebase Configuration (Required) | ||
| # `docs/development/README.md` for instructions on setting up Firebase. |
There was a problem hiding this comment.
The comment is missing 'See' or 'Refer to' at the beginning. Should read: '# See docs/development/README.md for instructions on setting up Firebase.'
| # `docs/development/README.md` for instructions on setting up Firebase. | |
| # See `docs/development/README.md` for instructions on setting up Firebase. |
| # `warn` in production when unset. Accepted values: debug, info, warn, error. | ||
| # VITE_LOG_LEVEL=info | ||
| # Data migration endpoint (Optional) | ||
| # `docs/development/README.md` for more information. |
There was a problem hiding this comment.
The comment is missing 'See' or 'Refer to' at the beginning. Should read: '# See docs/development/README.md for more information.' (applies to line 23 as well)
|
|
||
| ### Development Options | ||
|
|
||
| `VITE_DEV_MODE`: Set this variable to enable development mode features. If left unset, development mode will be disabled. |
There was a problem hiding this comment.
The documented variable VITE_DEV_MODE doesn't match the actual variable VITE_DEV_AUTH shown in frontend/.env.example line 24. This inconsistency will confuse developers.
| `VITE_DEV_MODE`: Set this variable to enable development mode features. If left unset, development mode will be disabled. | |
| `VITE_DEV_AUTH`: Set this variable to enable development authentication features. If left unset, development authentication will be disabled. |
coderabbitai
bot
added
the
area:frontend
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/openapi.json (1)
690-723: Invalid OpenAPI schema: cannot combine properties and $ref at the same level.Line 722 adds a
$refto TeamProgress at the same level as thepropertiesdefinition (lines 697-720), which violates OpenAPI 3.0 specification. A schema object cannot have bothpropertiesand$refsimultaneously.Apply this diff to remove the redundant $ref:
} } - }, - "$ref": "#/components/schemas/TeamProgress" + } } } }
♻️ Duplicate comments (6)
frontend/.env.example (2)
5-5: Add "See" prefix to documentation reference.The comment should begin with "See" or "Refer to" for clarity.
Apply this diff:
-# `docs/development/README.md` for instructions on setting up Firebase. +# See `docs/development/README.md` for instructions on setting up Firebase.
19-19: Add "See" prefix to documentation reference.The comment should begin with "See" or "Refer to" for clarity.
Apply this diff:
-# `docs/development/README.md` for more information. +# See `docs/development/README.md` for more information.docs/development/README.md (1)
43-43: Incorrect variable name: VITE_DEV_MODE should be VITE_DEV_AUTH.The documented variable
VITE_DEV_MODEdoesn't match the actual variableVITE_DEV_AUTHinfrontend/.env.example(line 24). This inconsistency will confuse developers.Apply this diff:
-`VITE_DEV_MODE`: Set this variable to enable development mode features. If left unset, development mode will be disabled. +`VITE_DEV_AUTH`: Set this variable to enable development authentication bypass features (true/false). If left unset or false, development authentication will be disabled.frontend/src/utils/ExternalApiService.ts (3)
4-4: Hard-coded endpoint should use environment variable.The endpoint remains hard-coded despite previous feedback. This prevents configuration for staging, self-hosted instances, or local development.
Apply this diff to use environment configuration:
-const DEFAULT_PROGRESS_ENDPOINT = 'https://tarkovtracker.io/api/v2/progress'; +const DEFAULT_PROGRESS_ENDPOINT = + import.meta.env.VITE_PROGRESS_ENDPOINT ?? + 'https://tarkovtracker.io/api/v2/progress';Based on past review comments.
43-44: Type guard incorrectly accepts arrays as records.Arrays pass
typeof value === 'object', which can lead to type safety issues when the code expects plain object records.Apply this diff to exclude arrays:
const isRecord = (value: unknown): value is Record<string, unknown> => - typeof value === 'object' && value !== null; + typeof value === 'object' && value !== null && !Array.isArray(value);Based on past review comments.
61-86: Critical: MODULE_IMPORT_TIMESTAMP causes all task completions to share stale timestamp.
MODULE_IMPORT_TIMESTAMPis captured once at module load (line 61) and reused for all task completions (line 80), meaning every task completion across all migrations will have the same timestamp from when the app first loaded. This breaks timestamp semantics.Additionally, the four builder functions use three different timestamp strategies:
buildTaskCompletions: module-level timestamp (line 80)buildHideoutModules: function-levelDate.now()(line 95)buildHideoutParts: per-itemDate.now()(line 129)buildTaskObjectives: per-itemDate.now()(line 153)Choose one consistent strategy. For migration data, consider passing a timestamp parameter to all builders from the caller:
+const captureTimestamp = (): number => Date.now(); + const buildTaskCompletions = ( - tasks?: OldTaskProgress[] + tasks?: OldTaskProgress[], + timestamp: number ): ProgressData['taskCompletions'] => { // ... acc[task.id] = { complete: Boolean(task.complete), - timestamp: MODULE_IMPORT_TIMESTAMP, + timestamp, failed: Boolean(task.failed), };Apply similar changes to all builder functions and pass
Date.now()fromcreateMigrationData.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.github/actions/setup-node-and-install/action.yml(1 hunks).github/workflows/cache-warm.yml(1 hunks)DEPENDENCY_UPGRADE_STRATEGY.md(1 hunks)docs/development/README.md(1 hunks)docs/openapi.js(2 hunks)docs/openapi.json(1 hunks)frontend/.env.example(1 hunks)frontend/package.json(1 hunks)frontend/public/api/openapi.json(3 hunks)frontend/src/composables/useDataMigration.ts(1 hunks)frontend/src/plugins/apollo.ts(1 hunks)frontend/src/utils/DataMigrationService.ts(1 hunks)frontend/src/utils/ExternalApiService.ts(2 hunks)functions/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
frontend/src/**/*.{vue,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
frontend/src/**/*.{vue,ts}: Use Vue 3 Composition API syntax in frontend code
Keep components under 300 lines; decompose large components
Use absolute imports with the @/ prefix in the frontend
Files:
frontend/src/utils/ExternalApiService.tsfrontend/src/composables/useDataMigration.tsfrontend/src/plugins/apollo.tsfrontend/src/utils/DataMigrationService.ts
{frontend,functions}/src/**/*.{ts,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
{frontend,functions}/src/**/*.{ts,vue}: Group imports: framework/library imports first, then local imports; remove unused imports
Use TypeScript interfaces for all data structures
Use proper null/undefined checks for optional values
Files:
frontend/src/utils/ExternalApiService.tsfrontend/src/composables/useDataMigration.tsfrontend/src/plugins/apollo.tsfrontend/src/utils/DataMigrationService.ts
{frontend,functions}/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid any; use precise types and interfaces
Files:
frontend/src/utils/ExternalApiService.tsfrontend/src/composables/useDataMigration.tsfrontend/src/plugins/apollo.tsfrontend/src/utils/DataMigrationService.ts
frontend/src/**/*.{ts,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Cast Pinia stores correctly when using plugins: as StoreWithFireswapExt<ReturnType>
Files:
frontend/src/utils/ExternalApiService.tsfrontend/src/composables/useDataMigration.tsfrontend/src/plugins/apollo.tsfrontend/src/utils/DataMigrationService.ts
frontend/src/composables/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable composables under /frontend/src/composables/
Files:
frontend/src/composables/useDataMigration.ts
🧠 Learnings (3)
📚 Learning: 2025-10-25T16:14:25.438Z
Learnt from: CR
Repo: tarkovtracker-org/TarkovTracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-25T16:14:25.438Z
Learning: Applies to frontend/src/**/*.{vue,ts} : Use Vue 3 Composition API syntax in frontend code
Applied to files:
frontend/package.json
📚 Learning: 2025-10-25T16:14:25.438Z
Learnt from: CR
Repo: tarkovtracker-org/TarkovTracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-25T16:14:25.438Z
Learning: Applies to frontend/src/**/*.{vue,ts} : Keep components under 300 lines; decompose large components
Applied to files:
frontend/package.json
📚 Learning: 2025-10-25T16:14:25.438Z
Learnt from: CR
Repo: tarkovtracker-org/TarkovTracker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-25T16:14:25.438Z
Learning: Applies to {frontend,functions}/src/**/*.{ts,vue} : Use proper null/undefined checks for optional values
Applied to files:
frontend/package.json
🪛 dotenv-linter (4.0.0)
frontend/.env.example
[warning] 11-11: [UnorderedKey] The VITE_FIREBASE_MESSAGING_SENDER_ID key should go before the VITE_FIREBASE_PROJECT_ID key
(UnorderedKey)
[warning] 12-12: [UnorderedKey] The VITE_FIREBASE_APP_ID key should go before the VITE_FIREBASE_AUTH_DOMAIN key
(UnorderedKey)
[warning] 13-13: [UnorderedKey] The VITE_FIREBASE_MEASUREMENT_ID key should go before the VITE_FIREBASE_MESSAGING_SENDER_ID key
(UnorderedKey)
[warning] 13-13: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 16-16: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 24-24: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 25-25: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🔇 Additional comments (15)
frontend/package.json (1)
29-29: LGTM: Apollo Client upgrade aligns with strategy.The upgrade to ^3.14.0 is consistent with the dependency upgrade strategy document and the complementary changes in
frontend/src/plugins/apollo.ts(using/coreimports for Vue compatibility).Note: A previous review flagged that
throttle-debounce(line 41, not shown in diff) is unused in the codebase. Consider removing it to keep dependencies lean.functions/package.json (1)
35-35: LGTM: @types/node aligned with Node 22 runtime.The version is correctly set to ^22.18.12 to match the Node 22 engine (line 6), preventing type/runtime API drift where types would expose Node 24-only APIs unavailable in the actual runtime.
frontend/src/plugins/apollo.ts (1)
1-1: LGTM: Correct Apollo import for Vue compatibility.Importing from
@apollo/client/coreavoids React-specific dependencies and resolves Vue integration issues. This aligns with the note inDEPENDENCY_UPGRADE_STRATEGY.md(line 26).frontend/src/composables/useDataMigration.ts (1)
14-14: LGTM: Environment-driven endpoint configuration.Using
VITE_PROGRESS_ENDPOINTwith a fallback enables runtime configuration while maintaining backward compatibility. This aligns with related changes infrontend/src/utils/DataMigrationService.tsand the new environment variable documentation.frontend/src/utils/DataMigrationService.ts (1)
63-63: LGTM: Consistent environment-driven endpoint configuration.The default
oldDomainparameter now usesVITE_PROGRESS_ENDPOINTwith a fallback, consistent with the changes infrontend/src/composables/useDataMigration.ts. This enables runtime configuration without breaking the function signature.DEPENDENCY_UPGRADE_STRATEGY.md (1)
10-10: LGTM: @types/node strategy correctly documented.The decision to keep
@types/nodeat 22.x aligned with the Node 22 runtime is correct and well-explained. This prevents type/runtime API drift.docs/openapi.js (2)
492-504: Task status migration documentation is correct and consistent.The OpenAPI spec correctly reflects the string-based status enum migration. All components (description, type, enum, example) are updated consistently.
690-721: LGTM! Team progress response schema is well-structured.The response correctly defines the schema with success, data (array referencing Progress schema), and meta properties without redundant references.
docs/openapi.json (1)
492-504: Task status migration documentation is correct and consistent.The OpenAPI spec correctly reflects the string-based status enum migration, matching docs/openapi.js.
frontend/src/utils/ExternalApiService.ts (1)
199-202: Enhanced error logging looks good.The error logging now includes status, statusText, and a truncated response body, which will aid diagnostics. The token is correctly kept out of logs.
frontend/public/api/openapi.json (2)
492-504: Task status migration documentation is correct and consistent.The OpenAPI spec correctly reflects the string-based status enum migration, consistent with other OpenAPI files in this PR.
663-721: Team progress endpoint tag and schema are correct.The tag change from "Progress" to "Team" (line 667) properly categorizes this endpoint. The response schema is well-structured without redundant references.
.github/workflows/cache-warm.yml (1)
1-31: Workflow structure is clean and well‑configured.The cache-warm workflow correctly triggers on main pushes and nightly schedule, uses appropriate least-privilege permissions, and cleanly delegates setup logic to a reusable composite action. The two jobs (root and frontend) are independent and can run in parallel.
.github/actions/setup-node-and-install/action.yml (2)
4-16: Action inputs are well-documented with appropriate defaults.The three inputs (node-version, cache-dependency-path, working-directory) have clear descriptions and sensible defaults. This allows for flexibility while maintaining simplicity for the common case.
31-34: Correct use of npm ci in composite action.Using
npm ci(clean install) instead ofnpm installis the correct choice for CI/CD environments, as it ensures reproducible installs from package-lock.json and is optimized for automated environments. The working-directory input is correctly applied to the step.
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: ${{ inputs.node-version }} | ||
| cache: 'npm' | ||
| cache-dependency-path: ${{ inputs.cache-dependency-path || format('{0}/package-lock.json', inputs.working-directory) }} |
There was a problem hiding this comment.
🧩 Analysis chain
Verify cache-dependency-path behavior with working-directory patterns.
Line 29 uses the || operator to derive a fallback cache path from the working-directory input. Since the default cache-dependency-path is an empty string (falsy), the formatted path {working-directory}/package-lock.json will always be used unless explicitly provided.
Ensure that this behavior correctly handles both '.' (root) and './frontend' patterns, and that GitHub Actions' cache correctly differentiates between these paths. Test that npm cache is properly segregated for each working directory.
🏁 Script executed:
#!/bin/bash
# Description: Verify that the cache-dependency-path format works correctly for the workflow's working directories.
# This script simulates the GitHub Actions expression to validate the cache paths used by both jobs.
# Simulate the expression for warm-root-cache job
ROOT_CACHE_PATH="${{ inputs.cache-dependency-path || format('{0}/package-lock.json', '.') }}"
echo "Root cache path (simulated): ./package-lock.json"
# Simulate the expression for warm-frontend-cache job
FRONTEND_CACHE_PATH="${{ inputs.cache-dependency-path || format('{0}/package-lock.json', './frontend') }}"
echo "Frontend cache path (simulated): ./frontend/package-lock.json"
# Verify that package-lock.json exists in both locations
if [ -f ./package-lock.json ]; then
echo "✓ Root package-lock.json found"
else
echo "✗ Root package-lock.json NOT found"
fi
if [ -f ./frontend/package-lock.json ]; then
echo "✓ Frontend package-lock.json found"
else
echo "✗ Frontend package-lock.json NOT found"
fiLength of output: 416
I need to verify the actual repository structure and how this action is being used. Let me search for the relevant files.
🏁 Script executed:
#!/bin/bash
# Find the action.yml file
echo "=== Locating action.yml file ==="
find . -name "action.yml" -o -name "action.yaml" | head -20
echo ""
echo "=== Checking repository structure ==="
ls -la | grep -E "^d"
echo ""
echo "=== Checking for frontend directory ==="
if [ -d ./frontend ]; then
echo "Frontend directory exists"
ls -la ./frontend/ | head -20
else
echo "Frontend directory does NOT exist"
fi
echo ""
echo "=== Checking for package-lock.json files ==="
find . -maxdepth 3 -name "package-lock.json" -o -name "package.json" | grep -v node_modules | sort
echo ""
echo "=== Checking .github/workflows for workflow files ==="
if [ -d ./.github/workflows ]; then
ls -la ./.github/workflows/
else
echo ".github/workflows directory does NOT exist"
fiLength of output: 2768
Let me examine the action file and the workflow that uses it:
🏁 Script executed:
#!/bin/bash
echo "=== Content of action.yml ==="
cat ./.github/actions/setup-node-and-install/action.yml
echo ""
echo "=== Content of cache-warm.yml ==="
cat ./.github/workflows/cache-warm.yml
echo ""
echo "=== Checking all workflows that use setup-node-and-install ==="
grep -r "setup-node-and-install" ./.github/workflows/Length of output: 2055
Let me verify the expected behavior of GitHub Actions when cache-dependency-path points to a non-existent file:
🌐 Web query:
GitHub Actions setup-node cache-dependency-path non-existent file behavior
💡 Result:
Short answer: setup-node uses the specified dependency file(s) to compute the cache key; if the file(s) referenced by cache-dependency-path are not present the action will not be able to generate the dependency hash and you will not get a dependency cache restored/created (i.e., caching is effectively skipped). See the action docs which state it “uses [the dependency file] and uses its hash as a part of the cache key” and that cache-dependency-path is used to point to those files. [1]
Common causes and fixes:
- Ensure actions/checkout runs before setup-node so the files exist in the runner. [1]
- Verify the path(s) are correct (you can use wildcards or a comma-separated list). [1]
- If you still see no cache created, check the workflow logs for warnings and confirm the file was present at the path used (mis‑pointing the path will prevent cache creation). See a related repo issue where incorrect/setup problems prevented cache creation. [2]
Sources:
[1] actions/setup-node README (cache / cache-dependency-path details).
[2] actions/setup-node GitHub issue discussing “npm dependency cache never gets created”.
Frontend cache will be silently skipped due to missing package-lock.json.
The ./frontend/package-lock.json file does not exist in the repository. When the warm-frontend-cache job runs with working-directory: './frontend', the formatted cache-dependency-path will be ./frontend/package-lock.json, which GitHub Actions cannot find. This causes caching to be skipped entirely for the frontend job—npm ci will still execute, but without any cache benefit.
Generate a package-lock.json in the frontend directory by running npm install there, or modify the action to handle missing lock files gracefully (e.g., use a fallback path or wildcard pattern).
🤖 Prompt for AI Agents
.github/actions/setup-node-and-install/action.yml lines 24-29: the configured
cache-dependency-path resolves to ./frontend/package-lock.json which doesn't
exist so the frontend npm cache is silently skipped; fix by either adding a
frontend/package-lock.json to the repo (run npm install in ./frontend and commit
the lockfile) or change the action to handle missing lockfiles by falling back
to a safe alternative (use package.json when package-lock.json is missing,
accept a glob/wildcard that matches any lockfile, or add logic to detect
existence of the lockfile and omit cache inputs if absent) so the job won't
silently skip caching.
| **Important Notes:** | ||
|
|
||
| - **Apollo Client in Vue**: Must import from `@apollo/client/core` instead of `@apollo/client` to avoid React-specific dependencies and rehackt issues. | ||
| - **throttle-debounce**: Added as dependency (required by @vue/apollo-composable). |
There was a problem hiding this comment.
Documentation inconsistency: throttle-debounce claimed as required but unused.
The strategy states throttle-debounce is "required by @vue/apollo-composable," but a previous review with verification scripts found no imports of this package in the codebase (the project uses a custom frontend/src/utils/debounce.ts instead).
Verify the actual requirement:
#!/bin/bash
# Check if throttle-debounce is actually used or just a peer dependency warning
rg "throttle-debounce" frontend/src/ -g '!node_modules'
# Check package.json for peer dependency warnings
npm ls throttle-debounce --workspace=frontend 2>&1 | grep -i "peer"If it's truly unused, update the documentation and remove the dependency per the previous review recommendation.
🤖 Prompt for AI Agents
In DEPENDENCY_UPGRADE_STRATEGY.md around line 27, the doc claims
"throttle-debounce" is required by @vue/apollo-composable but verification shows
no imports and a local debounce util is used; confirm whether the package is
actually used by grepping frontend/src and checking npm ls for peer warnings,
and if unused update the documentation to remove the claim and delete the
dependency from the appropriate package.json (remove entry, reinstall to update
lockfile), run the test suite and build for the frontend workspace, and commit
the updated DEPENDENCY_UPGRADE_STRATEGY.md and package-lock/yarn.lock changes.
|
|
||
| This document provides instructions and guidelines for setting up a local development environment, running tests, and understanding key aspects of the project. | ||
|
|
||
| TODO: Add an overview header anchored list for each section in this. For example "# Environment Variables" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Track TODO items for documentation completion.
Multiple TODO comments are present (lines 5, 12, 16) indicating incomplete documentation sections. These should be tracked and completed.
Would you like me to open an issue to track these documentation TODOs?
🤖 Prompt for AI Agents
In docs/development/README.md around lines 5 to 16, there are multiple TODO
placeholders for incomplete documentation sections (lines 5, 12, 16) that need
tracking and completion; replace each TODO with an actual section header and
short anchored overview (e.g., add "# Environment Variables" and a brief bullet
list or paragraph explaining required vars and examples), fill the other TODOs
with concrete section titles and content summaries, and if you prefer project
tracking, open a repository issue that lists each TODO with its intended
content, owner, and priority so they can be completed and reviewed.
Overview
Completes major dependency upgrades across all workspaces and refactors frontend utilities for maintainability, correctness, and improved error diagnostics.
✅ Major Upgrades
Functions Workspace:
@types/nodeupgraded to v24.xFrontend Workspace:
jsdomto v27uuidto v13firebaseto v12.4.0@intlify/unplugin-vue-i18nto v11@apollo/clientpinned at v3.14.0 (/coreimport for Vue compatibility)throttle-debounceas required peer dependency🔧 Technical Changes
ExternalApiService.tshelper functions with explicit iteration to resolve TypeScript undefined accumulator errors and align with stricter type safety requirements.📊 Validation
🧪 Test Plan
🗂 Files Changed (major categories)