Repository navigation
refactor(core): explicit per-field operation variants + deprecate renderMarkdown - #27
Merged
Merged
Conversation
This was referenced May 23, 2026
…derMarkdown Two API hygiene changes bundled. 1. Replace Partial<CanvasNode> with explicit operation variants (closes #5) The previous `updateNodeContent(nodeId, Partial<CanvasNode>)` shape was the last FFI-hostile item from the original audit — Partial<T> has no clean Rust equivalent and would have forced either a stringly-typed HashMap or a sparse-field struct across the JSI/wasm-bindgen boundary. Replaced with six per-field operation variants and matching CanvasState methods: - updateNodeColor(nodeId, color) - updateTextNodeText(nodeId, text) - updateLinkNodeUrl(nodeId, url) - updateFileNode(nodeId, file, subpath) - updateGroupNodeLabel(nodeId, label) - updateGroupNodeBackground(nodeId, background, backgroundStyle) Each Operation variant carries the new value(s) and the prev value(s) for invertOperation, which now has a case per variant. Apply paths verify node type before mutating (silent no-op on missing or mismatched type, consistent with removeNode's `if (!node) return` pattern). Exported `GroupBackgroundStyle = 'cover' | 'ratio' | 'repeat'` so consumers can type-narrow the backgroundStyle parameter without reaching into the GroupNode type. x / y / width / height stay in moveNode / resizeNode where they were before — they're not "content" updates. Breaking change: `updateNodeContent` is removed from the public API. No external consumers in this repo (grep confirmed). Workspace's canvas-ui has the same shape and will need the same migration when it next pulls from upstream. 2. Deprecate renderMarkdown (closes #26) The prop on CanvasView is vestigial — nothing in src/renderer consumes it because TextNodeContent returns null and all text-node bodies render through Skia's paragraphBuilder. The prop has been silently accepted-and-ignored for the entire life of the library. Marked @deprecated in both declaration sites (CanvasView.tsx, CanvasContext.tsx) with a JSDoc explaining the situation and pointing at the actual markdown-rendering pathway. README's "What's inside" section gains a "Markdown rendering" subsection enumerating what's actually supported (inline marks, blocks, GFM extensions) and what isn't (anything needing a React overlay). No behavioural change — the prop continues to be accepted. Removal is a separate decision for a future major version. Verified - npm test: 38/38 (tests reference state.history.canRedo which is unchanged) - npm run typecheck: clean - npm run lint: exit 0 Refs: #5, #26 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LeslieOA
force-pushed
the
refactor/explicit-update-variants
branch
from
October 4, 2026 20:09
9642005 to
6455ce1
Compare
Review of #27 before landing: - The six per-field operations had no tests. Adds 11: each applies, undoes and redoes; a cleared field is absent from the serialised document; a missing node or wrong node type is a no-op with nothing to undo; invertOperation round-trips. - The README's new markdown section claimed tables and task-list checkboxes are supported. Running parseToSegments shows tables, images and nested list items are dropped, and task items draw as plain bullets. The section now says what draws and what doesn't, names the parser (parseToSegments, not paragraphBuilder), and is scoped to the React Native renderer. - AGENTS.md still said a host can supply a markdown component through renderMarkdown, which this PR deprecates as ignored. - GroupBackgroundStyle duplicated GroupNode's union in operations.ts. It now lives in core/types.ts, and GroupNode uses it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LeslieOA
added a commit
that referenced
this pull request
Oct 4, 2026
CanvasView accepted renderMarkdown and put it in the canvas context, and nothing read it: text nodes have drawn through Skia since March. #27 marked it deprecated. Its only callers, Workspace's desktop and mobile apps, stopped passing it in workspace-sh/workspace#644. BREAKING CHANGE: CanvasView no longer has a renderMarkdown prop, and the value from useCanvasContext no longer has a renderMarkdown field. A caller that still passes the prop fails typecheck; delete the prop, it had no effect. The goal the prop was for, text nodes that match a markdown document, is #88. Refs: #26, #88 Co-Authored-By: Claude Opus 5.5 <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
1 participant
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.
Summary
Two changes to the public API.
1. Explicit per-field operations replace
Partial<CanvasNode>(closes #5)updateNodeContent(nodeId, Partial<CanvasNode>)has no clean Rust equivalent across JSI or wasm-bindgen. Six operations replace it:updateNodeColor(nodeId, color)updateTextNodeText(nodeId, text)updateLinkNodeUrl(nodeId, url)updateFileNode(nodeId, file, subpath)updateGroupNodeLabel(nodeId, label)updateGroupNodeBackground(nodeId, background, backgroundStyle)Each
Operationvariant carries the new and the previous values, forinvertOperation. Each is a no-op, with nothing recorded in the history, when the node is missing or of the wrong type.GroupBackgroundStyleis exported from the core types, andGroupNodeuses it.Breaking:
updateNodeContentis gone. Nothing in this repo or in Workspace's apps calls it (desktop and mobile useCanvasView; Linux usesparseCanvasand the types).2.
renderMarkdownis deprecated (closes #26)Nothing in
src/rendererreads the prop: text nodes always draw through Skia. It is marked@deprecated, still accepted, and ignored, so nothing breaks. Workspace's desktop and mobile apps pass it today and can drop it.The README gains a "Markdown in text nodes" section saying what draws and what doesn't. Checked by running
parseToSegments: tables, images and nested list items are dropped, and task-list items draw as plain bullets.Corrected in review
invertOperationround-trip). There were none.AGENTS.mdstill describedrenderMarkdownas a way to supply a markdown component.Verification
npm test: 133 pass on this branchnpm run typecheck: cleannpm run lint: 0 errorsCloses #5, closes #26
🤖 Generated with Claude Code