feat: wire up CredentialService in onboarding API Keys screen#140
feat: wire up CredentialService in onboarding API Keys screen#140jbdevprimary merged 12 commits intomainfrom
Conversation
- Update `src/pages/onboarding/api-keys.tsx` to use `CredentialService` for validation and storage. - Integrate `useCredentialStore` to update application state with valid credentials. - Ensure keys are stored securely using hardware-backed encryption via `CredentialService.store`. - Mask secrets before storing them in the Zustand state for UI display. Co-authored-by: jbdevprimary <2650679+jbdevprimary@users.noreply.github.com>
- Sort imports in `src/pages/onboarding/api-keys.tsx` to satisfy Biome linting rules. - Update `sonar-project.properties` to remove reference to non-existent `app` directory and point to valid sources (`src`, `packages`). Co-authored-by: jbdevprimary <2650679+jbdevprimary@users.noreply.github.com>
- Fix `TS2353` error in `src/pages/onboarding/api-keys.tsx` by removing the `status` property from `addCredential` calls, as it is omitted from the action's input type. - Restore `src/pages/onboarding/__tests__/api-keys.test.tsx` with updated expectations to match the type fix and ensure code coverage. Co-authored-by: jbdevprimary <2650679+jbdevprimary@users.noreply.github.com>
- Fix Biome linting errors in `src/pages/onboarding/__tests__/api-keys.test.tsx` by sorting imports, using template literals instead of string concatenation, and replacing `as any` casts with `vi.mocked()` for better type safety. Co-authored-by: jbdevprimary <2650679+jbdevprimary@users.noreply.github.com>
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
- Confirm fixes for Lint, Type Check, and SonarCloud failures. - Ensure all test files comply with Biome rules. - Validate `sonar-project.properties` configuration. Co-authored-by: jbdevprimary <2650679+jbdevprimary@users.noreply.github.com>
- Extract `APIKeyInput` component to reduce complexity in `src/pages/onboarding/api-keys.tsx` and improve SonarCloud maintainability rating. - Add comprehensive test cases in `src/pages/onboarding/__tests__/api-keys.test.tsx` covering "Skip" functionality, disabled states, and invalid inputs to meet coverage thresholds. Co-authored-by: jbdevprimary <2650679+jbdevprimary@users.noreply.github.com>
- Implement robust error handling (try-catch) for all `CredentialService` operations. - Add race condition protection using `AbortController` and debouncing for input validation. - Fix `addCredential` usage by correctly chaining `setValidationResult` to set status. - Refactor storage logic into `storeCredential` helper to reduce duplication. - Remove unused imports in tests and ensure alignment with implementation. Co-authored-by: jbdevprimary <2650679+jbdevprimary@users.noreply.github.com>
- Remove `as any` casts: use explicit `'anthropic' | 'openai'` type instead of unsafe `any` cast (Amazon Q + Gemini type safety feedback) - Fix Biome formatting in test file: correct waitFor callback formatting and assertion line lengths to match Biome's expected format (CI failure) - Race condition handling already in place via AbortController pattern - Error handling already added with try-catch in validateKey/storeCredential - setValidationResult correctly called after addCredential - Code refactored into storeCredential helper with Promise.all https://claude.ai/code/session_01PQd4hGQQpmGTgpHc7kGjAE
|
Warning Rate limit exceeded
⌛ 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. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello @jbdevprimary, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the onboarding API Keys screen by integrating the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR successfully implements CredentialService integration with proper error handling, race condition protection, and comprehensive test coverage. The implementation addresses all previous review feedback including AbortController patterns, try-catch blocks, cleanup on unmount, and code deduplication. No critical defects found that would block merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully wires up the CredentialService in the onboarding API keys screen, replacing previous placeholders. The changes are well-implemented, incorporating important features like debouncing with abort controllers to prevent race conditions, comprehensive error handling, and cleanup logic in useEffect. The code is also well-structured, with the creation of a reusable APIKeyInput component and a thorough new test suite. My feedback focuses on a couple of areas to further improve the code: enhancing the robustness of the tests by using more specific mock data, and increasing type safety in a helper function to align with the existing good practice of avoiding unnecessary type casting. Overall, this is a high-quality contribution.
| }); | ||
|
|
||
| // Mock useCredentialStore | ||
| const mockAddCredential = vi.fn(() => 'test-cred-id'); |
There was a problem hiding this comment.
The current mock for addCredential returns a static ID 'test-cred-id'. This can lead to less specific tests, especially when multiple credentials are added, as you can't distinguish between them. To make the test more robust and accurately reflect the behavior of adding multiple unique credentials, consider making the returned ID unique for each provider.
| const mockAddCredential = vi.fn(() => 'test-cred-id'); | |
| const mockAddCredential = vi.fn((cred) => `test-cred-id-${cred.provider}`); |
| expect(mockSetValidationResult).toHaveBeenCalledWith( | ||
| 'test-cred-id', | ||
| expect.objectContaining({ isValid: true }), | ||
| ); |
There was a problem hiding this comment.
Following the suggestion to make mockAddCredential return unique IDs, this assertion can be strengthened to verify that setValidationResult is called correctly for each of the stored credentials. This ensures both keys are processed as expected. It's also good practice to assert the number of times the mock was called.
expect(mockSetValidationResult).toHaveBeenCalledTimes(2);
expect(mockSetValidationResult).toHaveBeenCalledWith(
'test-cred-id-anthropic',
expect.objectContaining({ isValid: true }),
);
expect(mockSetValidationResult).toHaveBeenCalledWith(
'test-cred-id-openai',
expect.objectContaining({ isValid: true }),
);
| provider: CredentialProvider, | ||
| keyState: APIKeyState, | ||
| name: string, | ||
| secureStoreKey: string, | ||
| ) => { | ||
| if (keyState.isValid && keyState.key) { | ||
| try { | ||
| // Cast provider to the store/mask API type ('anthropic' | 'openai') | ||
| const apiProvider = provider as 'anthropic' | 'openai'; | ||
| await CredentialService.store(apiProvider, keyState.key); | ||
|
|
||
| const credId = addCredential({ | ||
| provider, | ||
| name, | ||
| secureStoreKey, | ||
| maskedValue: CredentialService.maskSecret(keyState.key, apiProvider), | ||
| }); | ||
|
|
||
| // Explicitly set validation result as valid since we just stored it | ||
| setValidationResult(credId, { | ||
| isValid: true, | ||
| expiresAt: undefined, | ||
| }); | ||
| } catch (error) { | ||
| console.error(`Failed to store ${provider} credential:`, error); | ||
| throw new Error(`Failed to save ${name} key`); | ||
| } | ||
| } |
There was a problem hiding this comment.
The storeCredential function can be made more type-safe. By narrowing the provider type in the signature to 'anthropic' | 'openai', you can eliminate the need for the type cast to apiProvider and use provider directly throughout the function. This improves readability and type safety.
provider: 'anthropic' | 'openai',
keyState: APIKeyState,
name: string,
secureStoreKey: string,
) => {
if (keyState.isValid && keyState.key) {
try {
await CredentialService.store(provider, keyState.key);
const credId = addCredential({
provider,
name,
secureStoreKey,
maskedValue: CredentialService.maskSecret(keyState.key, provider),
});
// Explicitly set validation result as valid since we just stored it
setValidationResult(credId, {
isValid: true,
expiresAt: undefined,
});
} catch (error) {
console.error(`Failed to store ${provider} credential:`, error);
throw new Error(`Failed to save ${name} key`);
}
}
|
- Acknowledge that this PR is superseded by #140. - No further code changes required here. Co-authored-by: jbdevprimary <2650679+jbdevprimary@users.noreply.github.com>
2 participants
Summary
Replaces #135 with all review feedback addressed.
Wires up
CredentialService.validateCredential()andCredentialService.store()in the onboarding API Keys screen, replacing the previous TODO placeholders.Review Feedback Addressed
AbortControllerpattern to cancel in-flight validation requests when user types rapidlyCredentialServicecalls with user-friendly error messagessetValidationResult()afteraddCredential()since addCredential defaults status to 'unknown'storeCredential()helper, both providers stored viaPromise.allas anycasts, using explicit'anthropic' | 'openai'union typewaitForcallback and assertion formattingChanges
src/pages/onboarding/api-keys.tsx- Full CredentialService integration with error handlingsrc/pages/onboarding/components/APIKeyInput.tsx- Reusable API key input componentsrc/pages/onboarding/__tests__/api-keys.test.tsx- Comprehensive tests covering validation, storage, and error statesTest plan
Closes #135
https://claude.ai/code/session_01PQd4hGQQpmGTgpHc7kGjAE