Add code style and conventions to AGENTS.md + new CONVENTIONS.md by aidemsined · Pull Request #1490 · tenstorrent/ttnn-visualizer

aidemsined · 2026-05-15T02:21:09Z

Summary

Docs-only change. Codifies the style discipline observed across recent PRs into guidance that contributors and AI agents can follow without reverse-engineering it from review threads.

What's added

`AGENTS.md` — Code style and conventions (15 sub-sections, ~93 lines)

Auto-loaded by Cursor / Claude Code / Codex / Aider as the canonical entry point. Each sub-section is a one-liner-per-rule scan-friendly reference:

Comments — explain why not what; preserve formatter-placeholder // lines in array literals; stale comments are bugs.
TypeScript — prefer named enums over string-literal unions; spell out generic parameters (DataSet<T>); respect react-hooks/exhaustive-deps; default to interface ComponentNameProps for component props.
CSS / SCSS — promote colour values to CSS custom properties in _base.scss + GRAPH_COLORS / cssVar() instead of hardcoded hex.
Lint discipline — don't fix pre-existing lints in unrelated PRs; suppressions need an explanatory comment; assess warning validity before suppressing.
Testing — Vitest (FE, client fixture wraps Flask's app.test_client() for BE); use shared fixture helpers and cross-cutting invariants for larger suites (cites the tests/mlirFixtures/ pattern).
Frontend data integrity — prefer client-side JSON validation before backend round-trips.
Upload security — apply Path(filename).name to user-supplied filenames before composing destination paths; add regression tests for path traversal.
Toolchain — pnpm-only (engines.pnpm >= 11); honour .nvmrc; corepack-managed pnpm.
Database schema changes — Alembic migrations only; nullable=True for new columns referenced by ORM models.
State management (Jotai) — shared atoms in src/store/app.ts with *Atom suffix; useAtomValue for read-only; atomWithStorage for persistent prefs.
Data fetching (React Query) — useQuery<Data, AxiosError> typing; query-key tuple shape; *_QUERY_KEY constants for cross-module invalidation; staleTime: Infinity for report-bound queries.
Errors and toasts — getResponseError for error-string extraction; createToastNotification as the single entry point for toasts.
File organization — definitions/ vs model/ split; Endpoints / ROUTES / TEST_IDS centralization.
Naming — function-prefix table (use*/handle*/get*/is*/fetch*); SCREAMING_SNAKE_CASE constants; backend _private helpers.
Backend conventions — module-level logger; view decorator stack order; @local_only for SERVER_MODE gating; response_* helpers and StatusMessage; str_to_bool for env-var booleans.

`CONVENTIONS.md` — Full reference (~930 lines)

New companion doc with rationale, file/line examples, and counter-examples for every rule in AGENTS.md, plus additional patterns that fit the long form better than a one-liner:

SPDX headers — header format and scripts/check-spdx.mjs enforcement; creation-year (not edit-year) convention.
Network layer — shared axiosInstance is mandatory; instanceId travels as a query param only; cross-cutting retries belong in the response interceptor; SocketProvider socket is module-scope to survive React StrictMode double-mount.
Routing and page metadata — route definitions go through routeObjectList + stripFirstSlash; page titles via react-helmet-async with the layout owning titleTemplate.
CSS / SCSS — @use over @import with the as * vs short-namespace convention; SCSS file naming (underscore-prefixed partials vs PascalCase component sheets); stylesheet imports via the styles/ alias.
Lint discipline — no-floating-promises is error-level with an explicit void escape hatch for intentional fire-and-forget.
Testing — frontend tests live in tests/ (not co-located) with .spec.ts(x) extension; backend tests use client.get(url, query_string={...}); patch where the symbol is bound, not where it's defined.
Frontend data integrity — verified useMLIR synthetic-422 AxiosError pattern with the three-thing copy-paste rubric.
Backend conventions — single module-scope Blueprint("api", __name__) in views.py; prefer Response(orjson.dumps(...)) over jsonify for read-mostly endpoints (with the orjson.Fragment rationale).

Known inconsistencies section flags ~19 pre-existing drift points (folder-upload sanitisation, extract_npe_name misnomer, errorMessage vs statusMessage, upload size cap, React.FC deliberately avoided, altCongestionColorsAtom key drift, raw toast() in useBufferFocus, useAtom mis-use in SocketProvider, print() in sockets.py, kebab-case error-page.tsx, .tsx on non-JSX GraphColors, flake8 79 vs black 88, @with_instance 404 vs 400, Config.__new__ missing return annotation, four useQuery hooks with implicit error generics, dataclasses.asdict vs to_dict() with enum fields, relative ../scss/... imports, SocketProvider cleanup missing fileTransferProgress off()).

The two files are kept aligned by a maintenance contract documented at the top of CONVENTIONS.md: any new rule lands in both — AGENTS.md for the one-liner, CONVENTIONS.md for the detail.

Why now

Several PRs in the past few weeks have surfaced these conventions in review comments (path traversal hardening in MLIR upload, ref-in-render lint validity, hardcoded colour promotion, fixture-builder pattern for the MLIR test suite, MLIR useQuery<GraphBundle, AxiosError> typing fix, etc.). Codifying them upfront should cut down on repeated triage and give new contributors — human or AI — a consistent baseline.

Test plan

Markdown renders correctly in the GitHub PR preview
AGENTS.md remains parseable by Claude Code, Codex, Aider, Cursor (well-formed markdown, consistent heading hierarchy)
CONVENTIONS.md TOC anchors resolve to their respective sections
Copilot review feedback addressed (5 threads resolved)
All 15 CI checks green

Capture the style discipline observed across the codebase — comment norms, TypeScript/SCSS preferences, lint suppression policy, testing patterns, upload security, toolchain, and DB migration rules — so contributors and AI agents share the same baseline expectations without reverse-engineering them from review comments. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Adds a new “Code style and conventions” section to AGENTS.md to codify contributor guidance (comments, TypeScript, CSS/SCSS, linting, testing, data validation, upload security, toolchain, and DB schema practices) so future PRs don’t need to rediscover these conventions from review history.

Changes:

Documented conventions for comments, TypeScript patterns, styling (CSS variables + GRAPH_COLORS), and lint suppression discipline.
Added guidance for testing practices (FE Vitest; BE pytest), frontend JSON validation, and upload path-traversal hardening.
Documented toolchain expectations (pnpm/.nvmrc/corepack) and DB schema change practices (Alembic + nullable defaults).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Extend the Code style and conventions section with additional rules distilled from a full-codebase pass: - State management (Jotai): atoms live in src/store/app.ts with the *Atom suffix; useAtomValue for read-only; atomWithStorage for persistent prefs. - Data fetching (React Query): useQuery<Data, AxiosError> typing; query-key tuple shape; *_QUERY_KEY constants for cross-module invalidation; staleTime: Infinity for report-bound queries. - Errors and toasts: getResponseError for error-string extraction; createToastNotification as the single entry point for toasts. - File organization: definitions vs model split; Endpoints / ROUTES / TEST_IDS centralization. - Naming: function-prefix table (use*/handle*/get*/is*/fetch*); SCREAMING_SNAKE_CASE constants; backend _private helpers. - Backend: module-level logger; view decorator stack order; @local_only for SERVER_MODE gating; response_* helpers and StatusMessage; str_to_bool for env-var booleans. - TypeScript: interface ComponentNameProps as the default for props. Co-authored-by: Cursor <cursoragent@cursor.com>

- Testing: reference the shared `client` fixture (Flask's `app.test_client()`, in `backend/ttnn_visualizer/tests/conftest.py`) instead of the lower-level `werkzeug.test.Client`. That's the API contributors actually use via the pytest fixture. - Toolchain: correct the pnpm engine constraint from `>= 10` to `>= 11` to match `package.json` (`"pnpm": ">=11"`). Co-authored-by: Cursor <cursoragent@cursor.com>

Companion to AGENTS.md. AGENTS.md states each convention in one line; CONVENTIONS.md expands every rule with rationale, file/line examples, and counter-examples. New conventions go in both — AGENTS.md for the one-liner so agents and contributors catch it at the entry point; CONVENTIONS.md for the detail so reviewers and humans can dig in. Sections mirror the AGENTS.md "Code style and conventions" structure: comments, TypeScript, CSS/SCSS, state management (Jotai), data fetching (React Query), errors and toasts, file organization, naming, lint discipline, testing, frontend data integrity, upload security, toolchain, database schema changes, and backend conventions. Closes out with a "Known inconsistencies" section so reviewers can spot codebase divergences quickly. AGENTS.md adds a single-line callout to CONVENTIONS.md under the section header. Co-authored-by: Cursor <cursoragent@cursor.com>

Re-ran the codebase style audit to fill gaps the first pass missed. Adds the following new rules with verified file:line examples: - SPDX headers: section pinning the project format and creation-year convention, plus the scripts/check-spdx.mjs enforcement path. - CSS/SCSS: `@use` over `@import` with the `as *` vs short-namespace convention; SCSS file naming (underscore-prefixed partials vs PascalCase component sheets); stylesheet imports via the `styles/` alias. - Network layer (new section): shared `axiosInstance` is mandatory; `instanceId` travels as a query param only; cross-cutting retries belong in the response interceptor; the SocketProvider `socket` is module-scope to survive React StrictMode double-mount. - Routing and page metadata (new section): route definitions go through `routeObjectList` + `stripFirstSlash`; page titles via `react-helmet-async` with the layout owning `titleTemplate`. - Lint discipline: `no-floating-promises` is error-level with an explicit `void` escape hatch for intentional fire-and-forget. - Testing: frontend tests live in `tests/` (not co-located) with `.spec.ts(x)` extension; backend tests use `client.get(url, query_string={...})`; patch where the symbol is bound, not where it's defined. - Frontend data integrity: replace the fabricated AxiosError example with the verified `useMLIR` synthetic-422 pattern, including the three-thing copy-paste rubric (preserve config/request, spread the response, use numeric HttpStatusCode). - Backend conventions: single module-scope `Blueprint("api", __name__)` in `views.py`; prefer `Response(orjson.dumps(...))` over `jsonify` for read-mostly endpoints (with the `orjson.Fragment` rationale). Adds 13 new bullets to "Known inconsistencies": altCongestionColorsAtom storage-key drift, raw `toast()` in useBufferFocus, `useAtom` mis-use in SocketProvider, `print()` in sockets.py, kebab-case error-page.tsx, `.tsx` for non-JSX GraphColors, flake8 vs black line-length disagreement, `@with_instance` 404 vs 400 semantics, `Config.__new__` missing return annotation, four `useQuery` hooks with implicit error generics, `dataclasses.asdict` vs `to_dict()` with enum fields, and the relative SCSS import holdouts. Updates the table of contents to reflect the two new sections. CONVENTIONS.md grows from 611 to 931 lines. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

@Timer

- SPDX rationale: clarify that check-spdx.mjs walks the whole repo, not just staged files (it uses walkDirectory(process.cwd())). - Testing section: drop the contradictory "next to the source as *.spec.ts(x) or in tests/" wording from the first-pass subsection; the canonical layout is "tests/ at the repo root", as stated in the dedicated subsection further down. Replace with a forward link. - View decorator stack order: align the example and ordering with current views.py — @api.route → @with_instance → @local_only (when needed) → @Timer (when present). The previous example had @local_only above @with_instance, which doesn't match any route in views.py; existing @local_only usages (delete_profiler_report:884, delete_performance_report:1027) have @with_instance above @local_only. Also clarify @Timer is selective, not mandatory. - SocketProvider listener pairing: soften the "must" wording to "convention is to pair", and add an explicit tech-debt note that the current cleanup doesn't off() fileTransferProgress (registered at SocketProvider.tsx:49:59). Document the same gap in the Known inconsistencies section. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

- SPDX scope: list the actual file extensions check-spdx.mjs walks (.js/.ts/.jsx/.tsx/.mjs/.scss/.xml/.py/.json + package.json per scripts/check-spdx.mjs:29:31) instead of "every source file"; explicitly note that markdown/YAML/TOML are skipped. - Backend test example: use the canonical upload-endpoint pattern matching backend/ttnn_visualizer/tests/test_file_uploads.py:328 - full /api prefix, instanceId query param via the make_report fixture. Without these, contributors copy-pasting the snippet would hit a 404 (wrong prefix) or 400 (missing instance). - Error-response rationale: drop the inaccurate claim that getResponseError falls back due to shape divergence (getResponseError.ts:18-20 explicitly recognises { error: string }). Replace with the real reasons to funnel through the helpers: (a) the right HTTPStatus constant matches the semantic, (b) the optional `detail` field is preserved, (c) single point of evolution if the response shape changes. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

- Qualify the Path(filename).name claim in both AGENTS.md and CONVENTIONS.md. The helper resolves to PurePosixPath on our Linux/macOS servers, which only treats '/' as a separator. The prior wording claimed backslashes and drive-letter prefixes were stripped; verified by `python3 -c "from pathlib import Path; print(repr(Path('foo\\\\bar').name))"` returning 'foo\\\\bar' unchanged. Containment still holds because backslashes become literal filename characters on POSIX rather than traversal vectors, but the doc shouldn't overstate the guarantee. Added an explicit caveat and a forward pointer to werkzeug.secure_filename for any future Windows-aware deployment. - Fix the self-contradiction in CONVENTIONS.md: the canonical backend test example built the URL as f"/api/local/upload/mlir?instanceId= {instance_id}" while the dedicated subsection further down prescribes `query_string={...}` and forbids manual concatenation. Updated the example to use `query_string={"instanceId": instance_id}` for consistency. Dropped the stale "mirroring test_file_uploads.py:328" citation since that file still uses the f-string form. - Add Known inconsistencies bullet noting that several upload tests in test_file_uploads.py still use f-string concatenation (pre-existing drift, do not replicate in new tests). Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

- The Known inconsistencies bullet claimed "React.FC is not used". Verified incorrect: `rg 'React.FC' src` returns 27 files with ~37 occurrences (including `src/libs/SocketProvider.tsx`, `src/routes/GraphView.tsx`, `src/components/OperationGraphComponent.tsx`, `src/components/npe/NPEViewComponent.tsx`, etc.), plus another 12 files using bare `: FC<...>`. Reframed the entry honestly: both patterns coexist, mirror the file you're editing, the React.FC foot-gun is a real consideration but not a project-wide migration. - The relative-SCSS-imports bullet listed only Collapsible.tsx:9 and ListItem.tsx:9. Expanded to the actual five files in the codebase: added OperationGraphComponent.tsx:16, cluster/ClusterRenderer.tsx:11, and tensor-sharding-visualization/TensorVisualisationComponent.tsx:13. Verified by grepping for `import '../...scss/'` patterns. - The CSS/SCSS section had a duplicate two-file citation pointing to the same incomplete list. Redirected the in-section mention to the expanded Known inconsistencies bullet to avoid drift if the list grows. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

- The backend test example wouldn't actually pass against the real handler. Three sub-issues, all valid: 1. SERVER_MODE: conftest.py:34 sets SERVER_MODE=True by default, and the MLIR upload handler returns 403 in server mode. The example needs `app.config["SERVER_MODE"] = False` (matching test_file_uploads.py:323). 2. LOCAL_DATA_DIRECTORY: conftest.py:38 ships it as a str, but the handler does `data_directory / config["MLIR_DIRECTORY_NAME"]` which needs Path. Cast it to Path (matching test_file_uploads.py:322). 3. Status assertion: StatusMessage.status is a ConnectionTestStates enum and serializes to its int value (2 for FAILED), not the string "FAILED". The canonical pattern is at test_file_uploads.py:549:550 with the explanatory comment. Updated to `body["status"] == ConnectionTestStates.FAILED.value`. Also added the missing imports (HTTPStatus, BytesIO, Path, ConnectionTestStates) so the snippet is copy-paste runnable, and a short explainer block right above the example listing the two conftest defaults that bite local-upload tests in particular. - SPDX rationale on JSON files was misleading. `check-spdx.mjs:96` matches `.json` extension AND `relativePath.includes('package.json')` — so the JSON check is a single-file special case on package.json, not a rule that covers every JSON file. Reframed the rationale as two distinct buckets: 1. Header check on the JS-style + Python-style extensions. 2. license+author object check on package.json specifically. Also reworded the follow-up "JSON files (currently just package.json)" line to drop the "currently just" parenthetical, which implied the set might expand. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

`DataSet` is imported from `vis-data`, not `vis-network`. Verified via `rg "from 'vis-(data|network)'" src`: - src/components/OperationGraphComponent.tsx:8:9 - src/components/operation-details/DeviceOperationsGraphComponent.tsx:9:10 Both files import `DataSet` from `vis-data` and `Edge`/`Network`/ `Node`/`IdType` from `vis-network`. Updated the TypeScript-generics example to reflect the correct package and reference the canonical import block. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

Four accuracy nits, all valid: 1. AGENTS.md backend conventions: "helper trio" was wrong. `backend/ttnn_visualizer/exceptions.py` exposes five helpers (response_bad_request, _not_found, _forbidden, _unprocessable_entity, _internal_server_error). Reworded to "helpers". 2. CONVENTIONS.md Network layer: the heading and rationale read as an absolute rule against `import axios`, but `getResponseError.ts:5` does `import axios from 'axios'` for `isAxiosError`, `useRemote.tsx:5` does the same, and many files import `AxiosError` / `HttpStatusCode` / `AxiosProgressEvent` / `AxiosRequestConfig` types from `axios`. Rewrote both heading and rationale to scope the rule to **HTTP request methods** (.get/.post/.put/.delete/...), and explicitly allow type/helper imports from `axios` with file/line citations. 3. CONVENTIONS.md `instanceId` rule: claimed the frontend "never embeds the instance ID in the URL" because of the request interceptor. SocketProvider.tsx:18 builds the socket URL with `?instanceId=...` because io(...) doesn't go through axios. Added a "Documented exception" paragraph and narrowed the headline rule to "HTTP API calls going through axiosInstance". 4. CONVENTIONS.md Backend tests: claimed url_prefix="/api", but app.py:86 actually does `url_prefix=f"{app.config['BASE_PATH']}api"`, so the prefix is `/api` only when BASE_PATH=`/`. Rewrote to describe the dynamic prefix, include both the single-tenant and prefixed-deployment cases, and call out that `/api/...` is correct in tests but not a hard-coded assumption in production-facing examples. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

…efix - AGENTS.md enum example used a bare `enum NodeRelation { Input, Output }`, which is a *numeric* enum (Input=0, Output=1) and silently breaks the string comparisons of the union it claims to replace (`'input' | 'output'`). The actual codebase uses `NodeRelation.Input = 'input'`. Updated the example to be string-valued and added a short explainer about why bare enums are a trap here. - CONVENTIONS.md Backend conventions section still claimed `app.py` mounts the blueprint at `url_prefix="/api"`. Round 8 fixed the Testing section but missed this duplicate mention at line 875. Aligned it with the actual `url_prefix=f"{app.config['BASE_PATH']}api"` registration so the doc reads consistently with itself. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

smountenay-tt · 2026-05-15T16:08:00Z

+
+### One module-scope `api = Blueprint("api", __name__)`
+
+`backend/ttnn_visualizer/views.py:104` declares the single blueprint:


I don't think it's a great idea to have line numbers referenced in this file, which could easily change at any time, and it's unlikely anyone will think to come update them here.

smountenay-tt

Overall it's good, but I don't like that it has specific line numbers mentioned throughout, which are subject to change at any time.

…ncies that are solved

aidemsined requested review from Copilot and smountenay-tt May 15, 2026 02:21

Copilot started reviewing on behalf of aidemsined May 15, 2026 02:21 View session

aidemsined requested a review from dcblundell May 15, 2026 02:22