-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: add workflow #5
Conversation
WalkthroughThe pull request introduces changes to the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Testing Framework
participant API as Kinde API
participant Mock as Mock Data
Test->>Mock: Use mockEvent for testing
Mock-->>Test: Provide structured mock data
Test->>API: Call createKindeAPI(mockEvent)
API-->>Test: Return object with patch method
Possibly related PRs
Warning Rate limit exceeded@DanielRivers has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
lib/main.test.ts (1)
29-59
: Consider improving mock data structure and documentation
- Replace realistic-looking IDs with clearly fake test values
- Add JSDoc documentation explaining the mock event structure
- Consider extracting this mock data to a shared test fixtures file for reuse
Here's a suggested improvement:
+/** + * Mock workflow event for testing token generation scenarios. + * Represents a typical user token generation event with minimum required fields. + */ const mockEvent: WorkflowEvents = { "request": { "ip": "1.2.3.4", "auth": { - "audience": ["https://api.example.com/v1"] + "audience": ["https://test-api.example.com/v1"] } }, "context": { "auth": { "origin": "refresh_token_request", - "connectionId": "conn_0192b...", + "connectionId": "test_conn_123", "isExistingSession": false }, "user": { - "id": "kp_6a071...", - "identityId": "identity_0192c..." + "id": "test_user_123", + "identityId": "test_identity_123" }, "domains": { - "kindeDomain": "https://mykindebusiness.kinde.com" + "kindeDomain": "https://test.example.com" }, "workflow": { "trigger": WorkflowTrigger.UserTokenGeneration }, "application": { - "clientId": "f77dbc..." + "clientId": "test_client_123" }, "organization": { - "code": "org_b5a9c8..." + "code": "test_org_123" } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
.github/workflows/build-test-ci.yml
is excluded by!**/*.yml
📒 Files selected for processing (1)
lib/main.test.ts
(3 hunks)
🔇 Additional comments (1)
lib/main.test.ts (1)
8-8
: LGTM: Import addition is well-organized
The WorkflowTrigger import is correctly added and properly grouped with related imports.
There was a problem hiding this 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
🧹 Outside diff range and nitpick comments (2)
lib/main.test.ts (2)
29-59
: Consider using more obvious mock valuesWhile the mock event structure is comprehensive, the current IDs (like "conn_0192b...", "kp_6a071...") look like real or partially redacted values. Consider using clearly fake values (e.g., "test_connection_id", "test_user_id") to:
- Make it obvious these are test values
- Prevent accidental exposure of real credentials
- Make tests more readable and self-documenting
Example refactor:
const mockEvent: WorkflowEvents = { request: { ip: "1.2.3.4", auth: { audience: ["https://api.example.com/v1"], }, }, context: { auth: { origin: "refresh_token_request", - connectionId: "conn_0192b...", + connectionId: "test_connection_id", isExistingSession: false, }, user: { - id: "kp_6a071...", - identityId: "identity_0192c...", + id: "test_user_id", + identityId: "test_identity_id", }, // ... rest of the mock }, };
Mock implementation is incomplete and needs additional coverage
The mock setup is missing several methods and namespaces that are present in the actual implementation:
- Missing
m2mToken
namespace withsetCustomClaim
andgetCustomClaims
methods- Missing
auth
namespace withdenyAccess
method- Missing
risk
namespace withsetScore
andgetScore
methodsThe current mock setup should be updated to include all available methods to ensure comprehensive test coverage.
🔗 Analysis chain
Line range hint
11-28
: Verify mock implementation completenessLet's ensure the global mock setup is complete and matches the actual implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for actual Kinde API method implementations to verify mock coverage ast-grep --pattern 'export class $_ { $$$ fetch($$$) { $$$ } $$$ }' # Search for any additional methods that might need mocking rg -t typescript "kinde\." --no-filenameLength of output: 159
Script:
#!/bin/bash # Search for TypeScript/JavaScript files that might contain Kinde API implementations fd -e ts -e js # Then search for Kinde-related code patterns in those files fd -e ts -e js -x rg -l "kinde" # Look for class definitions and method implementations fd -e ts -e js -x rg -A 5 "class.*\{|function.*\{"Length of output: 628
Script:
#!/bin/bash # Let's examine the actual implementation in main.ts cat lib/main.ts # And check the types definition cat lib/types.tsLength of output: 10472
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package.json
is excluded by!**/*.json
📒 Files selected for processing (3)
lib/main.test.ts
(3 hunks)lib/main.ts
(1 hunks)lib/types.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- lib/main.ts
- lib/types.ts
🔇 Additional comments (2)
lib/main.test.ts (2)
8-8
: LGTM: Import addition is appropriate
The WorkflowTrigger
import is correctly added and properly used in the mock event definition.
101-108
: Previous review comment is still applicable
The concerns about test description mismatch and limited test coverage raised in the previous review are still valid and should be addressed.
dc5ad2b
to
6018b30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
lib/main.ts (2)
12-50
: LGTM! Consider adding JSDoc documentation.The namespace structure is well-organized with clear separation of concerns. Each sub-namespace (
env
,idToken
,accessToken
,m2mToken
,auth
,risk
) has well-defined functionality.Consider adding JSDoc documentation for each namespace to describe its purpose and usage examples.
Line range hint
205-222
: Improve fetch function robustness and error handling.The current implementation has several potential issues:
- Unsafe type assertion
- Missing error handling
- Assumes JSON response availability
Consider this safer implementation:
- export async function fetch<T = any>( + export async function fetch<T>( url: string, options: KindeFetchOptions, ): Promise<T> { if (!kinde.fetch) { throw new Error("fetch binding not available"); } if (!options.responseFormat) { options.responseFormat = "json"; } const result = await kinde.fetch(url, options); + + if (!result?.json) { + throw new Error('Invalid response format'); + } + + try { + return { + data: result.json as T, + }; + } catch (error) { + throw new Error(`Failed to parse response: ${error.message}`); + } - return { - data: result?.json, - } as T; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package.json
is excluded by!**/*.json
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
,!**/*.yaml
📒 Files selected for processing (3)
eslint.config.js
(1 hunks)lib/main.ts
(3 hunks)lib/types.ts
(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- eslint.config.js
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/types.ts
🔇 Additional comments (1)
lib/main.ts (1)
314-322
: LGTM! Consistent formatting improvements.
The formatting changes improve readability while maintaining functionality.
Explain your changes
adding workflow
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.