Third party developers #23
Conversation
WalkthroughThis pull request updates the SDK to require an additional client identifier ( Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant OSP as OpenSecretProvider
participant API as API Service
U->>OSP: Provide apiUrl & clientId
OSP->>OSP: Validate clientId presence
OSP->>API: signIn(email, password, clientId)
API-->>OSP: Return login response
OSP-->>U: Deliver authenticated session
sequenceDiagram
participant Client
participant Verifier as verifyAttestation
participant Fake as fakeAuthenticate
participant Real as authenticate
Client->>Verifier: Submit attestation document
Verifier->>Verifier: Check if running in local environment
alt Local environment
Verifier->>Fake: Process attestation
Fake-->>Verifier: Return fake attestation result
else Production
Verifier->>Real: Process attestation
Real-->>Verifier: Return attestation result
end
Verifier-->>Client: Return verification outcome
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/lib/api.test.ts (1)
85-127: Consider re-enabling guest conversion tests.The guest conversion tests are commented out due to email sending. Consider mocking the email service to enable these tests.
Would you like me to help implement a mock email service to re-enable these tests?
src/lib/attestation.ts (1)
268-273: Consider using environment variable for development mode.Instead of hardcoding development URLs, consider using an environment variable to determine the development mode.
- if ( - apiUrl && - (apiUrl === "http://127.0.0.1:3000" || - apiUrl === "http://localhost:3000" || - apiUrl === "http://0.0.0.0:3000") - ) { + if (apiUrl && process.env.NODE_ENV === 'development') {src/lib/signing.test.ts (2)
15-32: Consider addressing potential race condition.While the delay helps ensure tokens are set, it might indicate an underlying race condition. Consider implementing a more robust solution, such as using events or promises to track token setup completion.
- // Add a small delay to ensure tokens are properly set - await new Promise((resolve) => setTimeout(resolve, 100)); + // Wait for token to be available + let attempts = 0; + const maxAttempts = 10; + while (!window.localStorage.getItem("access_token") && attempts < maxAttempts) { + await new Promise((resolve) => setTimeout(resolve, 50)); + attempts++; + }🧰 Tools
🪛 ESLint
[error] 29-29: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
132-248: Consider optimizing test execution.The test includes delays between API calls, suggesting possible rate limiting. Consider:
- Documenting the rate limits in comments
- Using test doubles for rate-limited endpoints
- Implementing parallel test execution where possible
src/lib/getAttestation.ts (1)
28-31: Parameter Formatting & Unused Parameter NoticeThe reformatting of the function signature for
getAttestationimproves readability. However, note that the newapiUrlparameter is defined but not used within the function body. If the parameter is intended for future enhancements or external configurations, consider adding an inline comment (or TODO) to clarify its purpose. Otherwise, it might be prudent to remove it to avoid confusion.🧰 Tools
🪛 ESLint
[error] 30-30: 'apiUrl' is defined but never used.
(@typescript-eslint/no-unused-vars)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.md(2 hunks)package.json(1 hunks)src/global.d.ts(1 hunks)src/lib/ai.test.ts(4 hunks)src/lib/api.test.ts(6 hunks)src/lib/api.ts(9 hunks)src/lib/attestation.ts(2 hunks)src/lib/attestationForView.ts(3 hunks)src/lib/getAttestation.ts(1 hunks)src/lib/main.tsx(16 hunks)src/lib/pcr.ts(2 hunks)src/lib/signing.test.ts(11 hunks)src/lib/test/api-url-loader.ts(1 hunks)src/lib/test/utils.ts(1 hunks)src/main.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- src/lib/test/utils.ts
- src/lib/test/api-url-loader.ts
- src/lib/attestationForView.ts
- src/global.d.ts
- src/lib/pcr.ts
🧰 Additional context used
🪛 GitHub Check: test
src/lib/api.test.ts
[failure] 20-20: error: Test credentials must be set in .env.local
at /home/runner/work/OpenSecret-SDK/OpenSecret-SDK/src/lib/api.test.ts:20:9
src/lib/signing.test.ts
[failure] 12-12: error: Test credentials must be set in .env.local
at /home/runner/work/OpenSecret-SDK/OpenSecret-SDK/src/lib/signing.test.ts:12:9
src/lib/ai.test.ts
[failure] 12-12: error: Test credentials must be set in .env.local
at /home/runner/work/OpenSecret-SDK/OpenSecret-SDK/src/lib/ai.test.ts:12:9
🪛 GitHub Actions: Library Tests
src/lib/api.test.ts
[error] 1-1: Unhandled error between tests.
🪛 ESLint
src/lib/signing.test.ts
[error] 29-29: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/lib/getAttestation.ts
[error] 30-30: 'apiUrl' is defined but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (19)
src/main.tsx (1)
9-12: Verify environment variable setup.The
VITE_CLIENT_IDenvironment variable is now required. Ensure it's properly configured in your deployment environments.Run this script to check environment variable setup:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if VITE_CLIENT_ID is set in environment files. # Test: Search for VITE_CLIENT_ID in environment files. Expect: At least one occurrence. fd -t f '\.env' -x grep -l 'VITE_CLIENT_ID'Length of output: 43
ENV Variable Setup Requires Manual Verification
The automated search did not locate any occurrence of
VITE_CLIENT_IDin repository environment files. This could mean one of two things:
- The variable is configured externally in your deployment/CI environment rather than in version-controlled
.envfiles.- The variable setup is missing and needs to be added.
Please manually confirm that
VITE_CLIENT_IDis correctly configured in the intended deployment environment.src/lib/ai.test.ts (2)
8-13: LGTM! Environment variable check is comprehensive.The check for required test credentials is thorough and includes the new
TEST_CLIENT_ID.🧰 Tools
🪛 GitHub Check: test
[failure] 12-12: error: Test credentials must be set in .env.local
at /home/runner/work/OpenSecret-SDK/OpenSecret-SDK/src/lib/ai.test.ts:12:9
21-25: LGTM! Authentication flow updated correctly.The
fetchLogincall has been properly updated to include the requiredTEST_CLIENT_IDparameter.src/lib/api.test.ts (1)
19-21: LGTM! Environment variable check is comprehensive.The check for required test credentials is thorough and includes the new
TEST_CLIENT_ID.🧰 Tools
🪛 GitHub Check: test
[failure] 20-20: error: Test credentials must be set in .env.local
at /home/runner/work/OpenSecret-SDK/OpenSecret-SDK/src/lib/api.test.ts:20:9src/lib/attestation.ts (1)
243-245: LGTM! Schema definition is clear and focused.The
FakeAttestationDocumentSchemais well-defined and appropriately scoped for local development.src/lib/signing.test.ts (3)
9-13: LGTM! Good validation of test credentials.Early validation of required test credentials with a clear error message improves the developer experience.
🧰 Tools
🪛 GitHub Check: test
[failure] 12-12: error: Test credentials must be set in .env.local
at /home/runner/work/OpenSecret-SDK/OpenSecret-SDK/src/lib/signing.test.ts:12:9
64-96: LGTM! Comprehensive test coverage.The test thoroughly validates the signature format, hash matching, and cryptographic verification using noble/curves.
250-289: LGTM! Good test coverage for edge cases.The tests effectively cover authentication failures, signature uniqueness, and public key consistency.
src/lib/api.ts (3)
550-569: LGTM! Clean implementation of optional parameters.Good use of the spread operator for optional parameters and clean type definitions.
236-276: Breaking change: Added client_id to password reset functions.The addition of the required
client_idparameter to password reset functions is a breaking change. The error handling is well-structured with clear error messages.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for existing usages that need to be updated rg -A 2 "requestPasswordReset|confirmPasswordReset" --type tsLength of output: 1192
Breaking Change Confirmed: New
client_idParameter
- Both
requestPasswordResetandconfirmPasswordResetnow require theclient_idparameter.- This change is breaking as it forces all callers (e.g., in
src/lib/main.tsx) to provide this additional argument.- The error messages remain clear and well-structured.
45-107: Breaking change: Added required client_id parameter.The addition of the required
client_idparameter to authentication functions is a breaking change. Ensure this is documented in the changelog and migration guide.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for existing usages that need to be updated rg -A 2 "fetchLogin|fetchGuestLogin|fetchSignUp|fetchGuestSignUp" --type tsLength of output: 4476
Breaking Change Detected:
client_idas a Required ParameterThe grep results confirm that all usages of the authentication functions in the tests (and in the main code) have been updated to include the new required
client_idparameter. This breaking change is correctly propagated across the codebase.
Please ensure that the changelog and migration guide are updated accordingly to reflect this breaking change.src/lib/main.tsx (3)
21-385: LGTM! Well-documented context type definitions.The addition of
clientIdto the context is well-documented with clear JSDoc comments explaining its purpose and usage.
413-478: LGTM! Good input validation.The provider properly validates both
apiUrlandclientIdwith clear error messages.
483-552: LGTM! Consistent error handling.The authentication methods consistently handle errors and include helpful logging.
package.json (1)
23-24: LGTM! Good addition of formatting scripts.The addition of Prettier formatting scripts with comprehensive file patterns will help maintain consistent code style.
README.md (4)
15-17: Documentation Clarity Improved for Usage InstructionsThe added lines now clearly list both the backend URL and the project’s client ID as required props. This helps users understand the new requirements for the provider.
24-27: Correct JSX Provider Instantiation UpdateThe updated code snippet now correctly passes both
apiUrlandclientIdto the<OpenSecretProvider>. This aligns the usage example with the new prop requirements.
60-62: Enhanced API Reference DescriptionThe updated text in the API Reference section now accurately describes the two essential props:
apiUrlandclientId. This reinforces the configuration requirements presented in the usage section.
65-68: Consistent Example in API ReferenceThe updated code snippet example in the API Reference correctly demonstrates the updated component usage with both
apiUrlandclientId. The consistency between usage and reference is maintained.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/lib/ai.test.ts (3)
11-13: Enhance error message with specific missing variables.The error message could be more helpful by indicating which specific environment variables are missing.
- throw new Error("Test credentials must be set in .env.local"); + const missingVars = [ + !TEST_EMAIL && "VITE_TEST_EMAIL", + !TEST_PASSWORD && "VITE_TEST_PASSWORD", + !TEST_CLIENT_ID && "VITE_TEST_CLIENT_ID", + !API_URL && "VITE_OPEN_SECRET_API_URL" + ].filter(Boolean); + throw new Error(`Missing required environment variables: ${missingVars.join(", ")}`);🧰 Tools
🪛 GitHub Check: test
[failure] 12-12: error: Test credentials must be set in .env.local
at /home/runner/work/OpenSecret-SDK/OpenSecret-SDK/src/lib/ai.test.ts:12:9
43-46: Consider making the model name configurable.The model name is hardcoded, which could make the tests brittle if the model changes.
+const TEST_MODEL = process.env.VITE_TEST_MODEL || "ibnzterrell/Meta-Llama-3.3-70B-Instruct-AWQ-INT4"; - const model = "ibnzterrell/Meta-Llama-3.3-70B-Instruct-AWQ-INT4"; + const model = TEST_MODEL;
77-78: Remove model name duplication.The model name is duplicated from the test case. Consider extracting it to a shared constant.
+const TEST_MODEL = process.env.VITE_TEST_MODEL || "ibnzterrell/Meta-Llama-3.3-70B-Instruct-AWQ-INT4"; ... - model: "ibnzterrell/Meta-Llama-3.3-70B-Instruct-AWQ-INT4", + model: TEST_MODEL,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/ai.test.ts(4 hunks)
🧰 Additional context used
🪛 GitHub Check: test
src/lib/ai.test.ts
[failure] 12-12: error: Test credentials must be set in .env.local
at /home/runner/work/OpenSecret-SDK/OpenSecret-SDK/src/lib/ai.test.ts:12:9
🔇 Additional comments (3)
src/lib/ai.test.ts (3)
15-18: LGTM!The
ChatMessagetype definition is clear and well-structured.
109-116: LGTM!The test case is well-structured and properly validates the streaming functionality.
20-28:✅ Verification successful
LGTM! Verify token storage.
The function correctly handles the new client ID parameter and token storage.
🏁 Script executed:
#!/bin/bash # Description: Verify that the tokens are properly used in other test files. # Search for localStorage.setItem usage with tokens rg -A 2 'localStorage\.setItem.*token'Length of output: 3271
LGTM – Token Storage Confirmed
The
setupTestUserfunction now correctly integrates theTEST_CLIENT_IDparameter and reliably stores tokens. A repository-wide search confirms that all instances (in files likesrc/lib/ai.test.ts,src/lib/main.tsx, andsrc/lib/api.test.ts) correctly uselocalStorage.setItemfor token storage. No further changes are required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
60-62: API Reference Enhancements:
The API reference now explicitly states that theOpenSecretProviderrequires two props—apiUrlandclientId. The description forclientIdclearly mentions that it is a UUID that identifies the project, which is very helpful. You might consider emphasizing that theclientIdmust be a valid UUID for added clarity.
80-82: Authentication Methods Documentation Update:
The modifications to the authentication method descriptions (forsignInGuest,signUpGuest, andconvertGuestToUserAccount) now indicate that guest account operations are scoped based onclientId. This detail is both informative and necessary given the current SDK changes. As a minor nitpick, please review and ensure consistent punctuation (for example, that each descriptive sentence ends with a period) for uniformity.🧰 Tools
🪛 LanguageTool
[uncategorized] ~80-~80: Loose punctuation mark.
Context: ...tring, password: string): Promise`: Signs in a guest user with their ID and...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~81-~81: Loose punctuation mark.
Context: ...teCode: string): Promise`: Creates a new guest account with just a...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~82-~82: Loose punctuation mark.
Context: ...: string, name?: string): Promise`: Converts current guest account to a reg...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(3 hunks)src/lib/api.ts(11 hunks)src/lib/main.tsx(20 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/api.ts
- src/lib/main.tsx
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~80-~80: Loose punctuation mark.
Context: ...tring, password: string): Promise`: Signs in a guest user with their ID and...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~81-~81: Loose punctuation mark.
Context: ...teCode: string): Promise`: Creates a new guest account with just a...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~82-~82: Loose punctuation mark.
Context: ...: string, name?: string): Promise`: Converts current guest account to a reg...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (2)
README.md (2)
15-17: Usage Instructions Update:
The updated instructions clearly list the required props—apiUrlandclientId—for wrapping your application in theOpenSecretProvidercomponent. This added detail improves clarity for developers setting up the SDK.
24-27: Updated Provider Usage Example:
The usage example now correctly demonstrates how to pass both theapiUrland the newclientIdprop. This aligns perfectly with the SDK’s new requirements. Consider adding a brief note that these are placeholder values meant to be replaced in real configurations.
AnthonyRonning
force-pushed
the
add-client-id
branch
from
910b57f to
96eeddc
Compare
AnthonyRonning
force-pushed
the
add-client-id
branch
from
96eeddc to
1fcc7a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/lib/signing.test.ts (1)
29-29: Improve type safety by replacing 'any' with a specific error type.Consider using a more specific type for error handling instead of 'any'.
- } catch (error: any) { + } catch (error: Error | unknown) {🧰 Tools
🪛 ESLint
[error] 29-29: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/lib/api.ts (1)
236-253: Consider enhancing error messages for password reset functions.While the
client_idintegration is correct, the error messages could be more specific to help with debugging.- "Failed to request password reset" + "Failed to request password reset. Please verify your email and client ID." - "Failed to confirm password reset" + "Failed to confirm password reset. Please verify your credentials and client ID."Also applies to: 255-276
src/lib/main.tsx (3)
24-28: Consider enhancing clientId validation.While the provider validates that
clientIdis non-empty, it should also validate the UUID format.+ const UUID_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; if (!clientId || clientId.trim() === "") { throw new Error( "OpenSecretProvider requires a non-empty clientId. Please provide a valid project UUID." ); } + if (!UUID_REGEX.test(clientId)) { + throw new Error( + "OpenSecretProvider requires a valid UUID for clientId. Please provide a valid UUID v4." + ); + }Also applies to: 435-447
488-499: Consider enhancing error handling for authentication methods.While the implementation correctly uses
clientId, the error handling could be more specific about client ID related issues.} catch (error) { console.error(error); + if (error instanceof Error && error.message.includes("client_id")) { + throw new Error("Authentication failed: Invalid or unauthorized client ID."); + } throw error; }Also applies to: 501-517, 519-530, 532-547
495-498: Consider implementing structured error handling.The current error handling could be improved by implementing structured error types for better error management and user feedback.
type AuthError = { code: 'INVALID_CLIENT_ID' | 'UNAUTHORIZED' | 'NETWORK_ERROR'; message: string; details?: unknown; }; function handleAuthError(error: unknown): AuthError { if (error instanceof Error) { if (error.message.includes("client_id")) { return { code: 'INVALID_CLIENT_ID', message: 'Invalid or unauthorized client ID.' }; } // Add more error type checks } return { code: 'NETWORK_ERROR', message: 'An unexpected error occurred.', details: error }; }Also applies to: 513-516, 526-529, 543-546
README.md (1)
15-18: Consider adding security best practices for client ID handling.While the documentation clearly explains the client ID requirement, it would be helpful to add security best practices.
Add the following security notes:
### Security Best Practices - Store your client ID securely and never expose it in client-side source code - Use environment variables to inject the client ID during build/runtime - Regularly audit client ID usage and rotate if compromisedAlso applies to: 60-63
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
README.md(3 hunks)package.json(1 hunks)src/AI.tsx(5 hunks)src/App.css(1 hunks)src/App.tsx(10 hunks)src/global.d.ts(1 hunks)src/lib/ai.test.ts(4 hunks)src/lib/ai.ts(1 hunks)src/lib/api.test.ts(6 hunks)src/lib/api.ts(11 hunks)src/lib/attestation.ts(2 hunks)src/lib/attestationForView.ts(3 hunks)src/lib/getAttestation.ts(1 hunks)src/lib/main.tsx(20 hunks)src/lib/pcr.ts(2 hunks)src/lib/signing.test.ts(11 hunks)src/lib/test/api-url-loader.ts(1 hunks)src/lib/test/der-loader.ts(1 hunks)src/lib/test/utils.ts(1 hunks)src/main.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- src/lib/test/utils.ts
- src/lib/test/der-loader.ts
- package.json
- src/lib/test/api-url-loader.ts
- src/global.d.ts
- src/lib/ai.ts
- src/main.tsx
- src/AI.tsx
- src/lib/attestationForView.ts
- src/App.css
- src/lib/attestation.ts
- src/lib/pcr.ts
- src/App.tsx
🧰 Additional context used
🪛 GitHub Check: test
src/lib/api.test.ts
[failure] 20-20: error: Test credentials must be set in .env.local
at /home/runner/work/OpenSecret-SDK/OpenSecret-SDK/src/lib/api.test.ts:20:9
src/lib/signing.test.ts
[failure] 12-12: error: Test credentials must be set in .env.local
at /home/runner/work/OpenSecret-SDK/OpenSecret-SDK/src/lib/signing.test.ts:12:9
src/lib/ai.test.ts
[failure] 12-12: error: Test credentials must be set in .env.local
at /home/runner/work/OpenSecret-SDK/OpenSecret-SDK/src/lib/ai.test.ts:12:9
🪛 GitHub Actions: Library Tests
src/lib/api.test.ts
[error] 1-1: Unhandled error between tests
🪛 ESLint
src/lib/signing.test.ts
[error] 29-29: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/lib/getAttestation.ts
[error] 30-30: 'apiUrl' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 LanguageTool
README.md
[uncategorized] ~80-~80: Loose punctuation mark.
Context: ...tring, password: string): Promise`: Signs in a guest user with their ID and...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~81-~81: Loose punctuation mark.
Context: ...teCode: string): Promise`: Creates a new guest account with just a...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~82-~82: Loose punctuation mark.
Context: ...: string, name?: string): Promise`: Converts current guest account to a reg...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (12)
src/lib/getAttestation.ts (2)
28-31: Address unused parameter.The
apiUrlparameter is defined but never used in the function implementation.Do you want me to help implement the
apiUrlparameter usage or should it be removed?🧰 Tools
🪛 ESLint
[error] 30-30: 'apiUrl' is defined but never used.
(@typescript-eslint/no-unused-vars)
20-20: LGTM!The formatting change improves readability without affecting functionality.
src/lib/ai.test.ts (2)
8-13: LGTM!The credential validation ensures all required test credentials are present.
🧰 Tools
🪛 GitHub Check: test
[failure] 12-12: error: Test credentials must be set in .env.local
at /home/runner/work/OpenSecret-SDK/OpenSecret-SDK/src/lib/ai.test.ts:12:9
21-25: LGTM!The login function is correctly updated to include the new client ID parameter.
src/lib/api.test.ts (3)
17-21: LGTM!The credential validation ensures all required test credentials are present.
🧰 Tools
🪛 GitHub Check: test
[failure] 20-20: error: Test credentials must be set in .env.local
at /home/runner/work/OpenSecret-SDK/OpenSecret-SDK/src/lib/api.test.ts:20:9
23-32: LGTM!The email login and signup functions are correctly updated to include the client ID parameter.
63-84: LGTM!The guest signup and login flow correctly integrates the client ID parameter.
src/lib/signing.test.ts (1)
15-31: LGTM!The enhanced error handling in setupTestUser provides better debugging information.
🧰 Tools
🪛 ESLint
[error] 29-29: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/lib/api.ts (3)
45-54: LGTM! Authentication functions properly updated to include client_id.The changes consistently add the
client_idparameter across all authentication functions, with proper type definitions and request payload updates.Also applies to: 57-66, 69-92, 94-106
291-311: LGTM! OAuth functions properly handle client_id with comprehensive error handling.The implementation correctly:
- Adds
client_idto both GitHub and Google OAuth initiation- Maintains detailed error handling with specific error messages
Also applies to: 361-381
133-182: LGTM! Unmodified functions maintain their functionality.The key-value storage and user management functions correctly maintain their existing implementation, as they don't require client identification.
README.md (1)
80-83: LGTM! Clear documentation of guest account scoping.The documentation clearly explains how guest accounts are scoped to specific projects using
clientId.🧰 Tools
🪛 LanguageTool
[uncategorized] ~80-~80: Loose punctuation mark.
Context: ...tring, password: string): Promise`: Signs in a guest user with their ID and...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~81-~81: Loose punctuation mark.
Context: ...teCode: string): Promise`: Creates a new guest account with just a...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~82-~82: Loose punctuation mark.
Context: ...: string, name?: string): Promise`: Converts current guest account to a reg...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~83-~83: Loose punctuation mark.
Context: ...ated under. -signOut(): Promise<void>: Signs out the current user. #### Key-V...(UNLIKELY_OPENING_PUNCTUATION)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/lib/signing.test.ts (1)
29-31: Improve type safety by replacinganywith a specific error type.Consider using a more specific type for error handling instead of
any.- } catch (error: any) { + } catch (error: unknown) { + const errorMessage = error instanceof Error ? error.message : String(error); - throw new Error("Failed to set up test user: " + error.message); + throw new Error("Failed to set up test user: " + errorMessage);🧰 Tools
🪛 ESLint
[error] 29-29: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/lib/api.ts (2)
69-92: Consider reordering parameters to maintain consistency.The
client_idparameter is placed before the optionalnameparameter. While this works, it would be more consistent with TypeScript conventions to group required parameters first, followed by optional parameters.-export async function fetchSignUp( - email: string, - password: string, - inviteCode: string, - client_id: string, - name?: string | null -): Promise<LoginResponse> { +export async function fetchSignUp( + email: string, + password: string, + inviteCode: string, + name?: string | null, + client_id: string +): Promise<LoginResponse> {
236-252: Consider using a type for the reset data payload.The reset data structure could be reused across functions. Consider extracting it to a type definition.
+type PasswordResetRequestData = { + email: string; + hashed_secret: string; + client_id: string; +}; export async function requestPasswordReset( email: string, hashedSecret: string, client_id: string ): Promise<void> { - const resetData = { - email, - hashed_secret: hashedSecret, - client_id - }; + const resetData: PasswordResetRequestData = { + email, + hashed_secret: hashedSecret, + client_id + };src/lib/main.tsx (1)
663-670: Consider extracting password reset function types.The password reset function types could be extracted to improve readability and reusability.
+type PasswordResetRequest = (email: string, hashedSecret: string) => Promise<void>; +type PasswordResetConfirmation = ( + email: string, + alphanumericCode: string, + plaintextSecret: string, + newPassword: string +) => Promise<void>; - requestPasswordReset: (email: string, hashedSecret: string) => - api.requestPasswordReset(email, hashedSecret, clientId), - confirmPasswordReset: ( - email: string, - alphanumericCode: string, - plaintextSecret: string, - newPassword: string - ) => api.confirmPasswordReset(email, alphanumericCode, plaintextSecret, newPassword, clientId), + requestPasswordReset: ((email, hashedSecret) => + api.requestPasswordReset(email, hashedSecret, clientId)) as PasswordResetRequest, + confirmPasswordReset: ((email, alphanumericCode, plaintextSecret, newPassword) => + api.confirmPasswordReset( + email, + alphanumericCode, + plaintextSecret, + newPassword, + clientId + )) as PasswordResetConfirmation,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/test.yml(1 hunks)README.md(3 hunks)package.json(1 hunks)src/AI.tsx(5 hunks)src/App.css(1 hunks)src/App.tsx(10 hunks)src/global.d.ts(1 hunks)src/lib/ai.test.ts(4 hunks)src/lib/ai.ts(1 hunks)src/lib/api.test.ts(6 hunks)src/lib/api.ts(11 hunks)src/lib/attestation.ts(2 hunks)src/lib/attestationForView.ts(3 hunks)src/lib/getAttestation.ts(1 hunks)src/lib/main.tsx(20 hunks)src/lib/pcr.ts(2 hunks)src/lib/signing.test.ts(11 hunks)src/lib/test/api-url-loader.ts(1 hunks)src/lib/test/der-loader.ts(1 hunks)src/lib/test/utils.ts(1 hunks)src/main.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- src/lib/test/api-url-loader.ts
- src/lib/test/der-loader.ts
- src/lib/attestationForView.ts
- src/global.d.ts
- src/lib/test/utils.ts
- src/lib/ai.ts
- src/main.tsx
- src/lib/attestation.ts
- package.json
- src/AI.tsx
- src/App.css
- src/App.tsx
- src/lib/pcr.ts
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~80-~80: Loose punctuation mark.
Context: ...tring, password: string): Promise`: Signs in a guest user with their ID and...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~81-~81: Loose punctuation mark.
Context: ...teCode: string): Promise`: Creates a new guest account with just a...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~82-~82: Loose punctuation mark.
Context: ...: string, name?: string): Promise`: Converts current guest account to a reg...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~82-~82: The preposition “for” seems more likely in this position.
Context: ...ed with the same project it was created under. - signOut(): Promise<void>: Signs ou...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION)
🪛 ESLint
src/lib/getAttestation.ts
[error] 30-30: 'apiUrl' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/lib/signing.test.ts
[error] 29-29: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🔇 Additional comments (12)
src/lib/getAttestation.ts (1)
28-31: Use theapiUrlparameter or remove it.The
apiUrlparameter is defined but never used in the function. If this parameter is intended to be used for making API calls, please implement its usage. Otherwise, consider removing it.🧰 Tools
🪛 ESLint
[error] 30-30: 'apiUrl' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/lib/ai.test.ts (1)
8-13: LGTM!Good addition of
TEST_CLIENT_IDvalidation and proper error handling for missing environment variables.src/lib/api.test.ts (1)
17-21: LGTM!Good integration of
TEST_CLIENT_IDacross all authentication flows, including email and guest user scenarios.src/lib/signing.test.ts (1)
9-13: LGTM!Good addition of
TEST_CLIENT_IDvalidation and proper error handling for missing environment variables.src/lib/api.ts (4)
45-54: LGTM! The client_id parameter is correctly added to fetchLogin.The function signature and payload are properly updated to include the new client_id parameter.
57-66: LGTM! The client_id parameter is correctly added to fetchGuestLogin.The function signature and payload are properly updated to include the new client_id parameter.
361-380:❓ Verification inconclusive
Verify Google OAuth integration with client_id parameter.
The Google OAuth flow has been updated to include client_id. Ensure this change is coordinated with the Google OAuth application settings.
🏁 Script executed:
#!/bin/bash # Description: Check if Google OAuth callback endpoint handles client_id parameter # Search for Google OAuth callback handler implementation rg -A 10 "google/callback"Length of output: 495
Google OAuth Integration: Confirm Coordination on the
client_idUpdateThe updated
initiateGoogleAuthfunction now correctly includes theclient_idin its POST payload to${apiUrl}/auth/google. A search also confirms that the corresponding callback endpoint (${apiUrl}/auth/google/callback) exists and is invoked withcallbackData. Please verify that:
- The server-side logic for the Google OAuth callback is updated as needed to handle (or safely ignore) the
client_id—if its management is required on this endpoint.- The Google OAuth application settings (in the Google API Console) are coordinated with this change to ensure consistency across the OAuth flow.
291-310:❓ Verification inconclusive
Verify GitHub OAuth integration with client_id parameter.
The GitHub OAuth flow has been updated to include client_id. Ensure this change is coordinated with the GitHub OAuth application settings.
🏁 Script executed:
#!/bin/bash # Description: Check if GitHub OAuth callback endpoint handles client_id parameter # Search for GitHub OAuth callback handler implementation rg -A 10 "github/callback"Length of output: 495
Action: Confirm GitHub OAuth Integration with Updated client_id Parameter
- The updated
initiateGitHubAuthfunction now explicitly passes theclient_id(along with an optional invite code) when calling the GitHub auth endpoint.- Our search confirms that the GitHub callback endpoint is implemented (as seen in
src/lib/api.ts), though it doesn’t directly reference theclient_idparameter—consistent with the expected flow.- Please ensure that the GitHub OAuth application settings are adjusted to accommodate this change and that any backend handling (especially around the callback) remains in sync with these client_id updates.
src/lib/main.tsx (2)
24-28: LGTM! The clientId property is well documented.The JSDoc comments clearly explain the purpose and type of the clientId property.
435-447: LGTM! Proper validation of required props.The useEffect hook correctly validates both apiUrl and clientId, throwing descriptive errors if either is missing or empty.
.github/workflows/test.yml (1)
31-31: LGTM! Test environment properly configured.The VITE_TEST_CLIENT_ID secret is correctly added to the test environment.
README.md (1)
15-17: LGTM! Documentation is clear and comprehensive.The README updates clearly explain:
- The requirement for both apiUrl and clientId
- The purpose of the clientId (project/tenant identification)
- Updated usage examples with both props
Also applies to: 24-27, 60-62
Summary by CodeRabbit
Documentation
New Features
Chores
Refactor / Style
Bug Fixes