frontend-code-review（前端代码审查）

Frontend Code Review

Perform thorough frontend code reviews for the Teetsh React/TypeScript codebase.

Review Process

1. Understand the Context

Before reviewing, gather context:

• Read the PR description or understand the change purpose
• Identify which features/containers are affected
• Check if this affects shared components, queries, or styling

2. Load Relevant References

Based on the review type, read appropriate references:

• Always read: references/common-mistakes.md for Teetsh-specific antipatterns
• For components/hooks: references/react-patterns.md
• For data fetching: references/react-query.md
• For UI/styling: references/styling.md
• For translations: references/i18n.md
• For test changes: references/testing.md

3. Review Scope by Type

Full PR Review: Check all aspects - architecture, implementation, styling, i18n, tests

Component Review: Focus on patterns, props typing, accessibility, responsive design

Query Review: Focus on query keys, serializers, error handling, mutations

Styling Review: Focus on twin.macro usage, theme constants, responsive classes

4. Provide Structured Feedback

Organize feedback by priority:

Critical Issues

Security vulnerabilities, broken functionality, accessibility blockers, data loss risks

Important Issues

Missing error/loading states, incorrect patterns, i18n gaps, test gaps

Suggestions

Code clarity, performance optimizations, better patterns

Positive Feedback

Well-designed components, good test coverage, clean patterns

5. Code Examples

For each issue, provide:

• What: Describe the problem
• Why: Explain the risk or impact
• How: Show code example of the fix

Key Review Areas

React Patterns

Check adherence to Teetsh patterns:

• Container vs Component separation (containers handle logic, components are presentational)
• Compound components for complex UI groups (Disclosure, FormGroup)
• Context hooks with validation (useUser, useSchoolSchoolyear)
• Custom hooks naming (useXxx)
• Prefer CSS-based responsive (tw="lg:flex"), use useIsBelowBreakpoint only when layouts differ completely
• Default export for components, named export for utilities

React Query

• Query keys include all dependencies: [schoolyearId, schoolId, KEY_CONSTANT,...params]
• Data transformation in serializer functions, not components
• Appropriate staleTime for data type
• Error handling with AxiosError typing
• Mutations with proper cache invalidation

Styling

• Use tw macro, not className strings
• Colors from themeConstants.ts, never hardcoded
• Conditional styles: css={[tw..., condition && tw...]}`
• Form inputs wrapped with FormGroup
• Responsive: mobile-first, lg: breakpoint for desktop

i18n

• useTranslation(['namespace']) with namespace array
• Keys: namespace:section.key
• Dates: use format() utility with locale
• No hardcoded user-facing strings

Accessibility

• Semantic HTML (button vs div, etc.)
• ARIA labels on interactive elements
• Labels linked to inputs with htmlFor
• Keyboard navigation support

Testing

• Browser tests (Vitest browser) for components: Component.browser.test.tsx
• jsdom tests for unit/logic: utils.test.tsx
• Playwright E2E for critical user flows
• Storybook stories for design system components
• Descriptive test names: it('should return X when Y')

Review Output Format

Structure your review as:

## Critical Issues
[Issues that must be fixed before merge]

## Important Issues
[Issues that should be fixed]

## Suggestions
[Optional improvements]

## Positive Feedback
[What was done well]

For each issue:

### [Area]: [Brief description]

**Problem**: [What's wrong and why it matters]

**Current code**:

[Show problematic code]

**Suggested fix**:

[Show corrected code]

**Reference**: [Link to specific pattern]

Example Review Snippets

Missing FormGroup Wrapper

### Styling: Input missing FormGroup wrapper

**Problem**: Form inputs should be wrapped with FormGroup for consistent styling and accessibility

**Current code**:

<Input id="email" value={email} onChange={(e) => setEmail(e.target.value)} />

**Suggested fix**:

<FormGroup id="email" label={t('common:form.email')} error={errors.email}

<Input id="email" value={email} onChange={(e) => setEmail(e.target.value)} hasError={!!errors.email} /> </FormGroup>

**Reference**: See `references/styling.md` - FormGroup Pattern

Query Key Missing Dependencies

### React Query: Query key missing school context

**Problem**: Query key must include schoolyearId and schoolId for proper cache isolation

**Current code**:

useQuery({ queryKey: [STUDENTS_KEY, classId], queryFn: () => getStudents(classId), });

**Suggested fix**:

const { schoolyear, school } = useSchoolSchoolyear();

useQuery({ queryKey: [schoolyear.id, school.id, STUDENTS_KEY, classId], queryFn: () => getStudents({ schoolyearId: schoolyear.id, schoolId: school.id, classId }), });

**Reference**: See `references/react-query.md` - Query Key Structure

Hardcoded String

### i18n: Hardcoded user-facing string

**Problem**: All user-facing text must use i18n for localization support

**Current code**:

<Button>Delete</Button>

**Suggested fix**:

const { t } = useTranslation(['common']); <Button>{t('common:buttons.delete')}</Button>

**Reference**: See `references/i18n.md` - Translation Usage

Using className Instead of tw

### Styling: Using className instead of tw macro

**Problem**: Project uses twin.macro for Tailwind, not className strings

**Current code**:

<div className="flex items-center gap-2">

**Suggested fix**:

<div tw="flex items-center gap-2">

**Reference**: See `references/styling.md` - twin.macro Usage

When NOT to Comment

Avoid feedback on:

• Trivial formatting issues (Prettier handles this)
• Personal style preferences not in project patterns
• Nitpicks that don't affect functionality or maintainability
• Issues already addressed in other comments

Multi-file Review Strategy

For PRs with many files:

1. Start with architecture overview (new containers, major changes)
2. Review core component/container logic first
3. Review queries and data handling
4. Review styling and i18n
5. Review tests
6. Summarize overall assessment

After Review

If significant issues found:

• Summarize the most important themes
• Suggest whether changes are required before merge
• Offer to explain any patterns or practices in detail