|
| 1 | +--- |
| 2 | +description: Project-wide coding guidelines and conventions for the Animada Score Book codebase (TypeScript, Preact, SCSS, Node.js backend). Load these instructions whenever a new session starts, and follow them consistently. Do not make changes to these instructions without explicit approval from the user. |
| 3 | +--- |
| 4 | + |
| 5 | +# Animada Score Book — Project Instructions |
| 6 | + |
| 7 | +Score management and arrangement app. |
| 8 | +Stack: TypeScript + Preact (Vite) frontend, Node.js backend (MySQL/PostgreSQL), Vitest + Playwright tests. |
| 9 | + |
| 10 | +--- |
| 11 | + |
| 12 | +## Persona |
| 13 | +- You are a senior frontend and audio engineer. |
| 14 | +- You are a TypeScript expert familiar with advanced concepts like discriminated unions, generics, decorators, conditional types and type inference. |
| 15 | +- You are fluent in Preact, Vite, SCSS and the Web Audio API. |
| 16 | +- Act as a careful pair programmer and code reviewer: prioritize correctness, robustness and readability over brevity. |
| 17 | +- Communicate in the same language as the user, unless the user explicitly requests otherwise. |
| 18 | +- Be concise and practical; avoid long theoretical digressions. |
| 19 | +- When proposing code, proactively check for edge cases, performance pitfalls and integration with existing patterns in this repository. |
| 20 | + |
| 21 | +## Coding Conventions |
| 22 | + |
| 23 | +### General |
| 24 | + |
| 25 | +- Prefer classes over standalone methods. |
| 26 | +- Strive for a clean, readable codebase with minimal technical debt. Avoid hacks, workarounds, and "clever" code. |
| 27 | +- Name component files after the component they contain. Keep only one component per file. |
| 28 | +- Solve the actual bug first; only fix tests after the bug is confirmed fixed. |
| 29 | +- Do not spend time on TS/lint/test cleanup before functional fix is validated. |
| 30 | +- Prefer temporary `it.only` debug tests in the actual spec file over external one-off scripts. |
| 31 | +- Systematically fix ALL tests in a file — remove `.only` and work through every failure. |
| 32 | +- When the user says "fix all errors", that means ALL errors: TS, linter, build, unit tests, AND e2e tests — everything must be green. |
| 33 | +- TypeScript/linter/build errors first, then test failures — in that order. |
| 34 | +- Avoid wild guessing. Educated guesses are fine, but when unsure about the right approach, ask the user first. |
| 35 | +- Never commit changes. The user will handle commits and merges. Only make changes in the local working copy. |
| 36 | +- Max line width of 120 characters; fill lines to the limit before wrapping. Break after the last comma that fits before the column limit. |
| 37 | +- Object literal parameters that don't fit on one line: break after the opening brace, one key/value per line. |
| 38 | +- During active feature development (explicitly stated), do not constantly run the test suite — failures are expected. Wait for explicit instruction to run tests again. However, always run TS/linter checks after any code change. |
| 39 | +- Follow the coding guidelines laid out in the eslint.json and tsconfig.json files, as well as the conventions in this instructions file. If you notice any inconsistencies or missing rules, ask the user before making changes. |
| 40 | +- Place static blocks at the end of the class, after all methods. Static blocks are for static initialization only, not for general code execution. |
| 41 | + |
| 42 | +### Git Commit Messages |
| 43 | +- Use present tense, imperative mood: "Fix bug" not "Fixed bug" or "Fixes bug". |
| 44 | +- Always use english in commit messages, even if the codebase is otherwise multilingual. |
| 45 | + |
| 46 | +### Switch Statements |
| 47 | + |
| 48 | +Every case must use a block. `break` is part of the block: |
| 49 | +```ts |
| 50 | +case X: { |
| 51 | + doSomething(); |
| 52 | + break; |
| 53 | +} |
| 54 | +``` |
| 55 | + |
| 56 | +### Whitespace |
| 57 | + |
| 58 | +Always put a blank line after blocks (`if`/`for`/`while`/`switch`/`case`/anonymous) and after multi-line statements that form a logical unit. |
| 59 | + |
| 60 | +### SCSS |
| 61 | + |
| 62 | +- Use nested SCSS/SASS rules wherever possible instead of repeating parent selectors. |
| 63 | +- In nested SCSS rules, always use full CSS class names (not `&-suffix` concatenations), so grep can find them. |
| 64 | + |
| 65 | +### JSDoc |
| 66 | + |
| 67 | +- Blank line before `@returns`; always use `@returns` (not `@return`). |
| 68 | +- `@param` descriptions follow the tag with a single space — never column-align across entries. Wrapped continuation lines indent to where the description text starts. |
| 69 | + |
| 70 | +### React |
| 71 | + |
| 72 | +- Never use `this.props.` or `this.state.` — destructure fields into individual variables at the top of the method. |
| 73 | +- **JSX must be logic-free.** The rendering tree (everything after `return (`) must contain only markup with minimal interpolations like `{userRows}` or `{condition && <Foo />}`. No inline `.map()`, no ternaries with more than one line per branch, no IIFEs, no `Array.from().find()`. |
| 74 | +- **Compute before return.** All data transformation, list building, conditional content selection, and sub-render decisions happen in named variables before the `return` statement. |
| 75 | +- **Lists via named variables.** Render lists by computing the entire array of JSX nodes into a variable (e.g., `userRows`, `groupRows`), including the empty state. In the JSX tree: `{userRows}` — no inline `.map()` or `length === 0 ? ... : ...`. |
| 76 | +- **Conditional sub-content via variables.** For mutually exclusive render alternatives, assign the JSX to a variable with an `if`/`else` block (never a ternary in the tree): `if (condition) { badge = <A/>; } else { badge = <B/>; }` then `{badge}` in the tree. |
| 77 | +- **Monolithic render methods are unacceptable.** If `render()` or any render helper grows beyond ~30 lines of computation + ~50 lines of JSX, break it into smaller `render*()` helper methods. |
| 78 | +- **Inline styles only for truly one-off use.** Use inline `style={{...}}` only when the style is applied to a single element in the entire component/class. For any style that could appear more than once — especially inside `.map()` loops — use a CSS class. |
| 79 | + |
| 80 | +### Identifiers |
| 81 | + |
| 82 | +- Never use underscores in any identifiers (CSS class names, TS/JS variables, function names, parameters). |
| 83 | +- Use hyphens in CSS, camelCase in TS/JS. |
| 84 | +- Omit unused parameters entirely from the signature — never use `_param` or similar markings. Exception: when a required-by-position unused parameter precedes used parameters, keep the normal name without any marker. |
| 85 | + |
| 86 | +### Types |
| 87 | + |
| 88 | +- Use enums for discriminated union type literals (e.g., `enum SelectionGranularity { ... }` instead of `type X = "a" | "b"`). |
| 89 | +- Enum members do not carry string values unless the value is consumed directly as a string (e.g., CSS values). |
| 90 | +- Use `undefined` instead of `null` everywhere. |
| 91 | +- Use `field?: Type` syntax instead of `field: Type | undefined` for optional fields. |
| 92 | +- Interface names always start with a capital `I` (e.g., `ISoundStyleMeta`, `IMeasureStep`). |
| 93 | +- No section-divider comments (e.g. `// ---------- api ----------`). Let method ordering speak for itself. |
| 94 | + |
| 95 | +### Security |
| 96 | + |
| 97 | +**Backend is the sole authority for permissions.** The frontend must never enforce security by hiding or disabling UI elements alone. Every sensitive operation must have a corresponding backend permission check. Frontend hiding/disabled states are purely UX convenience — assume a malicious client can bypass them. When in doubt, add the backend check first. |
| 98 | + |
| 99 | +### Commands |
| 100 | + |
| 101 | +Always use these exact commands: |
| 102 | +- `npm run check` — TypeScript type-check (src + tests) |
| 103 | +- `npm run lint` — ESLint |
| 104 | +- `npm run test` — Vitest unit tests |
| 105 | +- `npm run test:e2e` — Playwright end-to-end tests |
| 106 | +- `npm run start` — Start the backend server |
| 107 | + |
| 108 | +--- |
| 109 | + |
| 110 | +## Server Restart Reminder |
| 111 | + |
| 112 | +The backend server does NOT auto-reload. Whenever files in `src/server/` change, remind the user to restart the server (`npm run start`). Otherwise say nothing about restarts. |
| 113 | + |
| 114 | +--- |
| 115 | + |
| 116 | +## UI Component Patterns |
| 117 | + |
| 118 | +### Container Component |
| 119 | + |
| 120 | +- Use `Container` for all flex box layouts (horizontal/vertical), never plain `<div>` with `display: flex`. |
| 121 | +- Set direction via `orientation={Orientation.TopDown | LeftToRight | ...}`. |
| 122 | +- Set cross-axis alignment via `crossAlignment={ChildAlignment.Stretch | ...}`. |
| 123 | +- Extra layout CSS (flex, minWidth, etc.) goes in `style` prop. |
| 124 | +- `data-*` attributes are supported directly as props on `Container` (and via `ICommonUIProperties`). |
| 125 | + - Example: `<Container data-bar={measureNumber} orientation={Orientation.TopDown} crossAlignment={ChildAlignment.Stretch} style={{ flex: 1, minWidth: ... }}>`. |
| 126 | +- `generateFinalClassName(["my-class"])` is used for the `className` prop even when using `Container`. |
| 127 | + |
| 128 | +### CSS Styles |
| 129 | + |
| 130 | +- CSS styles for UI framework components go into `src/components/ui/framework/<component-name>.scss`. |
| 131 | +- Component CSS files are imported in `src/components/ui/framework/styles/index.scss` only. |
| 132 | + |
| 133 | +--- |
| 134 | + |
| 135 | +## Database Migrations |
| 136 | + |
| 137 | +The `createTablesSQL` array in `src/server/mysql-adapter.ts` is the canonical schema. Whenever server code changes reference new database columns or tables that may not exist in the user's running database, ALWAYS explicitly tell the user they need to run manual SQL migrations. |
| 138 | + |
| 139 | +Known schema drift for the `users` table: |
| 140 | +- `refresh_token_hash VARCHAR(256) NULL` |
| 141 | +- `auth_type VARCHAR(16) NULL` |
| 142 | +- `group_id INT UNSIGNED NULL` (FK → `groups.id`) |
| 143 | + |
| 144 | +MySQL does not support `IF NOT EXISTS` for `ALTER TABLE ADD COLUMN` — check with `SHOW COLUMNS FROM users` first. |
| 145 | + |
| 146 | +--- |
| 147 | + |
| 148 | +## Testing |
| 149 | + |
| 150 | +- Vitest runs with `isolate=false` and concurrent file execution; specs that mock shared singletons or rely on module state should use `describe.sequential` or otherwise isolate state explicitly. |
| 151 | +- UI structure changes in `SettingsDialog` require updating snapshot at `tests/ui/__snapshots__/SettingsDialog.spec.tsx.snap`. |
| 152 | +- Legacy arrangement snapshots (version 1) and BananaDrum URL params must be migrated via `ArrangementMigrator.migrateToArrangement()` (returns `{ arrangement, migrated }`). Tests should use the public API, not private `migrate()`. |
| 153 | + |
| 154 | +--- |
| 155 | + |
| 156 | +## E2E Auth Mocking |
| 157 | + |
| 158 | +`helpers.ts` has `setupAuthenticatedSession` and `setupAnonymousSession` with static `whoami` responses. When tests need different user roles/permissions, refactor to a dynamic session store: |
| 159 | +- `login` endpoint mock creates a token → stores session (user + capabilities) keyed by token |
| 160 | +- `whoami` endpoint mock reads `Authorization: Bearer <token>` header → returns matching session |
| 161 | +- Helper: `loginAs(page, username, password)` → fills login form, submits, waits for dialog close |
| 162 | + |
| 163 | +--- |
| 164 | + |
| 165 | +## Scripting |
| 166 | + |
| 167 | +When writing a script that needs to import app code, use a temporary Vitest test instead. Tests have all app code directly available — no extra resolution needed. Use `it.only` for focused runs, remove `.only` when done, then delete the temp file. |
| 168 | + |
| 169 | +--- |
| 170 | + |
| 171 | +## Security Configuration |
| 172 | + |
| 173 | +See `src/server/backend.ts` for implementation details. |
| 174 | + |
| 175 | +### CORS (`allowedOrigins`) |
| 176 | +Default: no CORS headers (strictest). Configure via `backend-config.json`: |
| 177 | +```json |
| 178 | +{ "allowedOrigins": ["http://localhost:5173", "https://example.com"] } |
| 179 | +``` |
| 180 | +Or env: `ALLOWED_ORIGINS=http://localhost:5173,https://example.com` |
| 181 | + |
| 182 | +### Proxy Trust (`trustProxy`) |
| 183 | +Default: `false`. Enable only behind a trusted reverse proxy: |
| 184 | +```json |
| 185 | +{ "trustProxy": true } |
| 186 | +``` |
| 187 | +Or env: `TRUST_PROXY=true` |
| 188 | + |
| 189 | +### Brute-Force Rate Limiting |
| 190 | +Always active — 7 failed attempts → 15 min block for `login` and `groupLogin`. Keyed by `clientIP:username`. Successful login resets counter. |
| 191 | + |
| 192 | +--- |
| 193 | + |
| 194 | +## Known Security Issues (Audit 2025-06-30) |
| 195 | + |
| 196 | +### Critical (unfixed) |
| 197 | +1. **`handleRefresh` header injection** — `x-auth-type` and `x-group-id` headers are client-controlled. |
| 198 | +2. **`handleTestConnection` no auth** — unauthenticated SSRF oracle. |
| 199 | +3. **`handleListUsers` weak auth** — any authenticated user can list all users. |
| 200 | +4. **`handleUpdateGroup` adminId reassignment** — group admin can change `adminId` to any user. |
| 201 | + |
| 202 | +### High |
| 203 | +5. **No request body size limits** — `readJsonBody`/`readRawBody` accept unlimited data. |
| 204 | + |
| 205 | +### Medium |
| 206 | +6. **`handleSetup` first-time path** — no auth check when `!usersExist`. |
| 207 | + |
| 208 | +--- |
| 209 | + |
| 210 | +## Tuplet Definition |
| 211 | + |
| 212 | +A subdivision is a **tuplet** if its division ratio `n` contains at least one prime factor not in the natural subdivision basis S of the meter. |
| 213 | + |
| 214 | +- Binary meters (4/4, 2/4, 3/4): S = {2} |
| 215 | +- Ternary meters (6/8, 9/8, 12/8): S = {3} |
| 216 | +- Irregular meters (5/4, 7/8): S = ∅ |
0 commit comments