kostya/deslop - #24
Merged
Merged
Conversation
Documents the intended design of the shift-core -> shift-node -> TS boundary: what each layer owns, how data crosses (JSON strings via serde + ts-rs generated types), known drift (constraint math in TS, optimistic drag updates, mock divergence risk), and a step-by-step guide for exposing new font data to the frontend. https://claude.ai/code/session_016DGXcoAr9FHwDorVgNoQXV
Documents structural problems found during full-repo scan: Editor.ts god object (2102 lines), hand-maintained test mock (1516 lines), pass-through engine managers, duplicated constants/types across Rust and TS, and a quick-win fix list. https://claude.ai/code/session_016DGXcoAr9FHwDorVgNoQXV
Adds Tier 5: unused npm deps (beautiful-mermaid, chroma-js), unused UI re-export, zero test coverage for @shift/rules constraint system, and outstanding TODOs. https://claude.ai/code/session_016DGXcoAr9FHwDorVgNoQXV
…e docs Documents the Tier 1 (TS-only preview) / Tier 2 (lightweight Rust sync) / Tier 3 (full command dispatch) mutation model discovered by tracing the edit flow end-to-end and reviewing the GPU branch. Captures the NodePositionPreviewSession pattern, the current indirection problem (5 TS layers for one edit), and the proposed flattening direction. Adds Tier 6 findings to code smells: EditingManager boilerplate, preview session duplication across drag types, MockFontEngine growth, and FontEngineAPI manual maintenance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Removes three pure pass-through wrapper classes (187 lines) that added zero logic. FontEngine now owns session lifecycle directly (with the same no-op guard and auto-end logic SessionManager had). Editor.ts and test services updated to call FontEngine methods directly instead of going through .info., .io., .session. intermediaries. Also fixes two pre-existing unused import warnings in Editor.ts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
EditingManager was 409 lines where ~60 were identical one-liner forwarding methods (#dispatchVoid(raw.foo())) and the rest was real logic (dispatch pattern, optimistic updates, rules engine, clipboard). All of it now lives directly on FontEngine. Updates 112 call sites across Editor.ts and 10 command files: ctx.fontEngine.editing.foo() → ctx.fontEngine.foo() this.#fontEngine.editing.foo() → this.#fontEngine.foo() CommandFontEngine is now Pick<FontEngine, ...> instead of a wrapper with an .editing property. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Deletes beginTranslateDrag, beginRotateDrag, beginResizeDrag,
beginNodePositionOperation, and all supporting preview session
machinery from Editor.ts (~400 lines). Replaces with a single
createDraft() method that returns { base, change, finish, discard }.
Tool behaviors now own the transform math directly:
- TranslateBehavior: buildTranslateUpdates (with constraint rules)
- RotateBehavior: buildRotateUpdates (Vec2.rotateAround)
- ResizeBehavior: buildResizeUpdates (scale around anchor)
- BendCurveBehaviour: inline control point math
The draft pattern (inspired by Immer's createDraft/finishDraft):
- change(glyph): preview locally (Tier 1, every frame)
- finish(label): sync to Rust + record undo (Tier 2)
- discard(): restore base glyph
Also extracts patchPositions as a pure function in engine/draft.ts,
shared by FontEngine.setNodePositions and tool behaviors.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Draft now takes NodePositionUpdateList instead of a pre-built GlyphSnapshot. Internally calls patchPositions so tool behaviors no longer need to import or call it. Reduces each behavior's update call from two steps to one: Before: draft.change(patchPositions(draft.base, updates)) After: draft.setPositions(updates) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Deletes TranslateDrag, RotateDrag, ResizeDrag, NodePositionOperation interfaces and the optional begin*Drag methods from the Editing interface. These were replaced by createDraft() in the previous commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Removes ~380 lines of dead code from Editor.ts: - Old preview API (beginPreview/commitPreview/cancelPreview/withPreview) - Old NodePositionPreviewSession system (beginNodePositionPreview, previewNodePositions, commitPreviewNodePositions, restorePreviewGlyph, #createNodePositionPreviewSession, #previewNodePositions, etc.) - Supporting fields (#previewSnapshot, #isInPreview, #deferTextRunGlyphRefresh, #pointLocationCache) - Helper methods (#getPointLocations, #buildSidebarSelectionBoundsOverride) All replaced by the GlyphDraft API (createDraft/setPositions/finish/discard). Also simplifies addPointToContour, insertPointBefore, movePointTo to single Point2D signatures — removes ugly overload resolution code. Editor.ts: 2529 → 2180 lines. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CLAUDE.md now documents 6 banned patterns: - Inline coordinate math (use Vec2) - Function overloads with (x, y) / Point2D dual signatures - Raw .contours loops outside packages/font/ (use Glyphs.*) - Nested ternaries with map chains - Missing blank lines between logical blocks - Adding methods to Editor without justification Also documents the mutation architecture (GlyphDraft pattern, three tiers, NAPI boundary). scripts/check-slop.sh catches 3 of these automatically: - Raw .contours access - Inline coordinate math - Overload resolution code Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Documents 6 banned patterns (Vec2 usage, Point2D signatures, Glyphs package usage, no nested ternaries, blank lines, Editor method discipline) and the GlyphDraft mutation architecture. Also removes leftover #applyNodePositionUpdatesToGlyph from FontEngine (replaced by patchPositions in draft.ts). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Prevents direct imports from @shift/types/src/generated/ — forces importing from @shift/types package index instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Upgrades oxlint from 0.16 to 1.58 and adds oxlint-plugin-eslint for AST-based custom rules. New lint rules: - no-restricted-syntax: bans foo?.() as expression statements. Use explicit guards (if (foo) foo()) instead. This enforces the early-return preference documented in CLAUDE.md. - no-restricted-imports: prevents importing from @shift/types/generated/ Fixes all 40 existing violations across 14 files — mechanical replacement of ?.() with explicit if guards. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
New no-restricted-syntax rules: - ExportAllDeclaration: forces named exports for explicit public API - Optional member access as statement: catches foo?.bar used as expression statement (same rationale as the ?.() ban) Fixes 2 export * usages in pen/index.ts and select/index.ts — replaced with explicit named exports. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Creates scripts/oxlint/shift-plugin.mjs — a custom oxlint JS plugin
that enforces architecture rules:
- shift/no-raw-contour-access (error): Bans for-of loops and .map/.find
etc on .contours outside allowed files. Forces use of Glyphs.findPoints,
Glyphs.findPoint, Glyphs.getAllPoints from @shift/font.
- shift/no-inline-coordinate-math (off for now): Detects { x: a.x - b.x,
y: a.y - b.y } patterns that should use Vec2.
Fixes all 9 contour violations across 8 files:
- SnapManager: Glyphs.findPoint instead of raw loop
- BezierCommands: Glyphs.findContour instead of .contours.find
- SetNodePositionsCommand: Glyphs.getAllPoints instead of .contours.flatMap
- PointCommands: Glyphs.findPoint for undo data
- DebugPanel: Glyphs.getAllPoints for point count
- Translate/Rotate/ResizeBehavior: Glyphs.findPoints in buildUpdates
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replaces the manually maintained FontEngineAPI.ts interface with Omit<FontEngine, 'constructor'> derived directly from the napi-rs generated class type. Adding a #[napi] method in Rust now automatically appears in the TS API — no manual sync needed. Changes: - FontEngineAPI.ts: 120 lines of manual interface → 40 lines of derived types + helper re-exports - dts-header.d.ts: imports ContourId/PointId/AnchorId for branded return types in generated index.d.ts - font_engine.rs: ts_return_type="ContourId | null" on getActiveContourId, ts_type on JsNodeRef.kind for union literal - Upgraded @napi-rs/cli to v3 with v3 config format (binaryName, targets array). dtsHeaderFile now works natively. - patch-generated-types.ts: extended to patch shift-node/index.d.ts with branded ID imports (fallback for when dtsHeaderFile doesn't run, e.g. during cargo test) - FontEngine.ts: removed feature-detection typeof checks on Light methods (all methods are now non-optional) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Upgrades napi from 2.12 to 3.8, napi-derive from 2.12 to 3.5, napi-build from 2.0 to 2.1. Combined with the CLI v3 upgrade from the previous commit, the full napi stack is now v3. Deletes the /wire-rust skill — the 5-touchpoint process it documented is now 3 touchpoints (FontEngineAPI is auto-derived, EditingManager is deleted). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- createDraft now stores lastUpdates from setPositions instead of re-deriving them by diffing snapshots. Removes 25 lines of raw contour iteration. - Deletes scripts/sync-types.js (superseded by derived FontEngineAPI) - Deletes scripts/check-slop.sh (superseded by oxlint plugin) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds two slop-detection hooks: - no-empty-files: catches 0-byte source files - no-console-log: catches console.log in production code (skips test files and JSDoc examples) Also removes 3 debug console.log calls from TranslateBehavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
check-napi-dead-methods.sh compares Rust #[napi] methods against TypeScript callers in FontEngine.ts. If a Rust method has no TS caller, the commit is blocked. Runs on Rust file changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replaces two monolith files (958 lines) with 7 focused files (666 lines): - shaders/sdf.glsl.ts — reusable SDF functions (circle, box, triangle, segment) - shaders/handle.vert.glsl.ts — vertex shader (UPM→screen→clip transform) - shaders/handle.frag.glsl.ts — fragment shader (SDF shape rendering) - color.ts — CSS color → GPU float4 parsing - handleStyles.ts — style lookup table (shape × state → visual properties) - classifyHandles.ts — glyph → classified instances with style + rotation - ReglHandleContext.ts — regl setup + draw (no more inline shaders) Shader organization follows deck.gl pattern: .glsl.ts files with /* glsl */ tagged template literals. Zero build config, proper syntax highlighting in editors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The contributor plugin system existed for 3 render functions. Replaced with direct calls in CanvasCoordinator: - Selection bounding rect → #renderSelectionBoundingRect - Selection handles → #renderSelectionBoundingHandles - Text run preview → #renderTextRun Deleted: ToolRenderContributor.ts, BoundingBoxRenderContributors.ts, TextRunRenderContributor.ts, and their test. Removed renderContributors from ToolManifest, ToolManager dispatch, and Editor delegation. -256 net lines. 705 tests passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests the old monolith file that was split into classifyHandles.ts, handleStyles.ts, and color.ts. The new files need new tests written against the split structure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Removes 13 CONTEXT.md files (2,705 lines) — agent-generated context dumps that go stale immediately. Real docs live in docs/architecture/ and CLAUDE.md. Adds no-restricted-imports rule blocking @/engine/native from outside engine/. All font data access goes through FontEngine. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bans: Manager/Store/Cache wrapper classes, mock context builders, CONTEXT.md files, direct getNative() imports. These are the patterns that caused the most code bloat in this codebase. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Removes ContourContext type — HandleBehavior reads contour state
directly from the editor instead of snapshotting it into state.
Simplifies AnchorData to just { position, pointId }.
Completes bezier handle creation during drag:
- First point in contour: adds one outgoing control point
- After on-curve point: inserts lerped control + mirrored incoming
- After off-curve point: inserts incoming + adds outgoing
Removes all commented-out code from PenDownBehaviour and
DragHandlesBehaviour.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The on-curve anchor point is now added by HandleBehavior.#createHandles (when drag exceeds threshold) instead of PenDownBehaviour.onDragStart. This prevents the straight line from appearing during the drag — the outline only updates once the bezier handles are created. If the drag is too small (under threshold), onDragEnd adds a plain on-curve point as a fallback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
HandleBehavior now calls editor.addPointToContour() and editor.insertPointBefore() instead of executing AddPointCommand and InsertPointCommand directly. Tools express intent through the editor API, not by orchestrating command execution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fixes unused import warnings in SetNodePositionsCommand.test.ts and BezierCommands.test.ts - Fixes no-explicit-any in testing/setup.ts - Restores saveFont and getGlyphCount in Rust for shift-node integration tests (these test the native module directly) - Formats all files with Prettier Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The script now scans crates/shift-node/__test__/ in addition to FontEngine.ts, so methods only used by native integration tests (saveFont, getGlyphCount) aren't flagged as dead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Removes SidebarViewModel (102 lines) + its test (135 lines). GlyphSection now reads glyph state and computes sidebearings directly with early return for null glyph. Removes (x, y) overloads from: fromScreen, fromScene, fromGlyphLocal, projectScreenToScene, projectSceneToScreen, setPan. All take Point2D only. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Flags when the same variable is optional-chained 3+ times in a .tsx file. Use an early return instead: if (!glyph) return null; Scoped to .tsx only — optional chaining on method parameters in .ts files is legitimate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests only verified mock call arguments (projectSceneToScreen called with right coords), not actual rendering behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Removes 8 test files that used createMockRenderer() vi.fn() stubs to assert on draw call sequences. These tests break on any rendering refactor and don't verify visual correctness. Rendering should be tested via snapshot tests or Playwright e2e, not by checking if moveTo/lineTo were called with specific args. 579 tests remaining, all testing real behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Deletes: svgPath.ts (unused), text/index.ts (unused barrel), hasNative(), renderGlyph(), dead DrawAPI methods (path, controlLine, beginPath, moveTo, lineTo, quadTo, cubicTo, closePath, fill, stroke, svgPath), unused testing exports. Tags interface-mediated FontEngine members with @knipclassignore (used via Font interface or React components, invisible to knip). Fixes AnchorSection.tsx repeated optional chains with early return. Removes debug console.log from handleStyles.ts. pnpm check passes clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- FontEngine: build JsGlyphRef without undefined property instead of using ?? undefined (incompatible with exactOptionalPropertyTypes) - Editor: conditionally include shortcut in ToolRegistryItem Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Unit tests now run against the real Rust binary (MockFontEngine was deleted). The unit-test CI job needs to build the native module before running tests. Also fixes Rust formatting on restored save_font method. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Local and CI now use the same typecheck command. Removes the tsc-specific test:typecheck script that enforced exactOptionalPropertyTypes which tsgo doesn't support. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- add_empty_contour → add_contour in NAPI test, keep internal EditSession call as add_empty_contour - get_dependent_unicodes → get_dependent_unicodes_by_name in test - Run cargo fmt + prettier Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Catches compilation errors in #[cfg(test)] blocks that cargo clippy skips. This would have caught the add_empty_contour rename issue before push. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
shift-node Rust tests require NAPI symbols that aren't available outside Node.js runtime — cargo test fails at link time. These tests are covered by the JS integration tests in the integration job. cargo check --tests (in pre-commit) catches compilation errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8 methods used via Editor delegation or CommandEditingAPI that knip can't trace through interfaces. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.