Skip to content

Fix 72 issues from comprehensive code review#6

Merged
Wintersta7e merged 7 commits intomainfrom
code-review-fixes
Mar 13, 2026
Merged

Fix 72 issues from comprehensive code review#6
Wintersta7e merged 7 commits intomainfrom
code-review-fixes

Conversation

@Wintersta7e
Copy link
Copy Markdown
Owner

@Wintersta7e Wintersta7e commented Mar 13, 2026

Summary

Comprehensive code review identified 72 issues across the codebase, all resolved in 7 logical commits:

Critical Bug Fixes

  • Timeline drag overlap reverted wrong event's frame instead of the dragged event
  • Autosave recovery set events before openProjectPath() which cleared them
  • || vs ?? coercion throughout mz-converter.js — opacity 0, scale 0, duration 0 were silently overridden to defaults (identical class of bug already fixed for showText/screenFlash, but never applied to other event types)
  • parseInt || 0 zeroed property fields mid-keystroke during editing

Security Hardening

  • Rebuilt showConfirmDialog with safe DOM APIs (was using innerHTML with parameters)
  • HTML-escape filesystem names in image browser/picker (XSS via malicious folder names)
  • Set textarea value programmatically (was interpolating user text inside innerHTML)
  • Added Content-Security-Policy meta tag
  • Fixed openExternal hostname check (endsWith → exact match, preventing evil-github.com)
  • Added webSecurity: true explicitly to webPreferences

Error Handling

  • Added try/catch to 8 unprotected IPC handlers (save-scene, load-scene, scanDirectory, get-folder-contents, etc.)
  • Surfaced autosave write failures to user after 3 consecutive errors
  • Added user notification on folder structure load failure
  • Captured error variables in 6 catch blocks that were discarding them
  • Added .catch() to async event bus handlers
  • Added critical DOM element null-checks in elements.js
  • Added try/catch to saveSettings for QuotaExceededError

Performance

  • Playback no longer rebuilds the entire timeline DOM at 60fps — uses lightweight updateTimelineCursor() for cursor-only updates; full rebuild reserved for structural changes
  • Added updateMinimapCursor() for minimap cursor-only updates during playback
  • Eliminated O(n²) indexOf in renderPreviewAtFrame by using loop index directly
  • Debounced window resize handler on minimap
  • Debounced image search filter (150ms)
  • Removed will-change: transform from every virtual dropdown item

Architecture Cleanup

  • Removed 5 unused event constants and 1 orphan listener
  • Deduplicated getPreviewScale() (was defined in both preview/index.js and drag.js)
  • Fixed double render on Delete key
  • Added debounced undo state for arrow key image movement
  • Cleared processedTextEvents on scene change
  • Fixed DEFAULT_DURATION inconsistency (local 30 vs state.js 60)
  • Fixed export dropdown ordering bug (cache-hit path)

CSS & Accessibility

  • Replaced outline: none with focus-visible pattern on all inputs
  • Added aria-label to all toolbar buttons and minimap canvas
  • Changed semantic <footer> to <section> for timeline
  • Added <label> for timeline length input
  • Moved notification keyframes from JS-injected <style> to styles.css
  • Removed dead .btn[title] rule, unnecessary !important, duplicate selectors
  • Scoped .btn transition to specific properties instead of all

Stats

  • 28 files changed, +418 / -226 lines
  • 121 tests passing (updated 2 tests for removed event constants)
  • 0 regressions

@Wintersta7e
Fix critical bugs: timeline drag revert, autosave recovery, || vs ?? …
49f5d94 
…coercion, parseInt handling

- Fix timeline drag reverting wrong event on text overlap (drag.js)
- Fix autosave recovery setting events before openProjectPath which cleared them
- Replace all || with ?? in mz-converter.js to allow valid falsy values
  (opacity 0, scale 0, duration 0, blend 0, origin 0 now work correctly)
- Fix parseInt(...) || 0 in bind-input.js zeroing fields mid-keystroke
- Fix tintPicture wait defaulting to !== false instead of ?? true

