Fix Kanban board: Add GraphQL support and hybrid config/context method #72
Conversation
…Hub task synchronization
…data management features
- Added new Label component using @radix-ui/react-label. - Added new Switch component using @radix-ui/react-switch. - Updated package.json to include new dependencies for DnD Kit and Radix UI components. - Updated TypeScript types for node and react in devDependencies.
…ession management improvements
…ems store with GraphQL integration
…nd improve column management UI
… auth, kanban, and settings stores
…d user interfaces
…improve TaskDetailModal for better user experience
…, and enhance type safety in GitHubSettingsForm and GitHubGraphQLClient
…ser experience - Added Suspense fallback loading states to ActionRequiredPage, LoginPage, QuickWinsPage, and SearchPage for better UX during data fetching. - Extracted content components (ActionRequiredContent, LoginContent, QuickWinsContent, SearchContent) for cleaner structure and improved readability. - Updated Layout component to include Suspense for Sidebar rendering. - Adjusted loading logic in SearchPage to handle loading states more gracefully. - Ensured IDs in useQuickWins hook are converted to strings for consistency. - Removed unnecessary loading checks from individual pages, centralizing loading state management.
…and improved accordion handling
…GitHub tasks and improve task clearing functionality
|
Caution Review failedThe pull request is closed. WalkthroughAdds GraphQL-backed "Action Required" APIs and store, a persisted Kanban with DnD and GitHub sync, NextAuth/middleware and cookie-based auth changes, multiple page refactors into Suspense boundaries, new settings store/form and UI primitives, sidebar/layout updates, and dependency additions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant P as Action Required Page
participant API as /api/action-required
participant GQL as GitHubGraphQLClient
participant GH as GitHub GraphQL API
U->>P: Load /action-required?tab=assigned
P->>API: GET (cookie-auth)
API->>GQL: getActionRequiredItems(username)
GQL->>GH: GraphQL query (assigned, mentions, stale)
GH-->>GQL: Query result
GQL-->>API: Mapped ActionRequiredResult
API-->>P: JSON payload
P-->>U: Render tabs, counts, lists
sequenceDiagram
autonumber
actor U as User
participant S as GitHubSettingsForm
participant K as useKanbanStore
participant A as useActionItemsStore
participant GQL as GitHubGraphQLClient
participant GH as GitHub GraphQL API
U->>S: Click "Save & Sync"
S->>K: syncFromGitHub()
K->>A: refreshData() / read action items
A->>GQL: getActionRequiredItems(username)
GQL->>GH: GraphQL query
GH-->>GQL: Data
GQL-->>A: Mapped items
A-->>K: Provide items
K-->>K: Create Kanban tasks, persist
K-->>S: Return created count
S-->>U: Show success alert
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (8)
✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (4)
src/app/search/page.tsx (1)
151-152: Fix type of timeoutId for browser compatibility.Using NodeJS.Timeout in client code commonly causes TS type errors when DOM typings are active. Prefer ReturnType.
- let timeoutId: NodeJS.Timeout | undefined; + let timeoutId: ReturnType<typeof setTimeout> | undefined; let lastExecTime = 0;src/lib/api/github-api-client.ts (3)
107-113: Critical: Singleton client + tokenful cache can leak data across users and return wrong resultsThe class stores a mutable token on the instance and exports a singleton. Combined with a cache keyed only by endpoint (not by token), responses for one user can be served to another, and testConnection() may report the wrong user. This is a data-leak and correctness risk under concurrent requests.
Two-part mitigation (prefer both):
- Preferred: Stop exporting a singleton; create a per-request instance.
- Additionally: Make the cache key token-aware to avoid mixing responses across tokens.
Apply both diffs:
- Token-aware cache key
- private async fetchWithCache<T>(endpoint: string, useGithub = false, isCommitData = false): Promise<T> { - const cacheKey = endpoint + private async fetchWithCache<T>(endpoint: string, useGithub = false, isCommitData = false): Promise<T> { + const authKey = useGithub && this.githubToken ? this.githubToken.slice(0, 8) : 'anon' + const cacheKey = `${endpoint}|auth=${authKey}`
- Replace singleton with factory to ensure per-request isolation
-export const githubAPIClient = new GitHubAPIClient() +export const createGitHubAPIClient = () => new GitHubAPIClient()Follow-up: update call sites to use a per-request instance (e.g., within route handlers or server actions) instead of a global singleton.
Also applies to: 167-174, 190-198, 206-209, 968-969
121-157: Token validation rejects valid OAuth tokens due to contradictory length checksYou correctly detect token formats, but later enforce a blanket minimum length of 40 characters, which conflicts with your OAuth criteria (20–255). This will throw on valid OAuth tokens 20–39 chars long.
Refine the length checks to be format-specific:
- if (trimmedToken.length < 40) { - throw new Error('GitHub token is too short. Minimum length is 40 characters.') - } - - if (trimmedToken.length > 255) { - throw new Error('GitHub token is too long. Maximum length is 255 characters.') - } + // Enforce format-appropriate length ranges + if (isOAuthToken) { + if (trimmedToken.length < 20) { + throw new Error('OAuth token is too short. Minimum length is 20 characters.') + } + } else { + if (trimmedToken.length < 40) { + throw new Error('GitHub token is too short. Minimum length is 40 characters.') + } + } + if (trimmedToken.length > 255) { + throw new Error('GitHub token is too long. Maximum length is 255 characters.') + }Also applies to: 148-154
710-714: Infinite recursion risk in getUserAnalytics when no repos foundCalling this.getUserAnalytics(username) again with identical conditions can loop forever and overflow the stack.
Remove the recursion and proceed to compute behavior with an empty overview:
- if (overview.length === 0) { - console.warn('No repositories found, using demo overview'); - return this.getUserAnalytics(username); - } + if (overview.length === 0) { + console.warn('No repositories found; proceeding with empty overview'); + }
🧹 Nitpick comments (43)
package.json (2)
45-49: Remove unused devDependency ‘critters’ or wire it up explicitlyYou enabled experimental.optimizeCss in next.config, which uses Next’s built-in CSS optimizer. If you’re not integrating critters via a plugin, this dep is likely unused and can be removed.
Apply this diff to remove it:
"devDependencies": { "@eslint/eslintrc": "^3", "@tailwindcss/postcss": "^4", "@types/node": "^20.19.10", "@types/react": "^19.1.9", - "critters": "^0.0.23", "eslint": "^9", "eslint-config-next": "15.3.4", "tailwindcss": "^4", "tw-animate-css": "^1.3.4", "typescript": "^5" }
12-14: DnD-kit imports are correctly scoped to client componentsVerified that all components importing
@dnd-kit/*live behind a'use client'boundary:
- src/components/kanban/KanbanBoard.tsx has
'use client'at line 1 and pulls in:
@dnd-kit/core@dnd-kit/sortable@dnd-kit/utilities- No other server-rendered modules import DnD-kit packages.
Optional performance tweak:
In the parent page (e.g.
src/app/dashboard/page.tsx), lazy-load the board to defer the DnD-kit bundle:import dynamic from 'next/dynamic' const KanbanBoard = dynamic( () => import('@/components/kanban/KanbanBoard'), { ssr: false } ) export default function DashboardPage() { return <KanbanBoard /* …props */ /> }This keeps the initial payload lighter and loads DnD-kit code only when the board is mounted.
src/app/api/auth/logout/route.ts (1)
9-15: Cookie clearing: prefer cookies.delete and verify httpOnly semanticsIf ‘githubmon-auth’ represents auth state, making it accessible to JS (httpOnly: false) is risky. Also, clearing a cookie should generally match original attributes; using httpOnly: false may fail to clear a previously httpOnly cookie. Prefer the built-in delete helper.
Apply this diff:
- response.cookies.set('githubmon-auth', '', { - expires: new Date(0), - path: '/', - httpOnly: false, - secure: process.env.NODE_ENV === 'production', - sameSite: 'strict' - }) + response.cookies.delete('githubmon-auth')If the original cookie was httpOnly, consider ensuring it’s set as httpOnly in login/session code and cleared similarly here. If you intentionally keep it readable by JS (e.g., non-sensitive UI state), document that and confirm XSS hardening elsewhere.
src/config/menu.ts (1)
6-44: Add literal immutability for safer downstream typingExport as a readonly literal to improve type inference for labels, hrefs, and child IDs, avoiding accidental mutation.
Apply this diff:
-export const menuItems = { +export const menuItems = { ... -} +} as constsrc/components/ui/textarea.tsx (1)
5-16: Forward the ref to the underlying textarea for parity and flexibility.Forwarding refs is standard for UI primitives (focus management, form libs, accessibility). Aligns with the pattern used by your other Radix-based components.
-function Textarea({ className, ...props }: React.ComponentProps<"textarea">) { - return ( - <textarea - data-slot="textarea" - className={cn( - "border-input placeholder:text-muted-foreground focus-visible:border-ring focus-visible:ring-ring/50 aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive dark:bg-input/30 flex field-sizing-content min-h-16 w-full rounded-md border bg-transparent px-3 py-2 text-base shadow-xs transition-[color,box-shadow] outline-none focus-visible:ring-[3px] disabled:cursor-not-allowed disabled:opacity-50 md:text-sm", - className - )} - {...props} - /> - ) -} +const Textarea = React.forwardRef< + HTMLTextAreaElement, + React.ComponentProps<"textarea"> +>(({ className, ...props }, ref) => { + return ( + <textarea + ref={ref} + data-slot="textarea" + className={cn( + "border-input placeholder:text-muted-foreground focus-visible:border-ring focus-visible:ring-ring/50 aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive dark:bg-input/30 flex field-sizing-content min-h-16 w-full rounded-md border bg-transparent px-3 py-2 text-base shadow-xs transition-[color,box-shadow] outline-none focus-visible:ring-[3px] disabled:cursor-not-allowed disabled:opacity-50 md:text-sm", + className + )} + {...props} + /> + ) +})Additionally (outside the selected range), consider:
Textarea.displayName = "Textarea"src/components/ui/label.tsx (1)
8-22: Forward the ref to Radix Label root.Forwarding the ref improves interoperability (focus/aria associations, form libraries) and matches common patterns for Radix wrappers.
-function Label({ - className, - ...props -}: React.ComponentProps<typeof LabelPrimitive.Root>) { - return ( - <LabelPrimitive.Root - data-slot="label" - className={cn( - "flex items-center gap-2 text-sm leading-none font-medium select-none group-data-[disabled=true]:pointer-events-none group-data-[disabled=true]:opacity-50 peer-disabled:cursor-not-allowed peer-disabled:opacity-50", - className - )} - {...props} - /> - ) -} +const Label = React.forwardRef< + React.ElementRef<typeof LabelPrimitive.Root>, + React.ComponentProps<typeof LabelPrimitive.Root> +>(({ className, ...props }, ref) => { + return ( + <LabelPrimitive.Root + ref={ref} + data-slot="label" + className={cn( + "flex items-center gap-2 text-sm leading-none font-medium select-none group-data-[disabled=true]:pointer-events-none group-data-[disabled=true]:opacity-50 peer-disabled:cursor-not-allowed peer-disabled:opacity-50", + className + )} + {...props} + /> + ) +})Optionally (outside the selected range):
Label.displayName = "Label"src/components/ui/switch.tsx (1)
8-29: Forward the ref to Radix Switch root.This enables focus management, form libs integration, and consistency with Radix patterns used elsewhere.
-function Switch({ - className, - ...props -}: React.ComponentProps<typeof SwitchPrimitive.Root>) { - return ( - <SwitchPrimitive.Root - data-slot="switch" - className={cn( - "peer data-[state=checked]:bg-primary data-[state=unchecked]:bg-input focus-visible:border-ring focus-visible:ring-ring/50 dark:data-[state=unchecked]:bg-input/80 inline-flex h-[1.15rem] w-8 shrink-0 items-center rounded-full border border-transparent shadow-xs transition-all outline-none focus-visible:ring-[3px] disabled:cursor-not-allowed disabled:opacity-50", - className - )} - {...props} - > - <SwitchPrimitive.Thumb - data-slot="switch-thumb" - className={cn( - "bg-background dark:data-[state=unchecked]:bg-foreground dark:data-[state=checked]:bg-primary-foreground pointer-events-none block size-4 rounded-full ring-0 transition-transform data-[state=checked]:translate-x-[calc(100%-2px)] data-[state=unchecked]:translate-x-0" - )} - /> - </SwitchPrimitive.Root> - ) -} +const Switch = React.forwardRef< + React.ElementRef<typeof SwitchPrimitive.Root>, + React.ComponentProps<typeof SwitchPrimitive.Root> +>(({ className, ...props }, ref) => { + return ( + <SwitchPrimitive.Root + ref={ref} + data-slot="switch" + className={cn( + "peer data-[state=checked]:bg-primary data-[state=unchecked]:bg-input focus-visible:border-ring focus-visible:ring-ring/50 dark:data-[state=unchecked]:bg-input/80 inline-flex h-[1.15rem] w-8 shrink-0 items-center rounded-full border border-transparent shadow-xs transition-all outline-none focus-visible:ring-[3px] disabled:cursor-not-allowed disabled:opacity-50", + className + )} + {...props} + > + <SwitchPrimitive.Thumb + data-slot="switch-thumb" + className={cn( + "bg-background dark:data-[state=unchecked]:bg-foreground dark:data-[state=checked]:bg-primary-foreground pointer-events-none block size-4 rounded-full ring-0 transition-transform data-[state=checked]:translate-x-[calc(100%-2px)] data-[state=unchecked]:translate-x-0" + )} + /> + </SwitchPrimitive.Root> + ) +})Optionally (outside the selected range):
Switch.displayName = "Switch"src/stores/settings.ts (2)
4-18: Types are not exported as claimed; export interfaces for external consumption.The AI summary mentions exported types, but in this file they are not exported. Exporting them helps consumers (e.g., forms) type props and helpers.
-interface GitHubSettings { +export interface GitHubSettings { repositories: string[] labels: string[] assignedToMe: boolean mentionsMe: boolean authoredByMe: boolean reviewRequestedFromMe: boolean stalePRDays: number } -interface SettingsState { +export interface SettingsState { githubSettings: GitHubSettings updateGitHubSettings: (settings: Partial<GitHubSettings>) => void resetSettings: () => void }
41-43: Consider adding persist versioning and migrations.Future schema changes (e.g., adding/removing keys) will be easier to manage without breaking existing users’ localStorage.
{ name: 'githubmon-settings', - storage: createJSONStorage(() => localStorage) + storage: createJSONStorage(() => localStorage), + version: 1, + // Example migration scaffold + migrate: (persistedState: any, version) => { + if (version < 1) { + // perform migrations if needed + } + return persistedState + } }src/app/search/page.tsx (2)
312-312: Remove stray empty element.This empty div appears to be a leftover and adds unnecessary DOM noise.
- <div></div>
215-237: Stabilize performSearch and effect dependenciesWrap performSearch in useCallback and include it in the effect’s dependency array to prevent stale closures and satisfy the linter.
• Wrap performSearch:
- const performSearch = async (query: string, type: "users" | "repos") => { + const performSearch = useCallback(async (query: string, type: "users" | "repos") => { setSearchResults((prev) => ({ ...prev, loading: true, error: null })); … - }; + }, []);• Update the effect’s deps:
- }, [userParam, repoParam, setCurrentQuery, setCurrentSearchType, loadUserAnalytics]); + }, [userParam, repoParam, setCurrentQuery, setCurrentSearchType, loadUserAnalytics, performSearch]);Optional: run your linter to verify the hook deps are now clean. For example:
pnpm lint -- --max-warnings=0 --ext .ts,.tsx src/app/search/page.tsxsrc/lib/api/github-api-client.ts (4)
11-15: Avoid type duplication: import GitHubSearchResponse from shared typesThis local interface duplicates src/types/api.ts. Prefer a single source of truth.
+import type { GitHubSearchResponse } from '@/types/api' - -interface GitHubSearchResponse<T> { - items: T[] - total_count: number - incomplete_results: boolean -}
213-224: Be cautious with verbose error logging of API responsesDumping full error bodies and rate-limit headers is useful for debugging but noisy in production and can reveal details about repositories or orgs. Consider gating behind an environment flag.
- console.error(`❌ GitHub API Error: ${response.status} ${response.statusText}`) - console.error(`📍 Endpoint: ${endpoint}`) - console.error(`🔑 Token: ${this.githubToken ? 'Present' : 'Missing'}`) + if (process.env.NODE_ENV !== 'production' || process.env.DEBUG_GITHUB === '1') { + console.error(`❌ GitHub API Error: ${response.status} ${response.statusText}`) + console.error(`📍 Endpoint: ${endpoint}`) + console.error(`🔑 Token: ${this.githubToken ? 'Present' : 'Missing'}`) + } ... - console.error(`⏰ Rate Limit - Remaining: ${rateLimitRemaining}, Reset: ${rateLimitReset}`) + if (process.env.NODE_ENV !== 'production' || process.env.DEBUG_GITHUB === '1') { + console.error(`⏰ Rate Limit - Remaining: ${rateLimitRemaining}, Reset: ${rateLimitReset}`) + } ... - console.error(`📄 Error Response:`, errorText) + if (process.env.NODE_ENV !== 'production' || process.env.DEBUG_GITHUB === '1') { + console.error(`📄 Error Response:`, errorText) + }
411-439: Type the return shape (avoid Promise<unknown[]>) and align fieldsReturning unknown[] hampers TS safety; define a shared ActionItem type for assigned/authored/review requests (with optional fields like assignee/mentionType) to keep mapping strongly typed.
Example (outside this range):
- Introduce ActionItem interface and use Promise<ActionItem[]>
441-471: Tidy logging and normalize id types
- Remove console noise in production; wrap logs behind a debug flag.
- Consider normalizing id types across methods (number vs 'review-') to reduce downstream checks.
- if (!this.githubToken) { - console.log('❌ No GitHub token for review requests') + if (!this.githubToken) { return [] } ... - console.log(`🔍 Getting review requests for: ${user}`)src/app/api/auth/[...nextauth]/route.ts (1)
73-85: Avoid any in callbacks; leverage NextAuth types for safer evolutionUse the typed callback params to catch breakages on provider/account/token shape changes.
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - async jwt({ token, account, user }: any) { + async jwt({ token, account, user }: import('next-auth').JWTCallbackParams) {Or annotate authOptions as NextAuthOptions:
- import type { NextAuthOptions } from 'next-auth'
- const authOptions: NextAuthOptions = { ... }
src/components/layout/Sidebar.tsx (1)
88-97: Nit: show type-specific loading instead of any quick-wins loaderCurrently, good-first-issues shows '...' when easy-fixes is loading (and vice versa). Gate the loader per type for better UX.
- if (type === 'goodFirstIssues' || type === 'easyFixes') { - if (quickWinsLoading.goodIssues || quickWinsLoading.easyFixes) return '...' - } else { + if (type === 'goodFirstIssues') { + if (quickWinsLoading.goodIssues) return '...' + } else if (type === 'easyFixes') { + if (quickWinsLoading.easyFixes) return '...' + } else { if (loading[type]) return '...' }src/app/page.tsx (1)
23-26: Add cleanup for delayed redirect to avoid stray navigationThe 100ms delay helps avoid middleware conflicts, but the timer should be cleared on unmount to prevent a stale redirect firing after the component unmounts or dependencies change.
Apply this diff:
- // Small delay to ensure middleware doesn't conflict - setTimeout(() => { - router.replace('/dashboard') - }, 100) + // Small delay to ensure middleware doesn't conflict; ensure we clear the timer on unmount + const timer = window.setTimeout(() => { + router.replace('/dashboard') + }, 100) + return () => window.clearTimeout(timer)src/app/api/action-required/route.ts (3)
1-3: Consider disabling route caching and (optionally) pin Node.js runtimeIf these results depend on user auth and/or change frequently, explicitly mark the route dynamic to avoid caching surprises in production. If your GitHub client requires Node APIs, opt-in to node runtime.
Apply this diff:
+export const dynamic = 'force-dynamic' +// If your GraphQL client needs Node APIs (e.g., uses process.env, node-fetch), enable Node.js runtime: +// export const runtime = 'nodejs' + import { NextRequest, NextResponse } from 'next/server' import { githubGraphQLClient } from '@/lib/api/github-graphql-client'
6-7: Use request.nextUrl to parse query paramsMinor simplification; NextRequest exposes nextUrl with searchParams.
Apply this diff:
- const { searchParams } = new URL(request.url) + const { searchParams } = request.nextUrl
20-25: Good error handling; consider structured loggingCurrent try/catch with 500 is sufficient. If you have a centralized logger, prefer it over console.error for consistency and observability.
src/components/providers/OAuthSessionSync.tsx (2)
3-6: Leverage store’s cookie sync to avoid re-registering listeners and stale closuresInstead of manually reading cookies and toggling multiple setters, use the store’s checkCookieSync() to centralize logic. This also lets you keep the event listener stable and avoid dependency-churn re-registration.
Apply this diff:
-import { useEffect } from 'react' +import { useEffect } from 'react' import { useAuthStore } from '@/stores' -import { cookieUtils } from '@/lib/cookies' export function OAuthSessionSync() { - const { setOrgData, setConnected, setTokenExpiry, isConnected } = useAuthStore() + const { checkCookieSync } = useAuthStore() -useEffect(() => { - const handleFocus = () => { - const authData = cookieUtils.getAuth() - if (!authData && isConnected) { - setConnected(false) - setOrgData(null) - setTokenExpiry(null) - } - } - - window.addEventListener('focus', handleFocus) - return () => window.removeEventListener('focus', handleFocus) -}, [isConnected, setConnected, setOrgData, setTokenExpiry]) +useEffect(() => { + const handleFocus = () => { + checkCookieSync() + } + window.addEventListener('focus', handleFocus) + return () => window.removeEventListener('focus', handleFocus) +}, [checkCookieSync])Also applies to: 10-22
10-22: Add visibilitychange listener (optional) for tab switches without focusSome browsers/tabs won’t trigger focus when switching tabs or windows. Listening to document.visibilitychange can improve reliability of cookie invalidation checks.
Proposed addition within the same effect:
const handleVisibility = () => { if (document.visibilityState === 'visible') { checkCookieSync() } } document.addEventListener('visibilitychange', handleVisibility) return () => document.removeEventListener('visibilitychange', handleVisibility)src/components/quick-wins/hooks/useQuickWins.ts (1)
45-56: Harden mapping: null-safe labels and extracted daysOld calculator (dedupe logic).Avoid potential runtime errors when labels is undefined and remove duplicated date math. Also clamp negative values if client clock skews or created_at is invalid.
Apply these diffs to both mappings:
- labels: issue.labels.map(l => l.name), - daysOld: Math.floor((Date.now() - new Date(issue.created_at).getTime()) / (1000 * 60 * 60 * 24)) + labels: (issue.labels ?? []).map(l => l.name), + daysOld: computeDaysOld(issue.created_at)And add this helper (outside the shown ranges, e.g., near the top of the file):
function computeDaysOld(createdAt?: string | Date): number { if (!createdAt) return 0 const t = new Date(createdAt).getTime() if (Number.isNaN(t)) return 0 return Math.max(0, Math.floor((Date.now() - t) / 86_400_000)) }Optionally, extract a shared mapper (mapIssueToActionItem) to eliminate duplication between goodIssues and easyFixes.
Also applies to: 63-74
src/app/login/page.tsx (1)
82-97: De-duplicate loading spinners with a tiny Spinner component.Same spinner is rendered for both “already authenticated” and “auth loading” cases. Extract once to keep things DRY and easier to tweak.
Example (outside the selected lines):
function FullPageSpinner() { return ( <div className="h-screen bg-background flex items-center justify-center"> <div className="w-8 h-8 border-2 border-current/30 border-t-current rounded-full animate-spin" /> </div> ) }Then replace both return blocks with
<FullPageSpinner />.src/components/kanban/TaskDetailModal.tsx (2)
127-129: Cancel should revert unsaved edits.Currently cancel only exits edit mode but keeps edited (unsaved) values visible. Reset form state to task’s original values on cancel.
- <Button variant="outline" size="sm" onClick={() => setIsEditing(false)}> + <Button variant="outline" size="sm" onClick={handleCancelEdit}> <X className="w-4 h-4" /> </Button>Add the helper (outside the selected lines):
const handleCancelEdit = useCallback(() => { if (task) { setEditData({ title: task.title, description: task.description || '', priority: task.priority, notes: task.notes || '' }) } setIsEditing(false) }, [task])
79-87: Tighten typing for priority helper.Constrain parameter type to the union used by KanbanTask for better type safety.
- const getPriorityDisplay = (priority: string) => { + const getPriorityDisplay = (priority: KanbanTask['priority']) => {src/components/widget/TodoDashboard.tsx (1)
10-16: Avoid double sync on mount.Two effects call
syncFromGitHub()on mount, and React StrictMode can double-invoke effects in dev. Remove the first effect and keep the githubSettings-dependent one to trigger initial and subsequent syncs.- useEffect(() => { - syncFromGitHub() - }, [syncFromGitHub]) - useEffect(() => { syncFromGitHub() }, [githubSettings, syncFromGitHub])If duplicate calls are still observed in dev, consider guarding with a store-level
isSyncingflag or a simple throttle.src/app/quick-wins/page.tsx (1)
46-51: Use replace instead of push for tab URL updates (cleaner history).Switching tabs shouldn’t clutter browser history. Replace avoids forcing users to press Back multiple times to leave the page.
- router.push(`/quick-wins?tab=${tab}`) + router.replace(`/quick-wins?tab=${tab}`)src/components/kanban/AddTaskModal.tsx (2)
49-49: Guard Dialog onOpenChange to only reset on closePassing handleClose directly will also run on open-change events. Prefer checking the boolean to avoid wiping local state when the modal opens.
- <Dialog open={isOpen} onOpenChange={handleClose}> + <Dialog + open={isOpen} + onOpenChange={(open) => { + if (!open) handleClose() + }} + >
57-63: Improve UX: auto-focus the title inputAuto-focusing the primary field speeds up task entry.
<Input id="title" value={title} onChange={(e) => setTitle(e.target.value)} placeholder="Task title..." required + autoFocus />src/app/settings/page.tsx (2)
57-63: Wrap GitHubSettingsForm in a Card (CardContent without Card)CardContent is intended to be used within a Card. Wrap the form to keep structure and styles consistent.
- <TabsContent value="github" className="space-y-6"> - - <CardContent> - <GitHubSettingsForm /> - </CardContent> - - </TabsContent> + <TabsContent value="github" className="space-y-6"> + <Card> + <CardHeader> + <CardTitle>GitHub Integration</CardTitle> + </CardHeader> + <CardContent> + <GitHubSettingsForm /> + </CardContent> + </Card> + </TabsContent>
42-56: Tabs grid columns don’t match the number of triggersUsing grid-cols-5 with two triggers leaves three empty columns. Right-size the grid (or let it size naturally).
- <TabsList className="grid w-full grid-cols-5"> + <TabsList className="grid w-full grid-cols-2 sm:grid-cols-2">Alternatively, drop the grid and use a flex row for auto sizing:
- <TabsList className="grid w-full grid-cols-5"> + <TabsList className="flex w-full gap-2"src/app/auth/callback/page.tsx (1)
24-53: Avoid orphaned timers: add cleanup for the delayed redirectEnsure setTimeout is cleared if the component unmounts before it fires.
-useEffect(() => { +useEffect(() => { if (status === 'loading') return const extendedSession = session as ExtendedSession if (status === 'authenticated' && extendedSession?.accessToken && extendedSession.user) { const expiryDate = new Date() expiryDate.setDate(expiryDate.getDate() + 30) // GitHub username'i öncelikle login field'ından al const githubUsername = extendedSession.user.login || 'Unknown' const displayName = extendedSession.user.name || extendedSession.user.login || 'Unknown' setOrgData({ orgName: displayName, username: githubUsername, token: extendedSession.accessToken }) setTokenExpiry(expiryDate.toISOString()) setConnected(true) // Wait a moment for state to update, then redirect - setTimeout(() => { - router.replace('/dashboard') - }, 500) + const timer = window.setTimeout(() => { + router.replace('/dashboard') + }, 500) + return () => window.clearTimeout(timer) } else if (status === 'unauthenticated') { // If not authenticated, redirect to login with error router.replace('/login?error=authentication_failed') } }, [session, status, router, setOrgData, setConnected, setTokenExpiry])src/stores/index.ts (1)
149-155: Reduce re-renders by selecting only needed auth fieldsUsing the entire store object can cause extra re-renders. Select the fields you need.
-export const useIsAuthenticated = () => { - const hasHydrated = useStoreHydration() - const { isConnected, orgData, isTokenValid } = useAuthStore() - - if (!hasHydrated) return false - return isConnected && orgData?.token && isTokenValid() -} +export const useIsAuthenticated = () => { + const hasHydrated = useStoreHydration() + const { isConnected, orgData, isTokenValid } = useAuthStore(s => ({ + isConnected: s.isConnected, + orgData: s.orgData, + isTokenValid: s.isTokenValid + })) + if (!hasHydrated) return false + return isConnected && orgData?.token && isTokenValid() +}src/components/kanban/KanbanBoard.tsx (1)
325-333: Minor consistency: use the Button component for the “Add Task” actionYou’re using raw button markup amidst a UI kit. Consider the design system Button for consistent styling and behavior (focus, disabled, etc.).
Example:
- <button - className="w-full p-2 border-2 border-dashed border-muted rounded hover:border-primary hover:bg-primary/5 transition-colors text-muted-foreground hover:text-primary text-xs" - onClick={() => { - setAddTaskColumnId(columnId); - setShowAddTaskModal(true); - }} - > - <Plus className="w-3 h-3 mx-auto" /> - </button> + <Button + variant="outline" + className="w-full border-2 border-dashed text-xs" + onClick={() => { + setAddTaskColumnId(columnId) + setShowAddTaskModal(true) + }} + > + <Plus className="w-3 h-3 mr-1" /> + Add Task + </Button>src/stores/kanban.ts (4)
251-257: Assign PRs to Review column by default (context currently unused)You compute contextSuggestions but then route everything to todo. At minimum, send PRs to review by default.
- const targetColumn = contextSuggestions.length > 0 ? 'todo' : 'todo' + const targetColumn = task.type === 'github-pr' ? 'review' : 'todo'Longer-term: actually apply contextSuggestions (e.g., emergency → top of todo, urgent-reviews → review, etc.).
20-26: Replace any in GitHubDataContext with concrete typesStrong typing here will prevent a class of bugs (e.g., created_at vs createdAt). Use the item types from actionItems and the real settings type.
-interface GitHubDataContext { - assignedItems: any[] - mentionItems: any[] - staleItems: any[] - currentTime: Date - userSettings: any -} +interface GitHubDataContext { + assignedItems: import('./actionItems').AssignedItem[] + mentionItems: import('./actionItems').MentionItem[] + staleItems: import('./actionItems').StalePR[] + currentTime: Date + userSettings: ReturnType<typeof useSettingsStore.getState>['githubSettings'] +}If you prefer imports at top-level, see next comment.
5-6: Import concrete item types for stronger inferenceAligns with the previous refactor to eliminate any[].
-import type { ActionItem } from './actionItems' +import type { ActionItem, AssignedItem, MentionItem, StalePR } from './actionItems'
188-201: Reduce noisy console logs in the storeThese logs will persist in production and can clutter consoles. Consider guarding behind a DEBUG flag or removing.
Also applies to: 215-217
src/stores/actionItems.ts (3)
35-69: Inconsistency: goodFirstIssues/easyFixes appear retained despite GraphQL refactorAI summary says these groups were removed from the public surface and persistence, but they’re still part of state, loading/errors, and lastRefresh. This increases complexity without data sources updating them.
Consider removing goodFirstIssues/easyFixes from:
- State shape (data/loading/errors/lastRefresh)
- Getters (getTotalCount/getCountByType)
- clearAll
Or reintroduce their fetch/update flows if still desired.
170-179: Align getCountByType with supported typesThe switch still exposes counts for goodFirstIssues/easyFixes even though they’re not populated in the GraphQL path.
- case 'goodFirstIssues': return state.goodFirstIssues.length - case 'easyFixes': return state.easyFixes.lengthAnd narrow the type annotation accordingly if you keep this API.
256-258: Unify comment languageNon-English comments can be confusing in a mixed-language codebase. Prefer concise English.
- // ✅ GraphQL ile tek çağrıda tüm action required items'ı al + // Fetch all action-required items in a single GraphQL call ... - // ✅ Hepsi için veriyi güncelle + // Update all categoriesAlso applies to: 285-303
…type definitions and icon imports
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/app/api/auth/[...nextauth]/route.ts (1)
4-18: Avoid suggesting that accessToken is available on the client SessionYou’ve removed the code that assigns accessToken to the session (good), but the Session type augmentation still declares
accessToken?. This implies to consumers that they can (or should) read the token on the client. To enforce least privilege and avoid future regressions, dropaccessTokenfrom the Session type.Apply this diff:
declare module "next-auth" { interface Session { - accessToken?: string user: { name?: string | null email?: string | null image?: string | null login?: string } } interface User { login?: string } }src/stores/kanban.ts (4)
220-235: Fix priority derivation: stop reading mentionType off ActionItem; use item.priorityReading mentionType off ActionItem is a type leak and will either TS-error or silently misprioritize. The store already receives a normalized priority from ActionItems; rely on it.
Apply:
- const newTasks: KanbanTask[] = selectedItems.map(item => { - const isReviewRequest = 'mentionType' in item && item.mentionType === 'review_request' - const itemWithExtras = item as unknown as GitHubItemWithExtras - - return { - id: `action-${item.id}`, - title: item.title, - description: itemWithExtras.description?.substring(0, 200), - type: item.type === 'issue' ? 'github-issue' : 'github-pr', - priority: isReviewRequest ? 'high' : 'medium', - githubUrl: item.url, - labels: itemWithExtras.labels?.map((l: { name: string }) => l.name) || [], - createdAt: new Date(item.createdAt), - updatedAt: new Date(item.updatedAt || item.createdAt) - } - }) + const newTasks: KanbanTask[] = uniqueSelected.map((item) => { + const itemWithExtras = item as unknown as GitHubItemWithExtras + return { + id: `action-${item.id}`, + title: item.title, + description: itemWithExtras.description?.substring(0, 200), + type: item.type === 'issue' ? 'github-issue' : 'github-pr', + priority: item.priority, + githubUrl: item.url, + labels: itemWithExtras.labels?.map((l: { name: string }) => l.name) || [], + createdAt: new Date(item.createdAt), + updatedAt: new Date(item.updatedAt || item.createdAt), + } + })
294-321: Guard against invalid indices and missing columns in moveTask; clamp newIndexAs written, splice(-1,1) will remove the last item if taskId is not found, and missing columns will crash. Also clamp insertion index and avoid duplicate entries in the destination.
Apply:
moveTask: (taskId, fromColumnId, toColumnId, newIndex) => { set((state) => { const fromColumn = state.columns[fromColumnId] const toColumn = state.columns[toColumnId] + if (!fromColumn || !toColumn) return state const fromTaskIds = [...fromColumn.taskIds] const toTaskIds = fromColumnId === toColumnId ? fromTaskIds : [...toColumn.taskIds] const taskIndex = fromTaskIds.indexOf(taskId) - fromTaskIds.splice(taskIndex, 1) + if (taskIndex < 0) return state + fromTaskIds.splice(taskIndex, 1) - toTaskIds.splice(newIndex, 0, taskId) + // Ensure no duplicates in destination + const existingDestIndex = toTaskIds.indexOf(taskId) + if (existingDestIndex >= 0) { + toTaskIds.splice(existingDestIndex, 1) + } + const insertAt = Math.max(0, Math.min(newIndex, toTaskIds.length)) + toTaskIds.splice(insertAt, 0, taskId) return { columns: { ...state.columns, [fromColumnId]: { ...fromColumn, taskIds: fromTaskIds }, [toColumnId]: { ...toColumn, taskIds: toTaskIds } } } }) },
367-372: Avoid mutating defaultColumns when clearing; deep-clone per columnresetColumns is a shallow clone; assigning taskIds mutates the shared defaultColumns singleton.
Apply:
- const resetColumns = { ...defaultColumns } - Object.keys(resetColumns).forEach(columnId => { - resetColumns[columnId].taskIds = personalTaskIds.filter(taskId => - state.columns[columnId]?.taskIds.includes(taskId) || false - ) - }) + const resetColumns = Object.fromEntries( + Object.entries(defaultColumns).map(([columnId, col]) => [ + columnId, + { + ...col, + taskIds: personalTaskIds.filter( + (taskId) => state.columns[columnId]?.taskIds.includes(taskId) || false + ), + }, + ]) + ) as Record<string, KanbanColumn>
425-427: SSR-safe persistence and revive Date fields on rehydrateDirectly referencing localStorage breaks on SSR; persisted Date objects rehydrate as strings. Guard storage and coerce timestamps back to Date.
Apply:
- name: 'githubmon-kanban', - storage: createJSONStorage(() => localStorage) + name: 'githubmon-kanban', + storage: createJSONStorage(() => { + if (typeof window === 'undefined') { + return { + getItem: () => null, + setItem: () => {}, + removeItem: () => {}, + } as any + } + return localStorage + }), + onRehydrateStorage: () => (state) => { + if (!state?.tasks) return + for (const id of Object.keys(state.tasks)) { + const t = (state.tasks as any)[id] + if (t?.createdAt && typeof t.createdAt === 'string') t.createdAt = new Date(t.createdAt) + if (t?.updatedAt && typeof t.updatedAt === 'string') t.updatedAt = new Date(t.updatedAt) + } + }
🧹 Nitpick comments (13)
src/app/api/auth/[...nextauth]/route.ts (2)
46-50: OAuth scopes: confirm necessity of broad “repo” scopeThe
reposcope grants full private repo access. If you only need public repo data, preferpublic_repo. If private repo read is required, consider documenting why and whether a narrower combination suffices.- scope: "read:user user:email read:org repo" + // If private repos are not required: + // scope: "read:user user:email read:org public_repo" + // Otherwise document why full repo scope is necessary: + scope: "read:user user:email read:org repo"Would you confirm whether private repositories are strictly needed for the GraphQL queries that power “Action Required”? If not, let’s tighten this.
1-1: Nit: Use the recommended App Router import and type the optionsFor App Router route handlers, NextAuth recommends importing from "next-auth" and typing options. This helps with intellisense and compile-time checks.
-import NextAuth from "next-auth/next" +import NextAuth, { type NextAuthOptions } from "next-auth" @@ -const authOptions = { +const authOptions: NextAuthOptions = {src/middleware.ts (5)
15-21: Verify token-in-cookie design and ensure secrets aren’t client-readableThe auth check relies on
authData.orgData?.tokenandtokenExpirystored in thegithubmon-authcookie. If this token is a GitHub access token (or similarly sensitive), storing it in a client-accessible cookie is risky. If you need it server-side only, ensure the cookie is httpOnly, secure, sameSite, and that its contents do not expose raw access tokens to the browser.
- Confirm what
orgData.tokencontains. If it’s a GH access token, move to server-only storage (e.g., NextAuth JWT) and let the cookie hold non-sensitive auth state (e.g., isConnected + expiry).- If you must keep the token in a cookie, set httpOnly/secure/sameSite when writing it so it’s not readable by JS.
I can help refactor to rely on the NextAuth JWT via
getTokenin API routes, while keeping middleware gate logic token-agnostic. Want a patch?
37-44: Use cookie delete API and consider redirecting after logoutYou can simplify cookie clearing and avoid attribute mismatches by using
response.cookies.delete. Optionally redirect to/loginfor a clearer UX unless the/api/auth/logoutroute handles it.if (pathname === '/api/auth/logout') { - const response = NextResponse.next() - response.cookies.set('githubmon-auth', '', { - expires: new Date(0), - path: '/' - }) + const response = NextResponse.next() + response.cookies.delete('githubmon-auth', { path: '/' }) return response }
32-36: Prefix checks can overmatch; consider path-boundary checks
startsWith('/settings')will also match unexpected paths like/settings-old. If that’s not intended, use a boundary-aware check.Example:
-const isProtectedRoute = protectedRoutes.some(route => pathname.startsWith(route)) +const isProtectedRoute = protectedRoutes.some(route => + pathname === route || pathname.startsWith(`${route}/`) +)Apply the same pattern to
protectedApiRoutesandauthRoutesif appropriate.
30-36: Exact-match public routes may exclude trailing slashesWith
pathname === route,/privacy-policy/would not be considered public. If you serve both with and without trailing slashes, adjust accordingly.-const isPublicRoute = publicRoutes.some(route => pathname === route) +const isPublicRoute = publicRoutes.some(route => + pathname === route || pathname === `${route}/` +)
74-77: Matcher excludes common assets, but consider widening file extensionsYou already exclude several image formats. To reduce middleware overhead further, consider excluding other static types (css, js, map, txt, xml, ico).
matcher: [ - '/((?!_next/static|_next/image|favicon.ico|.*\\.(?:svg|png|jpg|jpeg|gif|webp)$).*)', + '/((?!_next/static|_next/image|favicon.ico|.*\\.(?:svg|png|jpg|jpeg|gif|webp|css|js|map|txt|xml|ico)$).*)', ],src/stores/kanban.ts (6)
109-114: Prefer mentionedAt for mentions and guard invalid dates in weekend detectionMentions have a specific timestamp (mentionedAt). Also, be defensive against invalid dates.
Apply:
- const isMonday = context.currentTime.getDay() === 1 - const weekendMentions = context.mentionItems.filter(item => { - const itemDate = new Date(item.createdAt || item.updatedAt) - const dayOfWeek = itemDate.getDay() - return dayOfWeek === 0 || dayOfWeek === 6 - }) + const isMonday = context.currentTime.getDay() === 1 + const weekendMentions = context.mentionItems.filter((item) => { + const ts = item.mentionedAt || item.createdAt || item.updatedAt + const d = new Date(ts) + if (isNaN(d.getTime())) return false + const day = d.getDay() + return day === 0 || day === 6 + })
95-97: Simplify type-narrowing: you’re already in MentionItem[]The extra 'mentionType' in item check is redundant here.
Apply:
- const reviewCount = context.mentionItems.filter(item => - 'mentionType' in item && item.mentionType === 'review_request' - ).length + const reviewCount = context.mentionItems.filter( + (item) => item.mentionType === 'review_request' + ).length
256-260: Clean up redundant ternary; always adds to 'todo'The condition has identical branches; simplify to avoid confusion.
Apply:
- const targetColumn = contextSuggestions.length > 0 ? 'todo' : 'todo' + const targetColumn = 'todo' updatedColumns[targetColumn] = { ...updatedColumns[targetColumn], taskIds: [...updatedColumns[targetColumn].taskIds, task.id] }
60-61: Type-safety: disallow updating a column’s id fieldPrevent accidental id mutation via the updates type.
Apply:
- updateColumn: (id: string, updates: Partial<KanbanColumn>) => void + updateColumn: (id: string, updates: Partial<Omit<KanbanColumn, 'id'>>) => void
37-39: Give userSettings a precise typeAvoid unknown; tie it to the settings store for type safety.
Apply:
- userSettings: unknown + userSettings: ReturnType<typeof useSettingsStore.getState>['githubSettings']
409-418: Optional UX: handle orphan tasks when deleting a columnDeleting a column currently leaves tasks orphaned (not visible in any column). Consider moving its tasks to 'todo' (or the first column in columnOrder) before removal.
I can draft a safe migration that moves tasks and preserves order; want me to push a patch?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
package.json(2 hunks)src/app/action-required/page.tsx(8 hunks)src/app/api/auth/[...nextauth]/route.ts(2 hunks)src/components/kanban/KanbanBoard.tsx(1 hunks)src/middleware.ts(1 hunks)src/stores/kanban.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/kanban/KanbanBoard.tsx
- src/app/action-required/page.tsx
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/stores/kanban.ts (2)
src/stores/actionItems.ts (5)
AssignedItem(20-22)MentionItem(24-27)StalePR(29-33)useActionItemsStore(91-386)ActionItem(6-18)src/stores/settings.ts (1)
useSettingsStore(30-45)
🔇 Additional comments (5)
src/app/api/auth/[...nextauth]/route.ts (4)
51-66: Profile mapping looks solidGood normalization of id, name fallback to login, and preserving avatar/email. This aligns well with downstream usage that expects
user.login.
73-83: JWT callback mapping is correctPersisting
account.access_tokenin the JWT and carrying overuser.loginis appropriate. No issues.
84-89: Session callback keeps tokens off the client — goodOnly projecting
loginontosession.useris safe and sufficient. Thanks for not sending the access token to the client.
4-18: Note: AI summary inconsistency — session does not expose accessTokenThe AI summary claims the session callback exposes
accessToken, but the code does not set it. Keeping it this way is correct from a security standpoint.src/middleware.ts (1)
54-63: 401 JSON for protected API routes looks goodReturning a 401 JSON from middleware for API calls is appropriate and avoids HTML redirects in API consumers.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/stores/actionItems.ts (1)
86-89: Narrow markAsRead/refreshData unions or implement missing casesYou accept 'goodFirstIssues' and 'easyFixes' but don’t handle them in markAsRead (or in refreshData’s switch). Either add cases or narrow the union to the three supported types.
Apply this diff if you prefer narrowing:
- markAsRead: (type: 'assigned' | 'mentions' | 'stale' | 'goodFirstIssues' | 'easyFixes', id: string) => void - refreshData: (type?: 'assigned' | 'mentions' | 'stale' | 'goodFirstIssues' | 'easyFixes') => Promise<void> + markAsRead: (type: 'assigned' | 'mentions' | 'stale', id: string) => void + refreshData: (type?: 'assigned' | 'mentions' | 'stale') => Promise<void>If you keep them, mirror the patterns used for the other types and hook up githubGraphQLClient.getGoodFirstIssues/getEasyFixes.
🧹 Nitpick comments (16)
src/components/kanban/TaskDetailModal.tsx (5)
58-63: Prefer AlertDialog over window.confirm for deletion (UX + accessibility).window.confirm blocks the UI and is hard to style/localize. Consider using your UI library’s AlertDialog for consistency and better a11y.
206-210: Use stable keys for labels (avoid index as key).Index keys can cause subtle rendering issues on reordering. Labels are unique enough to be keys.
- {task.labels.map((label, index) => ( - <Badge key={index} variant="outline" className="text-xs"> + {task.labels.map((label) => ( + <Badge key={label} variant="outline" className="text-xs"> {label} </Badge> ))}
25-26: Tighten types around priority to the KanbanTask union.Improves type-safety and prevents accidental invalid values.
- priority: 'medium' as 'low' | 'medium' | 'high' | 'urgent', + priority: 'medium' as KanbanTask['priority'], @@ - const getPriorityDisplay = (priority: string) => { + const getPriorityDisplay = (priority: KanbanTask['priority']) => { @@ - <Select value={editData.priority} onValueChange={(value: 'low' | 'medium' | 'high' | 'urgent') => setEditData({ ...editData, priority: value })}> + <Select value={editData.priority} onValueChange={(value) => setEditData({ ...editData, priority: value as KanbanTask['priority'] })}>Also applies to: 78-86, 144-154
127-130: Disable Save when title is empty.Prevents saving invalid tasks with blank titles.
- <Button size="sm" onClick={handleSave}> + <Button size="sm" onClick={handleSave} disabled={!editData.title.trim()}> <Save className="w-4 h-4 mr-1" /> Save </Button>
59-60: Unify localization: mixed Turkish and English strings + Turkish date locale.The confirm dialog is Turkish, static labels/buttons are English, and dates use 'tr-TR'. Consider centralizing locale and strings via i18n (and/or a user setting) for consistency.
Also applies to: 69-76, 121-124, 129-137, 232-233, 236-237
src/lib/cookies.ts (3)
17-22: Add Max-Age and revisit SameSite default (Strict can break auth redirects).
- Complement Expires with Max-Age to make expiry more robust and less sensitive to client clock skew.
- Defaulting to SameSite=Strict is often too restrictive for OAuth/login callbacks (cookies won’t be sent on cross-site top-level redirects). If the server needs this cookie on the OAuth callback, prefer Lax or make it configurable.
Apply this minimal diff to add Max-Age:
- `SameSite=Strict` + `SameSite=Strict`, + `Max-Age=${days * 24 * 60 * 60}`If you want to make SameSite configurable without a breaking change, consider adding an overload that accepts options (example outside the changed lines):
// Example sketch outside selected lines: type SameSite = 'Strict' | 'Lax' | 'None' type CookieOpts = { days?: number; sameSite?: SameSite } set: (name: string, value: string, daysOrOpts: number | CookieOpts = 7) => { if (typeof document === 'undefined') return const days = typeof daysOrOpts === 'number' ? daysOrOpts : (daysOrOpts.days ?? 7) const sameSite: SameSite = (typeof daysOrOpts === 'object' && daysOrOpts.sameSite) || 'Lax' // ... }
24-26: Use isSecureContext instead of protocol string for setting Secure.
window.isSecureContextis a more semantically correct and future-proof check than comparinglocation.protocol.- if (location.protocol === 'https:') { + if (window.isSecureContext) { attributes.push('Secure') }
54-54: Auth cookie naming & security enhancementsWe verified that your
AuthCookieData(3 small strings:orgName,username,token) serializes to well under 4 KB in normal use, so truncation isn’t a concern. However, because this cookie is client-readable (and thus exposed to XSS), we recommend one of the following:
Server-set HttpOnly cookie
• Store only a session identifier in an HttpOnly, Secure, SameSite cookie.
• Fetch user/org metadata server-side or on demand.__Host- prefix for client cookie (no Domain, Path=/, Secure)
If you must keep a client-readable cookie, rename it to__Host-githubmon-authto enforce Secure+Path=/ by browser policy.If you choose the
__Host-prefix, update every occurrence of'githubmon-auth':• src/lib/cookies.ts
– Line 54:cookieUtils.set('__Host-githubmon-auth', JSON.stringify(data), 30)
– Line 58:cookieUtils.get('__Host-githubmon-auth')
– Line 69:cookieUtils.remove('__Host-githubmon-auth')• src/middleware.ts
– Line 7:request.cookies.get('__Host-githubmon-auth')?.value
– Line 39:response.cookies.set('__Host-githubmon-auth', '', { … })• src/app/api/auth/logout/route.ts
– Line 9:response.cookies.set('__Host-githubmon-auth', '', { … })src/lib/api/github-graphql-client.ts (4)
636-638: Mention type inference fallback should not rely solely on __typenameUntil __typename is added, infer PR mentions by presence of PR-only fields.
Apply this diff:
- const mentionType = item.__typename === 'PullRequest' ? 'comment' : 'mention' + const isPR = (item as any).__typename === 'PullRequest' || 'reviewRequests' in (item as any) + const mentionType = isPR ? 'comment' : 'mention' return this.mapToActionItem(item, mentionType)
54-59: Stale items carry reviewStatus; reflect it in the return typeYou construct stale items with a reviewStatus field, but ActionRequiredResult.stale is typed as GitHubActionItem[]. Adjust the type to avoid accidental drops and enable downstream typing.
Apply this diff:
-interface ActionRequiredResult { - assigned: GitHubActionItem[] - mentions: GitHubActionItem[] - stale: GitHubActionItem[] - rateLimit: RateLimit -} +interface ActionRequiredResult { + assigned: GitHubActionItem[] + mentions: GitHubActionItem[] + stale: Array<GitHubActionItem & { reviewStatus: 'APPROVED' | 'CHANGES_REQUESTED' | 'REVIEW_REQUIRED' | 'PENDING' }> + rateLimit: RateLimit +}
601-606: Include rateLimit.cost for consistency with RateLimit interfaceRateLimit includes cost in this file; add it to the selection or drop it from the interface. Adding it here is simplest.
Apply this diff:
rateLimit { limit + cost remaining resetAt }
481-485: Avoid string interpolation in search queries; pass as variablesInterpolating username and dates directly into the GraphQL string is brittle. Build search queries as variables (String!) and pass them to search(query: $var). This prevents accidental injection and quoting bugs.
I can provide a full query rewrite using variables for:
- staleQuery:
is:pr is:open author:${username} updated:<${staleDate}- mentionsQuery:
mentions:${username} is:open- reviewRequestedQuery:
is:pr is:open review-requested:${username}Want me to push a patch for that?
Also applies to: 510-514, 566-569
src/stores/actionItems.ts (4)
274-279: Preserve reviewStatus from GraphQL for stale items (map to store’s enum)You currently hardcode 'pending' and drop upstream reviewDecision. Map and store the actual status to improve prioritization in the Kanban.
Apply this diff:
- get().setStaleItems(actionData.stale.map(item => ({ - ...item, - lastActivity: item.updatedAt, - daysStale: item.daysOld, - reviewStatus: 'pending' as const - }))) + get().setStaleItems(actionData.stale.map(item => { + const s = (item as any).reviewStatus as string | undefined + const mapped = + s === 'APPROVED' ? 'approved' : + s === 'CHANGES_REQUESTED' ? 'changes_requested' : + 'pending' + return { + ...item, + lastActivity: item.updatedAt, + daysStale: item.daysOld, + reviewStatus: mapped as const, + } + }))Optionally, centralize this mapping in a small helper to reuse in the “all” branch below.
294-299: Mirror reviewStatus mapping in the “all” branchKeep stale items’ reviewStatus consistent when refreshing all types.
Apply this diff:
- get().setStaleItems(actionData.stale.map(item => ({ - ...item, - lastActivity: item.updatedAt, - daysStale: item.daysOld, - reviewStatus: 'pending' as const - }))) + get().setStaleItems(actionData.stale.map(item => { + const s = (item as any).reviewStatus as string | undefined + const mapped = + s === 'APPROVED' ? 'approved' : + s === 'CHANGES_REQUESTED' ? 'changes_requested' : + 'pending' + return { + ...item, + lastActivity: item.updatedAt, + daysStale: item.daysOld, + reviewStatus: mapped as const, + } + }))
238-245: Avoid setting loading flags for types you don’t fetch hereIn the “all” path you set goodFirstIssues/easyFixes loading=true but never fetch them in this action. Either remove those here or wire them up to fetch. Otherwise UI can briefly show spinners for data that won’t change.
Here’s a minimal change to only toggle the three fetched groups:
- loading: { - assigned: true, - mentions: true, - stale: true, - goodFirstIssues: true, - easyFixes: true - }, + loading: { + assigned: true, + mentions: true, + stale: true, + goodFirstIssues: false, + easyFixes: false, + },If you plan to fetch them here, hook up githubGraphQLClient.getGoodFirstIssues/getEasyFixes and keep the existing flags.
170-178: Counts include Quick Wins but refresh doesn’t fetch them; align behaviorgetCountByType reports goodFirstIssues/easyFixes, yet refreshData doesn’t load them. Consider dropping them from this store or implementing refresh for them to keep the surface coherent.
If you decide to narrow scope, this minimal change keeps counts to the three types:
- case 'goodFirstIssues': return state.goodFirstIssues.length - case 'easyFixes': return state.easyFixes.length + case 'goodFirstIssues': return 0 + case 'easyFixes': return 0Or wire up the fetch paths and retain the current counts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/components/kanban/KanbanBoard.tsx(1 hunks)src/components/kanban/TaskDetailModal.tsx(1 hunks)src/components/layout/Sidebar.tsx(6 hunks)src/components/ui/textarea.tsx(1 hunks)src/lib/api/github-graphql-client.ts(2 hunks)src/lib/cookies.ts(2 hunks)src/stores/actionItems.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/ui/textarea.tsx
- src/components/layout/Sidebar.tsx
- src/components/kanban/KanbanBoard.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/kanban/TaskDetailModal.tsx (1)
src/stores/kanban.ts (1)
KanbanTask(7-18)
🔇 Additional comments (6)
src/components/kanban/TaskDetailModal.tsx (3)
91-96: LGTM: onOpenChange fix matches Radix Dialog API and avoids accidental close.Using
(open) => { if (!open) handleClose() }resolves the earlier type mismatch and behaves correctly on close.
113-118: LGTM: Safe external link handling.Using target="_blank" with rel="noopener noreferrer" avoids reverse tabnabbing.
65-76: Nice: Robust date formatting with invalid/empty handling.Gracefully handles undefined/invalid dates and formats with Intl API.
src/lib/cookies.ts (1)
28-29: Confirm host-only cookie intent (no Domain attribute).Omitting Domain makes the cookie host-only, which is a good default for security. If you need the cookie across subdomains (e.g., app.example.com and api.example.com), you’ll need to add
Domain=.example.com. If not needed, keeping it host-only is preferable.src/stores/actionItems.ts (2)
267-271: Good: mentionType is now preserved from GraphQLYou no longer hardcode 'mention' and keep the actual mentionType when provided.
288-292: Good: “all” branch also preserves mentionTypeConsistent with the per-type branch. Nice.
… action item mapping
…date serialized task type handling
…e Max-Age is set correctly
1 participant
The Kanban board has been fixed.
The required action is drawn with GraphQL.
Kanban works with a hybrid method. It works both config- and context-based.
Summary by CodeRabbit
Here are the updated release notes based on the provided information:
New Features
Improvements
Chores