Merged
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
WalkthroughThe PR refactors user API routes to employ explicit schema validation through post/get handlers with AJV-compiled validators, extends API context types with connection and two-factor metadata, makes parameter properties optional in user query helpers, adds comprehensive E2E test coverage for validation scenarios, and introduces new parameter validator modules for users endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API.v1.post/get
participant Validator as AJV Validator
participant Handler as Route Handler
participant Response as Response Builder
Client->>API: POST/GET request with body/query
API->>Validator: Validate against compiled schema
alt Validation Success
Validator-->>API: ✓ Validation passed
API->>Handler: Execute handler with validated params
Handler->>Response: Generate success response
Response-->>API: Apply response schema
API-->>Client: 200 with typed response
else Validation Failed
Validator-->>API: ✗ Validation error
API-->>Client: 400 BadRequestErrorResponse
end
alt Authorization Check
Handler->>Handler: Check connection.token
alt Authorized
Handler->>Handler: Process request
else Forbidden
Handler-->>API: 403 response
API-->>Client: 403 with error details
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested labels
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 |
ggazzo
force-pushed
the
chore/users-endpoint-missing-tests
branch
from
95df55e to
6bea966
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/apis #39843 +/- ##
==============================================
- Coverage 70.57% 70.56% -0.01%
==============================================
Files 3256 3256
Lines 115791 115791
Branches 21039 21074 +35
==============================================
- Hits 81716 81710 -6
- Misses 32009 32020 +11
+ Partials 2066 2061 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…ernameSuggestion - Add test for forgotPassword with empty body expecting 400 with error-invalid-params - Add test for getUsernameSuggestion without auth expecting 401 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Migrate the following endpoints from addRoute to TypedAction: - users.deleteOwnAccount - users.sendWelcomeEmail - users.getPreferences - users.2fa.enableEmail - users.2fa.disableEmail - users.2fa.sendEmailCode - users.sendConfirmationEmail - users.removeOtherTokens - users.resetE2EKey - users.resetTOTP All endpoints now have response schemas, use body/query instead of validateParams, and include proper error response validators. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Migrate the following endpoints from addRoute to TypedAction: - users.delete - users.deactivateIdle - users.resetAvatar - users.checkUsernameAvailability - users.generatePersonalAccessToken - users.regeneratePersonalAccessToken - users.getPersonalAccessTokens - users.removePersonalAccessToken - users.requestDataDownload - users.logoutOtherClients - users.listTeams - users.logout All endpoints now have strict response schemas, body/query validators, and proper error response validators. Shared schemas extracted for reuse (tokenNameBodySchema, tokenResponseSchema). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Fixes temporal dead zone error where voidSuccessResponse was used in users.resetAvatar and users.removePersonalAccessToken before its const declaration. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…edAction Migrate the final 16 endpoints from addRoute to TypedAction: - users.getAvatar, users.update, users.updateOwnBasicInfo - users.setPreferences, users.setAvatar, users.create - users.setActiveStatus, users.info, users.list - users.listByStatus, users.register, users.presence - users.autocomplete, users.getPresence, users.setStatus - users.getStatus users.ts is now fully migrated (40/40 endpoints). Shared schemas extracted: userObjectResponse, statusType. All endpoints have response schemas and proper body/query validators. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
After migrating to TypedAction with body/query validators, the error type changes from 'invalid-params' to 'error-invalid-params'. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…red fields - Move voidSuccessResponse declaration before first usage (fixes TDZ crash) - Add 403 validateForbiddenErrorResponse to endpoints using forbidden(): setAvatar, list, listByStatus, logout, setStatus - Add authToken to required array in createToken response schema - Import validateForbiddenErrorResponse Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Document pending improvements for after endpoint migration: shared schema extraction, relaxed schema strengthening, explicit authRequired, OpenAPI tags, changesets, and test coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Add tests for endpoints that had no e2e coverage after migration: - users.createToken: 401 + 400 (missing required fields) - users.resetE2EKey: 401 + 400 (invalid properties) - users.resetTOTP: 401 + 400 (invalid properties) - users.2fa.enableEmail: 401 - users.2fa.disableEmail: 401 - users.2fa.sendEmailCode: 400 (missing emailOrUsername) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Add 24 unauthorized (401) tests and 11 invalid-params (400) tests across existing describe blocks for migrated user endpoints. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Create AJV query validators for endpoints that read from queryParams but had no query schema defined: - UsersGetAvatarParamsGET - UsersListParamsGET - UsersPresenceParamsGET - UsersRequestDataDownloadParamsGET - UsersGetPresenceParamsGET - UsersGetStatusParamsGET Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
These properties were available in SharedOptions and ActionThis but missing from TypedThis, causing TS errors when migrated endpoints accessed this.queryOperations. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
These properties were available in ActionThis but missing from TypedThis, causing TS errors when migrated endpoints like updateOwnBasicInfo called executeSaveUserProfile.call(this). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The 307 redirect return type is not supported by TypedAction's Results type. Reverting to addRoute until the framework supports custom status codes. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…onse error type - Add failure(result: unknown) overload to APIClass for catch blocks - Change BadRequestErrorResponse.error from string to unknown to support API.v1.failure(e) with unknown error values Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- W1: Remove unused isUsersGetAvatarParamsGET import - W2: Add comments explaining relaxed schemas (user object, preferences, exportOperation, array items) that vary by projection/permissions - W4: Add anyOf required constraint to users.delete body schema requiring at least one of userId, username, or user - W5: Make FindPaginatedUsersByStatusProps fields optional to match query params and remove unnecessary type casts Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Adjusted indentation for 'users' and 'preferences' properties in various API response schemas to ensure consistent formatting. - Updated the condition in the action function to correctly check user parameters.
…amsGET AJV 2020 does not allow 'nullable' without 'type'. The 'ids' field uses anyOf which has no top-level type, so nullable is invalid. The field is already optional via the TypeScript type. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The unknown overload was intercepting failure('string') calls before
the generic failure<T>. Removing it works because the generic already
accepts unknown and BadRequestErrorResponse.error is now typed as
unknown.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…ions - Revert users.list to addRoute: accepts arbitrary query filter fields that cannot be statically defined in a schema - Revert 3 POST endpoint test assertions back to 'invalid-params': POST body validation keeps the original errorType, only GET query validation changes to 'error-invalid-params' - Remove unused isUsersListParamsGET import Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
ggazzo
force-pushed
the
chore/users-endpoint-missing-tests
branch
from
01b6943 to
39c51ed
Compare
Password policy errors return details as an array of objects, but the schema only accepted string or object. Add array as a valid type. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…response The handler was passing the MongoDB UpdateResult directly to API.v1.success(), exposing internal fields (acknowledged, modifiedCount, etc.) and failing response validation with additionalProperties: false. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Changed errorType assertion from 'error-invalid-status' to 'invalid-params' for consistency in error handling. - Updated error message to remove redundant information while maintaining clarity. - Marked the '/users.removeOtherTokens' test suite as exclusive for focused testing.
AJV body validator now rejects invalid status values before the
handler runs, so the error message is from AJV ('must be equal to
one of the allowed values') not from the handler.
ggazzo
force-pushed
the
chore/users-endpoint-missing-tests
branch
from
2ba4258 to
7971cbb
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 13 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="docs/api-endpoint-migration.md">
<violation number="1" location="docs/api-endpoint-migration.md:770">
P2: The new paginated response example uses `{ type: 'array' }` without `items`, which weakens runtime validation and contradicts the checklist guidance in the same section.</violation>
</file>
<file name="packages/rest-typings/src/v1/users/UsersPresenceParamsGET.ts">
<violation number="1" location="packages/rest-typings/src/v1/users/UsersPresenceParamsGET.ts:11">
P2: `from` is typed as `string | undefined` but the schema allows `null`, creating a type/validation mismatch in the compiled type guard.</violation>
</file>
<file name="packages/rest-typings/src/v1/Ajv.ts">
<violation number="1" location="packages/rest-typings/src/v1/Ajv.ts:47">
P2: `error` type was widened to `unknown`, but the AJV schema still restricts it to `string`, causing inconsistent compile-time vs runtime validation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const paginatedUsersResponse = ajv.compile<{ users: IUser[]; count: number; offset: number; total: number }>({ | ||
| type: 'object', | ||
| properties: { | ||
| users: { type: 'array' }, |
Contributor
There was a problem hiding this comment.
P2: The new paginated response example uses { type: 'array' } without items, which weakens runtime validation and contradicts the checklist guidance in the same section.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/api-endpoint-migration.md, line 770:
<comment>The new paginated response example uses `{ type: 'array' }` without `items`, which weakens runtime validation and contradicts the checklist guidance in the same section.</comment>
<file context>
@@ -745,3 +745,95 @@ Weak types detected:
+const paginatedUsersResponse = ajv.compile<{ users: IUser[]; count: number; offset: number; total: number }>({
+ type: 'object',
+ properties: {
+ users: { type: 'array' },
+ count: { type: 'number' },
+ offset: { type: 'number' },
</file context>
Suggested change
| users: { type: 'array' }, | |
| users: { type: 'array', items: { $ref: '#/components/schemas/IUser' } }, |
| const UsersPresenceParamsGetSchema = { | ||
| type: 'object', | ||
| properties: { | ||
| from: { type: 'string', nullable: true }, |
Contributor
There was a problem hiding this comment.
P2: from is typed as string | undefined but the schema allows null, creating a type/validation mismatch in the compiled type guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/rest-typings/src/v1/users/UsersPresenceParamsGET.ts, line 11:
<comment>`from` is typed as `string | undefined` but the schema allows `null`, creating a type/validation mismatch in the compiled type guard.</comment>
<file context>
@@ -0,0 +1,19 @@
+const UsersPresenceParamsGetSchema = {
+ type: 'object',
+ properties: {
+ from: { type: 'string', nullable: true },
+ ids: {
+ anyOf: [{ type: 'string' }, { type: 'array', items: { type: 'string' } }],
</file context>
| type BadRequestErrorResponse = { | ||
| success: false; | ||
| error?: string; | ||
| error?: unknown; |
Contributor
There was a problem hiding this comment.
P2: error type was widened to unknown, but the AJV schema still restricts it to string, causing inconsistent compile-time vs runtime validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/rest-typings/src/v1/Ajv.ts, line 47:
<comment>`error` type was widened to `unknown`, but the AJV schema still restricts it to `string`, causing inconsistent compile-time vs runtime validation.</comment>
<file context>
@@ -44,10 +44,10 @@ export { ajv, ajvQuery };
type BadRequestErrorResponse = {
success: false;
- error?: string;
+ error?: unknown;
errorType?: string;
stack?: string;
</file context>
Suggested change
| error?: unknown; | |
| error?: string; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ge parameter
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
…ge parameter
Summary by CodeRabbit
Refactor
Bug Fixes
Documentation
Tests