🧪 [testing improvement] Add tests for getRecentAppIds error paths#37
🧪 [testing improvement] Add tests for getRecentAppIds error paths#37
Conversation
🎯 What: Address testing gap for getRecentAppIds, especially handling of invalid JSON in localStorage. 📊 Coverage: Added tests for invalid JSON, non-array JSON, valid mixed type array, and missing window object. Also exported RECENT_APP_STORAGE_KEY. ✨ Result: Improved reliability and unit test coverage of getRecentAppIds handling edge cases. Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
✅ Deploy Preview for pushy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis PR establishes test infrastructure and adds comprehensive test coverage for authentication and utility functions. It introduces Bun test setup with module mocking, ambient TypeScript types, and generation scripts. Additionally, URL validation is added to prevent unsafe redirects in payment and commit flows, with corresponding test coverage. ChangesTest Infrastructure & Configuration
URL Security Validation & Comprehensive Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The PR introduces test infrastructure alongside URL validation and comprehensive test suites. Review requires understanding test setup configurations, new type declarations, URL allowlist validation logic, and test coverage scope. The changes span multiple files but follow consistent patterns: type definitions, URL validation integration, and test case assertions. Moderate complexity arises from the diversity of concerns (infrastructure, security, testing) rather than logic density. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
|
@jules fix the conflicts |
I have fixed the merge conflicts while maintaining the test improvements. |
…entAppIds 🎯 What: Fix merge conflicts while keeping the testing gap addressed for getRecentAppIds handling of invalid JSON in localStorage. 📊 Coverage: Kept the tests for invalid JSON, non-array JSON, valid mixed type array, and missing window object. Also exported RECENT_APP_STORAGE_KEY. Also updated globals.d.ts to include the expect methods. ✨ Result: Improved reliability and unit test coverage of getRecentAppIds handling edge cases. Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
![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
🧹 Nitpick comments (1)
src/services/auth.test.ts (1)
64-71: ⚡ Quick win
useNavigatemock returnsundefined— wire it tomockNavigateinstead.
() => {}is an empty-block arrow function whose implicit return value isundefined. Any code path inauth.tsthat callsconst nav = useNavigate(); nav(...)will throwTypeError: nav is not a function. A safer and more consistent mock reuses the sharedmockNavigatespy:🛡️ Proposed fix
mock.module('react-router-dom', () => ({ createHashRouter: () => ({ state: { location: { search: '', pathname: '' } }, navigate: () => {}, }), redirect: () => {}, - useNavigate: () => {}, // mock useNavigate because it failed on export earlier + useNavigate: () => mockNavigate, }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/auth.test.ts` around lines 64 - 71, The current test mock for react-router-dom returns an empty function for useNavigate which yields undefined when called; update the mock to return the existing spy by changing useNavigate: () => {} to useNavigate: () => mockNavigate (ensure the mockNavigate spy is defined in the test scope), so any call like const nav = useNavigate(); nav(...) will call the spy instead of throwing a TypeError; update the mock.module block that defines createHashRouter, redirect and useNavigate to reference mockNavigate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/user.tsx`:
- Around line 563-568: The branch that checks orderResponse?.payUrl may return
without redirect but doesn't reset the button loading state; update the
else/invalid branch (the block that logs 'Invalid payment URL' and calls
message.error) to also clear the pending/loading flag used by the purchase flow
(e.g., call the same setter used when starting the purchase such as
setLoading(false) or setIsPurchasing(false)). Ensure you place the call in both
the invalid-URL branch and the case where payUrl is missing so the UI button
isn't left stuck; reference the existing orderResponse?.payUrl check and
isValidExternalUrl(...) call when adding the reset.
In `@src/services/auth.test.ts`:
- Around line 96-116: The test setup for window.location drops prototype getters
by spreading originalLocation and leaks a synthetic global.window because it
isn't restored; fix by saving originalWindow = global.window at top of the
beforeEach, then replace only the reload behavior on the real Location instead
of spreading (e.g., use Object.defineProperty(global.window, 'location', {
value: originalLocation, writable: true, configurable: true }) and separately
define or override the reload property as a no-op so prototype getters remain
intact), and in afterEach restore both window.location to originalLocation and
global.window to originalWindow (undoing the synthetic {} when originalWindow
was undefined).
In `@src/utils/helper.test.ts`:
- Around line 95-101: The test that deletes global.window before calling
getRecentAppIds should ensure restoration in a finally block so global state
isn't leaked on failure; wrap the delete and expect call in try { /* call
getRecentAppIds() and assertions */ } finally { global.window = originalWindow;
} to guarantee restoration of the originalWindow even if the test throws.
---
Nitpick comments:
In `@src/services/auth.test.ts`:
- Around line 64-71: The current test mock for react-router-dom returns an empty
function for useNavigate which yields undefined when called; update the mock to
return the existing spy by changing useNavigate: () => {} to useNavigate: () =>
mockNavigate (ensure the mockNavigate spy is defined in the test scope), so any
call like const nav = useNavigate(); nav(...) will call the spy instead of
throwing a TypeError; update the mock.module block that defines
createHashRouter, redirect and useNavigate to reference mockNavigate.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b016bb6-59b7-4dc6-8113-36dbbea31ccd
📒 Files selected for processing (11)
bun-test-setup.tspatch_global.shpatch_test.shsrc/globals.d.tssrc/pages/manage/components/commit.tsxsrc/pages/manage/hooks/useManageContext.tsxsrc/pages/reset-password/components/set-password.tsxsrc/pages/user.tsxsrc/services/auth.test.tssrc/utils/helper.test.tssrc/utils/helper.ts
💤 Files with no reviewable changes (2)
- src/pages/reset-password/components/set-password.tsx
- src/pages/manage/hooks/useManageContext.tsx
| if (orderResponse?.payUrl && isValidExternalUrl(orderResponse.payUrl)) { | ||
| window.location.href = orderResponse.payUrl; | ||
| } else if (orderResponse?.payUrl) { | ||
| console.error('Invalid payment URL:', orderResponse.payUrl); | ||
| message.error('支付链接无效'); | ||
| } |
There was a problem hiding this comment.
Reset button loading state when payment URL is invalid/missing.
This branch can return without redirect, but callers keep loading=true, so purchase buttons can remain stuck after a rejected URL.
Proposed fix
-async function purchase(tier?: string) {
+async function purchase(tier?: string): Promise<boolean> {
const orderResponse = await api.createOrder({ tier });
if (orderResponse?.payUrl && isValidExternalUrl(orderResponse.payUrl)) {
window.location.href = orderResponse.payUrl;
- } else if (orderResponse?.payUrl) {
+ return true;
+ }
+ if (orderResponse?.payUrl) {
console.error('Invalid payment URL:', orderResponse.payUrl);
- message.error('支付链接无效');
+ }
+ message.error('支付链接无效');
+ return false;
- }
}// PurchaseButton onClick
- setLoading(true);
- await purchase(tier);
+ setLoading(true);
+ try {
+ await purchase(tier);
+ } finally {
+ setLoading(false);
+ }
// UpgradeDropdown handlers (both call sites)
- setLoading(true);
- await purchase(key);
+ setLoading(true);
+ try {
+ await purchase(key);
+ } finally {
+ setLoading(false);
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/user.tsx` around lines 563 - 568, The branch that checks
orderResponse?.payUrl may return without redirect but doesn't reset the button
loading state; update the else/invalid branch (the block that logs 'Invalid
payment URL' and calls message.error) to also clear the pending/loading flag
used by the purchase flow (e.g., call the same setter used when starting the
purchase such as setLoading(false) or setIsPurchasing(false)). Ensure you place
the call in both the invalid-URL branch and the case where payUrl is missing so
the UI button isn't left stuck; reference the existing orderResponse?.payUrl
check and isValidExternalUrl(...) call when adding the reset.
| originalLocation = global.window | ||
| ? global.window.location | ||
| : ({} as Location); | ||
| if (!global.window) { | ||
| global.window = {} as any; | ||
| } | ||
|
|
||
| Object.defineProperty(global.window, 'location', { | ||
| value: { ...originalLocation, reload: mock(() => {}) }, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| Object.defineProperty(global.window, 'location', { | ||
| value: originalLocation, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Two test-isolation issues in the window.location mock setup.
1. Spread of originalLocation loses prototype getters (Line 104).
{ ...originalLocation, reload: mock(() => {}) } only copies own-enumerable properties. In DOM-capable environments (happy-dom), Location's href, pathname, search, assign, etc. are prototype getters and are silently dropped. If any code path in auth.ts reads those properties during a test, it gets undefined without any error. A minimal, explicit mock avoids the trap:
- Object.defineProperty(global.window, 'location', {
- value: { ...originalLocation, reload: mock(() => {}) },
- writable: true,
- configurable: true,
- });
+ Object.defineProperty(global.window, 'location', {
+ value: { reload: mock(() => {}) },
+ writable: true,
+ configurable: true,
+ });2. global.window is never restored when it was initially undefined (Lines 99–101 / 110–116).
afterEach restores window.location but leaves the synthetic global.window = {} in place. From the second test onward, global.window is the leaked {}, so the global.window ? … : ({} as Location) guard in beforeEach evaluates the wrong branch. Track and restore the original state:
🛡️ Proposed fix
+ let windowWasUndefined = false;
beforeEach(() => {
// ...
- originalLocation = global.window
- ? global.window.location
- : ({} as Location);
- if (!global.window) {
- global.window = {} as any;
- }
+ windowWasUndefined = typeof global.window === 'undefined';
+ if (windowWasUndefined) {
+ global.window = {} as any;
+ }
+ originalLocation = global.window.location;
Object.defineProperty(global.window, 'location', {
value: { reload: mock(() => {}) },
writable: true,
configurable: true,
});
});
afterEach(() => {
- Object.defineProperty(global.window, 'location', {
- value: originalLocation,
- writable: true,
- configurable: true,
- });
+ if (windowWasUndefined) {
+ delete (global as any).window;
+ } else {
+ Object.defineProperty(global.window, 'location', {
+ value: originalLocation,
+ writable: true,
+ configurable: true,
+ });
+ }
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/auth.test.ts` around lines 96 - 116, The test setup for
window.location drops prototype getters by spreading originalLocation and leaks
a synthetic global.window because it isn't restored; fix by saving
originalWindow = global.window at top of the beforeEach, then replace only the
reload behavior on the real Location instead of spreading (e.g., use
Object.defineProperty(global.window, 'location', { value: originalLocation,
writable: true, configurable: true }) and separately define or override the
reload property as a no-op so prototype getters remain intact), and in afterEach
restore both window.location to originalLocation and global.window to
originalWindow (undoing the synthetic {} when originalWindow was undefined).
| test('should return empty array when window is undefined', () => { | ||
| const originalWindow = global.window; | ||
| // @ts-expect-error | ||
| delete global.window; | ||
| expect(getRecentAppIds()).toEqual([]); | ||
| global.window = originalWindow; | ||
| }); |
There was a problem hiding this comment.
Guard global window restoration with finally in the undefined-window test.
If this test fails before restoration, later tests can break from leaked global state.
Proposed fix
test('should return empty array when window is undefined', () => {
const originalWindow = global.window;
- // `@ts-expect-error`
- delete global.window;
- expect(getRecentAppIds()).toEqual([]);
- global.window = originalWindow;
+ try {
+ // `@ts-expect-error`
+ delete global.window;
+ expect(getRecentAppIds()).toEqual([]);
+ } finally {
+ global.window = originalWindow;
+ }
});📝 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.
| test('should return empty array when window is undefined', () => { | |
| const originalWindow = global.window; | |
| // @ts-expect-error | |
| delete global.window; | |
| expect(getRecentAppIds()).toEqual([]); | |
| global.window = originalWindow; | |
| }); | |
| test('should return empty array when window is undefined', () => { | |
| const originalWindow = global.window; | |
| try { | |
| // `@ts-expect-error` | |
| delete global.window; | |
| expect(getRecentAppIds()).toEqual([]); | |
| } finally { | |
| global.window = originalWindow; | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/helper.test.ts` around lines 95 - 101, The test that deletes
global.window before calling getRecentAppIds should ensure restoration in a
finally block so global state isn't leaked on failure; wrap the delete and
expect call in try { /* call getRecentAppIds() and assertions */ } finally {
global.window = originalWindow; } to guarantee restoration of the originalWindow
even if the test throws.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
1 participant
🎯 What: Address testing gap for getRecentAppIds, especially handling of invalid JSON in localStorage.
📊 Coverage: Added tests for invalid JSON, non-array JSON, valid mixed type array, and missing window object. Also exported RECENT_APP_STORAGE_KEY.
✨ Result: Improved reliability and unit test coverage of getRecentAppIds handling edge cases.
PR created automatically by Jules for task 2911372941532518971 started by @sunnylqm
Summary by CodeRabbit
Bug Fixes
Tests