Co-Authored-By: Rooty
@Wintersta7e
Fix error handling in IPC handlers and file operations
cb19d10 
- Add try/catch to save-scene and load-scene IPC handlers
- Add per-directory error handling in scanDirectory (skip unreadable dirs)
- Add try/catch around get-folder-contents readdir
- Add user notification on drag-drop scene load failure
- Capture error variable in catch blocks (screen-resolution, autosave-read)
- Log errors in get-maps and get-map-events handlers
- Replace || with ?? for pageIndex and screenWidth defaults
- Add validation for terminating code 0 in export-to-map splice
- Remove dead _scaleFactor code
- Add webSecurity: true to webPreferences
- Clear processedTextEvents on scene change (newScene, loadScene, loadSceneFromFile)

Co-Authored-By: Rooty
@Wintersta7e
Fix security vulnerabilities: XSS prevention, CSP, hostname validation
c8d31c9 
- Rebuild showConfirmDialog with safe DOM APIs (no innerHTML with params)
- Add focus trap, Escape-to-close, and focus restore to confirm dialog
- Escape HTML in filesystem names (image-browser, image-picker)
- Set textarea value programmatically instead of interpolating in innerHTML
- Add Content-Security-Policy meta tag to index.html
- Fix openExternal hostname check: exact match instead of endsWith
- Add console.debug in preload catch block instead of empty catch
- Escape img.path in data-attribute context

Co-Authored-By: Rooty
@Wintersta7e
Improve error handling across renderer modules
3e5dc6b 
- Add try/catch to saveSettings for QuotaExceededError
- Capture error in getSettings and getLastExport catch blocks
- Show user notification on folder structure load failure
- Surface autosave write failures after 3 consecutive errors
- Add .catch() to OPEN_RECENT_PROJECT async handler
- Remove orphan SCENE_LOADED event listener from init.js
- Add null-check warnings for critical DOM elements in elements.js
- Add debounced image search filter (150ms)
- Add aria-label to notification close button

Co-Authored-By: Rooty
@Wintersta7e
Optimize timeline and preview performance during playback
4a50dd1 
- Add lightweight updateTimelineCursor() for playback (avoids full DOM rebuild at 60fps)
- Add updateMinimapCursor() for cursor-only minimap updates during playback
- Debounce window resize handler on minimap with requestAnimationFrame
- Eliminate O(n²) indexOf in renderPreviewAtFrame by using loop index directly
- Playback uses cursor-only updates; full rebuild reserved for structural changes

Co-Authored-By: Rooty
@Wintersta7e
Fix medium bugs and clean up architecture
021000b 
- Fix double render on Delete key (deleteSelectedEvent already emits RENDER)
- Add debounced undo state for arrow key image movement
- Deduplicate getPreviewScale: drag.js now imports from preview/index.js
- Remove unused event constants (STATE_CHANGED, EVENT_SELECTED, FRAME_CHANGED, SCENE_SAVED, SCENE_LOADED, RENDER_PROPERTIES)
- Remove will-change from virtual dropdown items (reduces GPU memory)
- Update event-bus tests to match removed constants

Co-Authored-By: Rooty
@Wintersta7e
Fix CSS: accessibility, dead rules, duplicate selectors, notification…
3403a14 
… keyframes

- Replace outline:none with focus-visible pattern on inputs and search
- Add slideIn/slideOut keyframes (moved from notifications.js)
- Remove dead .btn[title] rule
- Remove unnecessary !important on timeline event min-width
- Consolidate duplicate showText/erasePicture selectors
- Scope .btn transition to specific properties instead of 'all'
- Remove will-change and backface-visibility from dropdown items
- Add focus-visible support to virtual-dropdown-search
- Add aria-labels and semantic HTML (section instead of footer)

Co-Authored-By: Rooty
@Wintersta7e Wintersta7e merged commit 7da61a8 into main Mar 13, 2026
1 check passed
@Wintersta7e Wintersta7e deleted the code-review-fixes branch March 13, 2026 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Wintersta7e