PR Guidelines
Code review guidelines to speed up the team review process.
Code Duplication
# Code Duplication
### What
Extract repeated UI patterns with identical Tailwind styling into reusable components instead of copy-pasting them.
### Why
Duplicated styled code blocks become maintenance nightmares and create inconsistency vectors when updates are applied to only some instances.
### Good
```jsx
// Button.jsx
export const Button = ({ children }) => (
<button className="px-4 py-2 bg-blue-500 text-white rounded">
{children}
</button>
);
// Usage
<Button>Save</Button>
<Button>Delete</Button>
```
### Bad
```jsx
// Repeated everywhere
<button className="px-4 py-2 bg-blue-500 text-white rounded">Save</button>
<button className="px-4 py-2 bg-blue-500 text-white rounded">Delete</button>
<button className="px-4 py-2 bg-blue-500 text-white rounded">Cancel</button>
```
Context Over Prop Drilling
# Context Over Prop Drilling
### What
Use React Context to share state and callbacks across deeply nested components instead of passing props through multiple intermediate layers. When callbacks involve logic that primarily uses context values, define them inside the context provider and expose them through the context.
### Why
Prop drilling creates tight coupling between components, makes refactoring painful, and clutters component signatures with props that are merely "passed through." Context centralizes shared state and logic, making components cleaner and the data flow more maintainable.
### Good
```jsx
// AuthContext.jsx
const AuthContext = createContext(null)
export const AuthProvider = ({ children }) => {
const [user, setUser] = useState(null)
const [isLoading, setIsLoading] = useState(false)
const login = async (credentials) => {
setIsLoading(true)
const userData = await authApi.login(credentials)
setUser(userData)
setIsLoading(false)
}
const logout = () => {
authApi.logout()
setUser(null)
}
return (
<AuthContext.Provider value={{ user, isLoading, login, logout }}>
{children}
</AuthContext.Provider>
)
}
export const useAuth = () => useContext(AuthContext)
// DeepNestedComponent.jsx
const DeepNestedComponent = () => {
const { user, logout } = useAuth()
return <button onClick={logout}>Logout {user.name}</button>
}
```
### Bad
```jsx
// Prop drilling through multiple layers
const App = () => {
const [user, setUser] = useState(null)
const logout = () => {
authApi.logout()
setUser(null)
}
return <Layout user={user} logout={logout} />
}
const Layout = ({ user, logout }) => <Sidebar user={user} logout={logout} />
const Sidebar = ({ user, logout }) => <UserMenu user={user} logout={logout} />
const UserMenu = ({ user, logout }) => (
<button onClick={logout}>Logout {user.name}</button>
)
```
Data Slot Approach
We wrote about why we do prefer the data-slot approach over multiple class props in the 01 - The Slot Approach log.
# Data Slot Approach
### What
Use `data-slot` attributes to name inner elements instead of exposing multiple `className` props like `titleClassName`, `descriptionClassName`, etc. One `className` prop for the root element is fine.
### Why
Multiple className props clutter component signatures and make the API messier as the component grows. The `data-slot` approach leverages CSS-first styling, keeping props clean while still allowing parent components to style inner elements via the `**:data-[slot=name]` selector.
### Good
```jsx
// Component with data-slot attributes
export function CardLink({ href, title, description, className }) {
return (
<Card className={cn('hover:bg-accent/50', className)} asChild>
<Link href={href}>
<div data-slot="card-title">
{title}
<ArrowRightIcon />
</div>
<p data-slot="card-description">{description}</p>
</Link>
</Card>
)
}
// Usage - styling inner elements via CSS selector
<CardLink className="**:data-[slot=card-description]:opacity-50" />
```
### Bad
```jsx
// Multiple className props - clutters the API
export function CardLink({
href,
title,
description,
className,
titleClassName,
descriptionClassName,
}) {
return (
<Card className={cn('hover:bg-accent/50', className)} asChild>
<Link href={href}>
<div className={titleClassName}>
{title}
<ArrowRightIcon />
</div>
<p className={descriptionClassName}>{description}</p>
</Link>
</Card>
)
}
// Usage
<CardLink titleClassName="font-bold" descriptionClassName="opacity-50" />
```
JS Media Queries
# Avoid JS-Based Media Queries for Initial Render
### What
Never use JavaScript-based media queries (`window.matchMedia`, `useMediaQuery` hooks, or `innerWidth` checks) to determine layout or visibility of elements on initial render. Only use them in components where JavaScript has already loaded and React has fully hydrated.
### Why
During Server-Side Rendering (SSR), the server has no knowledge of the client's viewport size. JavaScript media queries must default to a fallback value (often `false` or a desktop-first assumption), which creates several problems:
1. **Hydration mismatch**: React expects the server-rendered HTML to match the initial client render. When the client hydrates with a different viewport result, React detects a mismatch, potentially causing rendering errors or forcing a full re-render.
2. **Layout shift (CLS)**: Users see content "jump" as elements resize, reposition, or toggle visibility after hydration. This hurts Core Web Vitals scores and creates a jarring user experience.
3. **Flash of incorrect content (FOUC)**: On slower connections or devices, users may see the wrong layout for several hundred milliseconds before JavaScript executes and corrects it. A mobile user might briefly see a desktop layout, or vice versa.
CSS media queries don't have this problem—they're evaluated by the browser immediately when parsing CSS, before any JavaScript runs, ensuring consistent rendering from the very first paint.
### Good
```tsx
// CSS-based responsive behavior - works correctly with SSR
export function Navigation() {
return (
<nav>
{/* Desktop nav - hidden on mobile via CSS */}
<div className="hidden md:flex gap-4">
<NavLinks />
</div>
{/* Mobile menu button - hidden on desktop via CSS */}
<MobileMenuButton className="flex md:hidden" />
</nav>
)
}
// JS media query is acceptable here because the modal
// only renders after user interaction (click), well after hydration
export function MobileMenu() {
const [isOpen, setIsOpen] = useState(false)
const isMobile = useMediaQuery('(max-width: 768px)')
// This is fine - component only matters after user clicks
if (!isOpen) return null
return <MobileMenuContent variant={isMobile ? 'fullscreen' : 'sidebar'} />
}
```
### Bad
```tsx
// JS-based responsive behavior - causes layout shift and hydration issues
export function Navigation() {
const isMobile = useMediaQuery('(max-width: 768px)')
// Server renders with isMobile=false (fallback)
// Client hydrates with isMobile=true on mobile devices
// Result: layout shift, hydration mismatch, flash of wrong content
return (
<nav>
{isMobile ? (
<MobileMenuButton />
) : (
<div className="flex gap-4">
<NavLinks />
</div>
)}
</nav>
)
}
```
### When JS Media Queries Are Acceptable
- **After user interaction**: Modals, dropdowns, and tooltips that only render after a click or hover.
- **Client-only components**: Components explicitly lazy-loaded or rendered after hydration (behind `useEffect` or `Suspense`).
- **Non-visual logic**: Analytics, feature flags, or behavior that doesn't affect the rendered layout.
```tsx
// Safe pattern: guard with hydration check for unavoidable cases
const [isHydrated, setIsHydrated] = useState(false)
useEffect(() => setIsHydrated(true), [])
const isMobile = useMediaQuery('(max-width: 768px)')
// Only use the JS result after hydration; render both variants via CSS first
const effectiveIsMobile = isHydrated ? isMobile : undefined
```
Kebab-Case File Naming
# Kebab-Case File Naming
### What
Use lowercase kebab-case (`my-component.tsx`) instead of PascalCase (`MyComponent.tsx`) for file and folder names.
### Why
File systems differ in case sensitivity—Linux/macOS are case-sensitive while Windows is case-insensitive. Mixing PascalCase can lead to confusing Git renames, mistaken file references, or broken imports when collaborating across different operating systems. Using all-lowercase kebab-case avoids these cross-platform issues entirely.
### Good
```
components/
button.tsx
card-link.tsx
file-dropzone.tsx
mobile-menu.tsx
hooks/
use-config.ts
use-file-upload.tsx
lib/
compose-refs.ts
external-registries.ts
```
### Bad
```
components/
Button.tsx
CardLink.tsx
FileDropzone.tsx
MobileMenu.tsx
hooks/
UseConfig.ts
useFileUpload.tsx
lib/
ComposeRefs.ts
ExternalRegistries.ts
```