|
| 1 | +# GitHub Copilot Instructions - ToolHive Cloud UI |
| 2 | + |
| 3 | +## Project Context |
| 4 | + |
| 5 | +This is a **Next.js 16 App Router** application for visualizing MCP (Model Context Protocol) servers running in user infrastructure. It's an **open-source** project using **TypeScript**, **React 19**, **Tailwind CSS 4**, and **Better Auth** for OIDC authentication. |
| 6 | + |
| 7 | +## Core Architecture Principles |
| 8 | + |
| 9 | +- **Server Components by default** - Use Client Components (`'use client'`) only when needed (event handlers, browser APIs, state hooks) |
| 10 | +- **🚫 NEVER USE `any` TYPE** - STRICTLY FORBIDDEN. Use `unknown` with type guards or proper types |
| 11 | +- **Stateless authentication** - JWT-based sessions, no server-side storage |
| 12 | +- **hey-api generated client** - Never create manual fetch logic, always use generated hooks |
| 13 | +- **async/await over promises** - No `.then()` chains, use async/await for readability |
| 14 | + |
| 15 | +## ⚠️ Next.js App Router - Validate Before Suggesting |
| 16 | + |
| 17 | +**This is Next.js 16 with App Router** - Before suggesting code, verify against [Next.js Documentation](https://nextjs.org/docs): |
| 18 | + |
| 19 | +**Common mistakes to flag**: |
| 20 | + |
| 21 | +- ❌ Pages Router patterns (getServerSideProps, getStaticProps, \_app.js, \_document.js) |
| 22 | +- ❌ Custom routing logic (use file-system routing: `page.tsx`, `layout.tsx`) |
| 23 | +- ❌ Missing `'use client'` when using hooks, events, or browser APIs |
| 24 | +- ❌ Adding `'use client'` to Server Components unnecessarily |
| 25 | +- ❌ Not understanding what Server Components can/cannot do |
| 26 | +- ❌ Ignoring Next.js caching (`next: { revalidate, tags }`) |
| 27 | + |
| 28 | +**Key App Router concepts**: |
| 29 | + |
| 30 | +- **File conventions**: `page.tsx` (route), `layout.tsx` (shared UI), `loading.tsx` (loading state), `error.tsx` (error boundary) |
| 31 | +- **Server Components** (default): No hooks, no events, can fetch data directly with async/await |
| 32 | +- **Client Components** (`'use client'`): Use when you need hooks, events, browser APIs |
| 33 | +- **Server Actions** (`'use server'`): For mutations, better than API routes for simple cases |
| 34 | +- **Caching**: Use `revalidatePath()`, `revalidateTag()` for on-demand cache updates |
| 35 | + |
| 36 | +## Code Review Focus Areas |
| 37 | + |
| 38 | +### 1. Component Structure |
| 39 | + |
| 40 | +**GOOD:** |
| 41 | + |
| 42 | +- Server Components for data fetching |
| 43 | +- Client Components marked with `'use client'` only when necessary |
| 44 | +- Small, focused components with single responsibility |
| 45 | +- Proper TypeScript interfaces for props |
| 46 | + |
| 47 | +**BAD:** |
| 48 | + |
| 49 | +- `'use client'` on every component |
| 50 | +- Large monolithic components |
| 51 | +- Missing or weak TypeScript types |
| 52 | +- Using `any` type |
| 53 | + |
| 54 | +### 2. Data Fetching |
| 55 | + |
| 56 | +**GOOD:** |
| 57 | + |
| 58 | +```typescript |
| 59 | +// Server Component |
| 60 | +async function ServerList() { |
| 61 | + const response = await fetch('/api/servers', { |
| 62 | + next: { revalidate: 3600 } |
| 63 | + }); |
| 64 | + const data = await response.json(); |
| 65 | + return <div>{data.map(...)}</div>; |
| 66 | +} |
| 67 | + |
| 68 | +// Client Component with hey-api |
| 69 | +'use client'; |
| 70 | +import { useGetApiV0Servers } from '@/generated/client/@tanstack/react-query.gen'; |
| 71 | + |
| 72 | +function ServerList() { |
| 73 | + const { data, isLoading, error } = useGetApiV0Servers(); |
| 74 | + if (isLoading) return <Skeleton />; |
| 75 | + if (error) return <ErrorMessage error={error} />; |
| 76 | + return <div>{data?.map(...)}</div>; |
| 77 | +} |
| 78 | +``` |
| 79 | + |
| 80 | +**BAD:** |
| 81 | + |
| 82 | +```typescript |
| 83 | +// Manual fetch with useState/useEffect |
| 84 | +const [data, setData] = useState(null); |
| 85 | +useEffect(() => { |
| 86 | + fetch("/api/servers") |
| 87 | + .then((res) => res.json()) |
| 88 | + .then(setData); |
| 89 | +}, []); |
| 90 | + |
| 91 | +// Using .then() instead of async/await |
| 92 | +fetch("/api/servers").then((res) => res.json()); |
| 93 | +``` |
| 94 | + |
| 95 | +### 3. API Integration |
| 96 | + |
| 97 | +**ALWAYS:** |
| 98 | + |
| 99 | +- Use hey-api generated hooks from `@/generated/client/@tanstack/react-query.gen` |
| 100 | +- Use `async/await` syntax, never `.then()` |
| 101 | +- Handle loading and error states |
| 102 | +- Regenerate client when API changes: `pnpm generate-client` |
| 103 | + |
| 104 | +**NEVER:** |
| 105 | + |
| 106 | +- Create manual fetch calls in components |
| 107 | +- Edit files in `src/generated/` directory (auto-generated) |
| 108 | +- Use `.then()` for promise handling |
| 109 | +- Ignore TypeScript errors from generated types |
| 110 | + |
| 111 | +### 4. UI Components |
| 112 | + |
| 113 | +**ALWAYS:** |
| 114 | + |
| 115 | +- Use shadcn/ui components from `@/components/ui/*` |
| 116 | +- Prefer composition over complex props |
| 117 | +- Follow Tailwind CSS 4 conventions |
| 118 | + |
| 119 | +**NEVER:** |
| 120 | + |
| 121 | +- Create custom Button, Dialog, Card components |
| 122 | +- Duplicate inline styles (extract to reusable component) |
| 123 | +- Use inline event handlers for complex logic |
| 124 | + |
| 125 | +### 5. Authentication |
| 126 | + |
| 127 | +**GOOD:** |
| 128 | + |
| 129 | +- Check auth status on protected routes |
| 130 | +- Use Better Auth hooks for client-side auth state |
| 131 | +- Server Actions for authenticated mutations |
| 132 | +- Graceful handling of unauthenticated users |
| 133 | + |
| 134 | +**BAD:** |
| 135 | + |
| 136 | +- Hardcoded auth checks |
| 137 | +- Exposing auth tokens in client code |
| 138 | +- Missing authentication on protected routes |
| 139 | + |
| 140 | +### 6. TypeScript Best Practices |
| 141 | + |
| 142 | +**🚫 CRITICAL: NEVER USE `any` TYPE - This is a code review blocker.** |
| 143 | + |
| 144 | +**GOOD:** |
| 145 | + |
| 146 | +```typescript |
| 147 | +// Use 'as const' instead of enums |
| 148 | +const Status = { |
| 149 | + Active: "active", |
| 150 | + Inactive: "inactive", |
| 151 | +} as const; |
| 152 | + |
| 153 | +type Status = (typeof Status)[keyof typeof Status]; |
| 154 | + |
| 155 | +// Type guards for runtime validation with unknown |
| 156 | +function isServer(value: unknown): value is Server { |
| 157 | + return typeof value === "object" && value != null && "name" in value; |
| 158 | +} |
| 159 | + |
| 160 | +function processServer(data: unknown) { |
| 161 | + if (isServer(data)) { |
| 162 | + // Type-safe usage |
| 163 | + return data.name; |
| 164 | + } |
| 165 | + throw new Error("Invalid server data"); |
| 166 | +} |
| 167 | + |
| 168 | +// Proper null checking |
| 169 | +if (value != null) { |
| 170 | + /* ... */ |
| 171 | +} |
| 172 | +const result = value ?? defaultValue; |
| 173 | +``` |
| 174 | + |
| 175 | +**BAD (FORBIDDEN):** |
| 176 | + |
| 177 | +```typescript |
| 178 | +// Using enum |
| 179 | +enum Status { |
| 180 | + Active = "active", |
| 181 | + Inactive = "inactive", |
| 182 | +} |
| 183 | + |
| 184 | +// 🚫 FORBIDDEN - Using 'any' defeats TypeScript's purpose |
| 185 | +function process(data: any) { |
| 186 | + /* ... */ |
| 187 | +} |
| 188 | + |
| 189 | +// Verbose null checking |
| 190 | +if (value !== null && value !== undefined) { |
| 191 | + /* ... */ |
| 192 | +} |
| 193 | +const result = value || defaultValue; |
| 194 | +``` |
| 195 | + |
| 196 | +### 7. Code Quality |
| 197 | + |
| 198 | +**ALWAYS:** |
| 199 | + |
| 200 | +- Write concise, readable code with meaningful names |
| 201 | +- Add JSDoc comments explaining **why** (purpose/reasoning), not **what** (implementation) |
| 202 | +- Follow DRY principle - extract repeated logic |
| 203 | +- Use modern JavaScript/TypeScript syntax (optional chaining, nullish coalescing, array methods) |
| 204 | + |
| 205 | +**NEVER:** |
| 206 | + |
| 207 | +- Comment obvious code |
| 208 | +- Create unnecessary abstractions |
| 209 | +- Use verbose patterns when simpler alternatives exist |
| 210 | +- Mass refactor working code without reason |
| 211 | + |
| 212 | +### 8. Error Handling |
| 213 | + |
| 214 | +**GOOD:** |
| 215 | + |
| 216 | +```typescript |
| 217 | +try { |
| 218 | + await riskyOperation(); |
| 219 | +} catch (error) { |
| 220 | + logger.error("Operation failed:", error); |
| 221 | + toast.error("Failed to complete operation"); |
| 222 | + // Optionally report to monitoring service |
| 223 | +} |
| 224 | +``` |
| 225 | + |
| 226 | +**BAD:** |
| 227 | + |
| 228 | +```typescript |
| 229 | +try { |
| 230 | + await riskyOperation(); |
| 231 | +} catch (error) { |
| 232 | + // Silent failure - error is lost |
| 233 | +} |
| 234 | +``` |
| 235 | + |
| 236 | +### 9. Next.js Specific |
| 237 | + |
| 238 | +**GOOD:** |
| 239 | + |
| 240 | +- Use file-system routing (no custom routing logic) |
| 241 | +- Implement `error.tsx`, `loading.tsx`, `not-found.tsx` where appropriate |
| 242 | +- Use Server Actions for mutations instead of API routes |
| 243 | +- Leverage Next.js caching with proper revalidation |
| 244 | +- Use `generateMetadata` for dynamic SEO |
| 245 | + |
| 246 | +**BAD:** |
| 247 | + |
| 248 | +- Custom routing implementations |
| 249 | +- Ignoring Next.js caching behavior |
| 250 | +- Creating API routes for simple mutations |
| 251 | +- Missing error boundaries |
| 252 | + |
| 253 | +### 10. Testing |
| 254 | + |
| 255 | +**REQUIRED:** |
| 256 | + |
| 257 | +- Test user interactions with Testing Library |
| 258 | +- Mock API calls with MSW |
| 259 | +- Test error scenarios |
| 260 | +- Test authentication flows |
| 261 | + |
| 262 | +## Anti-Patterns to Flag |
| 263 | + |
| 264 | +1. ❌ **Using `any` type in TypeScript** - STRICTLY FORBIDDEN. Flag immediately and suggest `unknown` + type guards |
| 265 | +2. ❌ Using `'use client'` everywhere |
| 266 | +3. ❌ Manual state management for API calls (useState + useEffect) |
| 267 | +4. ❌ Using `.then()` instead of `async/await` |
| 268 | +5. ❌ Creating custom UI components instead of using shadcn/ui |
| 269 | +6. ❌ Editing auto-generated files in `src/generated/` |
| 270 | +7. ❌ Ignoring linter/TypeScript errors |
| 271 | +8. ❌ Duplicating code instead of extracting reusable logic |
| 272 | +9. ❌ Missing error handling |
| 273 | +10. ❌ Creating API routes instead of Server Actions for simple mutations |
| 274 | + |
| 275 | +## Pre-Commit Checklist |
| 276 | + |
| 277 | +When reviewing code, ensure: |
| 278 | + |
| 279 | +- [ ] No TypeScript errors |
| 280 | +- [ ] No linter errors (Biome) |
| 281 | +- [ ] Tests pass |
| 282 | +- [ ] Uses hey-api generated hooks (no manual fetch) |
| 283 | +- [ ] Server Components by default, Client Components only when needed |
| 284 | +- [ ] Proper error handling and loading states |
| 285 | +- [ ] Uses `async/await` (no `.then()`) |
| 286 | +- [ ] Follows existing patterns in the codebase |
| 287 | +- [ ] JSDoc comments for complex functions (explains why, not what) |
| 288 | +- [ ] No mass refactoring without clear reason |
| 289 | + |
| 290 | +## Quick References |
| 291 | + |
| 292 | +- **Generate API Client**: `pnpm generate-client` |
| 293 | +- **Run Tests**: `pnpm test` |
| 294 | +- **Lint**: `pnpm lint` |
| 295 | +- **Type Check**: `pnpm type-check` |
| 296 | +- **Dev Server**: `pnpm dev` (includes OIDC mock) |
| 297 | + |
| 298 | +## Project Links |
| 299 | + |
| 300 | +- [Backend API](https://github.com/stacklok/toolhive-registry-server) |
| 301 | +- [Next.js Docs](https://nextjs.org/docs) |
| 302 | +- [Better Auth Docs](https://www.better-auth.com/) |
| 303 | +- [hey-api Docs](https://heyapi.vercel.app/) |
| 304 | +- [shadcn/ui](https://ui.shadcn.com/) |
0 commit comments