Skip to content

Conversation

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Oct 24, 2025

Blocked for now, as WebStorm still doesn't support vitest 4 properly.

@kibertoad kibertoad requested review from a team and drdaemos as code owners October 24, 2025 09:26
@kibertoad kibertoad marked this pull request as draft October 24, 2025 09:27
@coderabbitai
Copy link

coderabbitai bot commented Oct 24, 2025

📝 Walkthrough

Summary by CodeRabbit

  • Chores
    • Updated CI/CD pipeline to support Node 25.x alongside 24.x
    • Upgraded CI workflow tools to latest versions
    • Bumped base runtime image to Node 24.10.0
    • Updated dependencies across AWS SDK, OpenTelemetry, BullMQ, logging, and testing frameworks
    • Refined test configuration and coverage thresholds

Walkthrough

This PR updates infrastructure, dependencies, and test configurations. The base Node image in the Dockerfile is upgraded from 24.9.0 to 24.10.0. CI workflows expand to test against Node 25.x using the newer v6 setup-node action. Multiple runtime and development dependencies receive updates, including AWS SDK (3.908.0 to 3.916.0), OpenTelemetry packages, BullMQ, Drizzle ORM, ioredis, New Relic, pino, and vitest. The OpenAPI spec generation script is refactored to call app.swagger() directly instead of making an HTTP request. Import syntax for Scalar Fastify API reference changes from default to named import. Vitest configuration is adjusted to use maxWorkers and isolate settings, coverage exclusions are expanded, and the branch coverage threshold is lowered from 85 to 80.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The diff spans multiple heterogeneous areas: infrastructure updates (GitHub Actions, Dockerfile), distributed dependency version bumps (repetitive pattern across many packages with minimal cognitive load per update), configuration changes to vitest, a straightforward API spec generation refactor, and a trivial import syntax adjustment. While most individual changes are simple, the variety of files and need to verify dependency version compatibility across different package types (AWS, OpenTelemetry, testing, ORM, etc.) justifies moderate review effort.

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Update dependencies (vitest 4 and friends)" directly captures the primary focus of the changeset. The raw summary shows extensive dependency updates across package.json, including a major vitest upgrade from 3.2.4 to 4.0.2, along with updates to AWS SDK packages, OpenTelemetry, Drizzle ORM, and numerous other dependencies. The title is concise, specific enough to highlight the notable vitest 4 major version upgrade, and clearly conveys the main change without being vague or overly broad.
Description Check ✅ Passed The description "Blocked for now, as WebStorm still doesn't support vitest 4 properly" is related to the changeset in that it specifically references vitest 4, which is a major update present in the PR's dependency changes. While the description doesn't detail what is being changed, it provides meaningful context about the state of the PR and is directly relevant to one of the key updates (vitest major version bump). The description is neither completely off-topic nor vague—it clearly communicates a specific constraint related to the changes.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
vitest.config.mts (1)

39-39: Consider documenting the rationale for lowering branch coverage.

The branch coverage threshold was reduced from 85% to 80%. While this may be necessary due to vitest 4 changes or new code patterns, it's worth documenting why this reduction was needed to prevent gradual coverage erosion.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4568bac and 5566588.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (6)
  • .github/workflows/ci.yml (1 hunks)
  • Dockerfile (1 hunks)
  • package.json (3 hunks)
  • scripts/generateOpenApi.ts (1 hunks)
  • src/app.ts (1 hunks)
  • vitest.config.mts (3 hunks)
🔇 Additional comments (7)
.github/workflows/ci.yml (1)

12-12: LGTM! Expanded Node version support.

Adding Node 25.x to the test matrix and upgrading to setup-node v6 improves compatibility testing and keeps the CI tooling current.

Also applies to: 18-18

Dockerfile (1)

2-2: LGTM! Node version update.

The Node base image update from 24.9.0 to 24.10.0 is a standard patch-level maintenance update.

scripts/generateOpenApi.ts (1)

22-23: LGTM! Improved OpenAPI generation approach.

The refactor from HTTP-based spec retrieval to direct app.swagger() call is more efficient and eliminates HTTP overhead. This simplifies the script and removes the dependency on the server being fully running.

vitest.config.mts (2)

7-8: Sequential test execution may impact performance.

Setting maxWorkers: 1 and isolate: false forces tests to run sequentially without isolation. While this can resolve issues with shared state or resource contention, it will significantly slow down test execution.

Consider investigating whether tests can be made more independent to allow parallel execution in the future, which would improve developer experience.


18-22: LGTM! Coverage exclusions are appropriate.

Adding infrastructure files (otel.ts, healthchecks.ts, AbstractPeriodicJob.ts, CommonModule.ts) to coverage exclusions is reasonable as these are typically integration/setup code that's difficult to unit test effectively.

package.json (1)

38-40: LGTM! Dependency updates look reasonable.

The dependency updates are primarily patch and minor version bumps. The major change is vitest 3.2.4 → 4.0.2, which aligns with the PR's objective and the corresponding configuration changes in vitest.config.mts.

Also applies to: 65-68, 73-75, 82-82, 85-87, 98-99, 103-109, 114-114

src/app.ts (1)

22-22: Based on my investigation, the import pattern import { default as scalarFastifyApiReference } appears to be unnecessary and deviates from official Scalar documentation.

The official Scalar documentation consistently recommends using dynamic import: fastify.register(import('@scalar/fastify-api-reference'), ...) rather than a static import with destructured default syntax. The current code uses a static import and passes it directly to app.register(), which does not follow the recommended pattern.

The destructured default syntax ({ default as ... }) is typically a workaround for ESM/CJS interop issues, but there is no evidence that version 1.38.1 requires this specific pattern. However, I cannot execute the code to verify if this causes runtime issues or if it actually works despite deviating from official recommendations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants