Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughReplaces the Portal module with a new AdminPortal module: introduces Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Greptile SummaryThis PR renames Both previous review concerns are now addressed: the new Confidence Score: 5/5Safe to merge — the rename is clean, tests are comprehensive, and both prior review concerns have been addressed. All remaining findings are P2: the misleading auto-generated header comment and the intent optionality change. Neither blocks correct runtime behavior for any currently exercised code path. src/workos.ts and src/index.ts carry a misleading "do not edit" comment; src/admin-portal/interfaces/generate-link.interface.ts changed intent from required to optional. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant WorkOS as workos.adminPortal
participant Serializer as generateLink serializer
participant API as WorkOS API
Caller->>WorkOS: generateLink(payload)
Note over Caller,WorkOS: { intent, organization, returnUrl,<br/>intentOptions?, adminEmails? }
WorkOS->>Serializer: serializeGenerateLink(payload)
Note over Serializer: camelCase to snake_case conversion
Serializer-->>WorkOS: { intent, organization, return_url,<br/>intent_options?, admin_emails? }
WorkOS->>API: POST /portal/generate_link
API-->>WorkOS: PortalLinkResponseWire { link }
WorkOS->>Serializer: deserializePortalLinkResponse(data)
Serializer-->>WorkOS: PortalLinkResponse { link }
WorkOS-->>Caller: PortalLinkResponse { link }
Reviews (2): Last reviewed commit: "fix: address review comments on admin po..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1
to
+62
| // This file is auto-generated by oagen. Do not edit. | ||
|
|
||
| import { | ||
| deserializeSSOIntentOptions, | ||
| serializeSSOIntentOptions, | ||
| } from './serializers/sso-intent-options.serializer'; | ||
| import { | ||
| deserializeIntentOptions, | ||
| serializeIntentOptions, | ||
| } from './serializers/intent-options.serializer'; | ||
| import { | ||
| deserializeGenerateLink, | ||
| serializeGenerateLink, | ||
| } from './serializers/generate-link.serializer'; | ||
| import { | ||
| deserializePortalLinkResponse, | ||
| serializePortalLinkResponse, | ||
| } from './serializers/portal-link-response.serializer'; | ||
| import type { SSOIntentOptionsResponse } from './interfaces/sso-intent-options.interface'; | ||
| import type { IntentOptionsResponse } from './interfaces/intent-options.interface'; | ||
| import type { GenerateLinkResponse } from './interfaces/generate-link.interface'; | ||
| import type { PortalLinkResponseWire } from './interfaces/portal-link-response.interface'; | ||
| import sSOIntentOptionsFixture from './fixtures/sso-intent-options.fixture.json'; | ||
| import intentOptionsFixture from './fixtures/intent-options.fixture.json'; | ||
| import generateLinkFixture from './fixtures/generate-link.fixture.json'; | ||
| import portalLinkResponseFixture from './fixtures/portal-link-response.fixture.json'; | ||
|
|
||
| describe('SSOIntentOptionsSerializer', () => { | ||
| it('round-trips through serialize/deserialize', () => { | ||
| const fixture = sSOIntentOptionsFixture as SSOIntentOptionsResponse; | ||
| const deserialized = deserializeSSOIntentOptions(fixture); | ||
| const reserialized = serializeSSOIntentOptions(deserialized); | ||
| expect(reserialized).toEqual(expect.objectContaining(fixture)); | ||
| }); | ||
| }); | ||
|
|
||
| describe('IntentOptionsSerializer', () => { | ||
| it('round-trips through serialize/deserialize', () => { | ||
| const fixture = intentOptionsFixture as IntentOptionsResponse; | ||
| const deserialized = deserializeIntentOptions(fixture); | ||
| const reserialized = serializeIntentOptions(deserialized); | ||
| expect(reserialized).toEqual(expect.objectContaining(fixture)); | ||
| }); | ||
| }); | ||
|
|
||
| describe('GenerateLinkSerializer', () => { | ||
| it('round-trips through serialize/deserialize', () => { | ||
| const fixture = generateLinkFixture as GenerateLinkResponse; | ||
| const deserialized = deserializeGenerateLink(fixture); | ||
| const reserialized = serializeGenerateLink(deserialized); | ||
| expect(reserialized).toEqual(expect.objectContaining(fixture)); | ||
| }); | ||
| }); | ||
|
|
||
| describe('PortalLinkResponseSerializer', () => { | ||
| it('round-trips through serialize/deserialize', () => { | ||
| const fixture = portalLinkResponseFixture as PortalLinkResponseWire; | ||
| const deserialized = deserializePortalLinkResponse(fixture); | ||
| const reserialized = serializePortalLinkResponse(deserialized); | ||
| expect(reserialized).toEqual(expect.objectContaining(fixture)); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
AdminPortal.generateLink
The deleted src/portal/portal.spec.ts had nine HTTP-mock tests covering every intent value, error responses (400), and serialized request-body shape assertions. The new serializers.spec.ts only validates round-trip serialize/deserialize, leaving AdminPortal.generateLink (the actual HTTP call, query serialization, and new fields adminEmails / intentOptions) untested at the integration level. Consider porting the HTTP-mock tests to a new admin-portal.spec.ts, especially for the new parameters.
Contributor
There was a problem hiding this comment.
Fixed in 24b982b — added admin-portal.spec.ts with 9 HTTP-mock integration tests covering all 7 intent values, the new intentOptions/adminEmails parameters, and error handling (400 with invalid organization). This matches the coverage from the old portal.spec.ts.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/admin-portal/admin-portal.ts`:
- Around line 19-24: Update the JSDoc in src/admin-portal/admin-portal.ts for
the GenerateLink function to reflect the current payload and real error mapping:
change the `@param` description from "Object containing organization" to list the
actual payload fields (e.g., organization, intent options, adminEmails) and
their types/optional flags, and update the `@throws` block to match the errors
produced by src/workos.ts (remove or correct the undocumented
AuthorizationException 403 entry—either map it to the actual error thrown by
workos.ts or remove the 403 line if not returned; keep BadRequestException 400,
NotFoundException 404, and UnprocessableEntityException 422 as appropriate).
Ensure the JSDoc references the GenerateLink function name so consumers see the
accurate payload and exception behavior.
In `@src/admin-portal/fixtures/portal-link-response.fixture.json`:
- Line 2: The fixture's "link" value contains a JWT-like token that triggers
leak scanners; update the value for the "link" key in
portal-link-response.fixture.json to use a clearly fake placeholder token (e.g.,
replace the query param value with "REDACTED_TOKEN" or "fake-token-123") while
keeping the URL structure (https://setup.workos.com?token=...) so tests still
parse the URL format but no real/secret-looking token remains.
In `@src/admin-portal/serializers.spec.ts`:
- Around line 28-62: Replace the four near-identical describe/it blocks with a
single table-driven test: create an array of cases each containing a name (e.g.,
"SSOIntentOptions", "IntentOptions", "GenerateLink", "PortalLinkResponse"), the
corresponding fixture variable (sSOIntentOptionsFixture, intentOptionsFixture,
generateLinkFixture, portalLinkResponseFixture) and the pair of functions
(deserializeSSOIntentOptions/serializeSSOIntentOptions,
deserializeIntentOptions/serializeIntentOptions,
deserializeGenerateLink/serializeGenerateLink,
deserializePortalLinkResponse/serializePortalLinkResponse). Iterate the cases
with test.each or a forEach and for each case call the deserialize function with
the fixture, reserialize it with the matching serialize function, and assert
expect(reserialized).toEqual(expect.objectContaining(fixture)); this removes
duplication while keeping the same assertions and using the existing fixture and
serializer function names to locate code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fb47fccb-f965-44be-a77f-cf4cbc152cc8
📒 Files selected for processing (25)
src/admin-portal/admin-portal.tssrc/admin-portal/fixtures/generate-link.fixture.jsonsrc/admin-portal/fixtures/intent-options.fixture.jsonsrc/admin-portal/fixtures/portal-link-response.fixture.jsonsrc/admin-portal/fixtures/sso-intent-options.fixture.jsonsrc/admin-portal/interfaces/generate-link.interface.tssrc/admin-portal/interfaces/index.tssrc/admin-portal/interfaces/intent-options.interface.tssrc/admin-portal/interfaces/portal-link-response.interface.tssrc/admin-portal/interfaces/sso-intent-options.interface.tssrc/admin-portal/serializers.spec.tssrc/admin-portal/serializers/generate-link.serializer.tssrc/admin-portal/serializers/intent-options.serializer.tssrc/admin-portal/serializers/portal-link-response.serializer.tssrc/admin-portal/serializers/sso-intent-options.serializer.tssrc/common/interfaces/generate-link-intent.interface.tssrc/index.tssrc/index.worker.tssrc/portal/fixtures/generate-link-invalid.jsonsrc/portal/fixtures/generate-link.jsonsrc/portal/interfaces/generate-portal-link-intent.interface.tssrc/portal/interfaces/index.tssrc/portal/portal.spec.tssrc/portal/portal.tssrc/workos.ts
💤 Files with no reviewable changes (6)
- src/portal/fixtures/generate-link-invalid.json
- src/portal/fixtures/generate-link.json
- src/portal/interfaces/index.ts
- src/portal/portal.spec.ts
- src/portal/interfaces/generate-portal-link-intent.interface.ts
- src/portal/portal.ts
Comment on lines
+19
to
+24
| * @param payload - Object containing organization. | ||
| * @returns {Promise<PortalLinkResponse>} | ||
| * @throws {BadRequestException} 400 | ||
| * @throws {AuthorizationException} 403 | ||
| * @throws {NotFoundException} 404 | ||
| * @throws {UnprocessableEntityException} 422 |
There was a problem hiding this comment.
Update JSDoc to match current payload and error behavior.
Line 19 still says “Object containing organization,” but GenerateLink now includes additional fields (e.g., intent options/admin emails). Also, Line 22 documents AuthorizationException for 403, which is not explicitly mapped in src/workos.ts error handling. This can mislead SDK consumers.
📝 Suggested doc fix
- * `@param` payload - Object containing organization.
+ * `@param` payload - Admin Portal link generation parameters.
...
- * `@throws` {AuthorizationException} 403
+ * `@throws` {OauthException | GenericServerException} 403📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * @param payload - Object containing organization. | |
| * @returns {Promise<PortalLinkResponse>} | |
| * @throws {BadRequestException} 400 | |
| * @throws {AuthorizationException} 403 | |
| * @throws {NotFoundException} 404 | |
| * @throws {UnprocessableEntityException} 422 | |
| * `@param` payload - Admin Portal link generation parameters. | |
| * `@returns` {Promise<PortalLinkResponse>} | |
| * `@throws` {BadRequestException} 400 | |
| * `@throws` {OauthException | GenericServerException} 403 | |
| * `@throws` {NotFoundException} 404 | |
| * `@throws` {UnprocessableEntityException} 422 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin-portal/admin-portal.ts` around lines 19 - 24, Update the JSDoc in
src/admin-portal/admin-portal.ts for the GenerateLink function to reflect the
current payload and real error mapping: change the `@param` description from
"Object containing organization" to list the actual payload fields (e.g.,
organization, intent options, adminEmails) and their types/optional flags, and
update the `@throws` block to match the errors produced by src/workos.ts (remove
or correct the undocumented AuthorizationException 403 entry—either map it to
the actual error thrown by workos.ts or remove the 403 line if not returned;
keep BadRequestException 400, NotFoundException 404, and
UnprocessableEntityException 422 as appropriate). Ensure the JSDoc references
the GenerateLink function name so consumers see the accurate payload and
exception behavior.
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "link": "https://setup.workos.com?token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9..." | |||
There was a problem hiding this comment.
Replace secret-like token in fixture to avoid leak-scanner failures.
Line 2 uses a JWT-looking token string. Even in test fixtures, this pattern can be treated as exposed credentials and fail security checks. Use an obviously fake token value instead.
Suggested change
- "link": "https://setup.workos.com?token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9..."
+ "link": "https://setup.workos.com?token=test_admin_portal_token"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "link": "https://setup.workos.com?token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9..." | |
| "link": "https://setup.workos.com?token=test_admin_portal_token" |
🧰 Tools
🪛 Betterleaks (1.1.2)
[high] 2-2: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin-portal/fixtures/portal-link-response.fixture.json` at line 2, The
fixture's "link" value contains a JWT-like token that triggers leak scanners;
update the value for the "link" key in portal-link-response.fixture.json to use
a clearly fake placeholder token (e.g., replace the query param value with
"REDACTED_TOKEN" or "fake-token-123") while keeping the URL structure
(https://setup.workos.com?token=...) so tests still parse the URL format but no
real/secret-looking token remains.
Comment on lines
+28
to
+62
| describe('SSOIntentOptionsSerializer', () => { | ||
| it('round-trips through serialize/deserialize', () => { | ||
| const fixture = sSOIntentOptionsFixture as SSOIntentOptionsResponse; | ||
| const deserialized = deserializeSSOIntentOptions(fixture); | ||
| const reserialized = serializeSSOIntentOptions(deserialized); | ||
| expect(reserialized).toEqual(expect.objectContaining(fixture)); | ||
| }); | ||
| }); | ||
|
|
||
| describe('IntentOptionsSerializer', () => { | ||
| it('round-trips through serialize/deserialize', () => { | ||
| const fixture = intentOptionsFixture as IntentOptionsResponse; | ||
| const deserialized = deserializeIntentOptions(fixture); | ||
| const reserialized = serializeIntentOptions(deserialized); | ||
| expect(reserialized).toEqual(expect.objectContaining(fixture)); | ||
| }); | ||
| }); | ||
|
|
||
| describe('GenerateLinkSerializer', () => { | ||
| it('round-trips through serialize/deserialize', () => { | ||
| const fixture = generateLinkFixture as GenerateLinkResponse; | ||
| const deserialized = deserializeGenerateLink(fixture); | ||
| const reserialized = serializeGenerateLink(deserialized); | ||
| expect(reserialized).toEqual(expect.objectContaining(fixture)); | ||
| }); | ||
| }); | ||
|
|
||
| describe('PortalLinkResponseSerializer', () => { | ||
| it('round-trips through serialize/deserialize', () => { | ||
| const fixture = portalLinkResponseFixture as PortalLinkResponseWire; | ||
| const deserialized = deserializePortalLinkResponse(fixture); | ||
| const reserialized = serializePortalLinkResponse(deserialized); | ||
| expect(reserialized).toEqual(expect.objectContaining(fixture)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider table-driven tests to reduce duplication.
Current coverage is good; a parameterized pattern would make adding serializer cases simpler and keep this file smaller.
♻️ Optional refactor
+type RoundTripCase<T> = {
+ name: string;
+ fixture: T;
+ deserialize: (input: T) => any;
+ serialize: (input: any) => T;
+};
+
+const cases: RoundTripCase<any>[] = [
+ {
+ name: 'SSOIntentOptionsSerializer',
+ fixture: sSOIntentOptionsFixture as SSOIntentOptionsResponse,
+ deserialize: deserializeSSOIntentOptions,
+ serialize: serializeSSOIntentOptions,
+ },
+ {
+ name: 'IntentOptionsSerializer',
+ fixture: intentOptionsFixture as IntentOptionsResponse,
+ deserialize: deserializeIntentOptions,
+ serialize: serializeIntentOptions,
+ },
+ {
+ name: 'GenerateLinkSerializer',
+ fixture: generateLinkFixture as GenerateLinkResponse,
+ deserialize: deserializeGenerateLink,
+ serialize: serializeGenerateLink,
+ },
+ {
+ name: 'PortalLinkResponseSerializer',
+ fixture: portalLinkResponseFixture as PortalLinkResponseWire,
+ deserialize: deserializePortalLinkResponse,
+ serialize: serializePortalLinkResponse,
+ },
+];
+
+describe.each(cases)('$name', ({ fixture, deserialize, serialize }) => {
+ it('round-trips through serialize/deserialize', () => {
+ const deserialized = deserialize(fixture);
+ const reserialized = serialize(deserialized);
+ expect(reserialized).toEqual(expect.objectContaining(fixture));
+ });
+});
-
-describe('SSOIntentOptionsSerializer', () => {
- it('round-trips through serialize/deserialize', () => {
- const fixture = sSOIntentOptionsFixture as SSOIntentOptionsResponse;
- const deserialized = deserializeSSOIntentOptions(fixture);
- const reserialized = serializeSSOIntentOptions(deserialized);
- expect(reserialized).toEqual(expect.objectContaining(fixture));
- });
-});
-
-... repeated blocks ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin-portal/serializers.spec.ts` around lines 28 - 62, Replace the four
near-identical describe/it blocks with a single table-driven test: create an
array of cases each containing a name (e.g., "SSOIntentOptions",
"IntentOptions", "GenerateLink", "PortalLinkResponse"), the corresponding
fixture variable (sSOIntentOptionsFixture, intentOptionsFixture,
generateLinkFixture, portalLinkResponseFixture) and the pair of functions
(deserializeSSOIntentOptions/serializeSSOIntentOptions,
deserializeIntentOptions/serializeIntentOptions,
deserializeGenerateLink/serializeGenerateLink,
deserializePortalLinkResponse/serializePortalLinkResponse). Iterate the cases
with test.each or a forEach and for each case call the deserialize function with
the fixture, reserialize it with the matching serialize function, and assert
expect(reserialized).toEqual(expect.objectContaining(fixture)); this removes
duplication while keeping the same assertions and using the existing fixture and
serializer function names to locate code.
- Fix incorrect @throws {AuthorizationException} JSDoc (class doesn't exist) to @throws {UnauthorizedException} - Update @param description to reflect full payload parameters - Add GenerateLinkIntent barrel export from common/interfaces/index.ts - Add HTTP-mock integration tests for AdminPortal.generateLink covering all 7 intents, new parameters (intentOptions, adminEmails), and error handling Co-Authored-By: garen.torikian <garen.torikian@workos.com>
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.
Description
I'm pulling this out of #1535 ahead of the v9 release (#1558). Not only does it align with the rest of our SDKs, it matches what comes out of the OpenAPI spec, and thus, allows me to drop a bunch of workaround code in the emitters.
Users can upgrade to this by changing
workos.portalcalls toworkos.adminPortal. In addition,workos.adminPortal.generateLinkadds several parameters missing from the oldportalclass, likeintent_optionsandadmin_emails.Summary by CodeRabbit