feat: add My Issues only filter to /issues page#140
Open
blinkeye-lcm wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a "My Issues only" toggle to the Issues table, synced with the ChangesMy Issues only filter
Sequence DiagramsequenceDiagram
participant User
participant IssuesPage
participant IssuesTable
participant useMinerLogin
participant IssueTableRow
User->>IssuesPage: Navigate with ?mine=1
IssuesPage->>IssuesTable: Render with Suspense
IssuesTable->>IssuesTable: Parse mine param from URL
IssuesTable->>useMinerLogin: Fetch current miner identity
useMinerLogin-->>IssuesTable: Return miner login
IssuesTable->>IssuesTable: Derive authorParam from mineOnly + miner
IssuesTable->>IssuesTable: Build issuesParams with author filter
IssuesTable->>IssueTableRow: Render each row with mine=true/false
IssueTableRow->>IssueTableRow: Apply attention color if mine=true
IssueTableRow-->>User: Display highlighted author cell
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/issues/page.tsx (1)
1-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove ineffective route config from client component.
The
export const dynamic = 'force-dynamic'has no effect in a client component. Route segment config options likedynamiconly apply to Server Components and Route Handlers in Next.js 15.Since this page is marked with
'use client', the dynamic export is ignored and serves no purpose.🧹 Proposed fix
'use client'; -export const dynamic = 'force-dynamic'; - import React, { Suspense } from 'react';🤖 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/app/issues/page.tsx` around lines 1 - 3, The file is a client component (it starts with 'use client') so the route config export export const dynamic = 'force-dynamic' is ineffective; remove that export from the module (delete the export const dynamic line) and keep only the client directive and component code so no server-only route config remains in the client component.
🧹 Nitpick comments (2)
src/app/issues/page.tsx (1)
24-26: 💤 Low valueConsider adding a loading skeleton to the Suspense fallback.
The
fallback={null}provides no visual feedback while the table loads. While this may be intentional for a fast render, consider using a skeleton loader to improve perceived performance, especially on slower connections.💡 Example with skeleton fallback
- <Suspense fallback={null}> + <Suspense fallback={<Box sx={{ p: 4 }}><TableRowsSkeleton rows={8} cols={[...]} /></Box>}> <IssuesTable /> </Suspense>🤖 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/app/issues/page.tsx` around lines 24 - 26, The Suspense wrapper around IssuesTable currently uses fallback={null}, giving no visual feedback while data loads; replace the null fallback with a lightweight skeleton component (e.g., IssuesTableSkeleton or a generic TableSkeleton) and render that in the Suspense fallback so users see a loading placeholder; update where Suspense is used around IssuesTable to import and pass the skeleton component (or create IssuesTableSkeleton if missing) and ensure its styling matches the table layout for smooth visual continuity.src/components/IssuesTable.tsx (1)
613-656: 💤 Low valueConsider removing hardcoded fallback color.
Line 627 includes a hardcoded
rgba(242, 201, 76, 0.14)fallback for--attention-subtle, but no fallback for--accent-subtle. This inconsistency might cause unexpected behavior if CSS variables are missing, and the hardcoded value may not match all theme variants.Modern Primer themes should define these variables, so the fallback is likely unnecessary. Consider removing it for consistency.
♻️ Proposed refactor
- const subtle = tone === 'attention' ? 'var(--attention-subtle, rgba(242, 201, 76, 0.14))' : 'var(--accent-subtle)'; + const subtle = tone === 'attention' ? 'var(--attention-subtle)' : 'var(--accent-subtle)';🤖 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/IssuesTable.tsx` around lines 613 - 656, In ToggleButton, remove the hardcoded RGBA fallback in the subtle color expression so both tones use only their CSS variables consistently; replace the current use of "var(--attention-subtle, rgba(242, 201, 76, 0.14))" with "var(--attention-subtle)" (so subtle is set via var(--attention-subtle) for 'attention' and var(--accent-subtle) for 'accent'), keeping the rest of the logic and names (emphasis, subtle, tone, active) unchanged.
🤖 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/IssuesTable.tsx`:
- Line 262: The current expression in IssuesTable that computes myCount calls
me.toLowerCase() which throws if me is null/undefined; change the lookup to
guard/normalize me first (e.g., compute a safe string like normalizedMe = (me ??
'').toLowerCase() or short-circuit when me is falsy) and then use normalizedMe
in the authorOptions.find predicate (referencing myCount, authorOptions, and me)
so unauthenticated visitors won't crash the component.
---
Outside diff comments:
In `@src/app/issues/page.tsx`:
- Around line 1-3: The file is a client component (it starts with 'use client')
so the route config export export const dynamic = 'force-dynamic' is
ineffective; remove that export from the module (delete the export const dynamic
line) and keep only the client directive and component code so no server-only
route config remains in the client component.
---
Nitpick comments:
In `@src/app/issues/page.tsx`:
- Around line 24-26: The Suspense wrapper around IssuesTable currently uses
fallback={null}, giving no visual feedback while data loads; replace the null
fallback with a lightweight skeleton component (e.g., IssuesTableSkeleton or a
generic TableSkeleton) and render that in the Suspense fallback so users see a
loading placeholder; update where Suspense is used around IssuesTable to import
and pass the skeleton component (or create IssuesTableSkeleton if missing) and
ensure its styling matches the table layout for smooth visual continuity.
In `@src/components/IssuesTable.tsx`:
- Around line 613-656: In ToggleButton, remove the hardcoded RGBA fallback in
the subtle color expression so both tones use only their CSS variables
consistently; replace the current use of "var(--attention-subtle, rgba(242, 201,
76, 0.14))" with "var(--attention-subtle)" (so subtle is set via
var(--attention-subtle) for 'attention' and var(--accent-subtle) for 'accent'),
keeping the rest of the logic and names (emphasis, subtle, tone, active)
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 Plus
Run ID: 6f328d3f-5ea4-43eb-b103-84399185dc07
📒 Files selected for processing (3)
src/app/docs/page.tsxsrc/app/issues/page.tsxsrc/components/IssuesTable.tsx
| const totalPages = data?.total_pages ?? page; | ||
| const safePage = Math.min(page, totalPages); | ||
| const authorOptions = data?.authors ?? []; | ||
| const myCount = authorOptions.find((a) => a.login.toLowerCase() === me.toLowerCase())?.count ?? 0; |
There was a problem hiding this comment.
Guard against null/undefined miner login.
Calling me.toLowerCase() will throw a TypeError if me is null or undefined (when user is signed out). This will crash the component for unauthenticated visitors.
🛡️ Proposed fix
- const myCount = authorOptions.find((a) => a.login.toLowerCase() === me.toLowerCase())?.count ?? 0;
+ const myCount = me ? authorOptions.find((a) => a.login.toLowerCase() === me.toLowerCase())?.count ?? 0 : 0;Or alternatively:
- const myCount = authorOptions.find((a) => a.login.toLowerCase() === me.toLowerCase())?.count ?? 0;
+ const myCount = authorOptions.find((a) => me && a.login.toLowerCase() === me.toLowerCase())?.count ?? 0;📝 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.
Suggested change
| const myCount = authorOptions.find((a) => a.login.toLowerCase() === me.toLowerCase())?.count ?? 0; | |
| const myCount = me ? authorOptions.find((a) => a.login.toLowerCase() === me.toLowerCase())?.count ?? 0 : 0; |
🤖 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/IssuesTable.tsx` at line 262, The current expression in
IssuesTable that computes myCount calls me.toLowerCase() which throws if me is
null/undefined; change the lookup to guard/normalize me first (e.g., compute a
safe string like normalizedMe = (me ?? '').toLowerCase() or short-circuit when
me is falsy) and then use normalizedMe in the authorOptions.find predicate
(referencing myCount, authorOptions, and me) so unauthenticated visitors won't
crash the component.
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
Adds a "My Issues only" toggle to
/issues, matching the existing "My PRs only" affordance on/pulls. The filter narrows the list to issues opened by the signed-in user, with state synced to the URL via?mine=1so the view survives reloads and stays shareable.Behavior mirrors the pulls page:
The existing inline "Tracked only" button was extracted into a small local
ToggleButtonhelper (same pattern as/pulls) so the two toggles share styling. The "Tracked only" color now matches the pulls page (accent instead of attention), keeping both pages visually consistent.The Issues page itself now wraps the table in
<Suspense>because the table readsuseSearchParams(). The docs page also picks up a short bullet describing the new filter.Related Issues
Closes #139
Screenshot
Type of Change
Testing
pnpm buildpasses (TypeScript compilation verified viatsc --noEmit)/issues:?mine=1Checklist
Summary by CodeRabbit