Skip to content

fix: resolve all remaining code review items#22

Merged
Wintersta7e merged 7 commits intomainfrom
fix/code-review-remaining
Mar 27, 2026
Merged

fix: resolve all remaining code review items#22
Wintersta7e merged 7 commits intomainfrom
fix/code-review-remaining

Conversation

@Wintersta7e
Copy link
Copy Markdown
Owner

Summary

Resolves all remaining items from the 2026-03-13 extensive code review (8 bugs, 2 security, 3 perf, 3 arch, test gaps).

Bug fixes:

  • BUG-1 (critical): Export convergence labels deferred to indent 0 — prevents infinite loops in RPG Maker when JUMP_TO_LABEL lands inside Show Choices/Conditional Branch blocks
  • BUG-5 (high): PreviewEngine deep-clones Maps/Sets/arrays into Zustand snapshots — fixes stale React renders
  • BUG-6 (medium): Menu choice reorder now swaps edge sourceHandle references so connections follow their choices
  • BUG-8 (low): Number keys 1-7 blocked during preview mode

Performance:

  • PERF-7: computeGuideLines skips nodes >600px away from dragged node

Security:

  • SEC-8: Export commands array validated before writing to map files
  • SEC-10: File size limits enforced (50MB interaction, 100MB project JSON)

Architecture:

  • ARCH-1: Shared API types in src/shared/api-types.ts (single source of truth)
  • ARCH-3: Removed unused electron-updater
  • ARCH-4: Replaced unmaintained dagre 0.8.5 → @dagrejs/dagre 3.0.0

Tests: 47 new tests (228 total, up from 181):

  • Export pipeline normal paths + BUG-1 regression tests
  • DocumentStore CRUD
  • HistoryStore undo/redo

Test plan

  • npm test — 228 tests passing across 17 files
  • npm run typecheck — clean (only pre-existing TS4023 in nodeTypes)
  • npm run lint — clean on all changed files

Co-Authored-By: Rooty

Convergence nodes (multiple incoming edges) were emitting their LABEL
commands at the current branch indent level. When JUMP_TO_LABEL landed
inside a Show Choices or Conditional Branch block, RPG Maker's interpreter
could re-enter the branch scope, causing infinite loops or wrong execution.

Now all convergence nodes are deferred and processed at indent 0 after the
main DFS traversal, ensuring labels are always outside branch structures.

Co-Authored-By: Rooty
The shallow spread { ...engine.state } shared mutable Maps, Sets, and
arrays between the engine's internal state and the Zustand store. Engine
mutations would silently alter the store snapshot, breaking React change
detection. Now clonePreviewState() creates independent copies of all
collection types.

Co-Authored-By: Rooty
Reordering menu choices via move up/down changed the array indices but
left edge sourceHandle references (choice-0, choice-1, etc.) pointing
at the old positions. Now moveChoice also remaps the affected edges so
connections follow their original choices to the new indices.

Co-Authored-By: Rooty
BUG-8: Number keys 1-7 no longer create nodes while the preview panel
is open — the quick-add shortcut now checks previewState.isOpen first.

PERF-7: computeGuideLines skips nodes whose center is more than 600px
from the dragged node, avoiding unnecessary alignment checks on large
graphs.

Co-Authored-By: Rooty
SEC-8: exportToMap now validates each command object has numeric code,
numeric indent, and a parameters array before writing to the map file.

SEC-10: file:load enforces a 50MB limit on .mzinteraction files.
Project JSON reads (maps, system) enforce a 100MB limit via a shared
readProjectFile() helper. Prevents memory exhaustion from oversized
or malicious files.

Co-Authored-By: Rooty
ARCH-1: Extract API interface types into src/shared/api-types.ts as a
single source of truth. Preload re-exports them; renderer api.d.ts
imports and narrows TemplateAPI with concrete NodeTemplate types.

ARCH-3: Remove unused electron-updater dependency (never imported).

ARCH-4: Replace unmaintained dagre 0.8.5 with @dagrejs/dagre 3.0.0
(actively maintained fork, API-compatible).

Co-Authored-By: Rooty
47 new tests covering the three biggest coverage gaps:

- export.test.ts: All action types (script, set_switch, set_variable,
  common_event, show_text, plugin_command), condition branches (switch,
  variable, script), static/dynamic menus, convergence node label
  placement at indent 0 (BUG-1 regression tests), empty graphs.

- documentStore.test.ts: Node/edge CRUD, preset management, bookmark
  toggle, removeNode edge cleanup, document lifecycle (set/new).

- historyStore.test.ts: Undo/redo cycles, 20-entry limit, push clears
  future, clear resets all, null returns on empty history.

Co-Authored-By: Rooty
@Wintersta7e Wintersta7e force-pushed the fix/code-review-remaining branch from e12ffff to 97454a6 Compare March 27, 2026 00:30
@Wintersta7e Wintersta7e merged commit 6206922 into main Mar 27, 2026
1 check passed
@Wintersta7e Wintersta7e deleted the fix/code-review-remaining branch March 27, 2026 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant