-
Notifications
You must be signed in to change notification settings - Fork 5
feat: Graphql used for API requests & added cache #66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…rom QuickWinsPage
WalkthroughThis update migrates the "quick wins" issue-fetching logic from a REST-based GitHub API client to a new GraphQL client, revises the caching strategy to a 1-hour expiration, and adjusts the minimum repository star threshold for issue queries. It also moves the API rate limit warning UI from the Quick Wins page to the shared page header. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PageHeader
participant useDataCacheStore
participant GitHubGraphQLClient
participant CacheStore
participant QuickWinsStore
User->>PageHeader: Loads page
PageHeader->>useDataCacheStore: Get rateLimitInfo
useDataCacheStore-->>PageHeader: Return rateLimitInfo
PageHeader-->>User: Show rate limit warning if critical
User->>QuickWinsStore: Request good issues/easy fixes
QuickWinsStore->>CacheStore: Check cache (1 hour expiry)
alt Cache valid and non-empty
CacheStore-->>QuickWinsStore: Return cached issues
else Cache expired/empty
QuickWinsStore->>GitHubGraphQLClient: Set token and fetch issues
GitHubGraphQLClient-->>QuickWinsStore: Return formatted issues
QuickWinsStore->>CacheStore: Update cache
end
QuickWinsStore-->>User: Return issues
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/stores/cache.ts (1)
60-63: Extract reusable constant & unify comment language
ONE_HOUR_IN_MSis re-declared every timeisQuickWinsCacheExpiredruns and the inline comment is Turkish (“1 saat”) while the rest of the file is English.
Move the constant to module scope (or a sharedconstants.ts) and keep comment language consistent.src/lib/api/github-graphql-client.ts (1)
294-304: Duplicate “high” check – consider tightening priority rules
calculatePriorityhas two branches that both return'high'. Consolidating them (or adding a medium branch for “p1/high”) would simplify the function.src/lib/api/github-api-client.ts (3)
430-434: DefaultissuesPerReporaised to 30 – double-check rate-limit impactWith 20 repos ×
labels.lengthrequests per batch, bumping the default to 30 can push the overall request count (and payload size) far higher than before, especially when the caller doesn’t override the parameter.
Confirm the new default won’t hit the REST v3 rate limit for unauthenticated users (60 req/h) or authenticated users (5 k req/h) and that the UX can tolerate the extra latency.
436-439:sincefilter uses date-only ISO – prefer full timestamp for accuracyGitHub’s
sinceparameter accepts full RFC 3339 timestamps. Passing onlyYYYY-MM-DDzero-times it at 00:00 UTC, which can exclude ~24 h of issues on the boundary day.
Consider:-const dateString = oneMonthAgo.toISOString().split('T')[0] +const dateString = oneMonthAgo.toISOString()This keeps precision while remaining standards-compliant.
456-458: Endpoint lacks explicit sort parameter
/issuesdefaults tocreatedsorting. If the goal is to surface recent activity, add&sort=updated&direction=descto the query string; otherwise thesincefilter alone may still return month-old but least-recently-created issues first.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/quick-wins/page.tsx(0 hunks)src/components/layout/PageHeader.tsx(2 hunks)src/components/quick-wins/hooks/useQuickWins.ts(4 hunks)src/lib/api/github-api-client.ts(3 hunks)src/lib/api/github-graphql-client.ts(1 hunks)src/stores/cache.ts(1 hunks)src/stores/quickWins.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/app/quick-wins/page.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/layout/PageHeader.tsx (1)
src/hooks/useAuth.ts (1)
useRequireAuth(22-30)
🔇 Additional comments (6)
src/components/layout/PageHeader.tsx (2)
14-18: LGTM – clear critical-limit calculationComputation is straightforward and handles both “0 remaining” and “<10 % left”.
32-40: LGTM – concise, accessible warning UIConditional render and colour choice are appropriate; no issues spotted.
src/components/quick-wins/hooks/useQuickWins.ts (1)
104-113: LGTM – manual refresh correctly invalidates cacheCache is cleared before each forced fetch; behaviour matches new 1-hour policy.
src/stores/quickWins.ts (1)
42-52: LGTM – early-return from cache avoids unnecessary callsCache check now validates non-empty arrays; good improvement.
src/lib/api/github-api-client.ts (2)
506-508: Star threshold dropped to 5 – verify signal-to-noise ratioReducing the floor from 20→5 will pull in a long tail of little-known projects. That may dilute “quick wins” quality or bloat API traffic. Validate the new threshold against user expectations and adjust if noise becomes an issue.
510-512: Low threshold + broad label set – monitor result relevanceCombining a 5-star minimum with five generic labels could return large, low-quality batches. Track cache hit rate and user clicks to ensure the feed stays useful; consider paginating or tightening filters if necessary.
| for (const label of labels) { | ||
| const query = ` | ||
| query GetEasyFixes($count: Int!) { | ||
| search( | ||
| query: "label:\\"${label}\\" state:open created:>${dateString}", | ||
| type: ISSUE, | ||
| first: $count | ||
| ) { | ||
| nodes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Math.floor(count / labels.length) may pass 0 – GraphQL rejects it
When count < labels.length the computed per-label count is 0, leading to a first: 0 query which GitHub rejects.
Guard with Math.max(1, …) or skip the query for that label.
- const perLabel = Math.floor(count / labels.length)
+ const perLabel = Math.max(1, Math.floor(count / labels.length))Also applies to: 247-249
🤖 Prompt for AI Agents
In src/lib/api/github-graphql-client.ts around lines 191 to 199 and also lines
247 to 249, the calculation using Math.floor(count / labels.length) can result
in 0 when count is less than labels.length, causing the GraphQL query to fail
with first: 0. To fix this, update the calculation to use Math.max(1,
Math.floor(count / labels.length)) to ensure the count is never zero, or
alternatively skip the query for that label if the computed count is zero.
Summary by CodeRabbit
New Features
Enhancements
Refactor
Bug Fixes
Style