-
Notifications
You must be signed in to change notification settings - Fork 23
Homework 2: Rules #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
TiSer
wants to merge
5
commits into
koldovsky:master
Choose a base branch
from
TiSer:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| # Start dev server | ||
|
|
||
| Run the Excalidraw web app locally for interactive development. | ||
|
|
||
| ## Do this | ||
|
|
||
| 1. Use the repository root as the working directory (this monorepo uses Yarn workspaces). | ||
| 2. Ensure dependencies are installed: `yarn install` (Node **>= 18** per `package.json`). | ||
| 3. Start the Vite dev server: | ||
|
|
||
| ```bash | ||
| yarn start | ||
| ``` | ||
|
|
||
| This runs `yarn --cwd ./excalidraw-app start`. | ||
|
|
||
| ## When packages need a rebuild | ||
|
|
||
| If you changed low-level packages and see resolution or build issues, build workspace packages first, then start: | ||
|
|
||
| ```bash | ||
| yarn build:packages | ||
| yarn start | ||
| ``` | ||
|
|
||
| ## Orientation | ||
|
|
||
| - Host app: `excalidraw-app/` | ||
| - Core editor package: `packages/excalidraw/` | ||
| - Other packages: `packages/common`, `packages/element`, `packages/math` | ||
|
|
||
| Respect project rules: canvas rendering and `actionManager` state (see `.cursor/rules/project-architecture.mdc`). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| # Test watch (Vitest) | ||
|
|
||
| Run unit tests in watch mode while editing—default workflow for `packages/*` and app code covered by Vitest. | ||
|
|
||
| ## Do this | ||
|
|
||
| From the repository root: | ||
|
|
||
| ```bash | ||
| yarn test | ||
| ``` | ||
|
|
||
| This runs `vitest` (watch mode is the typical default for local iteration). | ||
|
|
||
| ## Useful variants | ||
|
|
||
| **Single run (no watch), e.g. for scripts or CI-like local checks:** | ||
|
|
||
| ```bash | ||
| yarn test:app --watch=false | ||
| ``` | ||
|
|
||
| **Update snapshots** (only when you intentionally changed expected output): | ||
|
|
||
| ```bash | ||
| yarn test:update | ||
| ``` | ||
|
|
||
| **Coverage:** | ||
|
|
||
| ```bash | ||
| yarn test:coverage | ||
| ``` | ||
|
|
||
| **Vitest UI:** | ||
|
|
||
| ```bash | ||
| yarn test:ui | ||
| ``` | ||
|
|
||
| ## Notes | ||
|
|
||
| - Config: `vitest.config.mts` (aliases for `@excalidraw/*` packages). | ||
| - For the **full** gate including typecheck, ESLint, and Prettier, use `/verify-ci` instead of only Vitest. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| # Verify like CI (full check) | ||
|
|
||
| Run the full pre-merge validation suite before a PR or after large refactors. | ||
|
|
||
| ## Do this | ||
|
|
||
| From the repository root: | ||
|
|
||
| ```bash | ||
| yarn test:all | ||
| ``` | ||
|
|
||
| This script runs, in order: | ||
|
|
||
| | Step | Script | What it runs | | ||
| |------|--------|----------------| | ||
| | 1 | `yarn test:typecheck` | `tsc` | | ||
| | 2 | `yarn test:code` | ESLint (`.js`, `.ts`, `.tsx`, max warnings 0) | | ||
| | 3 | `yarn test:other` | Prettier `--list-different` on configured globs | | ||
| | 4 | `yarn test:app --watch=false` | Vitest once (no watch) | | ||
|
|
||
| ## If something fails | ||
|
|
||
| - **Typecheck**: fix TypeScript errors; re-run `yarn test:typecheck` for a fast loop. | ||
| - **ESLint**: auto-fix where safe: `yarn fix:code`, then re-run `yarn test:code`. | ||
| - **Prettier**: `yarn fix:other`, then re-run `yarn test:other`. | ||
| - **Vitest**: run a single file or pattern with Vitest’s CLI options if needed. | ||
|
|
||
| ## One-shot “fix formatting + lint” (optional) | ||
|
|
||
| ```bash | ||
| yarn fix | ||
| ``` | ||
|
|
||
| Then run `yarn test:all` again to confirm. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "mcpServers": { | ||
| "context7": { | ||
| "url": "https://mcp.context7.com/mcp" | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| --- | ||
| description: "Code conventions for components and utilities" | ||
| globs: "packages/**/*.ts,packages/**/*.tsx" | ||
| alwaysApply: false | ||
| --- | ||
| # Code Conventions | ||
| ## Components | ||
| - Functional components + hooks ONLY (no class components) | ||
| - Props type alias: `{ComponentName}Props` | ||
| - Named exports only (no default exports) | ||
| - Colocated tests: `ComponentName.test.tsx` | ||
| ## TypeScript | ||
| - Strict mode — no `any`, no `@ts-ignore` | ||
| - Prefer `type` over `interface` for simple and props object types | ||
| - Import types: `import type { X } from "..."` | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ## Excalidraw-specific | ||
| - Prefer `@excalidraw/*` package imports (or direct relative module imports) over barrel shortcuts that break package boundaries. | ||
| - Do not import `jotai` directly in package code; use project-specific modules (`editor-jotai` / `app-jotai`) where applicable. | ||
|
|
||
| ## How to verify | ||
|
|
||
| - `yarn test:typecheck` — TypeScript must pass (no `@ts-ignore` workarounds). | ||
| - `yarn test:code` — ESLint must pass with `--max-warnings=0` (includes restricted-import checks). | ||
| - `rg "^\s*export\s+default\s+" packages --glob "*.{ts,tsx}"` — should return no new matches in changed files. | ||
| - `yarn test:other` — Prettier check on configured globs. | ||
| - `yarn test:app --watch=false` — tests for touched components/utilities. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| --- | ||
| description: "Import/export, restore, and file-format compatibility" | ||
| globs: "packages/excalidraw/data/**" | ||
| alwaysApply: false | ||
| --- | ||
| # Data layer (`packages/excalidraw/data`) | ||
|
|
||
| ## Compatibility | ||
| - `.excalidraw` and library formats are **user data contracts**. Prefer extending with versioned migration steps instead of breaking existing files. | ||
| - `restore.ts` and related flows are especially sensitive — coordinate with `.cursor/rules/do-not-touch.mdc` before large edits. | ||
|
|
||
| ## Validation | ||
| - Validate incoming data shape and reject or normalize unknown fields rather than assuming trust. | ||
|
|
||
| ## Side effects | ||
| - File IO, encryption, and backend calls must preserve existing privacy and error semantics; do not silently drop user content. | ||
|
|
||
| ## Testing | ||
| - Add or update tests when changing serialization, migration, or restore behavior; run `yarn test:all` before merge. | ||
|
|
||
| ## How to verify | ||
|
|
||
| - `yarn test:all` — typecheck, lint, prettier, and Vitest (including data/restore tests). | ||
| - `yarn test:app --watch=false` — focused pass if you only touched tests under `packages/excalidraw/data/`. | ||
| - Manual: load old `.excalidraw` samples, export/re-import, and open library flows if library/JSON handling changed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| --- | ||
| description: "Protected files that should not be modified" | ||
| alwaysApply: true | ||
| --- | ||
| # Protected Files | ||
| NEVER modify these files without explicit approval: | ||
| - `packages/excalidraw/scene/Renderer.ts` — render pipeline | ||
| - `packages/excalidraw/data/restore.ts` — file format compatibility | ||
| - `packages/excalidraw/actions/manager.tsx` — action dispatch system | ||
| - `packages/excalidraw/types.ts` — core type definitions | ||
| Changes to protected files require: | ||
| 1. Full understanding of dependencies | ||
| 2. Running complete test suite | ||
| 3. Manual QA verification | ||
|
|
||
| ## How to verify | ||
|
|
||
| - `yarn test:all` — must pass before any change to the listed paths. | ||
| - After edits to `Renderer.ts` or `types.ts`: run `yarn start` and manually smoke-test canvas rendering, selection, and export. | ||
| - After edits to `restore.ts`: import/export `.excalidraw` files and library JSON; confirm round-trip and backward compatibility. | ||
| - After edits to `manager.tsx`: exercise menus, shortcuts, and actions that flow through `ActionManager` (e.g. load/save, theme, zoom). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| --- | ||
| description: "Host app (Vite shell) — not the core editor package" | ||
| globs: "excalidraw-app/**" | ||
| alwaysApply: false | ||
| --- | ||
| # excalidraw-app (host application) | ||
|
|
||
| ## Role | ||
| - This package is the **deployed web shell** (Vite, PWA, collaboration hooks, Firebase, etc.). Core drawing logic lives in `packages/excalidraw/` — avoid duplicating editor behavior here. | ||
|
|
||
| ## State | ||
| - App-level UI may use local patterns (e.g. Jotai in this repo). Do **not** introduce Redux/Zustand for editor state; editor state stays in Excalidraw’s `actionManager` / app state (`packages/excalidraw`). | ||
|
|
||
| ## Build and run | ||
| - Dev server: `yarn start` from monorepo root (runs this package’s `start` script). | ||
| - Prefer `yarn --cwd excalidraw-app` only when docs/scripts already use that form. | ||
|
|
||
| ## Networking | ||
| - Socket / collaboration / storage integrations: preserve error handling, reconnection, and privacy assumptions; do not weaken E2E or room behavior without explicit product review. | ||
|
|
||
| ## Dependencies | ||
| - No new npm packages without explicit approval (see root `package.json` / team process). | ||
|
|
||
| ## How to verify | ||
|
|
||
| - `yarn start` — dev server loads; exercise collaboration/PWA features if relevant to your change. | ||
| - `yarn build` or `yarn build:app` — production build succeeds when your change affects bundling or env. | ||
| - `yarn test:all` — full repo checks before PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| --- | ||
| description: "Translations and locale JSON files" | ||
| globs: "packages/excalidraw/locales/**" | ||
| alwaysApply: false | ||
| --- | ||
| # i18n (locales) | ||
|
|
||
| ## Files | ||
| - Locale data lives in `packages/excalidraw/locales/*.json`. **`en.json` is the source of truth** for keys; other locales follow the same key structure. | ||
|
|
||
| ## Editing rules | ||
| - Add or change **English strings in `en.json` first**, then mirror keys in other locale files only when the workflow expects it (often Crowdin syncs translations — do not bulk-edit all languages by hand without process). | ||
| - Preserve JSON validity (trailing commas are invalid). Run format check: `yarn test:other` (Prettier) if unsure. | ||
|
|
||
| ## Code usage | ||
| - Use the project’s existing i18n helpers / `t(...)` patterns from application code — do not concatenate translated strings in a way that breaks grammar for RTL or plural languages when avoidable. | ||
|
|
||
| ## Coverage | ||
| - For key churn, `locales-coverage` scripts exist at repo root (`yarn locales-coverage`) — use when required by contribution guidelines. | ||
|
|
||
| ## How to verify | ||
|
|
||
| - `yarn test:other` — Prettier check includes locale JSON; invalid JSON will fail earlier or in tooling. | ||
| - `yarn locales-coverage` — when adding or renaming keys (per contribution guidelines). | ||
| - `yarn start` — smoke-test UI paths that use new/changed strings (language switch if applicable). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| --- | ||
| description: "Geometry and math packages — keep pure and dependency-light" | ||
| globs: "packages/element/**,packages/math/**" | ||
| alwaysApply: false | ||
| --- | ||
| # packages/element and packages/math | ||
|
|
||
| ## Purpose | ||
| - **`packages/math`**: numeric types, points, vectors — keep **framework-agnostic** (no React, no DOM APIs). | ||
| - **`packages/element`**: element model, bindings, geometry helpers — consumed by the editor; avoid circular imports and keep public APIs stable. | ||
|
|
||
| ## Performance | ||
| - Hot paths run frequently during pointer moves and rendering. Prefer small allocations, avoid unnecessary object churn, and match existing patterns for immutability vs mutation. | ||
|
|
||
| ## Testing | ||
| - Unit-test math and geometry edge cases (degenerate shapes, zero length, NaN guards) when changing behavior. | ||
|
|
||
| ## Dependencies | ||
| - Do not pull `react`, `excalidraw-app`, or heavy UI libraries into these packages. No new npm deps without approval. | ||
|
|
||
| ## How to verify | ||
|
|
||
| - `yarn test:typecheck` — `tsc` must pass for all workspace references. | ||
| - `yarn test:app --watch=false` — run Vitest after changing tests or behavior. | ||
| - `yarn build:math && yarn build:element` — ESM builds succeed when public exports or entrypoints change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| --- | ||
| description: "Architecture constraints for the project" | ||
| globs: "packages/**" | ||
| alwaysApply: false | ||
| --- | ||
|
|
||
| # Project Architecture | ||
|
|
||
| ## State Management | ||
| - Custom state via actionManager — NOT Redux/Zustand/MobX | ||
| - State updates: use `actionManager.executeAction(...)` (and related `ActionManager` APIs) — not a `dispatch()` method; `ActionManager` is defined in `packages/excalidraw/actions/manager.tsx` | ||
| - State type: AppState (`packages/excalidraw/types.ts`) | ||
|
|
||
| ## Rendering | ||
| - Canvas 2D rendering — NOT React DOM for drawing | ||
| - Render pipeline: Scene → renderScene() → canvas context | ||
| - DO NOT use react-konva, fabric.js, pixi.js | ||
|
|
||
| ## Dependencies | ||
| - No new npm packages without explicit approval | ||
|
|
||
| ## How to verify | ||
|
|
||
| - `yarn test:all` — full gate (typecheck, ESLint, Prettier, Vitest once). | ||
| - `yarn start` — smoke-test the editor: drawing, undo/redo, export; confirm canvas-based (not DOM-rendered scene). | ||
| - Search for forbidden stacks: no new `redux` / `zustand` / `mobx` / `react-konva` / `fabric` / `pixi` imports in `packages/*` unless explicitly approved. | ||
| - Confirm action API: `ActionManager` exposes `executeAction` in `packages/excalidraw/actions/manager.tsx` — there is no `actionManager.dispatch` in this repo (`rg 'actionManager\\.dispatch' packages/excalidraw` should be empty). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| --- | ||
| description: "Security-sensitive patterns for app and editor code" | ||
| globs: "excalidraw-app/**,packages/excalidraw/**" | ||
| alwaysApply: false | ||
| --- | ||
| # Security | ||
|
|
||
| ## Secrets and config | ||
| - Never commit API keys, Firebase config secrets, or private tokens. Use env vars / existing `.env.*` patterns and deployment configuration — not hardcoded strings in source. | ||
| - Do not log collaboration payloads, room IDs, or user identifiers to `console` in production paths. | ||
|
|
||
| ## Untrusted data | ||
| - Treat imported `.excalidraw` JSON, URL params, clipboard payloads, and collaborator messages as **untrusted**. Validate shape and size before use; prefer existing restore/validation paths in `packages/excalidraw/data/`. | ||
| - Avoid `eval`, `new Function`, and dynamic script injection. If embedding or HTML is required, follow existing embeddable / iframe patterns only. | ||
|
|
||
| ## Web and DOM | ||
| - Avoid `dangerouslySetInnerHTML` unless it matches an existing audited pattern; never interpolate raw user strings into HTML. | ||
| - For `postMessage` / cross-origin flows, validate `event.origin` and message shape (follow existing collaboration / embed code). | ||
|
|
||
| ## Collaboration and Firebase | ||
| - Do not bypass or weaken end-to-end collaboration encryption behavior without explicit security review. | ||
| - Do not change Firebase security rules/policies without explicit review and documented rationale. | ||
|
|
||
| ## Dependencies | ||
| - No new dependencies for parsing or crypto without explicit review (`package.json` / team approval). | ||
|
|
||
TiSer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ## How to verify | ||
|
|
||
| - `yarn test:all` — ensure no regressions in data/import flows and app code. | ||
| - `yarn test:code` — ESLint may flag risky patterns depending on rules; fix or justify. | ||
| - Manual review: search changed files for `eval`, `new Function`, `dangerouslySetInnerHTML`, and raw `postMessage` handlers without origin checks. | ||
| - Confirm no secrets in diff: run `git diff` and ensure no new hardcoded keys or tokens. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| --- | ||
| description: "Vitest tests and test helpers for packages scope" | ||
| globs: "packages/**/*.test.ts,packages/**/*.test.tsx,packages/**/tests/**/*.ts,packages/**/tests/**/*.tsx" | ||
| alwaysApply: false | ||
| --- | ||
| # Testing (Vitest) | ||
|
|
||
| ## Runner and config | ||
| - Root config: `vitest.config.mts` (aliases `@excalidraw/*` — use them in tests like production code). | ||
| - From repo root: `yarn test` (watch), `yarn test:app --watch=false` (single run), `yarn test:all` before PRs. | ||
|
|
||
| ## Conventions | ||
| - Colocate tests as `*.test.ts` / `*.test.tsx` next to source when that matches existing patterns; integration suites often live under `packages/*/tests/`. | ||
| - Prefer existing helpers from `packages/excalidraw/tests/helpers/` (e.g. `API` helpers) instead of duplicating setup. | ||
| - Use `vitest-canvas-mock` / jsdom behavior already configured — do not add alternate test environments without a strong reason. | ||
| - Avoid `any` in tests; use `import type` and real types from `@excalidraw/*` packages. | ||
|
|
||
| ## Snapshots and updates | ||
| - Only update snapshots when output changes are intentional: `yarn test:update` (or Vitest `-u`), then review diffs carefully. | ||
|
|
||
| ## Scope | ||
| - Keep tests deterministic (no real network, no reliance on wall-clock timing without fake timers). | ||
|
|
||
| ## How to verify | ||
|
|
||
| - `yarn test:app --watch=false` — full Vitest run once (matches CI’s `test:app` step). | ||
| - `yarn test:all` — before PR: includes typecheck, ESLint, Prettier, and Vitest. | ||
| - For a single file: `yarn test:app -- run path/to/file.test.tsx` (from repo root; Vitest uses `vitest.config.mts`). |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.