diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index d99079e80e..18f33b0c32 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -103,6 +103,119 @@ This is a React-based monitoring and management interface for YDB clusters. The - Ensure all user-facing text is internationalized - Test with a local YDB instance when possible +## MANDATORY Code Review Rules (From 435 PR Analysis) + +### Top 5 Issues to Prevent + +1. **Hardcoded Strings (42 occurrences)** - #1 issue + - NEVER use hardcoded strings for user-facing text + - Even dashes must use i18n: `i18n('context_no-data')` not `'–'` +2. **Missing Tests (39 occurrences)** + - ALL new features must have tests + - Unit tests for components, E2E tests for features +3. **Improper State Management (16 occurrences)** + - Use Redux selectors, not direct state access + - NEVER mutate state in RTK Query +4. **Missing Loading States (12 occurrences)** + - ALL async operations must show loading indicators + - Use Loader and TableSkeleton components +5. **Poor Error Handling (9 occurrences)** + - Use ResponseError component for API errors + - Clear errors on user input + +### NEVER Do These + +- Use mock data in production code +- Make direct fetch/axios calls (use `window.api`) +- Skip required API parameters +- Create duplicate API calls +- Use improper type names (API types need 'T' prefix) +- Commit without running lint and typecheck + +### ALWAYS Do These + +- Test with real YDB backend: `docker run -dp 8765:8765 ghcr.io/ydb-platform/local-ydb:nightly` +- Include `fields_required: -1` for sysinfo API calls +- Make tables sortable, resizable with sticky headers +- Clear errors on user input in forms +- Use conventional commits with lowercase subjects + +### Specific API Requirements + +```typescript +// Required for threads data (PR #2599) +window.api.viewer.getSysInfo({ + nodeId: nodeId, + fields_required: -1, // MANDATORY parameter +}); +``` + +### Review Priority Matrix + +| Priority | Issue | Check For | +| -------- | ----------------- | ------------------------------- | +| P0 | Hardcoded strings | All text uses i18n() | +| P0 | Missing tests | New features have test coverage | +| P1 | Mock data | Only real backend data used | +| P1 | API patterns | window.api usage, no fetch() | +| P2 | Type naming | API types prefixed with 'T' | + +## Universal Code Review Standards + +Apply these standards consistently for ALL code reviews: + +### Backend & API Standards + +- NO mock data - always use real backend data +- Verify all required API parameters are included +- Check for duplicate API calls +- Ensure proper error handling + +### UI/UX Standards + +- Tables must have sticky headers, be sortable and resizable +- Proper data alignment in all UI components +- Use existing patterns from the codebase +- Loading states for all async operations + +### Code Quality Standards + +- Conventional commit format with lowercase subjects +- No unnecessary files (test scripts, debug code) +- No duplicate code or tests +- Proper TypeScript types (API types prefixed with 'T') +- Simplified event handler types + +### Testing Standards + +- All new features must have tests +- Verify functionality with real YDB instance +- Remove all console.logs and debug statements + +## Common Code Patterns to Flag + +```typescript +// ❌ Hardcoded string +{ + value || '–'; +} + +// ✅ Internationalized +{ + value || i18n('context_no-data'); +} + +// ❌ Verbose event type +(event: React.MouseEvent) => + // ✅ Simplified + (event: React.MouseEvent) => + // ❌ Direct API call + fetch('/api/data'); + +// ✅ Via window.api +window.api.module.getData(); +``` + ## Debugging Helpers - `window.api` - Access API methods in browser console diff --git a/AGENTS.md b/AGENTS.md index 087f56963c..030f23e203 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -123,6 +123,41 @@ REACT_APP_BACKEND=http://your-cluster:8765 # Single cluster mode - Commit messages must follow conventional commit format - Always run `npm run lint` and `npm run typecheck` to catch issues early +## PR & Commit Standards + +### Conventional Commits (REQUIRED) + +Based on 435 code reviews, all commits and PR titles must follow this format: + +``` +feat: add new feature +fix: resolve bug +chore: update dependencies +docs: update documentation +test: add tests +refactor: code improvements +``` + +**Critical**: Subject MUST be lowercase (no proper nouns, sentence-case, or upper-case) + +Examples: + +- ✅ `fix: update storage group count calculation` +- ❌ `Fix: Update Storage Group Count` +- ❌ `fix: Update API endpoints` + +### PR Checklist + +Before submitting a PR: + +- [ ] Title follows conventional commits with lowercase subject +- [ ] No mock data in code (test with real backend) +- [ ] Tested with real YDB backend: `docker run -dp 8765:8765 ghcr.io/ydb-platform/local-ydb:nightly` +- [ ] No duplicate API calls +- [ ] All strings internationalized +- [ ] Run `npm run lint` and `npm run typecheck` +- [ ] Add tests for new features + ### UI Framework The project uses Gravity UI (@gravity-ui/uikit) as the primary component library. When adding new UI components, prefer using Gravity UI components over custom implementations. @@ -150,10 +185,50 @@ The project uses Gravity UI (@gravity-ui/uikit) as the primary component library All API calls go through `window.api` global object with domain-specific modules (viewer, schema, storage, etc.) +**Critical API Parameters (from PR reviews):** + +```typescript +// For sysinfo calls - REQUIRED for threads data (PR #2599) +await window.api.viewer.getSysInfo({ + nodeId: nodeId, + fields_required: -1, // Must include this parameter +}); + +// Common API mistakes: +// ❌ Direct fetch calls +fetch('/api/endpoint'); + +// ❌ Missing required parameters +window.api.viewer.getSysInfo({nodeId}); + +// ✅ Correct pattern +window.api.viewer.getSysInfo({nodeId, fields_required: -1}); +``` + ### Table Implementation Use `PaginatedTable` component for data grids with virtual scrolling. Tables require columns, fetchData function, and a unique tableName. +**Table Requirements (from PR reviews by @Raubzeug and @artemmufazalov):** + +```typescript +// ✅ Correct table implementation + + +// Common table issues: +// - Missing sticky headers (flagged in PR #2577) +// - Incorrect sorting implementation (PR #2633) +// - Not using ResizeableDataTable for data tables +``` + ### Redux Toolkit Query Pattern API endpoints are injected using RTK Query's `injectEndpoints` pattern. Queries wrap `window.api` calls and provide hooks with loading states, error handling, and caching. @@ -197,6 +272,20 @@ All user-facing text must be internationalized using the i18n system. Follow the - **NEVER** use hardcoded strings in UI components - **ALWAYS** create i18n entries for all user-visible text +**Common i18n Mistakes (from 42 PR reviews):** + +```typescript +// ❌ Hardcoded dash +{ + row.UserSID || '–'; +} + +// ✅ Internationalized +{ + row.UserSID || i18n('context_no-data'); +} +``` + ### Performance Considerations - Tables use virtual scrolling for large datasets @@ -259,6 +348,35 @@ Build artifacts are placed in `/build` directory. For embedded deployments, file - `PUBLIC_URL` - Base URL for static assets (use `.` for relative paths) - `GENERATE_SOURCEMAP` - Set to `false` for production builds +## Common Issues from Code Reviews (Based on 435 PR Reviews) + +### Top 5 Issues to Avoid + +1. **Hardcoded Strings (42 occurrences)** + + - ALWAYS use i18n for ALL user-facing text + - Even dashes: Use `i18n('context_no-data')` not `'–'` + +2. **Missing Tests (39 occurrences)** + + - Add unit tests for new components + - E2E tests for new features + - Never merge without test coverage + +3. **Improper State Management (16 occurrences)** + + - Use Redux selectors, not direct state access + - Never mutate state in RTK Query + +4. **Missing Loading States (12 occurrences)** + + - Always show loading indicators + - Handle skeleton states for tables + +5. **Poor Error Handling (9 occurrences)** + - Use ResponseError component + - Clear errors on user input + ## Common Issues & Troubleshooting ### Development Issues @@ -273,6 +391,7 @@ Build artifacts are placed in `/build` directory. For embedded deployments, file 1. **CORS errors**: Check if backend allows localhost:3000 origin in development 2. **Authentication failures**: Verify credentials and authentication method 3. **Network timeouts**: Check backend availability and network configuration +4. **Double API calls**: Check for duplicate useEffect hooks or missing dependencies ### Performance Issues @@ -280,6 +399,68 @@ Build artifacts are placed in `/build` directory. For embedded deployments, file 2. **Bundle size**: Run `npm run analyze` to identify large dependencies 3. **Memory leaks**: Check for missing cleanup in useEffect hooks +## Real Code Review Examples & Solutions + +### Component Patterns (From Actual Reviews) + +#### Event Handler Types (PR #2642) + +```typescript +// ❌ Verbose - Flagged in review +const handleClick = (event: React.MouseEvent) => { + +// ✅ Simplified - Approved +const handleClick = (event: React.MouseEvent) => { +``` + +#### Double API Calls (PR #2599) + +```typescript +// ❌ Causes double requests - Flagged by @adameat +useEffect(() => { + fetchData(); +}, []); + +useEffect(() => { + fetchData(); +}, [someParam]); + +// ✅ Single effect with proper dependencies +useEffect(() => { + fetchData(); +}, [someParam]); +``` + +#### Mock Data Usage (PR #2599) + +```typescript +// ❌ Using mock data - Always flagged +const mockData = { threads: [...] }; +return mockData; + +// ✅ Always use real backend data +const response = await window.api.viewer.getSysInfo({ + nodeId, + fields_required: -1 +}); +return response; +``` + +### UI/UX Requirements (From Reviews) + +#### Progress Indicators (PR #2588 by @adameat) + +- 100% progress should always be displayed in green +- Replication progress must show only when disk is not replicated +- Proper alignment required in tooltips + +#### Table Features (PR #2577 by @Raubzeug) + +- Sticky headers for all scrollable tables +- Sortable columns (especially numeric data) +- Resizable columns +- Use common patterns from existing tables + ## Reference Resources - **YDB Documentation**: https://ydb.tech/en/docs/ diff --git a/CLAUDE.md b/CLAUDE.md index 087f56963c..030f23e203 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -123,6 +123,41 @@ REACT_APP_BACKEND=http://your-cluster:8765 # Single cluster mode - Commit messages must follow conventional commit format - Always run `npm run lint` and `npm run typecheck` to catch issues early +## PR & Commit Standards + +### Conventional Commits (REQUIRED) + +Based on 435 code reviews, all commits and PR titles must follow this format: + +``` +feat: add new feature +fix: resolve bug +chore: update dependencies +docs: update documentation +test: add tests +refactor: code improvements +``` + +**Critical**: Subject MUST be lowercase (no proper nouns, sentence-case, or upper-case) + +Examples: + +- ✅ `fix: update storage group count calculation` +- ❌ `Fix: Update Storage Group Count` +- ❌ `fix: Update API endpoints` + +### PR Checklist + +Before submitting a PR: + +- [ ] Title follows conventional commits with lowercase subject +- [ ] No mock data in code (test with real backend) +- [ ] Tested with real YDB backend: `docker run -dp 8765:8765 ghcr.io/ydb-platform/local-ydb:nightly` +- [ ] No duplicate API calls +- [ ] All strings internationalized +- [ ] Run `npm run lint` and `npm run typecheck` +- [ ] Add tests for new features + ### UI Framework The project uses Gravity UI (@gravity-ui/uikit) as the primary component library. When adding new UI components, prefer using Gravity UI components over custom implementations. @@ -150,10 +185,50 @@ The project uses Gravity UI (@gravity-ui/uikit) as the primary component library All API calls go through `window.api` global object with domain-specific modules (viewer, schema, storage, etc.) +**Critical API Parameters (from PR reviews):** + +```typescript +// For sysinfo calls - REQUIRED for threads data (PR #2599) +await window.api.viewer.getSysInfo({ + nodeId: nodeId, + fields_required: -1, // Must include this parameter +}); + +// Common API mistakes: +// ❌ Direct fetch calls +fetch('/api/endpoint'); + +// ❌ Missing required parameters +window.api.viewer.getSysInfo({nodeId}); + +// ✅ Correct pattern +window.api.viewer.getSysInfo({nodeId, fields_required: -1}); +``` + ### Table Implementation Use `PaginatedTable` component for data grids with virtual scrolling. Tables require columns, fetchData function, and a unique tableName. +**Table Requirements (from PR reviews by @Raubzeug and @artemmufazalov):** + +```typescript +// ✅ Correct table implementation + + +// Common table issues: +// - Missing sticky headers (flagged in PR #2577) +// - Incorrect sorting implementation (PR #2633) +// - Not using ResizeableDataTable for data tables +``` + ### Redux Toolkit Query Pattern API endpoints are injected using RTK Query's `injectEndpoints` pattern. Queries wrap `window.api` calls and provide hooks with loading states, error handling, and caching. @@ -197,6 +272,20 @@ All user-facing text must be internationalized using the i18n system. Follow the - **NEVER** use hardcoded strings in UI components - **ALWAYS** create i18n entries for all user-visible text +**Common i18n Mistakes (from 42 PR reviews):** + +```typescript +// ❌ Hardcoded dash +{ + row.UserSID || '–'; +} + +// ✅ Internationalized +{ + row.UserSID || i18n('context_no-data'); +} +``` + ### Performance Considerations - Tables use virtual scrolling for large datasets @@ -259,6 +348,35 @@ Build artifacts are placed in `/build` directory. For embedded deployments, file - `PUBLIC_URL` - Base URL for static assets (use `.` for relative paths) - `GENERATE_SOURCEMAP` - Set to `false` for production builds +## Common Issues from Code Reviews (Based on 435 PR Reviews) + +### Top 5 Issues to Avoid + +1. **Hardcoded Strings (42 occurrences)** + + - ALWAYS use i18n for ALL user-facing text + - Even dashes: Use `i18n('context_no-data')` not `'–'` + +2. **Missing Tests (39 occurrences)** + + - Add unit tests for new components + - E2E tests for new features + - Never merge without test coverage + +3. **Improper State Management (16 occurrences)** + + - Use Redux selectors, not direct state access + - Never mutate state in RTK Query + +4. **Missing Loading States (12 occurrences)** + + - Always show loading indicators + - Handle skeleton states for tables + +5. **Poor Error Handling (9 occurrences)** + - Use ResponseError component + - Clear errors on user input + ## Common Issues & Troubleshooting ### Development Issues @@ -273,6 +391,7 @@ Build artifacts are placed in `/build` directory. For embedded deployments, file 1. **CORS errors**: Check if backend allows localhost:3000 origin in development 2. **Authentication failures**: Verify credentials and authentication method 3. **Network timeouts**: Check backend availability and network configuration +4. **Double API calls**: Check for duplicate useEffect hooks or missing dependencies ### Performance Issues @@ -280,6 +399,68 @@ Build artifacts are placed in `/build` directory. For embedded deployments, file 2. **Bundle size**: Run `npm run analyze` to identify large dependencies 3. **Memory leaks**: Check for missing cleanup in useEffect hooks +## Real Code Review Examples & Solutions + +### Component Patterns (From Actual Reviews) + +#### Event Handler Types (PR #2642) + +```typescript +// ❌ Verbose - Flagged in review +const handleClick = (event: React.MouseEvent) => { + +// ✅ Simplified - Approved +const handleClick = (event: React.MouseEvent) => { +``` + +#### Double API Calls (PR #2599) + +```typescript +// ❌ Causes double requests - Flagged by @adameat +useEffect(() => { + fetchData(); +}, []); + +useEffect(() => { + fetchData(); +}, [someParam]); + +// ✅ Single effect with proper dependencies +useEffect(() => { + fetchData(); +}, [someParam]); +``` + +#### Mock Data Usage (PR #2599) + +```typescript +// ❌ Using mock data - Always flagged +const mockData = { threads: [...] }; +return mockData; + +// ✅ Always use real backend data +const response = await window.api.viewer.getSysInfo({ + nodeId, + fields_required: -1 +}); +return response; +``` + +### UI/UX Requirements (From Reviews) + +#### Progress Indicators (PR #2588 by @adameat) + +- 100% progress should always be displayed in green +- Replication progress must show only when disk is not replicated +- Proper alignment required in tooltips + +#### Table Features (PR #2577 by @Raubzeug) + +- Sticky headers for all scrollable tables +- Sortable columns (especially numeric data) +- Resizable columns +- Use common patterns from existing tables + ## Reference Resources - **YDB Documentation**: https://ydb.tech/en/docs/