Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
364 changes: 364 additions & 0 deletions .github/instructions/*.instructions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,364 @@
# code review instructions - datagenflow

ensure code follows standards, avoids anti-patterns, and documentation stays synchronized with changes.

---

## primary review objectives

focus areas (in order of importance):
1. documentation synchronization - llm/* files must reflect code changes
2. code quality - follow guides to avoid bad code and anti-design patterns
3. security and error handling - no silent failures, proper validation
4. testing - new features have tests, error cases covered

---

## important

this prompt is self-contained. all rules needed for review are embedded below.

do:
- use only the checklists and guidelines in this file
- identify which llm/* files need updates based on code changes
- flag violations of design patterns and code guides
- ensure anti-patterns are called out and fixed

do not:
- update llm/* files yourself (reviewer only identifies what needs updating)

---

## project-wide standards

### code style
- comments start in lowercase and explain why, not what
- minimal number of functions and comments needed
- avoid unnecessary `else` blocks; return early when possible
- keep names clear, functions small, logic simple
- make the code slightly better than before, not a complete rewrite

### key principles
1. simplicity over cleverness
2. clarity over abstraction
3. explain intent, not mechanics
4. one step better, not a full rewrite

### golden rule
if you can't explain what the code does in one sentence, it's too complex.

---

## backend review checklist

### anti-patterns to reject
- [ ] walrus operators and complex one-liners (violates simplicity)
- [ ] silent failures (empty except blocks, no logging)
- [ ] global variables instead of dependency injection
- [ ] god functions (>30 lines) or god classes (>7 public methods)
- [ ] magic numbers/strings without constants
- [ ] functions with >3 parameters without dataclass

### core principles
- [ ] simplicity over cleverness
- [ ] fail fast and loud (no silent failures, proper logging)
- [ ] explicit dependencies (no globals, use dependency injection)
- [ ] single responsibility (functions <30 lines, max 3 params, classes <7 public methods)

### error handling
- [ ] specific exceptions caught, never bare `Exception` without re-raise
- [ ] `logger.exception()` used to preserve stack traces
- [ ] error messages include context (detail dict with relevant info)
- [ ] no silent failures (no empty except blocks)

### database
- [ ] parameterized queries (never f-strings for values)
- [ ] transactions used for multi-step operations
- [ ] connection cleanup handled properly
- [ ] slow queries logged (>1s)

### api design
- [ ] pydantic models used for request validation
- [ ] consistent error response format
- [ ] dependency injection used for services
- [ ] size limits on file uploads enforced

### async/await
- [ ] async I/O used (aiofiles, aiosqlite - not blocking calls)
- [ ] `asyncio.gather` used for concurrent operations
- [ ] background tasks properly cancelled in cleanup

### testing
- [ ] tests exist for new features
- [ ] error cases tested, not just happy path
- [ ] test names follow: `test_<method>_<scenario>_<expected>`
- [ ] one behavior per test

### security
- [ ] no secrets in logs
- [ ] all inputs validated at API boundary
- [ ] size limits enforced
- [ ] SQL queries parameterized

### type hints
- [ ] all parameters typed
- [ ] all returns typed
- [ ] uses `| None` not `Optional`

---

## frontend review checklist

### anti-patterns to reject
- [ ] bloated components (too many hooks, mixed concerns)
- [ ] silent error handling (empty catch blocks)
- [ ] prop drilling (>5 props, excessive nesting)
- [ ] repeated JSX (copied 3+ times without extraction)
- [ ] direct localStorage/sessionStorage access (not abstracted)
- [ ] inline fetch calls (not in service layer)
- [ ] unstable dependencies in hooks (missing useCallback/useMemo)
- [ ] missing cleanup in useEffect (intervals, subscriptions, AbortController)
- [ ] `any` types or `as` type assertions

### core principles
- [ ] components are focused and maintainable
- [ ] explicit over implicit (props over context when reasonable)
- [ ] fail loudly, never silently (errors logged and shown to user)
- [ ] single responsibility per component

### component design
- [ ] components reasonably sized (extract if unwieldy)
- [ ] no repeated JSX (extracted into reusable components if copied 3+ times)
- [ ] max 5 props per component (context used for more)
- [ ] no excessive prop drilling

### state management
- [ ] related state grouped (useReducer for complex state)
- [ ] no direct localStorage access (abstracted into hooks)
- [ ] useState used for simple state

### react hooks
- [ ] stable dependencies (useCallback for functions in deps)
- [ ] cleanup functions returned from effects
- [ ] AbortController used for fetch calls
- [ ] mounted flags used for async operations
- [ ] useMemo used for expensive calculations

### api integration
- [ ] API calls through service layer (not inline fetch)
- [ ] custom hooks for data fetching
- [ ] error handling with user feedback
- [ ] loading states shown to user

### error handling
- [ ] no silent failures (empty catch blocks)
- [ ] errors logged to console
- [ ] user feedback shown for failures (toast/flash messages)
- [ ] error boundaries used

### typescript
- [ ] no `any` types (use `unknown` if needed)
- [ ] no `as` type assertions (use type guards)
- [ ] all props have interfaces
- [ ] nullable values handled properly

### performance
- [ ] expensive calculations use useMemo
- [ ] callbacks use useCallback
- [ ] components memoized where appropriate (React.memo)
- [ ] heavy components lazy loaded

### testing
- [ ] business logic extracted from components
- [ ] API calls mockable
- [ ] tests for new features exist

---

## architecture principles

- [ ] changes follow existing patterns in the codebase
- [ ] no unnecessary complexity introduced
- [ ] proper separation of concerns maintained
- [ ] changes are focused and minimal (not a rewrite)

---

## documentation review

### llm internal files
these files track project status and architecture across sessions. they must be updated when architecture changes.

when to update llm/* files:
- [ ] new architecture patterns introduced → update relevant llm/*.md file
- [ ] backend logic/structure changed → update llm/backend_technical_guide.md or llm/BACKEND.md
- [ ] frontend design/layout changed → update llm/frontend_technical_guide.md or llm/FRONTEND.md
- [ ] project structure changed → update llm/project_technical_guide.md or llm/POINT.md
- [ ] updates are gradual and incremental, not complete rewrites

critical rules:
- [ ] updates reflect actual code changes, not aspirational designs

### code documentation
- [ ] complex logic has comments explaining why (not what)
- [ ] comments are lowercase and concise
- [ ] no over-documentation of obvious code
- [ ] docstrings exist for public APIs (if applicable)

---

## review process

1. anti-pattern detection
- scan for anti-patterns in backend/frontend checklists
- flag violations immediately (these are blocking issues)
- ensure guides are followed to avoid bad code

2. documentation synchronization review
- identify which llm/* files need updates based on code changes
- list specific sections that need updating and why
- verify code comments explain why, not what

3. code quality review
- apply relevant checklist items from guidelines
- check for violations of core principles
- verify proper error handling and logging
- ensure security best practices followed

4. architecture consistency
- verify consistency with existing codebase patterns
- check if changes introduce unnecessary complexity
- ensure proper separation of concerns
- confirm changes are minimal, not a rewrite

5. testing coverage
- verify tests exist for new functionality
- check that error cases are tested
- ensure test names follow convention

---

## output format

provide feedback in this structure:

### anti-patterns found
list any anti-patterns detected (these are blocking):
- location: file:line_number
- anti-pattern: name/description
- why: which guide/principle violated
- fix: concrete suggestion to follow the guide

### documentation updates required
list llm/* files that need updates based on code changes:
- file: llm/[filename].md
- section: specific section to update
- reason: what changed and why it needs documentation
- details: specific content to add/update

### issues found
for each issue:
- severity: critical | high | medium | low
- location: file:line_number
- issue: description
- why: explanation of which guideline was violated
- fix: concrete suggestion

### recommendations
optional improvements that would enhance quality.

### checklist status
- anti-patterns: passed | failed
- documentation sync: complete | updates needed
- security: passed | failed
- testing: passed | failed
- guides followed: passed | failed

### verdict
- block - anti-patterns or critical issues must be fixed
- request changes - documentation updates or issues need addressing
- approve - ready to merge

---

## example review

### anti-patterns found

1. silent failure
- location: frontend/Review.tsx:112
- anti-pattern: empty catch block silently swallows errors
- why: violates "fail loudly" principle from frontend guide
- fix: log error and show toast: `catch (err) { console.error(err); showToast({type: "error", message: err.message}); }`

2. god function
- location: backend/pipeline.py:78
- anti-pattern: function has 6 parameters and 45 lines
- why: violates single responsibility (max 3 params, max 30 lines) from backend guide
- fix: create dataclass for parameters, extract validation logic into separate function

3. inline fetch
- location: frontend/Dashboard.tsx:34
- anti-pattern: direct fetch call in component instead of service layer
- why: violates api integration pattern from frontend guide
- fix: move to api service: `const data = await api.getDashboardData()`

### documentation updates required

1. backend architecture change
- file: llm/backend_technical_guide.md
- section: validation patterns
- reason: new validation pipeline introduced in api.py
- details: document parameterized query pattern used for user input sanitization

2. frontend state management
- file: llm/frontend_technical_guide.md
- section: error handling patterns
- reason: new centralized error toast system in Review component
- details: explain toast notification pattern and when to use it vs inline errors

3. project structure
- file: llm/project_technical_guide.md
- section: api layer architecture
- reason: new service layer abstraction added
- details: document service layer pattern and separation from components

### issues found

critical
- location: backend/api.py:45
- issue: SQL query uses f-string for user input
- why: enables SQL injection attack
- fix: use parameterized query: `db.execute("SELECT * FROM users WHERE id = ?", (user_id,))`

medium
- location: frontend/Review.tsx:89
- issue: missing AbortController for fetch call
- why: can cause memory leaks and race conditions
- fix: add controller and cleanup: `useEffect(() => { const ctrl = new AbortController(); ... return () => ctrl.abort(); }, [])`

### recommendations
- consider extracting validation logic into reusable validator class
- could add useMemo to expensive calculation at line 67

### checklist status
- anti-patterns: failed (3 found)
- documentation sync: updates needed (3 files)
- security: failed (SQL injection)
- testing: passed
- guides followed: failed (multiple violations)

### verdict
block - must fix anti-patterns (silent failure, god function, inline fetch) and critical SQL injection. update documentation after fixes.

---

## golden rules

1. anti-patterns are blocking - reject code that violates guides
2. architecture changes require llm/* documentation updates - always identify what needs updating
3. documentation must reflect actual code, not aspirational designs
4. if the code cannot be explained in one sentence, it's too complex
5. never approve silent failures or missing error handling
6. security and proper error handling are mandatory
7. all rules needed for review are embedded in this file - do not read external files
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ frontend/package-lock.json
# project notes (not for commits)
point.md
plan.md
llm/*.*

# website
website/node_modules/
Expand Down
Loading
Loading