more unit tests #245
more unit tests #245
Conversation
WalkthroughAdds optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
Deploying wallet-bitcoin with
|
| Latest commit: |
818f007
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e34c2804.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://unit-test-settings.wallet-bitcoin.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Select.tsx (1)
34-40: Remove redundantkeyprop.The
keyprop on line 37 is redundant since the parentdiv(line 33) already haskey={option}. React only requires the key on the outermost element returned from the map function.Apply this diff:
<FlexRow between testId={`select-option-${index}`} - key={option} onClick={() => onChange(option)} padding='0.8rem 0' >src/test/screens/settings/notifications.test.tsx (1)
19-27: Remove duplicate test case.The second test case is redundant—it verifies the exact same assertions as the first test (lines 8-17). The first test already checks for toggle presence and the
checked='true'attribute.Consider removing this test case entirely:
- it('renders the notifications screen with the correct toggle', () => { - render( - <ConfigContext.Provider value={mockConfigContextValue as any}> - <Notifications /> - </ConfigContext.Provider>, - ) - expect(screen.getByTestId('toggle-notifications')).toBeInTheDocument() - expect(screen.getByTestId('toggle-notifications').getAttribute('checked')).toBe('true') - })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/components/FlexRow.tsx(3 hunks)src/components/Select.tsx(1 hunks)src/components/Toggle.tsx(1 hunks)src/screens/Settings/Backup.tsx(1 hunks)src/screens/Settings/Notifications.tsx(1 hunks)src/test/screens/mocks.ts(2 hunks)src/test/screens/settings/about.test.tsx(1 hunks)src/test/screens/settings/backup.test.tsx(1 hunks)src/test/screens/settings/display.test.tsx(1 hunks)src/test/screens/settings/fiat.test.tsx(1 hunks)src/test/screens/settings/notifications.test.tsx(1 hunks)src/test/screens/settings/theme.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/test/screens/settings/backup.test.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/test/screens/settings/backup.test.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/test/screens/mocks.ts
🧬 Code graph analysis (9)
src/test/screens/settings/fiat.test.tsx (3)
src/providers/config.tsx (1)
ConfigContext(33-43)src/test/screens/mocks.ts (1)
mockConfigContextValue(37-46)src/screens/Settings/Fiat.tsx (1)
Fiat(9-28)
src/test/screens/settings/theme.test.tsx (3)
src/providers/config.tsx (1)
ConfigContext(33-43)src/test/screens/mocks.ts (1)
mockConfigContextValue(37-46)src/screens/Settings/Theme.tsx (1)
Theme(9-28)
src/screens/Settings/Notifications.tsx (1)
src/components/Toggle.tsx (1)
Toggle(14-28)
src/test/screens/settings/backup.test.tsx (3)
src/providers/config.tsx (1)
ConfigContext(33-43)src/test/screens/mocks.ts (1)
mockConfigContextValue(37-46)src/screens/Settings/Backup.tsx (1)
Backup(22-106)
src/test/screens/settings/about.test.tsx (3)
src/providers/asp.tsx (1)
AspContext(15-20)src/test/screens/mocks.ts (1)
mockAspContextValue(30-35)src/screens/Settings/About.tsx (1)
About(12-46)
src/components/Select.tsx (1)
src/components/FlexRow.tsx (1)
FlexRow(18-50)
src/test/screens/mocks.ts (2)
src/providers/asp.tsx (1)
AspInfo(6-6)src/lib/asp.ts (1)
emptyAspInfo(21-49)
src/components/Toggle.tsx (3)
src/components/FlexCol.tsx (1)
FlexCol(15-40)src/components/FlexRow.tsx (1)
FlexRow(18-50)src/components/Text.tsx (1)
Text(25-79)
src/test/screens/settings/display.test.tsx (3)
src/providers/config.tsx (1)
ConfigContext(33-43)src/test/screens/mocks.ts (1)
mockConfigContextValue(37-46)src/screens/Settings/Display.tsx (1)
Display(9-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-bitcoin
🔇 Additional comments (14)
src/components/Toggle.tsx (1)
11-11: LGTM! Clean testability enhancement.The optional
testIdprop is correctly added to the interface, destructured, and passed through toIonTogglevia thedata-testidattribute. This enables reliable test queries without affecting existing behavior.Also applies to: 14-14, 19-19
src/screens/Settings/Backup.tsx (1)
92-92: LGTM! Consistent test ID usage.The
testIdprop is correctly applied, enabling the toggle to be queried in tests.src/components/Select.tsx (1)
36-36: LGTM! Good testId pattern.The
testIdprop with patternselect-option-${index}enables reliable per-option querying in tests.src/screens/Settings/Notifications.tsx (1)
35-40: LGTM! Consistent test ID usage.The
testIdprop is correctly applied to the Toggle component.src/components/FlexRow.tsx (1)
15-15: LGTM! Clean testability enhancement.The optional
testIdprop is correctly added and propagated to the rootdivasdata-testid, enabling reliable test queries for components using FlexRow.Also applies to: 30-30, 46-46
src/test/screens/settings/fiat.test.tsx (1)
1-30: LGTM! Well-structured tests.The test suite appropriately verifies:
- Presence and content of select options using the new
testIdattributes- Default selection state via SVG presence
The use of
querySelectorto check for the SVG checkmark is a reasonable approach for validating selection state.src/test/screens/settings/notifications.test.tsx (1)
8-17: LGTM! Comprehensive test coverage.The first test case appropriately verifies the presence of UI elements and the initial toggle state.
src/test/screens/settings/about.test.tsx (1)
1-28: LGTM! Comprehensive About screen test.The test suite appropriately verifies the presence of all key information elements displayed on the About screen, including network details, durations, and identifiers.
src/test/screens/settings/theme.test.tsx (1)
1-30: LGTM! Clean test implementation.The tests appropriately verify the Theme screen rendering and default selection state. The use of test IDs enables reliable element selection, and the assertions cover the expected UI elements.
src/test/screens/settings/backup.test.tsx (1)
19-58: Good test coverage for Backup screen states.The three tests appropriately cover:
- Initial password-locked state
- Unlocked state with private key displayed
- Toggle state verification
The use of
findByTextfor async waiting is correct.src/test/screens/mocks.ts (3)
6-6: Good addition of type imports for stronger typing.Adding imports for
CurrencyDisplay,Fiats, andThemesenables the mock config to use proper enum values rather than string literals, improving type safety across the test suite.
13-13: Appropriate test network configuration.Adding
network: 'regtest'to the mock AspInfo aligns with standard test environment practices and matches the structure ofemptyAspInfo.
37-46: Enhanced mock config improves test coverage.Expanding
mockConfigContextValue.configto include all configuration fields (currencyDisplay,fiat,nostrBackup,notifications,theme) provides a more realistic test context and enables comprehensive testing of settings screens.src/test/screens/settings/display.test.tsx (1)
1-33: LGTM! Comprehensive Display screen tests.The tests appropriately verify:
- All three display options render with correct labels
- Default selection (CurrencyDisplay.Both) is indicated correctly
The test structure is consistent with other settings screen tests in this PR.
* more unit tests * fixes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.