diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 257c60b5eb2..8a0742754ae 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -546,4 +546,102 @@ make unit_ingestion_dev_env # For Python changes - Database migrations are automatically applied on startup - 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>({})` +- ✅ **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. \ No newline at end of file diff --git a/openmetadata-ui/UI_PR_REVIEW_GUIDELINES.md b/openmetadata-ui/UI_PR_REVIEW_GUIDELINES.md new file mode 100644 index 00000000000..1e83b7f1afc --- /dev/null +++ b/openmetadata-ui/UI_PR_REVIEW_GUIDELINES.md @@ -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>({})` +- [ ] **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