diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b8db675..a05e054 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -123,3 +123,131 @@ Husky hooks enforce a baseline before changes reach CI: You can bypass the hooks for a one-off push with `git push --no-verify`, but note that the same checks still run in CI and will block the pull request. + + + +# Contributing to TeachLink Mobile + +Thank you for contributing to TeachLink Mobile! + +## Pull Request Guidelines + +When submitting a Pull Request, you must fill out the provided PR template. +The template ensures that all necessary considerations are accounted for before merge. + +Please review the `.github/pull_request_template.md` which includes: +- **Summary & Type of Change**: Describe what the PR does. +- **Testing Done**: List the tests performed. +- **Security Considerations**: Address concerns like secure data storage, token handling, input validation, and deep link handling. +- **Performance Considerations**: Address concerns like hook optimization (`useCallback`, `useMemo`), `FlatList` optimization, and asynchronous patterns. +- **Checklist**: General checks, including checking whether an Architectural Decision Record (ADR) is needed. + +## Fast-Fail Syntax Gate + +We have a dedicated **Syntax Gate** workflow (`.github/workflows/syntax.yml`) that runs on every pull request `opened` or `synchronize` event. + +- Checks TypeScript compiler errors (`tsc --noEmit`) and ESLint (`eslint --max-warnings=0`) +- Optimized to complete in **under 90 seconds** using caching +- Required for branch protection — PRs cannot be merged if it fails +- Run checks locally before pushing to avoid CI failures + +## Architecture + +The intended module structure, layering and dependency direction are documented +in [docs/ARCHITECTURE.md](docs/ARCHITECTURE.md). The layering is enforced locally +and in CI with dependency-cruiser: + +```bash +npm run architecture:check +``` + +Read the architecture doc before adding a new module — the codebase already has +a single canonical implementation for error handling, logging, location, course +progress, sync conflict resolution, and feature flags, and duplicating one of +these is a review blocker. + +## Structured Logging + +**Never use `console.*` in `src/`.** The ESLint `no-console` rule is set to `error`, and CI will fail if any `console.*` call is introduced. Use `src/utils/logger` instead. + +### Why structured logging? + +`console.log` output is unstructured, always-on, and leaks information in production builds. `logger` gives you: +- Log level filtering (only `error` and `warn` in production) +- Consistent metadata (timestamp, component context) +- A single place to redirect logs to remote monitoring (e.g. Sentry, Datadog) + +### Log level guide + +| Level | Method | When to use | +|---|---|---| +| **error** | `logger.error(msg, err?)` | Unexpected failures that need immediate attention. Always include the `Error` object as the second argument. | +| **warn** | `logger.warn(msg, ctx?)` | Recoverable issues or deprecated code paths that should be investigated. | +| **info** | `logger.info(msg, ctx?)` | Key lifecycle events: component mount/unmount, navigation, background sync. Keep them meaningful, not noisy. | +| **debug** | `logger.debug(msg, ctx?)` | Verbose detail useful during development only. Stripped from production builds. | +| **component** | `logger.component(name, event, ctx?)` | Convenience wrapper for component lifecycle events — equivalent to `info` with a standardised format. | + +### Examples + +```ts +// ✅ Correct +import { logger } from '../../utils/logger'; + +logger.component('MyScreen', 'Mounted', { userId }); +logger.info('Resuming lesson from position:', position); +logger.warn('Quiz data missing for section:', sectionId); +logger.error('Failed to sync progress:', error); + +// ❌ Incorrect — will fail CI +console.log('user mounted', userId); +console.error('sync failed', error); +``` + +### Audit + +CI runs a console violation scan on every push. To run it locally: + +```bash +grep -rn "console\." src/ --include='*.ts' --include='*.tsx' +``` + +Zero matches is the expected output. + +## Local Quality Checks + +You can run the checks locally: + +```bash +# Run ESLint linting +npm run lint + +# Check formatting +npm run format:check + +# Run TypeScript type check (same check CI runs) +npm run typecheck + +# Continuously re-run the type check as you edit +npm run typecheck:watch +``` + +### Lint warning budget + +Lint warnings are capped by a single ratcheting budget in `lint-budget.json` +(`maxWarnings`), enforced by `ci.yml`. There is exactly one lint gate in CI. + +- `npm run lint:budget` — fails if the current warning count exceeds the budget, + or if the budget is looser than the measured count (so the ceiling can only + decrease over time). +- `npm run lint:budget:record` — measures the warning count and records it as the + new, lower budget. Run and commit this after removing warnings so the budget + ratchets down instead of silently growing. +### Git hooks + +Husky hooks enforce a baseline before changes reach CI: + +- **`pre-commit`** — runs `lint-staged` (Prettier + ESLint) on staged files. +- **`pre-push`** — runs `npm run typecheck` so type errors are caught before push. + +You can bypass the hooks for a one-off push with `git push --no-verify`, but note +that the same checks still run in CI and will block the pull request.