mirror of
https://github.com/open-metadata/OpenMetadata.git
synced 2025-12-14 00:57:09 +00:00
chore(ui): add pr review instructions (#24536)
This commit is contained in:
parent
bd46e5500b
commit
5e5f371f0d
98
.github/copilot-instructions.md
vendored
98
.github/copilot-instructions.md
vendored
@ -546,4 +546,102 @@ make unit_ingestion_dev_env # For Python changes
|
|||||||
- Database migrations are automatically applied on startup
|
- Database migrations are automatically applied on startup
|
||||||
- HTTPS is required for production deployments
|
- HTTPS is required for production deployments
|
||||||
|
|
||||||
|
## UI Pull Request Review Guidelines
|
||||||
|
|
||||||
|
**IMPORTANT: When reviewing UI pull requests, you MUST follow the comprehensive guidelines in [/openmetadata-ui/UI_PR_REVIEW_GUIDELINES.md](../openmetadata-ui/UI_PR_REVIEW_GUIDELINES.md)**
|
||||||
|
|
||||||
|
### Critical UI Standards to Enforce
|
||||||
|
|
||||||
|
#### Type Safety (Zero Tolerance)
|
||||||
|
- ❌ **REJECT**: Any use of `any` type in TypeScript
|
||||||
|
- ✅ **REQUIRE**: Proper type imports from `generated/` or `@rjsf/utils`
|
||||||
|
- ✅ **REQUIRE**: Defined interfaces for all component props in `.interface.ts` files
|
||||||
|
|
||||||
|
#### Internationalization (Zero Tolerance)
|
||||||
|
- ❌ **REJECT**: Any hardcoded string literals in UI components
|
||||||
|
- ✅ **REQUIRE**: All user-facing text uses `useTranslation` hook: `const { t } = useTranslation()`
|
||||||
|
- ✅ **REQUIRE**: Translation keys like `t('label.key')` from locale files
|
||||||
|
|
||||||
|
#### MUI Migration (Preferred)
|
||||||
|
- ⚠️ **FLAG**: New features using Ant Design components (should use MUI v7.3.1)
|
||||||
|
- ✅ **PREFER**: MUI components and theme tokens from `openmetadata-ui-core-components`
|
||||||
|
- ❌ **REJECT**: Hardcoded colors instead of theme tokens
|
||||||
|
|
||||||
|
#### Code Quality (Must Pass)
|
||||||
|
- ❌ **REJECT**: ESLint errors or warnings
|
||||||
|
- ❌ **REJECT**: Console.log statements in production code
|
||||||
|
- ❌ **REJECT**: Unnecessary comments explaining obvious code
|
||||||
|
- ✅ **REQUIRE**: Proper import organization (external → internal → relative → assets)
|
||||||
|
|
||||||
|
#### React Patterns (Must Follow)
|
||||||
|
- ✅ **REQUIRE**: Functional components only (no class components)
|
||||||
|
- ✅ **REQUIRE**: Proper dependency arrays in `useEffect`, `useCallback`, `useMemo`
|
||||||
|
- ✅ **REQUIRE**: Loading states as `useState<Record<string, boolean>>({})`
|
||||||
|
- ✅ **REQUIRE**: Error handling with `showErrorToast`/`showSuccessToast` from ToastUtils
|
||||||
|
- ✅ **REQUIRE**: Navigation with `useNavigate`, not direct history manipulation
|
||||||
|
|
||||||
|
#### File Naming (Must Follow)
|
||||||
|
- ✅ **REQUIRE**: Components named as `ComponentName.component.tsx`
|
||||||
|
- ✅ **REQUIRE**: Interfaces named as `ComponentName.interface.ts`
|
||||||
|
- ✅ **REQUIRE**: Custom hooks prefixed with `use` and placed in `src/hooks/`
|
||||||
|
|
||||||
|
### PR Review Checklist
|
||||||
|
|
||||||
|
When reviewing a UI PR, verify ALL of these:
|
||||||
|
|
||||||
|
1. **Pre-merge Commands Pass**:
|
||||||
|
```bash
|
||||||
|
yarn lint # Must pass with zero errors
|
||||||
|
yarn test # All tests must pass
|
||||||
|
yarn build # Build must succeed
|
||||||
|
```
|
||||||
|
|
||||||
|
2. **Type Safety**: Search for `any` type usage - must be zero occurrences
|
||||||
|
3. **i18n Compliance**: Search for hardcoded strings - must use translation keys
|
||||||
|
4. **Import Organization**: Check import order follows standard
|
||||||
|
5. **MUI Usage**: New components prefer MUI over Ant Design
|
||||||
|
6. **No Debug Code**: No console.log, commented code, or debug statements
|
||||||
|
7. **Performance**: Proper memoization, no unnecessary re-renders
|
||||||
|
8. **Accessibility**: Semantic HTML, ARIA labels, keyboard navigation
|
||||||
|
9. **Screenshots Provided**: UI changes include visual evidence
|
||||||
|
|
||||||
|
### Auto-Reject Conditions
|
||||||
|
|
||||||
|
Immediately flag these for revision:
|
||||||
|
- Any `any` type usage
|
||||||
|
- Hardcoded UI strings (not using `t()`)
|
||||||
|
- ESLint errors
|
||||||
|
- Failed tests or build
|
||||||
|
- Missing prop interfaces
|
||||||
|
- Console.log statements
|
||||||
|
- Ant Design components in new features (without justification)
|
||||||
|
|
||||||
|
### Review Response Template
|
||||||
|
|
||||||
|
Use this template when reviewing UI PRs:
|
||||||
|
|
||||||
|
```markdown
|
||||||
|
## UI PR Review
|
||||||
|
|
||||||
|
### ✅ Passed Checks
|
||||||
|
- [List what meets standards]
|
||||||
|
|
||||||
|
### ❌ Required Changes
|
||||||
|
- [List blocking issues with file:line references]
|
||||||
|
|
||||||
|
### ⚠️ Suggestions
|
||||||
|
- [List non-blocking improvements]
|
||||||
|
|
||||||
|
### 📋 Verification
|
||||||
|
- [ ] `yarn lint` passes
|
||||||
|
- [ ] `yarn test` passes
|
||||||
|
- [ ] `yarn build` succeeds
|
||||||
|
- [ ] No `any` types
|
||||||
|
- [ ] No hardcoded strings
|
||||||
|
- [ ] Proper MUI usage
|
||||||
|
- [ ] Screenshots provided
|
||||||
|
|
||||||
|
See [UI_PR_REVIEW_GUIDELINES.md](../openmetadata-ui/UI_PR_REVIEW_GUIDELINES.md) for complete checklist.
|
||||||
|
```
|
||||||
|
|
||||||
Remember: This is a complex multi-language project. Build times are substantial. NEVER cancel long-running builds or tests. Always validate changes with real user scenarios before considering the work complete.
|
Remember: This is a complex multi-language project. Build times are substantial. NEVER cancel long-running builds or tests. Always validate changes with real user scenarios before considering the work complete.
|
||||||
175
openmetadata-ui/UI_PR_REVIEW_GUIDELINES.md
Normal file
175
openmetadata-ui/UI_PR_REVIEW_GUIDELINES.md
Normal file
@ -0,0 +1,175 @@
|
|||||||
|
# UI Pull Request Review Guidelines
|
||||||
|
|
||||||
|
This document outlines the standards and best practices that must be followed when reviewing pull requests for the OpenMetadata UI project. GitHub Copilot and human reviewers should use this as a checklist.
|
||||||
|
|
||||||
|
## TypeScript Type Safety
|
||||||
|
|
||||||
|
- [ ] **No `any` types**: Code must never use `any` type - use proper types or `unknown` with type guards
|
||||||
|
- [ ] **Proper type imports**: All types imported from existing definitions (e.g., `RJSFSchema` from `@rjsf/utils`, types from `generated/`)
|
||||||
|
- [ ] **Interface definitions**: All component props have defined interfaces in `.interface.ts` files
|
||||||
|
- [ ] **Type assertions**: Avoid type assertions unless absolutely necessary and well-justified
|
||||||
|
- [ ] **Discriminated unions**: Use discriminated unions for action types and state variants
|
||||||
|
|
||||||
|
## React Component Standards
|
||||||
|
|
||||||
|
### File Structure and Naming
|
||||||
|
- [ ] **Component files**: Named as `ComponentName.component.tsx`
|
||||||
|
- [ ] **Interface files**: Named as `ComponentName.interface.ts`
|
||||||
|
- [ ] **Functional components only**: No class components
|
||||||
|
- [ ] **Custom hooks**: Prefixed with `use`, placed in `src/hooks/`, return typed objects
|
||||||
|
|
||||||
|
### Component Implementation
|
||||||
|
- [ ] **State management**: Uses `useState` with proper typing
|
||||||
|
- [ ] **Effect dependencies**: `useEffect` has correct dependency arrays
|
||||||
|
- [ ] **Performance optimization**: `useCallback` for event handlers, `useMemo` for expensive computations
|
||||||
|
- [ ] **Loading states**: Uses object state for multiple loading states: `useState<Record<string, boolean>>({})`
|
||||||
|
- [ ] **Error handling**: Uses `showErrorToast` and `showSuccessToast` from ToastUtils
|
||||||
|
- [ ] **Navigation**: Uses `useNavigate` from react-router-dom, not direct history manipulation
|
||||||
|
- [ ] **Data fetching**: Async functions wrapped in try-catch blocks with proper loading state updates
|
||||||
|
|
||||||
|
## Internationalization (i18n)
|
||||||
|
|
||||||
|
- [ ] **No string literals**: All user-facing strings use `useTranslation` hook
|
||||||
|
- [ ] **Translation usage**: Uses `const { t } = useTranslation()` and accesses strings like `t('label.key')`
|
||||||
|
- [ ] **No hardcoded text**: All labels, messages, and UI text must come from locale files
|
||||||
|
|
||||||
|
## Styling and UI Components
|
||||||
|
|
||||||
|
### MUI Migration
|
||||||
|
- [ ] **Prefer MUI v7.3.1**: New features use Material-UI components, not Ant Design
|
||||||
|
- [ ] **Theme tokens**: Uses theme colors and design tokens from MUI theme, not hardcoded values
|
||||||
|
- [ ] **Theme reference**: Styles reference `openmetadata-ui-core-components` theme data
|
||||||
|
- [ ] **Legacy components**: Ant Design components only acceptable in existing code, flag for future refactoring
|
||||||
|
|
||||||
|
### CSS/Styling
|
||||||
|
- [ ] **BEM naming**: Custom CSS classes follow BEM convention
|
||||||
|
- [ ] **Component-specific naming**: Styles in `.less` files use component-specific naming
|
||||||
|
- [ ] **CSS modules**: Used where appropriate for better scoping
|
||||||
|
|
||||||
|
## Code Quality
|
||||||
|
|
||||||
|
### Import Organization
|
||||||
|
- [ ] **Correct order**: Imports organized as:
|
||||||
|
1. External libraries (React, MUI, etc.)
|
||||||
|
2. Internal absolute imports (`generated/`, `constants/`, `hooks/`)
|
||||||
|
3. Relative imports (utilities, components)
|
||||||
|
4. Asset imports (SVGs, styles)
|
||||||
|
5. Type imports grouped separately
|
||||||
|
|
||||||
|
### ESLint and Code Standards
|
||||||
|
- [ ] **ESLint compliance**: No ESLint errors or warnings
|
||||||
|
- [ ] **No console statements**: Remove or properly handle console logs (project enforces no-console)
|
||||||
|
- [ ] **eslint-disable comments**: Only used when absolutely necessary with justification
|
||||||
|
- [ ] **Self-documenting code**: Clear variable and function names, minimal comments
|
||||||
|
- [ ] **Avoid over-engineering**: Only implement requested features, no speculative additions
|
||||||
|
|
||||||
|
### Comments Policy
|
||||||
|
- [ ] **No unnecessary comments**: Code is self-documenting
|
||||||
|
- [ ] **Justified comments only**: Comments only for:
|
||||||
|
- Complex business logic
|
||||||
|
- Non-obvious algorithms or workarounds
|
||||||
|
- TODO/FIXME with ticket references
|
||||||
|
- [ ] **No obvious comments**: Avoid comments like "increment counter" or "create new user"
|
||||||
|
|
||||||
|
## State Management
|
||||||
|
|
||||||
|
- [ ] **Local state preferred**: Keep component state local with `useState` when possible
|
||||||
|
- [ ] **Zustand for global state**: Use Zustand stores (e.g., `useLimitStore`, `useWelcomeStore`)
|
||||||
|
- [ ] **Context providers**: Use for feature-specific shared state (e.g., `ApplicationsProvider`)
|
||||||
|
|
||||||
|
## Testing
|
||||||
|
|
||||||
|
- [ ] **Unit tests**: Jest tests for new components and utilities
|
||||||
|
- [ ] **Test coverage**: Meaningful tests, not just coverage numbers
|
||||||
|
- [ ] **E2E tests**: Playwright tests for critical user flows (when applicable)
|
||||||
|
- [ ] **Tests pass**: All tests pass locally with `yarn test`
|
||||||
|
|
||||||
|
## Build and Lint
|
||||||
|
|
||||||
|
- [ ] **Build succeeds**: `yarn build` completes without errors
|
||||||
|
- [ ] **Lint passes**: `yarn lint` passes without errors
|
||||||
|
- [ ] **Format check**: Code follows project formatting standards
|
||||||
|
|
||||||
|
## Schema and Type Generation
|
||||||
|
|
||||||
|
- [ ] **Schema changes**: If schemas modified, `yarn parse-schema` has been run
|
||||||
|
- [ ] **Generated types**: Uses generated TypeScript interfaces from `generated/` directory
|
||||||
|
- [ ] **Schema resolution**: Connection schemas properly resolved, Application schemas handled at runtime
|
||||||
|
|
||||||
|
## Performance
|
||||||
|
|
||||||
|
- [ ] **Unnecessary re-renders**: Components don't re-render unnecessarily
|
||||||
|
- [ ] **Memoization**: Expensive computations properly memoized
|
||||||
|
- [ ] **Bundle size**: No unnecessary dependencies or imports
|
||||||
|
- [ ] **Lazy loading**: Large components lazy-loaded when appropriate
|
||||||
|
|
||||||
|
## Accessibility
|
||||||
|
|
||||||
|
- [ ] **Semantic HTML**: Uses appropriate semantic elements
|
||||||
|
- [ ] **ARIA labels**: Proper ARIA attributes for interactive elements
|
||||||
|
- [ ] **Keyboard navigation**: Interactive elements accessible via keyboard
|
||||||
|
- [ ] **MUI accessibility**: MUI components used correctly with accessibility in mind
|
||||||
|
|
||||||
|
## Security
|
||||||
|
|
||||||
|
- [ ] **No secrets**: No API keys, tokens, or credentials in code
|
||||||
|
- [ ] **Input validation**: User inputs properly validated
|
||||||
|
- [ ] **XSS prevention**: No dangerouslySetInnerHTML without sanitization
|
||||||
|
- [ ] **Secure dependencies**: No known vulnerabilities in dependencies
|
||||||
|
|
||||||
|
## Documentation
|
||||||
|
|
||||||
|
- [ ] **Prop interfaces**: Complex props documented with JSDoc when necessary
|
||||||
|
- [ ] **README updates**: Component README updated if adding new patterns
|
||||||
|
- [ ] **Migration notes**: Breaking changes documented
|
||||||
|
|
||||||
|
## Git and Version Control
|
||||||
|
|
||||||
|
- [ ] **Atomic commits**: Commits are logical and focused
|
||||||
|
- [ ] **No commented code**: Removed code deleted, not commented out
|
||||||
|
- [ ] **No debug code**: Console logs and debug statements removed
|
||||||
|
- [ ] **Clean git history**: No merge commits in feature branches (rebase preferred)
|
||||||
|
|
||||||
|
## Specific to OpenMetadata UI
|
||||||
|
|
||||||
|
- [ ] **Service utilities**: Follows patterns in existing service utility files
|
||||||
|
- [ ] **ApplicationsClassBase**: Applications properly use schema loading and configuration
|
||||||
|
- [ ] **RJSF forms**: Form schemas use React JSON Schema Form with custom UI widgets
|
||||||
|
- [ ] **Dynamic imports**: Application-specific schemas and assets use dynamic imports
|
||||||
|
- [ ] **Type safety with generated types**: API responses use interfaces from `generated/` directory
|
||||||
|
|
||||||
|
## Before Merging Checklist
|
||||||
|
|
||||||
|
- [ ] All automated checks pass (CI/CD)
|
||||||
|
- [ ] Code review approved by at least one maintainer
|
||||||
|
- [ ] No merge conflicts with target branch
|
||||||
|
- [ ] Branch is up to date with main/target branch
|
||||||
|
- [ ] All review comments addressed or discussed
|
||||||
|
- [ ] Screenshots/videos provided for UI changes
|
||||||
|
- [ ] Testing instructions clear and verified
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Quick Reference Commands
|
||||||
|
|
||||||
|
```bash
|
||||||
|
# Run before requesting review
|
||||||
|
yarn lint # Check for linting errors
|
||||||
|
yarn lint:fix # Auto-fix linting issues
|
||||||
|
yarn test # Run unit tests
|
||||||
|
yarn build # Verify build succeeds
|
||||||
|
yarn playwright:run # Run E2E tests (if applicable)
|
||||||
|
```
|
||||||
|
|
||||||
|
## Common Issues to Flag
|
||||||
|
|
||||||
|
1. **Hardcoded strings** instead of translation keys
|
||||||
|
2. **`any` type usage** anywhere in TypeScript
|
||||||
|
3. **Ant Design components** in new features (should use MUI)
|
||||||
|
4. **Hardcoded colors** instead of theme tokens
|
||||||
|
5. **Missing error handling** in async operations
|
||||||
|
6. **Console.log** statements left in code
|
||||||
|
7. **Unnecessary comments** explaining obvious code
|
||||||
|
8. **Missing TypeScript interfaces** for props
|
||||||
|
9. **Direct DOM manipulation** instead of React patterns
|
||||||
|
10. **Missing dependency arrays** in useEffect/useCallback/useMemo
|
||||||
Loading…
x
Reference in New Issue
Block a user