fix: separate personal and organization forks#211
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds organization support and owner-scoped fork management: a GitHubOrganization type and paginated API helpers, ForkTimeline state for selected owner/org loading, owner-scoped refresh/merge/sync logic, updated pagination/empty-state UI, and tests for owner filtering and failure handling. ChangesMulti-owner fork filtering and management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/services/githubApi.ts (1)
1115-1126: ⚡ Quick winAvoid repeated array reallocation in paginated accumulation.
allItems = [...allItems, ...items]copies the full accumulator on every page. For larger org/repo sets this turns the loop into avoidable O(n²) copying. Usepush(...items)to keep it linear.Proposed change
- let allItems: T[] = []; + const allItems: T[] = []; @@ - allItems = [...allItems, ...items]; + allItems.push(...items);🤖 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/githubApi.ts` around lines 1115 - 1126, The loop accumulates paginated results into allItems using repeated spread copies (allItems = [...allItems, ...items]) causing O(n²) behavior; change the accumulation to mutate the existing array instead (use allItems.push(...items)) after the makeRequest call and remove the spread reassignment so the loop runs in linear time; keep the rest of the pagination logic (perPage, page, break condition) and the makeRequest(...) usage unchanged.src/components/ForkTimeline.test.tsx (2)
166-172: ⚡ Quick winAvoid coupling to
setForkscall order in refresh assertions.Using
setForks.mock.calls[0]is brittle if mount-time or intermediate updates introduce additional calls. Assert against the last call (or a call matching owner/login) to keep this test stable.🤖 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/components/ForkTimeline.test.tsx` around lines 166 - 172, The test assumes the refreshed forks are in setForks.mock.calls[0], which is brittle; update the assertion to read the last call (or locate the call containing the expected owner) instead: when inspecting the mocked setForks calls (symbol: setForks) extract the final invocation's argument (previously assigned to refreshedForks) or search calls for an array containing an entry with owner.login 'tamina' and then assert its length and that refreshedForks[0] matches the expected ForkRepo fields (referenced symbols: refreshedForks, ForkRepo, personalFork).
175-184: ⚡ Quick winAssert the fallback UI remains usable after organization-load failure.
This test checks the warning toast, but it doesn’t verify the key requirement that the personal Fork view still works after
/user/orgsfails. Add an assertion that personal content is still rendered (and actionable) after the failure path.Suggested test strengthening
it('warns when organization owners cannot be loaded', async () => { MockGitHubApiService.mockImplementation(() => ({ getUserOrganizations: vi.fn().mockRejectedValue(new Error('missing scope')), } as unknown as GitHubApiService)); render(<ForkTimeline />); await waitFor(() => { expect(toastMock).toHaveBeenCalledWith('组织列表加载失败，请检查 GitHub token 权限。', 'warning'); }); + expect(screen.getByText('personal-fork')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: '刷新' })).toBeInTheDocument(); });🤖 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/components/ForkTimeline.test.tsx` around lines 175 - 184, The test currently stubs MockGitHubApiService.getUserOrganizations to reject and asserts toastMock was called, but it must also verify the personal Fork view remains usable: after rendering <ForkTimeline /> and waiting for the toast, assert that the personal/fallback UI is rendered and actionable (for example, look up the personal tab or a known personal-view marker and assert it's in the document and/or simulate a click to ensure it responds). Update the test that uses MockGitHubApiService and getUserOrganizations to add an expectation that the personal content (the element/text/button used to show personal forks in ForkTimeline) is present and interactive after the failure, in addition to the existing toastMock assertion.
🤖 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/components/ForkTimeline.tsx`:
- Around line 44-45: The cached Set in loadedForkOwners must be cleared whenever
the signed-in personal owner/account changes; add a useEffect in the
ForkTimeline component that watches the authenticated personal owner identifier
(the prop/state that represents the current session owner, e.g.,
authenticatedPersonalOwner or currentOwner) and calls setLoadedForkOwners(new
Set()) when it changes so the load gating logic that references loadedForkOwners
(used around the load gating at lines where loading is checked) will re-run for
the new account.
- Around line 286-290: The code currently overwrites forks using stale snapshots
(setForks([...currentForks.filter(...), ...updatedForks])) which causes
last-write-wins when concurrent owner refreshes overlap; change both places that
call setForks to use the functional updater form (setForks(prev => { ... })) so
you operate on the current state when removing the specific owner's forks and
merging updatedForks, and change setLoadedForkOwners(prev => new
Set(prev).add(ownerLogin)) to return a new Set (const s = new Set(prev);
s.add(ownerLogin); return s) inside its functional updater to avoid mutating the
previous Set; target the calls using the identifiers setForks, currentForks,
updatedForks, and setLoadedForkOwners.
---
Nitpick comments:
In `@src/components/ForkTimeline.test.tsx`:
- Around line 166-172: The test assumes the refreshed forks are in
setForks.mock.calls[0], which is brittle; update the assertion to read the last
call (or locate the call containing the expected owner) instead: when inspecting
the mocked setForks calls (symbol: setForks) extract the final invocation's
argument (previously assigned to refreshedForks) or search calls for an array
containing an entry with owner.login 'tamina' and then assert its length and
that refreshedForks[0] matches the expected ForkRepo fields (referenced symbols:
refreshedForks, ForkRepo, personalFork).
- Around line 175-184: The test currently stubs
MockGitHubApiService.getUserOrganizations to reject and asserts toastMock was
called, but it must also verify the personal Fork view remains usable: after
rendering <ForkTimeline /> and waiting for the toast, assert that the
personal/fallback UI is rendered and actionable (for example, look up the
personal tab or a known personal-view marker and assert it's in the document
and/or simulate a click to ensure it responds). Update the test that uses
MockGitHubApiService and getUserOrganizations to add an expectation that the
personal content (the element/text/button used to show personal forks in
ForkTimeline) is present and interactive after the failure, in addition to the
existing toastMock assertion.
In `@src/services/githubApi.ts`:
- Around line 1115-1126: The loop accumulates paginated results into allItems
using repeated spread copies (allItems = [...allItems, ...items]) causing O(n²)
behavior; change the accumulation to mutate the existing array instead (use
allItems.push(...items)) after the makeRequest call and remove the spread
reassignment so the loop runs in linear time; keep the rest of the pagination
logic (perPage, page, break condition) and the makeRequest(...) usage unchanged.
🪄 Autofix (Beta)
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: c8e21a7d-f007-4b5e-81c3-1d3af523adfb
📒 Files selected for processing (4)
src/components/ForkTimeline.test.tsxsrc/components/ForkTimeline.tsxsrc/services/githubApi.tssrc/types/index.ts
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #196
Testing
Notes
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UX Improvements
Tests