mirror of
https://github.com/suitenumerique/docs.git
synced 2026-10-01 05:55:16 +02:00
📝(project) share Claude Code skills in the repository
Agents working on Docs kept rediscovering the same project specifics: Makefile targets, backend test setup, E2E helpers, review focus points. Version the skills for backend work, E2E tests, PR reviews and accessibility reviews so that every contributor gets them. .claude/ was ignored as a whole: ignore only its content, except the skills folder, and keep the local-* skills out of git, as they hold personal workflows.
This commit is contained in:
Anthony LC
@@ -0,0 +1,75 @@
|
||||
---
|
||||
name: docs-accessibility-review
|
||||
description: Review the accessibility of added or modified frontend code in La Suite Docs (PRs, diffs, React components, styles, editor features). Covers HTML semantics, accessible names, ARIA, keyboard navigation, focus entry and restoration (Cancel, Escape, submit, removed trigger), screen-reader announcements, forms, dialogs, menus, the document tree, BlockNote and collaboration. Verifies in a real browser when one is available and keeps static, browser, axe and screen-reader evidence apart.
|
||||
---
|
||||
|
||||
# Docs accessibility review
|
||||
|
||||
Review the changed experience end to end. Report evidenced regressions, not a generic checklist. Stay in review mode unless the user asked for fixes.
|
||||
|
||||
## 1. Scope
|
||||
|
||||
- Read the diff the user asked for. For a branch, compare against the merge base with the real target branch (`git merge-base`), not only the last commit. Include uncommitted changes when asked. Never reset, stash or stage user work.
|
||||
- Map changed files to the rendered components, routes and shared primitives, and to their other consumers: a change in `src/components/` affects every caller. Include styles, portals, event handlers and async state. Backend-only changes are in scope only if they change what the UI renders (permissions, error responses).
|
||||
- Before reviewing, write a small matrix: **flow/state | expected focus | expected accessible information or announcement | how it was verified**. Cover the states that apply: idle, open, loading, success, empty, error, cancel, trigger removed.
|
||||
- Target WCAG 2.2 A/AA. APG is pattern guidance, not a normative requirement: do not report an APG deviation as a WCAG failure. Do not claim RGAA or legal conformance.
|
||||
|
||||
## 2. Docs landmarks
|
||||
|
||||
App: `src/frontend/apps/impress/src` (`@/` alias). Read the implementation of a primitive before reporting that it lacks semantics or focus handling: wrappers often delegate them. Conversely, using an accessible primitive does not make its usage accessible.
|
||||
|
||||
- **UI primitives:** `@gouvfr-lasuite/ui-components` (Modal, DropdownMenu, …) built on `react-aria-components`; local wrappers in `components/` (`components/modal/`, `components/dropdown-menu/`, `DropButton.tsx`). Floating UI (`@floating-ui/react`) for some popovers.
|
||||
- **Focus restoration:** `stores/useFocusStore.tsx` holds a **single** `lastFocusedElement`; `restoreFocus()` focuses it on the next frame and clears it. Check that nested overlays do not overwrite each other's element, and that the element is still connected when restored (focusing a detached node fails silently and focus drops to `body`).
|
||||
- **Route changes:** `hooks/useRouteChangeCompleteFocus.tsx` moves focus to the start of the main content (`layouts/utils.ts`) only after a keyboard-initiated navigation.
|
||||
- **Controlled DropdownMenu:** when `isOpen` is controlled, `onOpenChange` does not fire on open, only on react-aria-initiated changes (Escape, outside click, item chosen). Anything that observes the open state must be updated by the trigger handler itself.
|
||||
- **Announcements:** `announce(t('…'), 'polite' | 'assertive')` from `@react-aria/live-announcer` (see `useCreateFavoriteDoc.tsx`, `DocShareModal.tsx`, `FindReplace.tsx`). Reuse it; do not add another live region. A visible toast is not automatically announced.
|
||||
- **Editor:** `features/docs/doc-editor/` (BlockNote over ProseMirror, version in `package.json`). Check the installed BlockNote API before proposing selection or focus changes.
|
||||
- **i18n:** accessible names and announcements go through `t()`. Never edit `translations.json`: it is generated from Crowdin.
|
||||
|
||||
Search with the Grep tool (`rg` may not be installed in the shell), then narrow the search to the affected feature: `aria-|tabIndex|autoFocus|announce\(|restoreFocus|addLastFocus|initialFocus`.
|
||||
|
||||
## 3. Review the changed flows
|
||||
|
||||
Check only the dimensions the change touches:
|
||||
|
||||
1. **Semantics and names:** native elements over `div` plus ARIA, meaningful accessible names and labels, heading order and landmarks, valid ARIA relationships (`aria-controls`, `aria-describedby` targets exist), current state (`aria-expanded`, `aria-selected`, `aria-current`, `aria-pressed`). Icon-only buttons need a name. Do not duplicate the visible text in `aria-label` with different wording.
|
||||
2. **Keyboard:** every action reachable and operable with the keyboard, the expected keys for the widget (arrows in menus, trees and listboxes; Escape closes), no trap, focus indicator visible and not hidden by a sticky header or overlay (WCAG 2.4.11).
|
||||
3. **Focus lifecycle:** where focus lands on open, then after each exit path separately: Cancel, Escape, close button, outside click, success, failure, deletion, navigation, unmount. Focus returns to the trigger, or to a logical, visible, enabled successor when the trigger is gone (deleted item, closed sub-page). Check nested overlays.
|
||||
4. **Announcements:** state changes the user cannot see without moving focus (save, delete, copy link, search results count, errors) must be announced, once, translated, and with no race against a focus move that makes the screen reader read something else.
|
||||
5. **Editor continuity:** after a toolbar action, a dialog, or a cancelled AI action, the caret and selection come back where they were, not only focus on the `contenteditable`. Collaborators' edits must never move the local user's focus or selection.
|
||||
6. **Visual and pointer:** contrast of new colours (use Cunningham tokens), zoom to 200 % and reflow at 320 px, content shown on hover also shown on focus and dismissible, target size (2.5.8), an alternative to dragging (2.5.7, e.g. moving documents in the tree), `prefers-reduced-motion`.
|
||||
|
||||
Describe the expected behaviour before choosing a fix. Do not add ARIA, a focus trap or a live region by reflex.
|
||||
|
||||
## 4. Verify at runtime when possible
|
||||
|
||||
- Use whatever browser tooling this session exposes (Playwright MCP, Chrome DevTools MCP, Claude in Chrome, …). Discover the actual tool names; never invent them. No browser MCP is configured in the repository itself.
|
||||
- The stack must be running (`make run`, frontend on http://localhost:3000). Log in with the seeded accounts of `docker/auth/realm.json` (`user-e2e-chromium` / `password-e2e-chromium`, or `impress` / `impress`); do not guess others. Use a disposable document. Check that the served build contains the reviewed change.
|
||||
- Reach the UI with real key presses, then read the accessibility snapshot and `document.activeElement` at each transition. Take screenshots for focus visibility.
|
||||
- axe is **not** a dependency of the E2E suite. Use it only through available tooling (e.g. an MCP or DevTools injection); do not add the package without asking. Scan the rendered states, portals included.
|
||||
- If no browser is available, finish the source review, say so, and list the exact remaining manual steps. Never report a blocked check as passing, and never install tools or change MCP configuration silently.
|
||||
- An accessibility tree, a DOM mutation of the announcer or a clean axe run is not speech. Only an actual screen-reader session (NVDA, VoiceOver, …) verifies what is spoken and when.
|
||||
|
||||
Label every piece of evidence: **static**, **browser/DOM**, **axe**, **screen reader**, or **not verified**.
|
||||
|
||||
## 5. Tests
|
||||
|
||||
- Do not write E2E tests unless the user explicitly asks; when they do, follow the `docs-e2e-test` skill (Chromium only, focused spec, existing `utils-*.ts` helpers). In review mode, describe the smallest meaningful test instead.
|
||||
- For a component-level regression (names, roles, focus return of a wrapper), a Vitest + Testing Library test next to the component is often cheaper than E2E (see `components/dropdown-menu/__tests__/`).
|
||||
- In focus assertions, do not let the test fix the focus: `getEditor()` calls `.focus()` and `tryFocusEditorContent()` clicks, so never call them between the action under test and `toBeFocused()`. Use `page.keyboard` (Tab, Shift+Tab, Enter, Escape) for the measured interaction: a click or `.focus()` does not prove keyboard reachability.
|
||||
- Prefer `getByRole` / `getByLabel`; a test ID may locate a container but does not check the accessible name. No arbitrary waits, no `force: true` clicks.
|
||||
|
||||
## 6. Report
|
||||
|
||||
Lead with confirmed findings in order of impact. For each:
|
||||
|
||||
- **Severity / confidence**, `file:line`, flow and state.
|
||||
- **User impact**: the concrete key sequence that fails.
|
||||
- **Observed vs expected**, evidence label, WCAG criterion when the mapping is clear.
|
||||
- **Smallest fix**, reusing a local primitive, and the regression check.
|
||||
|
||||
Severity by user impact: **critical** — an essential flow cannot be completed and there is no alternative; **high** — a major navigation or operation barrier; **medium** — confusing or recoverable; **low** — minor friction.
|
||||
|
||||
Keep apart new regressions (checked against the base when possible, otherwise say it is unknown), pre-existing issues nearby, and unverified risks. Merge findings caused by the same shared primitive.
|
||||
|
||||
End with a compact coverage table (flow/state × static / browser / axe / screen reader) and the manual checks still needed. Say "No confirmed issue found in the reviewed scope" when that is the case, never "fully accessible" or a conformance percentage. Name the dimensions and tools that were skipped or failed.
|
||||
@@ -0,0 +1,560 @@
|
||||
---
|
||||
name: docs-backend-django
|
||||
description: Work safely and efficiently on the Django backend of La Suite Docs. Use this skill when modifying Python/Django backend code, API endpoints, serializers, permissions, models, migrations, Celery tasks, database queries, tests, or backend configuration.
|
||||
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
|
||||
|
||||
# Docs Django Backend
|
||||
|
||||
## description: Work safely and efficiently on the Django backend of La Suite Docs. Use this skill when modifying Python/Django backend code, API endpoints, serializers, permissions, models, migrations, Celery tasks, database queries, tests, or backend configuration.
|
||||
|
||||
# Docs Django Backend
|
||||
|
||||
Use this skill whenever working on the Django backend of La Suite Docs.
|
||||
|
||||
The backend source code lives in:
|
||||
|
||||
```text
|
||||
src/backend/
|
||||
```
|
||||
|
||||
The main development Docker Compose service is:
|
||||
|
||||
```text
|
||||
app-dev
|
||||
```
|
||||
|
||||
Inside this container, the backend is mounted at:
|
||||
|
||||
```text
|
||||
/app
|
||||
```
|
||||
|
||||
The development Django configuration is:
|
||||
|
||||
```text
|
||||
DJANGO_CONFIGURATION=Development
|
||||
```
|
||||
|
||||
The backend application is exposed locally on:
|
||||
|
||||
```text
|
||||
http://localhost:8071
|
||||
```
|
||||
|
||||
## Prefer project commands
|
||||
|
||||
Do not invent Docker, pytest, Django, lint, or migration commands when an existing Make target already exists.
|
||||
|
||||
Prefer the repository Makefile because it encapsulates the correct Docker user, environment, volumes, configuration, and service dependencies.
|
||||
|
||||
Before introducing a new development command, check whether an equivalent Make target already exists.
|
||||
|
||||
## Starting the backend
|
||||
|
||||
To start the backend and its required services:
|
||||
|
||||
```bash
|
||||
make run-backend
|
||||
```
|
||||
|
||||
This starts the backend-related stack, including the Django application and supporting services.
|
||||
|
||||
To inspect service status:
|
||||
|
||||
```bash
|
||||
make status
|
||||
```
|
||||
|
||||
To follow Django application logs:
|
||||
|
||||
```bash
|
||||
make logs
|
||||
```
|
||||
|
||||
Avoid running a second Django development instance manually when `app-dev` is already running unless there is a specific reason.
|
||||
|
||||
## Django container
|
||||
|
||||
The main Django service is:
|
||||
|
||||
```text
|
||||
app-dev
|
||||
```
|
||||
|
||||
For exceptional commands not covered by the Makefile, use:
|
||||
|
||||
```bash
|
||||
docker compose exec app-dev <command>
|
||||
```
|
||||
|
||||
For example:
|
||||
|
||||
```bash
|
||||
docker compose exec app-dev python manage.py check
|
||||
```
|
||||
|
||||
However, prefer existing Makefile targets whenever possible.
|
||||
|
||||
## Running Django management commands
|
||||
|
||||
The Makefile runs management commands through an ephemeral `app-dev` container with the expected development environment.
|
||||
|
||||
Available project commands include:
|
||||
|
||||
```bash
|
||||
make migrate
|
||||
make makemigrations
|
||||
make shell
|
||||
make dbshell
|
||||
make superuser
|
||||
```
|
||||
|
||||
Use these instead of directly invoking `python manage.py` from the host.
|
||||
|
||||
If an uncommon management command is needed, follow the same project convention:
|
||||
|
||||
```bash
|
||||
docker compose run --rm app-dev python manage.py <command>
|
||||
```
|
||||
|
||||
## Tests
|
||||
|
||||
Backend tests should be executed through the project's test commands.
|
||||
|
||||
### Run the whole backend test suite
|
||||
|
||||
Prefer:
|
||||
|
||||
```bash
|
||||
make test
|
||||
```
|
||||
|
||||
This currently runs backend tests in parallel.
|
||||
|
||||
Equivalent explicit command:
|
||||
|
||||
```bash
|
||||
make test-back-parallel
|
||||
```
|
||||
|
||||
The project uses pytest with automatic parallel execution for the complete suite.
|
||||
|
||||
### Run specific tests
|
||||
|
||||
Use:
|
||||
|
||||
```bash
|
||||
make test-back <pytest arguments>
|
||||
```
|
||||
|
||||
Examples:
|
||||
|
||||
```bash
|
||||
make test-back tests/test_documents.py
|
||||
```
|
||||
|
||||
```bash
|
||||
make test-back tests/test_documents.py::test_example
|
||||
```
|
||||
|
||||
```bash
|
||||
make test-back -k permission
|
||||
```
|
||||
|
||||
When implementing or fixing backend behavior:
|
||||
|
||||
1. run the most relevant focused tests first;
|
||||
2. fix any failure;
|
||||
3. run the broader affected test module;
|
||||
4. run the full backend suite when appropriate.
|
||||
|
||||
Do not repeatedly run the entire test suite while iterating on a small change if a focused pytest invocation can validate it faster.
|
||||
|
||||
## Linting and formatting
|
||||
|
||||
The backend lint command is:
|
||||
|
||||
```bash
|
||||
make lint
|
||||
```
|
||||
|
||||
It runs the repository's backend linting pipeline.
|
||||
|
||||
The pipeline currently includes:
|
||||
|
||||
```text
|
||||
ruff format
|
||||
ruff check --fix
|
||||
pylint
|
||||
```
|
||||
|
||||
Individual targets are also available:
|
||||
|
||||
```bash
|
||||
make lint-ruff-format
|
||||
make lint-ruff-check
|
||||
make lint-pylint
|
||||
```
|
||||
|
||||
Important: some lint commands modify files automatically.
|
||||
|
||||
In particular:
|
||||
|
||||
```bash
|
||||
ruff format .
|
||||
ruff check . --fix
|
||||
```
|
||||
|
||||
Therefore inspect the resulting diff after running lint.
|
||||
|
||||
Before considering a backend task complete:
|
||||
|
||||
```bash
|
||||
make lint
|
||||
```
|
||||
|
||||
and run the relevant tests.
|
||||
|
||||
## Database
|
||||
|
||||
The development PostgreSQL service is:
|
||||
|
||||
```text
|
||||
postgresql
|
||||
```
|
||||
|
||||
It listens internally on:
|
||||
|
||||
```text
|
||||
postgresql:5432
|
||||
```
|
||||
|
||||
and is exposed from the host on:
|
||||
|
||||
```text
|
||||
localhost:15432
|
||||
```
|
||||
|
||||
Prefer Django ORM access unless direct SQL inspection is useful.
|
||||
|
||||
For a database shell:
|
||||
|
||||
```bash
|
||||
make dbshell
|
||||
```
|
||||
|
||||
Do not assume schema changes are harmless.
|
||||
|
||||
When changing Django models:
|
||||
|
||||
1. determine whether a migration is required;
|
||||
2. run:
|
||||
|
||||
```bash
|
||||
make makemigrations
|
||||
```
|
||||
|
||||
3. inspect the generated migration;
|
||||
4. check that it only contains the intended changes;
|
||||
5. consider production migration safety;
|
||||
6. test the migration when appropriate.
|
||||
|
||||
Never commit an unintended migration generated by unrelated local model changes.
|
||||
|
||||
## Migration safety
|
||||
|
||||
For migrations, consider production consequences rather than only whether the migration succeeds locally.
|
||||
|
||||
Check for:
|
||||
|
||||
* long table locks;
|
||||
* table rewrites;
|
||||
* expensive data migrations;
|
||||
* adding non-null fields to populated tables;
|
||||
* expensive index creation;
|
||||
* uniqueness constraints on existing data;
|
||||
* backwards compatibility during rolling deployment;
|
||||
* code depending on the new schema before all instances have migrated.
|
||||
|
||||
Keep schema migrations and large data transformations separate when that reduces operational risk.
|
||||
|
||||
## Django ORM
|
||||
|
||||
Be particularly careful with query behavior.
|
||||
|
||||
Look for:
|
||||
|
||||
* N+1 queries;
|
||||
* queries inside loops;
|
||||
* repeated `.exists()`, `.count()` or `.first()` calls;
|
||||
* accidentally evaluating the same queryset several times;
|
||||
* fetching full objects when IDs or limited fields are sufficient;
|
||||
* missing `select_related`;
|
||||
* missing `prefetch_related`;
|
||||
* unbounded querysets;
|
||||
* expensive ordering/filtering without suitable indexes.
|
||||
|
||||
Do not add `select_related` or `prefetch_related` mechanically. Verify that the relationship is actually traversed and that the optimization reduces queries.
|
||||
|
||||
For potentially expensive endpoints, reason about how query count scales with the number of documents, users, organizations, or accesses.
|
||||
|
||||
## Transactions and concurrency
|
||||
|
||||
Docs is a collaborative application and may receive concurrent requests for the same resources.
|
||||
|
||||
When a backend change performs multiple related writes, consider whether it requires:
|
||||
|
||||
```python
|
||||
transaction.atomic()
|
||||
```
|
||||
|
||||
Also reason about:
|
||||
|
||||
* concurrent updates;
|
||||
* stale reads;
|
||||
* duplicate requests;
|
||||
* retries;
|
||||
* idempotency;
|
||||
* uniqueness constraints;
|
||||
* Celery retries;
|
||||
* multiple backend replicas;
|
||||
* race conditions between checks and writes.
|
||||
|
||||
Do not assume a sequence such as:
|
||||
|
||||
```python
|
||||
if not object_exists():
|
||||
create_object()
|
||||
```
|
||||
|
||||
is safe under concurrency.
|
||||
|
||||
Prefer enforcing critical invariants at the database level when appropriate.
|
||||
|
||||
## Permissions
|
||||
|
||||
Permissions are a critical part of Docs backend changes.
|
||||
|
||||
Never rely on frontend checks for authorization.
|
||||
|
||||
For every new or changed endpoint, determine:
|
||||
|
||||
* who can read the resource;
|
||||
* who can modify it;
|
||||
* who can delete it;
|
||||
* whether anonymous access is allowed;
|
||||
* whether public/share-link access changes behavior;
|
||||
* whether organization or workspace boundaries apply.
|
||||
|
||||
Check object-level permissions, not only authentication.
|
||||
|
||||
When an object ID comes from a request, make sure a user cannot substitute another valid ID and access a resource they do not own or cannot access.
|
||||
|
||||
Prefer filtering the queryset according to permissions over retrieving an unrestricted object and checking too late.
|
||||
|
||||
Permission-related changes should generally have explicit tests.
|
||||
|
||||
At minimum, consider tests for:
|
||||
|
||||
```text
|
||||
authorized user
|
||||
unauthorized authenticated user
|
||||
anonymous user
|
||||
different organization/workspace
|
||||
read-only user
|
||||
```
|
||||
|
||||
where applicable.
|
||||
|
||||
## API changes
|
||||
|
||||
When modifying Django REST API behavior, inspect:
|
||||
|
||||
* serializer validation;
|
||||
* writable/read-only fields;
|
||||
* optional vs required fields;
|
||||
* `null` vs missing values;
|
||||
* defaults;
|
||||
* HTTP status codes;
|
||||
* error response shape;
|
||||
* backwards compatibility;
|
||||
* pagination;
|
||||
* permission classes;
|
||||
* object lookup behavior.
|
||||
|
||||
If frontend code already consumes the endpoint, search for its usages before changing its contract.
|
||||
|
||||
Avoid silently changing response structures used by existing clients.
|
||||
|
||||
## Models
|
||||
|
||||
When modifying Django models:
|
||||
|
||||
* preserve domain invariants;
|
||||
* prefer explicit constraints when they protect data integrity;
|
||||
* consider indexes for new common lookup patterns;
|
||||
* avoid putting surprising network or heavy side effects in model methods;
|
||||
* consider deletion behavior and related objects;
|
||||
* review `on_delete` semantics;
|
||||
* consider whether nullable fields are actually desirable.
|
||||
|
||||
For uniqueness or consistency requirements, prefer database constraints over application-only checks when possible.
|
||||
|
||||
## Celery
|
||||
|
||||
The development worker runs in:
|
||||
|
||||
```text
|
||||
celery-dev
|
||||
```
|
||||
|
||||
It uses the same backend development image as Django.
|
||||
|
||||
When creating or changing Celery tasks, consider:
|
||||
|
||||
* retries;
|
||||
* duplicate execution;
|
||||
* idempotency;
|
||||
* partial failure;
|
||||
* transaction boundaries;
|
||||
* task execution before a database transaction is committed;
|
||||
* serialization of task arguments;
|
||||
* stale model state;
|
||||
* worker restarts.
|
||||
|
||||
Avoid passing large serialized model structures to tasks.
|
||||
|
||||
Prefer stable identifiers and reload the current state inside the worker.
|
||||
|
||||
When scheduling a task as part of a database transaction, consider whether it needs to be triggered only after commit.
|
||||
|
||||
## Redis
|
||||
|
||||
The development Redis service is:
|
||||
|
||||
```text
|
||||
redis
|
||||
```
|
||||
|
||||
Be cautious when using Redis for correctness-critical state.
|
||||
|
||||
Treat caches as disposable unless the architecture explicitly guarantees otherwise.
|
||||
|
||||
Avoid relying exclusively on cached data for authorization or durable application state.
|
||||
|
||||
## Object storage
|
||||
|
||||
The development environment uses MinIO.
|
||||
|
||||
Services involved include:
|
||||
|
||||
```text
|
||||
minio
|
||||
createbuckets
|
||||
```
|
||||
|
||||
When changing file or object-storage behavior, consider:
|
||||
|
||||
* failed uploads;
|
||||
* failed database writes after uploads;
|
||||
* orphaned objects;
|
||||
* deletion consistency;
|
||||
* retries;
|
||||
* permissions;
|
||||
* untrusted filenames;
|
||||
* content types;
|
||||
* object size.
|
||||
|
||||
Do not assume local filesystem behavior represents production object storage behavior.
|
||||
|
||||
## Backend architecture
|
||||
|
||||
Before implementing new behavior:
|
||||
|
||||
1. search for an existing equivalent pattern;
|
||||
2. inspect nearby views, serializers, services, models, permissions and tests;
|
||||
3. follow existing project conventions where they are sound;
|
||||
4. avoid creating a new abstraction when an existing one fits.
|
||||
|
||||
Prefer small, focused changes over broad refactoring unless the task explicitly requires architectural work.
|
||||
|
||||
Do not move unrelated code while implementing a feature or bug fix.
|
||||
|
||||
## Testing expectations
|
||||
|
||||
New behavior should normally include tests when it changes observable backend behavior.
|
||||
|
||||
Prioritize tests for:
|
||||
|
||||
* permissions;
|
||||
* API behavior;
|
||||
* validation;
|
||||
* domain invariants;
|
||||
* regression bugs;
|
||||
* concurrency-sensitive operations where feasible;
|
||||
* failure paths;
|
||||
* Celery task behavior;
|
||||
* migrations when they contain meaningful logic.
|
||||
|
||||
Avoid tests that merely reproduce Django or library behavior.
|
||||
|
||||
Test project-specific behavior.
|
||||
|
||||
## Debugging
|
||||
|
||||
When debugging a backend problem:
|
||||
|
||||
1. reproduce the issue;
|
||||
2. inspect the relevant logs;
|
||||
3. identify the request/task execution path;
|
||||
4. inspect related tests;
|
||||
5. inspect database state if needed;
|
||||
6. form a concrete hypothesis;
|
||||
7. make the smallest change that addresses the cause;
|
||||
8. run focused tests;
|
||||
9. run lint;
|
||||
10. review the final diff.
|
||||
|
||||
Do not start refactoring before the root cause is understood.
|
||||
|
||||
## Before finishing a backend task
|
||||
|
||||
Review:
|
||||
|
||||
```bash
|
||||
git diff
|
||||
```
|
||||
|
||||
Check that no unrelated files were modified.
|
||||
|
||||
Then run appropriate focused tests.
|
||||
|
||||
When practical, run:
|
||||
|
||||
```bash
|
||||
make lint
|
||||
```
|
||||
|
||||
and:
|
||||
|
||||
```bash
|
||||
make test-back <relevant tests>
|
||||
```
|
||||
|
||||
For significant backend changes, also consider:
|
||||
|
||||
```bash
|
||||
make test
|
||||
```
|
||||
|
||||
Before presenting the result, summarize:
|
||||
|
||||
* what changed;
|
||||
* important implementation decisions;
|
||||
* tests executed;
|
||||
* linting executed;
|
||||
* anything not verified.
|
||||
|
||||
Never claim tests or lint passed unless they were actually executed successfully.
|
||||
|
||||
If new setting is added the documentation should be updated accordingly: `/documentation/env.md`
|
||||
@@ -0,0 +1,625 @@
|
||||
---
|
||||
|
||||
name: docs-e2e-test
|
||||
description: Work on end-to-end tests for La Suite Docs using Playwright. Use this skill when creating, modifying, debugging, or reviewing E2E tests, especially under src/frontend/apps/e2e/**tests**/app-impress. Prefer focused Chromium tests, reuse existing utilities and existing tests before creating new code, and avoid running the full E2E suite unless explicitly necessary.
|
||||
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
|
||||
|
||||
# Docs E2E Tests
|
||||
|
||||
Use this skill whenever working on end-to-end tests for La Suite Docs.
|
||||
|
||||
The main Impress E2E tests live in:
|
||||
|
||||
```text
|
||||
src/frontend/apps/e2e/__tests__/app-impress
|
||||
```
|
||||
|
||||
The E2E application uses Playwright.
|
||||
|
||||
The relevant package is:
|
||||
|
||||
```text
|
||||
src/frontend/apps/e2e
|
||||
```
|
||||
|
||||
## Core principles
|
||||
|
||||
E2E tests are expensive.
|
||||
|
||||
They consume significantly more CPU, memory, browser resources, CI resources, and execution time than lower-level tests.
|
||||
|
||||
Therefore:
|
||||
|
||||
1. do not run the entire E2E test suite by default;
|
||||
2. run the smallest relevant subset of tests;
|
||||
3. test only with Chromium unless explicitly requested otherwise;
|
||||
4. reuse existing test utilities before writing custom Playwright code;
|
||||
5. reuse or improve existing tests before creating new tests;
|
||||
6. avoid duplicating scenarios already covered elsewhere;
|
||||
7. only add a new E2E test when it provides meaningful coverage that cannot reasonably fit into an existing scenario.
|
||||
|
||||
The goal is not to maximize the number of E2E tests.
|
||||
|
||||
The goal is to maintain high-value coverage with the smallest, fastest and most maintainable E2E suite possible.
|
||||
|
||||
## Chromium only
|
||||
|
||||
The Docs E2E suite currently targets Chromium for development and validation.
|
||||
|
||||
Do not run Firefox or WebKit.
|
||||
|
||||
Do not run:
|
||||
|
||||
```bash
|
||||
yarn test:ui::firefox
|
||||
yarn test:ui::webkit
|
||||
```
|
||||
|
||||
Do not add Firefox- or WebKit-specific validation unless explicitly requested.
|
||||
|
||||
When a Playwright project needs to be specified, use:
|
||||
|
||||
```bash
|
||||
--project=chromium
|
||||
```
|
||||
|
||||
For example:
|
||||
|
||||
```bash
|
||||
yarn test __tests__/app-impress/<relevant-test>.spec.ts --project=chromium
|
||||
```
|
||||
|
||||
or:
|
||||
|
||||
```bash
|
||||
yarn test __tests__/app-impress/<relevant-test>.spec.ts \
|
||||
--project=chromium \
|
||||
-g "test name"
|
||||
```
|
||||
|
||||
Do not spend development resources checking cross-browser compatibility unless the project's browser coverage policy changes.
|
||||
|
||||
## Do not run all E2E tests by default
|
||||
|
||||
Do not use:
|
||||
|
||||
```bash
|
||||
yarn test
|
||||
```
|
||||
|
||||
without restricting its scope unless there is a specific reason.
|
||||
|
||||
Running the whole Playwright suite is heavy for the development machine and should not be used as the default validation strategy.
|
||||
|
||||
When implementing or debugging a change:
|
||||
|
||||
1. identify the relevant existing test file;
|
||||
2. run only that test file;
|
||||
3. when possible, run only the relevant test with `-g`;
|
||||
4. run it on Chromium only;
|
||||
5. widen the test scope only when necessary.
|
||||
|
||||
Prefer:
|
||||
|
||||
```bash
|
||||
yarn test __tests__/app-impress/<relevant-test>.spec.ts \
|
||||
--project=chromium \
|
||||
-g "relevant test name"
|
||||
```
|
||||
|
||||
Then, if useful:
|
||||
|
||||
```bash
|
||||
yarn test __tests__/app-impress/<relevant-test>.spec.ts \
|
||||
--project=chromium
|
||||
```
|
||||
|
||||
Use actual existing filenames and test names found in the repository.
|
||||
|
||||
Do not invent test paths.
|
||||
|
||||
## Search before writing
|
||||
|
||||
Before creating or modifying E2E code, inspect the existing tests and utilities under:
|
||||
|
||||
```text
|
||||
src/frontend/apps/e2e/__tests__/app-impress
|
||||
```
|
||||
|
||||
Search for:
|
||||
|
||||
* the feature name;
|
||||
* relevant UI labels;
|
||||
* similar user workflows;
|
||||
* existing test cases;
|
||||
* existing fixtures;
|
||||
* existing utility functions;
|
||||
* authentication helpers;
|
||||
* document helpers;
|
||||
* editor helpers;
|
||||
* sharing helpers;
|
||||
* sub-page helpers;
|
||||
* mocking helpers.
|
||||
|
||||
Do not immediately start writing Playwright instructions from scratch.
|
||||
|
||||
First understand how equivalent operations are already implemented in the E2E suite.
|
||||
|
||||
## Reuse existing utilities
|
||||
|
||||
The E2E suite already contains utilities for common workflows.
|
||||
|
||||
Use them whenever they match the required behavior.
|
||||
|
||||
Before implementing custom Playwright interactions, inspect the relevant `utils-*` files.
|
||||
|
||||
Existing utilities include areas such as:
|
||||
|
||||
```text
|
||||
utils-common
|
||||
utils-editor
|
||||
utils-export
|
||||
utils-share
|
||||
utils-signin
|
||||
utils-sub-pages
|
||||
```
|
||||
|
||||
Examples of existing capabilities include:
|
||||
|
||||
* creating documents;
|
||||
* navigating documents;
|
||||
* updating document titles;
|
||||
* saving editor content;
|
||||
* opening header menus;
|
||||
* overriding frontend configuration;
|
||||
* mocking documents and API responses;
|
||||
* interacting with the editor;
|
||||
* writing content into the editor;
|
||||
* opening editor suggestion menus;
|
||||
* signing users in and out;
|
||||
* configuring sharing;
|
||||
* adding members;
|
||||
* changing member roles;
|
||||
* connecting another user to a collaborative document;
|
||||
* working with sub-pages;
|
||||
* export/PDF helpers.
|
||||
|
||||
Before writing something like:
|
||||
|
||||
```ts
|
||||
await page.getByRole(...).click();
|
||||
await page.waitForResponse(...);
|
||||
await page.getByLabel(...).fill(...);
|
||||
```
|
||||
|
||||
check whether an existing utility already encapsulates that workflow.
|
||||
|
||||
Prefer:
|
||||
|
||||
```ts
|
||||
await createDoc(...);
|
||||
```
|
||||
|
||||
over reimplementing document creation.
|
||||
|
||||
Prefer:
|
||||
|
||||
```ts
|
||||
await SignIn(...);
|
||||
```
|
||||
|
||||
over manually reproducing authentication.
|
||||
|
||||
Prefer:
|
||||
|
||||
```ts
|
||||
await writeInEditor(...);
|
||||
```
|
||||
|
||||
or the appropriate editor utility over duplicating editor-specific selectors.
|
||||
|
||||
Prefer:
|
||||
|
||||
```ts
|
||||
await updateShareLink(...);
|
||||
```
|
||||
|
||||
over duplicating the sharing workflow.
|
||||
|
||||
## Improve utilities when appropriate
|
||||
|
||||
If several tests require behavior that is almost covered by an existing utility, consider extending that utility instead of introducing slightly different implementations in multiple tests.
|
||||
|
||||
For example, prefer evolving:
|
||||
|
||||
```text
|
||||
createDoc(...)
|
||||
```
|
||||
|
||||
with a sensible optional argument when the new behavior is genuinely part of document creation, rather than creating:
|
||||
|
||||
```text
|
||||
createSpecialDoc(...)
|
||||
createAnotherDoc(...)
|
||||
createDocForFeatureX(...)
|
||||
```
|
||||
|
||||
with mostly duplicated implementations.
|
||||
|
||||
However, do not turn utilities into large generic abstractions with many unrelated options.
|
||||
|
||||
A utility should represent a clear reusable E2E operation.
|
||||
|
||||
## Do not duplicate utility logic inside tests
|
||||
|
||||
Avoid copying code from a utility into a test merely because the test needs a small variation.
|
||||
|
||||
Instead:
|
||||
|
||||
1. check whether the existing utility already supports the scenario;
|
||||
2. determine whether the utility can be safely improved;
|
||||
3. reuse the utility where appropriate;
|
||||
4. only write custom logic when the behavior is genuinely specific to the test.
|
||||
|
||||
When custom logic is necessary, keep it local unless it becomes reusable.
|
||||
|
||||
## Prefer existing tests
|
||||
|
||||
Before creating a new test case or test file:
|
||||
|
||||
1. search existing tests for the same feature;
|
||||
2. identify whether the workflow is already partially covered;
|
||||
3. determine whether the new assertion can fit naturally into that scenario;
|
||||
4. prefer extending the existing test when it remains readable.
|
||||
|
||||
Avoid creating a second E2E scenario that repeats:
|
||||
|
||||
```text
|
||||
sign in
|
||||
→ create document
|
||||
→ open document
|
||||
→ navigate somewhere
|
||||
→ perform one slightly different action
|
||||
```
|
||||
|
||||
when the existing scenario can validate the additional behavior cheaply and clearly.
|
||||
|
||||
## Minimize suite growth
|
||||
|
||||
Every additional E2E test has a permanent cost:
|
||||
|
||||
* execution time;
|
||||
* CI time;
|
||||
* CPU;
|
||||
* memory;
|
||||
* browser resources;
|
||||
* setup time;
|
||||
* maintenance;
|
||||
* debugging;
|
||||
* flakiness surface.
|
||||
|
||||
Before adding a test, ask:
|
||||
|
||||
```text
|
||||
Can this behavior be verified by improving an existing E2E test?
|
||||
```
|
||||
|
||||
If yes, prefer that.
|
||||
|
||||
Then ask:
|
||||
|
||||
```text
|
||||
Does this behavior really need E2E coverage?
|
||||
```
|
||||
|
||||
Some behavior is better covered through:
|
||||
|
||||
* frontend unit tests;
|
||||
* component tests;
|
||||
* backend tests;
|
||||
* API tests.
|
||||
|
||||
E2E tests should focus on important user-visible workflows and integration boundaries.
|
||||
|
||||
## Keep tests focused
|
||||
|
||||
A good E2E test should represent a meaningful user workflow.
|
||||
|
||||
Prefer coverage such as:
|
||||
|
||||
* creating and editing a document;
|
||||
* permissions and access behavior;
|
||||
* sharing;
|
||||
* collaboration;
|
||||
* sub-pages;
|
||||
* authentication;
|
||||
* important editor functionality;
|
||||
* critical navigation;
|
||||
* frontend/backend integration.
|
||||
|
||||
Avoid using E2E tests to verify implementation details.
|
||||
|
||||
## Keep tests independent
|
||||
|
||||
Tests should not depend on their execution order.
|
||||
|
||||
A test should not require another test to run first.
|
||||
|
||||
Avoid shared mutable state between tests unless the existing architecture explicitly requires it.
|
||||
|
||||
Create only the state necessary for the scenario.
|
||||
|
||||
## Avoid expensive UI setup when appropriate
|
||||
|
||||
Use existing helpers and setup mechanisms to avoid repeatedly executing UI flows that are unrelated to what the test validates.
|
||||
|
||||
For example, a test about editor behavior does not necessarily need to independently test every step of authentication and document creation.
|
||||
|
||||
Reuse the established helpers.
|
||||
|
||||
However, do not bypass a UI workflow if that workflow is itself what the test needs to validate.
|
||||
|
||||
## Playwright selectors
|
||||
|
||||
Follow the selectors already used by the E2E suite.
|
||||
|
||||
Prefer semantic selectors such as:
|
||||
|
||||
```text
|
||||
getByRole
|
||||
getByLabel
|
||||
getByText
|
||||
getByTestId
|
||||
```
|
||||
|
||||
Avoid fragile selectors based on:
|
||||
|
||||
* generated classes;
|
||||
* DOM hierarchy;
|
||||
* styling;
|
||||
* arbitrary indexes.
|
||||
|
||||
Do not introduce a new `data-testid` automatically if an existing accessible selector is stable enough.
|
||||
|
||||
## Assertions and waits
|
||||
|
||||
Prefer Playwright's observable conditions and auto-waiting.
|
||||
|
||||
Prefer:
|
||||
|
||||
```ts
|
||||
await expect(element).toBeVisible();
|
||||
```
|
||||
|
||||
over:
|
||||
|
||||
```ts
|
||||
await page.waitForTimeout(2000);
|
||||
```
|
||||
|
||||
Do not add arbitrary sleeps as the first solution to synchronization problems.
|
||||
|
||||
Some existing tests or helpers may contain `waitForTimeout`. Do not copy that pattern automatically into new tests.
|
||||
|
||||
One legitimate exception: the version history groups edits into versions by a time window (60 s by default), keyed on timestamps set by the yhub server, so `page.clock` cannot shorten it. Shrink the window with `overrideConfig(page, { COLLABORATION_VERSION_GRANULARITY_MS: '2000' })` and wait a real `waitForTimeout` longer than it between edits, as `doc-version.spec.ts` does.
|
||||
|
||||
If a deterministic event, request, element state, URL change, or response can be awaited instead, prefer that.
|
||||
|
||||
## Respect synchronization utilities
|
||||
|
||||
Some existing utilities encode important synchronization behavior.
|
||||
|
||||
For example, a helper may wait for:
|
||||
|
||||
* a backend PATCH;
|
||||
* a document save;
|
||||
* a grid reload;
|
||||
* a URL transition;
|
||||
* a collaborative update.
|
||||
|
||||
Do not bypass such utilities by reproducing only the visible interactions.
|
||||
|
||||
The synchronization behavior may be the reason the helper exists.
|
||||
|
||||
Understand an existing helper before replacing it with seemingly simpler Playwright commands.
|
||||
|
||||
## Collaborative editing
|
||||
|
||||
Docs is a collaborative application.
|
||||
|
||||
When testing multiple users or real-time editing, consider:
|
||||
|
||||
* initial synchronization;
|
||||
* WebSocket propagation;
|
||||
* concurrent changes;
|
||||
* reconnect behavior;
|
||||
* async document updates;
|
||||
* presence/awareness;
|
||||
* stale editor state.
|
||||
|
||||
Reuse existing collaboration helpers when available.
|
||||
|
||||
Do not add fixed delays to approximate collaborative synchronization when an observable state can be awaited.
|
||||
|
||||
## Flakiness
|
||||
|
||||
Treat flaky tests as problems to investigate.
|
||||
|
||||
Look for:
|
||||
|
||||
* missing awaits;
|
||||
* race conditions;
|
||||
* API completion;
|
||||
* asynchronous UI updates;
|
||||
* WebSocket synchronization;
|
||||
* unstable selectors;
|
||||
* test data collisions;
|
||||
* leaked state;
|
||||
* eventual consistency.
|
||||
|
||||
Do not hide flakes by blindly increasing timeouts or adding sleeps.
|
||||
|
||||
Find the synchronization point.
|
||||
|
||||
## Debugging tests
|
||||
|
||||
When debugging a failing test:
|
||||
|
||||
1. run only the failing test;
|
||||
2. run it with Chromium;
|
||||
3. reproduce the failure;
|
||||
4. inspect whether the problem is in the application or test;
|
||||
5. inspect existing helpers;
|
||||
6. fix the underlying issue;
|
||||
7. rerun the same test;
|
||||
8. optionally run closely related tests.
|
||||
|
||||
Do not rerun the full E2E suite after every modification.
|
||||
|
||||
## UI mode
|
||||
|
||||
The package exposes Playwright UI mode.
|
||||
|
||||
For Chromium:
|
||||
|
||||
```bash
|
||||
yarn test:ui::chromium
|
||||
```
|
||||
|
||||
Use UI mode when it materially helps investigate a failing or flaky test.
|
||||
|
||||
Do not start Firefox or WebKit UI modes.
|
||||
|
||||
## Linting
|
||||
|
||||
The E2E package provides:
|
||||
|
||||
```bash
|
||||
yarn lint
|
||||
```
|
||||
|
||||
Run linting for E2E code when practical.
|
||||
|
||||
Do not run unrelated repository-wide linting solely because an E2E test changed.
|
||||
|
||||
## When modifying an existing test
|
||||
|
||||
Prefer the smallest coherent change.
|
||||
|
||||
Do not reorganize unrelated test scenarios.
|
||||
|
||||
If an existing test naturally covers the new behavior, extend it.
|
||||
|
||||
If adding the behavior would make the scenario confusing or mix unrelated workflows, creating a separate test can be justified.
|
||||
|
||||
## When creating a new test
|
||||
|
||||
A new test should be justified by at least one of:
|
||||
|
||||
* no existing scenario covers the workflow;
|
||||
* extending an existing test would significantly harm readability;
|
||||
* the behavior is an important independent user journey;
|
||||
* independent failure reporting is valuable;
|
||||
* the setup or permissions differ substantially;
|
||||
* it represents a regression that deserves explicit isolated coverage.
|
||||
|
||||
Before creating a new `.spec.ts` file, check whether the test belongs in an existing feature file.
|
||||
|
||||
Prefer:
|
||||
|
||||
```text
|
||||
existing spec + new focused test
|
||||
```
|
||||
|
||||
over:
|
||||
|
||||
```text
|
||||
new spec file
|
||||
```
|
||||
|
||||
unless a new file is genuinely warranted.
|
||||
|
||||
## Avoid over-testing
|
||||
|
||||
Do not repeatedly assert common setup behavior in every scenario.
|
||||
|
||||
If document creation is used as setup, every test that creates a document does not need to fully validate document creation again.
|
||||
|
||||
Focus assertions on the behavior the test is intended to protect.
|
||||
|
||||
## Keep execution efficient
|
||||
|
||||
While writing or reviewing tests, look for:
|
||||
|
||||
* redundant authentication;
|
||||
* redundant document creation;
|
||||
* duplicate navigation;
|
||||
* unnecessary browser contexts;
|
||||
* duplicate scenarios;
|
||||
* unnecessary expensive setup.
|
||||
|
||||
Reuse utilities to remove such duplication.
|
||||
|
||||
Do not optimize by introducing shared mutable state or dependencies between tests.
|
||||
|
||||
Reliability remains more important than saving a few seconds.
|
||||
|
||||
## Before finishing an E2E task
|
||||
|
||||
Review:
|
||||
|
||||
```bash
|
||||
git diff
|
||||
```
|
||||
|
||||
Check that:
|
||||
|
||||
* no unrelated tests were modified;
|
||||
* existing utilities were reused where appropriate;
|
||||
* existing tests were extended where appropriate;
|
||||
* no duplicate scenario was introduced;
|
||||
* no unnecessary new helper was introduced;
|
||||
* selectors are stable;
|
||||
* arbitrary sleeps were not introduced unnecessarily.
|
||||
|
||||
Then run the smallest relevant test on Chromium.
|
||||
|
||||
Prefer:
|
||||
|
||||
```bash
|
||||
yarn test __tests__/app-impress/<relevant-test>.spec.ts \
|
||||
--project=chromium \
|
||||
-g "relevant test"
|
||||
```
|
||||
|
||||
If needed, run the whole relevant spec:
|
||||
|
||||
```bash
|
||||
yarn test __tests__/app-impress/<relevant-test>.spec.ts \
|
||||
--project=chromium
|
||||
```
|
||||
|
||||
Do not run the entire E2E suite solely to report that all tests pass.
|
||||
|
||||
Run the full suite only when:
|
||||
|
||||
* explicitly requested;
|
||||
* shared E2E infrastructure changed significantly;
|
||||
* global setup changed;
|
||||
* a broad validation is specifically justified.
|
||||
|
||||
Even in that case, use Chromium only unless explicitly instructed otherwise.
|
||||
|
||||
Before presenting the result, summarize:
|
||||
|
||||
* which existing tests were reused or modified;
|
||||
* which utilities were reused or changed;
|
||||
* whether a new test was created and why;
|
||||
* which exact tests were executed;
|
||||
* confirmation that Chromium was used;
|
||||
* linting performed;
|
||||
* anything not verified.
|
||||
|
||||
Never claim the complete E2E suite passes unless it was actually executed.
|
||||
|
||||
Never claim Firefox or WebKit compatibility was verified unless explicitly tested.
|
||||
@@ -0,0 +1,397 @@
|
||||
---
|
||||
|
||||
name: docs-pr-review
|
||||
description: Review pull requests and code changes in La Suite Docs. Use when reviewing a PR, branch, diff, or set of changes in the Docs repository. Focus on correctness, regressions, security, permissions, collaborative editing/Yjs behavior, API contracts, database performance, frontend behavior, concurrency, deployment risks, and missing tests. Produce a concise review prioritizing actionable issues over style comments.
|
||||
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
|
||||
|
||||
# Docs PR Review
|
||||
|
||||
Review the current pull request or code changes as a senior engineer familiar with the Docs architecture.
|
||||
|
||||
The goal is to identify real problems that could cause bugs, regressions, security issues, data corruption, performance degradation, deployment failures, or maintainability problems.
|
||||
|
||||
Do not modify the code unless explicitly asked.
|
||||
|
||||
Avoid low-value style comments and subjective preferences.
|
||||
|
||||
## 1. Understand the change
|
||||
|
||||
Before reviewing individual lines:
|
||||
|
||||
1. Determine what the PR is trying to achieve.
|
||||
2. Identify the components affected.
|
||||
3. Inspect the surrounding implementation when necessary.
|
||||
4. Understand the existing architecture before suggesting a different approach.
|
||||
5. Determine whether the change affects:
|
||||
|
||||
* frontend
|
||||
* backend
|
||||
* API contracts
|
||||
* PostgreSQL
|
||||
* permissions
|
||||
* authentication
|
||||
* collaborative editing
|
||||
* Yjs / WebSocket infrastructure
|
||||
* asynchronous processing
|
||||
* storage
|
||||
* deployment/configuration
|
||||
* tests
|
||||
|
||||
When a GitHub PR is available, inspect its title, description, commits, and diff.
|
||||
|
||||
When reviewing local changes without a PR, inspect the branch and diff against the appropriate base branch.
|
||||
|
||||
Do not review the diff in isolation when surrounding code is necessary to determine whether something is actually wrong.
|
||||
|
||||
## 2. Prioritize correctness
|
||||
|
||||
Look for concrete bugs first.
|
||||
|
||||
Check for:
|
||||
|
||||
* incorrect assumptions
|
||||
* broken edge cases
|
||||
* invalid state transitions
|
||||
* race conditions
|
||||
* concurrency problems
|
||||
* missing error handling
|
||||
* incorrect fallback behavior
|
||||
* inconsistent frontend/backend behavior
|
||||
* accidental behavior changes
|
||||
* backwards compatibility issues
|
||||
* partial failures leaving inconsistent state
|
||||
* incorrect handling of null, missing, empty, or unexpected values
|
||||
|
||||
Be particularly cautious when code appears correct for the happy path but can fail under retries, concurrent users, network failures, or stale state.
|
||||
|
||||
## 3. Security and permissions
|
||||
|
||||
Docs is a collaborative application handling user-controlled documents.
|
||||
|
||||
Pay special attention to authorization boundaries.
|
||||
|
||||
Check that:
|
||||
|
||||
* authentication and authorization are distinct where necessary
|
||||
* backend endpoints enforce permissions independently from the frontend
|
||||
* access to a document is checked server-side
|
||||
* organization/workspace/document boundaries cannot be crossed
|
||||
* IDs supplied by users cannot expose another user's resources
|
||||
* new API routes use appropriate permission classes/checks
|
||||
* write operations cannot be performed with read-only access
|
||||
* anonymous/public/share-link behavior is intentional
|
||||
* permission changes cannot leave stale elevated access
|
||||
* sensitive information is not exposed through API responses or logs
|
||||
|
||||
For user-controlled content, also consider:
|
||||
|
||||
* injection
|
||||
* unsafe HTML
|
||||
* URL handling
|
||||
* file handling
|
||||
* SSRF-like behavior
|
||||
* command execution
|
||||
* template injection
|
||||
* unsafe redirects
|
||||
|
||||
Only report a security issue when there is a plausible attack or authorization path. Explain that path.
|
||||
|
||||
## 4. Django / backend
|
||||
|
||||
For backend changes, inspect:
|
||||
|
||||
### API behavior
|
||||
|
||||
* request validation
|
||||
* response shape
|
||||
* HTTP status codes
|
||||
* error behavior
|
||||
* backwards compatibility
|
||||
* serialization/deserialization
|
||||
* permission enforcement
|
||||
|
||||
### Database access
|
||||
|
||||
Look for:
|
||||
|
||||
* N+1 queries
|
||||
* queries inside loops
|
||||
* missing `select_related` / `prefetch_related`
|
||||
* unnecessary queries
|
||||
* inefficient existence/count checks
|
||||
* loading large querysets into memory
|
||||
* missing indexes for new access patterns
|
||||
* expensive filters or sorts
|
||||
* changes that increase transaction duration
|
||||
|
||||
### Transactions and concurrency
|
||||
|
||||
Check:
|
||||
|
||||
* whether related writes should be atomic
|
||||
* race conditions between read and update
|
||||
* uniqueness assumptions
|
||||
* retries
|
||||
* idempotency
|
||||
* locking behavior
|
||||
* duplicate jobs/events
|
||||
|
||||
Pay extra attention to code that can run concurrently across multiple application instances.
|
||||
|
||||
### Migrations
|
||||
|
||||
For migrations, consider:
|
||||
|
||||
* locking
|
||||
* table rewrites
|
||||
* large data migrations
|
||||
* backwards compatibility during rolling deployments
|
||||
* adding non-null columns
|
||||
* indexes created on large tables
|
||||
* application code depending immediately on a migration
|
||||
|
||||
## 5. Collaborative editing / Yjs
|
||||
|
||||
Treat collaborative state as distributed state.
|
||||
|
||||
When changes touch Yjs, collaborative editing, providers, WebSockets, or document synchronization, explicitly reason about:
|
||||
|
||||
* two users editing simultaneously
|
||||
* reconnect after temporary network loss
|
||||
* duplicate messages
|
||||
* reordered messages
|
||||
* client reload
|
||||
* provider restart
|
||||
* multiple provider instances
|
||||
* stale client state
|
||||
* retry behavior
|
||||
* persistence failures
|
||||
* awareness/presence state
|
||||
* document initialization
|
||||
* synchronization between stored document state and live Yjs state
|
||||
|
||||
Look for situations where:
|
||||
|
||||
* an update can be lost
|
||||
* an update can be applied twice incorrectly
|
||||
* server state and client state diverge
|
||||
* initialization overwrites an existing document
|
||||
* reconnection produces stale content
|
||||
* assumptions only hold with one application instance
|
||||
|
||||
Do not flag normal CRDT behavior as a race condition without understanding the Yjs semantics involved.
|
||||
|
||||
## 6. Frontend
|
||||
|
||||
For React / Next.js / TypeScript changes, review:
|
||||
|
||||
### State
|
||||
|
||||
Check for:
|
||||
|
||||
* stale state
|
||||
* stale closures
|
||||
* incorrect effect dependencies
|
||||
* duplicated sources of truth
|
||||
* race conditions between requests
|
||||
* optimistic updates without correct rollback
|
||||
* cache invalidation problems
|
||||
* state updates after navigation/unmount
|
||||
* assumptions about request ordering
|
||||
|
||||
### TanStack Query
|
||||
|
||||
When applicable, inspect:
|
||||
|
||||
* query keys
|
||||
* invalidation
|
||||
* enabled conditions
|
||||
* stale/cache behavior
|
||||
* optimistic mutations
|
||||
* rollback
|
||||
* duplicate requests
|
||||
* server/client consistency
|
||||
|
||||
### React
|
||||
|
||||
Look for:
|
||||
|
||||
* unnecessary effects
|
||||
* effects used to derive state
|
||||
* unstable dependencies
|
||||
* remounts caused by unstable keys
|
||||
* unnecessary rerenders when they are significant
|
||||
* broken controlled/uncontrolled behavior
|
||||
* incorrect memoization assumptions
|
||||
|
||||
Do not recommend memoization without a concrete reason.
|
||||
|
||||
### User experience
|
||||
|
||||
Consider:
|
||||
|
||||
* loading states
|
||||
* empty states
|
||||
* error states
|
||||
* disabled states
|
||||
* permissions changing while the page is open
|
||||
* keyboard interaction where relevant
|
||||
* accessibility regressions
|
||||
* mobile/responsive regressions when relevant
|
||||
|
||||
## 7. API contract changes
|
||||
|
||||
When frontend and backend communicate through modified APIs:
|
||||
|
||||
Verify both sides agree about:
|
||||
|
||||
* field names
|
||||
* optional vs required fields
|
||||
* nullability
|
||||
* enums
|
||||
* error responses
|
||||
* pagination
|
||||
* status codes
|
||||
* defaults
|
||||
* backwards compatibility
|
||||
|
||||
Be alert to deployment situations where old frontend code can temporarily communicate with new backend code or vice versa.
|
||||
|
||||
## 8. Performance
|
||||
|
||||
Only report performance issues that have a realistic impact.
|
||||
|
||||
Look especially for:
|
||||
|
||||
* DB queries scaling with document count or user count
|
||||
* repeated API calls
|
||||
* expensive work performed on every render/request
|
||||
* serialization of unnecessarily large payloads
|
||||
* loading entire documents when partial information is sufficient
|
||||
* WebSocket message amplification
|
||||
* repeated collaborative-state conversions
|
||||
* synchronous work added to latency-sensitive paths
|
||||
* unbounded loops, collections, caches, or queues
|
||||
|
||||
Distinguish theoretical micro-optimizations from issues that can matter in production.
|
||||
|
||||
## 9. Configuration and deployment
|
||||
|
||||
For Docker, Helm, Kubernetes, environment variables, CI/CD, or deployment-related changes, check:
|
||||
|
||||
* safe defaults
|
||||
* missing environment variables
|
||||
* configuration differences between environments
|
||||
* backwards compatibility during rolling deployments
|
||||
* readiness/liveness behavior
|
||||
* startup failures
|
||||
* resource assumptions
|
||||
* service discovery
|
||||
* secrets handling
|
||||
* feature flags
|
||||
* rollback behavior
|
||||
|
||||
For feature flags:
|
||||
|
||||
* disabled behavior should preserve existing behavior
|
||||
* frontend and backend flags should remain compatible
|
||||
* partially rolled-out deployments should be considered
|
||||
|
||||
## 10. Tests
|
||||
|
||||
Determine whether the change is adequately tested.
|
||||
|
||||
Look for missing tests around:
|
||||
|
||||
* newly introduced behavior
|
||||
* bug regressions
|
||||
* permissions
|
||||
* failure paths
|
||||
* concurrency-sensitive behavior
|
||||
* API contracts
|
||||
* feature flags
|
||||
* migrations
|
||||
* collaborative editing behavior
|
||||
|
||||
Do not request tests merely for coverage.
|
||||
|
||||
Explain what behavior should be tested and what regression the test would prevent.
|
||||
|
||||
## 11. Avoid false positives
|
||||
|
||||
Before reporting an issue:
|
||||
|
||||
1. Inspect the relevant surrounding code.
|
||||
2. Check whether another layer already handles it.
|
||||
3. Confirm that the problematic execution path is actually reachable.
|
||||
4. Consider framework behavior before assuming custom handling is required.
|
||||
5. Distinguish a bug from a possible alternative design.
|
||||
|
||||
Do not report an issue solely because you would have implemented it differently.
|
||||
|
||||
Do not suggest broad refactors unrelated to the PR.
|
||||
|
||||
Do not comment on formatting or naming unless it creates ambiguity or a maintainability problem.
|
||||
|
||||
## 12. Review output
|
||||
|
||||
Start with a short summary of what the change does.
|
||||
|
||||
Then report findings ordered by severity.
|
||||
|
||||
Use these severities:
|
||||
|
||||
### Blocking
|
||||
|
||||
A problem likely to cause:
|
||||
|
||||
* security vulnerability
|
||||
* data loss or corruption
|
||||
* major production outage
|
||||
* fundamentally incorrect behavior
|
||||
|
||||
### Important
|
||||
|
||||
A concrete bug or regression worth fixing before merge.
|
||||
|
||||
### Minor
|
||||
|
||||
A real but lower-impact issue.
|
||||
|
||||
For each finding provide:
|
||||
|
||||
**[severity] Short title**
|
||||
|
||||
* Location: `path/to/file.ext:line`
|
||||
* Problem: explain what is wrong.
|
||||
* Scenario: describe how the problem can actually happen.
|
||||
* Impact: describe the consequence.
|
||||
* Suggested fix: give a concise direction for fixing it.
|
||||
|
||||
Whenever possible, point to exact files and lines.
|
||||
|
||||
If there are no meaningful issues, say explicitly:
|
||||
|
||||
> No blocking or important issue found.
|
||||
|
||||
Then optionally mention areas that deserve manual verification.
|
||||
|
||||
## Review principles
|
||||
|
||||
A good review should have a high signal-to-noise ratio.
|
||||
|
||||
Prefer finding one real production bug over writing ten speculative comments.
|
||||
|
||||
Prioritize:
|
||||
|
||||
1. security and authorization
|
||||
2. data integrity
|
||||
3. correctness
|
||||
4. concurrency and collaboration
|
||||
5. production regressions
|
||||
6. API compatibility
|
||||
7. performance
|
||||
8. tests
|
||||
9. maintainability
|
||||
|
||||
Be concise, specific, and actionable.
|
||||
+4
-1
@@ -87,7 +87,6 @@ db.sqlite3
|
||||
# AI
|
||||
CLAUDE.md
|
||||
!AGENTS.md
|
||||
.claude/
|
||||
.cursor/
|
||||
.cursorrules
|
||||
.windsurfrules
|
||||
@@ -95,3 +94,7 @@ CLAUDE.md
|
||||
.copilot/
|
||||
.github/copilot-instructions.md
|
||||
.playwright-mcp
|
||||
# AI Claude
|
||||
.claude/*
|
||||
!.claude/skills/
|
||||
.claude/skills/local-*/
|
||||
@@ -66,6 +66,7 @@ and this project adheres to
|
||||
offline
|
||||
- 🐛(frontend) stop the service worker from caching the collaboration server's
|
||||
rest api
|
||||
- 🤖 Add Claude skills #2746
|
||||
|
||||
### Changed
|
||||
|
||||
|
||||
Reference in New Issue
Block a user