fix(fortnox): use dedicated atomic locks collection for refresh + add concurrency tests#1079
fix(fortnox): use dedicated atomic locks collection for refresh + add concurrency tests#1079Patrikbjoh wants to merge 1 commit intostack-auth:mainfrom
Conversation
|
@Patrikbjoh is attempting to deploy a commit to the Stack Auth Team on Vercel. A member of the Team first needs to authorize it. |
Preview Screenshots⏳ Preview screenshots are being captured... Workspace and dev browser links will appear here once the preview environment is ready. Generated by cmux preview system |
|
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 You can disable this status message by setting the 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 |
|
|
There was a problem hiding this comment.
Pull request overview
This PR includes significant refactoring and new features, though the title suggests it's focused on Fortnox atomic locks (which are not visible in the provided diffs). The main changes include:
- Configuration CRUD refactoring by removing the generic CRUD handlers in favor of direct route implementations
- Cookie package upgrade from 0.6.0 to 0.7.0 across multiple packages
- Introduction of a new dev-stats feature for monitoring API performance during development
- Database schema improvements with new indexes for performance optimization
- Build and script refactoring with environment-specific command variants
- Extensive test refactoring replacing Auth.Otp.signIn with Auth.fastSignUp
Reviewed changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| turbo.json | Removed cache configuration for generate-openapi-fumadocs task |
| packages/*/package.json | Updated cookie dependency from 0.6.0 to 0.7.0 |
| packages/stack-shared/src/interface/crud/config.ts | Deleted config CRUD interface definitions |
| packages/stack-shared/src/interface/admin-interface.ts | Updated config methods to remove CRUD type dependencies |
| packages/template/src/lib/stack-app/apps/implementations/admin-app-impl.ts | Simplified type signatures for config handling |
| package.json | Refactored build scripts, removed generate-openapi-fumadocs, added codegen:backend |
| apps/backend/src/lib/dev-request-stats.tsx | New file implementing request statistics tracking for development |
| apps/backend/src/app/dev-stats/page.tsx | New UI page for displaying development request statistics |
| apps/backend/src/app/dev-stats/api/route.tsx | New API route for dev stats data retrieval and clearing |
| apps/backend/src/route-handlers/smart-route-handler.tsx | Added request stats recording integration |
| apps/backend/src/proxy.tsx | Minor refactoring moving URL creation earlier in execution |
| apps/backend/src/app/api/latest/internal/config/*.tsx | Refactored config routes from CRUD to direct implementations |
| apps/backend/scripts/run-email-queue.ts | Added 5-second startup delay |
| apps/backend/package.json | Major script refactoring with environment-specific variants |
| apps/backend/prisma/schema.prisma | Added indexes for Project.ownerTeamId and TeamMember selection |
| apps/backend/prisma/migrations/*.sql | New migration for email outbox partial indices and new indexes |
| apps/e2e/tests/**/*.test.ts | Mass refactoring of tests replacing Auth.Otp.signIn with Auth.fastSignUp |
| .github/workflows/check-prisma-migrations.yaml | Updated to use prisma:dev script variant |
| docs/scripts/generate-functional-api-docs.mjs | Updated error message referencing new codegen command |
| docker/server/Dockerfile | Removed debug cat command for pnpm-lock.yaml |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function recordRequestStats(method: string, path: string, durationMs: number): void { | ||
| if (getNodeEnvironment() !== "development") { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The recordRequestStats function is called unconditionally in the smart-route-handler, but only records in development mode. This means every request in production will make an unnecessary function call and environment check. Consider moving the environment check to the handler level to avoid this overhead in production.
| // Wait a few seconds to make sure the server is fully started | ||
| await wait(5_000); |
There was a problem hiding this comment.
The wait(5_000) call adds a 5-second delay on every script startup. While the comment explains this is to ensure the server is fully started, this is a brittle solution. Consider using a retry mechanism or health check polling instead of a fixed delay, which would be both more reliable and potentially faster.
| expect(enableAppResponse).toMatchInlineSnapshot(` | ||
| NiceResponse { | ||
| "status": 200, | ||
| "body": {}, | ||
| "body": { "success": true }, |
There was a problem hiding this comment.
The response body expectation has changed from an empty object to include a "success" field. This is a good improvement for API clarity, but ensure this change is documented and that any API consumers are updated to expect this new response format.
| body: { config_override_string: JSON.stringify(config) }, | ||
| }); | ||
| expect(response.body).toMatchInlineSnapshot(`{}`); | ||
| expect(response.body).toMatchInlineSnapshot(`{ "success": true }`); |
There was a problem hiding this comment.
The test expectation has changed to remove the "config_override_string" property that was previously returned. However, the backend-helpers.ts file still expects the body to match a snapshot with "success: true". This inconsistency should be verified - either both should return success: true, or the snapshot expectations should be aligned.
| expect(response.body).toMatchInlineSnapshot(`{ "success": true }`); |
| "^build" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
The PR title mentions "fix(fortnox): use dedicated atomic locks collection for refresh + add concurrency tests" but none of the file changes in this PR appear to be related to Fortnox or atomic locks. The actual changes include:
- Removal of config CRUD handlers
- Cookie package version updates
- Test helper refactoring (Auth.Otp.signIn → Auth.fastSignUp)
- New dev-stats feature
- Database index additions
- Build script refactoring
This suggests either the PR description is incorrect or the Fortnox-related changes are missing from the diff.
| // note that we run this only in CI environments | ||
| it.runIf(!!process.env.CI)("completes successfully", async ({ expect }) => { |
There was a problem hiding this comment.
The test is now conditional on the CI environment variable. While this makes sense for reducing local test execution time, ensure that the setup wizard is still adequately tested in your CI pipeline. Also consider adding a comment explaining why this test is CI-only.
| "^build" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
The removal of the "generate-openapi-fumadocs" task from turbo.json doesn't appear to be coordinated with the changes in package.json, where the script "generate-openapi-fumadocs" is being removed and replaced with "codegen" script. This cache: false configuration removal might cause caching issues if the task still exists elsewhere in the codebase.
| // Create second user | ||
| const user2Mailbox = await bumpEmailAddress(); | ||
| const { userId: user2Id } = await Auth.Otp.signIn(); | ||
| const { userId: user2Id } = await Auth.fastSignUp(); |
There was a problem hiding this comment.
Unused variable user2Id.
| const { userId: user2Id } = await Auth.fastSignUp(); | |
| await Auth.fastSignUp(); |
|
|
||
| backendContext.set({ mailbox: createMailbox() }); | ||
| const user2 = await Auth.Otp.signIn(); | ||
| const user2 = await Auth.fastSignUp(); |
There was a problem hiding this comment.
Unused variable user2.
|
|
||
| it("lets users be on multiple teams", async ({ expect }) => { | ||
| const { userId: creatorUserId } = await Auth.Otp.signIn(); | ||
| const { userId: creatorUserId } = await Auth.fastSignUp(); |
There was a problem hiding this comment.
Unused variable creatorUserId.
Greptile SummaryPR Title Mismatch: The PR description mentions "fortnox" and "atomic locks collection", but the actual changes are unrelated - this appears to be a mislabeled PR. Actual Changes:
Key Improvements:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant RouteHandler as Smart Route Handler
participant StatsLib as dev-request-stats
participant ConfigAPI as /internal/config
participant DevStatsAPI as /dev-stats/api
participant DevStatsUI as /dev-stats (UI)
Note over Client,DevStatsUI: Request Flow with Stats Recording
Client->>RouteHandler: HTTP Request (any endpoint)
RouteHandler->>RouteHandler: Execute handler
RouteHandler->>StatsLib: recordRequestStats(method, path, duration)
StatsLib->>StatsLib: Update in-memory Map (dev only)
RouteHandler->>Client: Response
Note over Client,DevStatsUI: Dev Stats Viewing
DevStatsUI->>DevStatsAPI: GET /dev-stats/api
DevStatsAPI->>StatsLib: getAggregateStats()
DevStatsAPI->>StatsLib: getMostCommonRequests()
DevStatsAPI->>StatsLib: getMostTimeConsumingRequests()
DevStatsAPI->>StatsLib: getSlowestRequests()
StatsLib-->>DevStatsAPI: Stats data
DevStatsAPI-->>DevStatsUI: JSON response
DevStatsUI->>DevStatsUI: Render tables & charts
Note over Client,DevStatsUI: Config Endpoints Migration
Client->>ConfigAPI: GET /internal/config
ConfigAPI->>ConfigAPI: SmartRouteHandler (was CRUD)
ConfigAPI-->>Client: config_string
Client->>ConfigAPI: PATCH /internal/config/override
ConfigAPI->>ConfigAPI: SmartRouteHandler (was CRUD)
ConfigAPI->>ConfigAPI: Validate & save override
ConfigAPI-->>Client: success (no body)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
apps/backend/src/app/api/latest/internal/config/override/route.tsx, line 27-36 (link)style: Consider validating that
config_override_stringis not an empty string if that's not a valid input. Currently""would pass theifcheck but might not be intended behavior.
58 files reviewed, 1 comment
2 participants
This PR adds a dedicated collection with a unique index and TTL index, refactors to create/remove locks atomically during token refresh, and adds concurrency integration tests simulating lock collisions. Migration included: .