tech-debt: tiers 2+3 refactoring#64
Open
sshcrack wants to merge 36 commits into
Open
Conversation
- Added virtual destructors to Scene, PostProcessingEffect, TransitionEffect, Post
- Rewrote PropertyMacros.h to use std::make_shared (avoids {} forwarding issue)
- Removed void(*)(T*) custom-deleter from unique_ptr return types in factory methods
- Converted all {new T(), [](T*){}} and unique_ptr<T, void(*)(T*)>(new T(), ...)
patterns to std::make_unique<T>()
- Fixed PixelJoint ScrapedPost internal types
- Fixed all create_scenes/create_effects/create_transitions/create_image_providers
factory methods across ~50 plugin files
- Removed unused deleteWrapper/deleteScene lambdas
sshcrack
commented
Jun 18, 2026
sshcrack
left a comment
sshcrack
left a comment
Owner
Author
There was a problem hiding this comment.
Code Review
This is a well-structured refactoring PR that consolidates duplicate code, modernizes C++ usage, improves CI/CD, and tightens security/safety. The extraction of shared color utilities, the apply_pixel_loop template, and the helper functions in scene_management.cpp all reduce code duplication meaningfully. The switch from rand() to <random>, from unique_ptr with custom deleters to make_unique/shared_ptr, and from file(GLOB_RECURSE) to explicit source lists are all good improvements.
I found one bug that will break the desktop app's ability to turn the matrix on/off during shutdown and window iconify.
…ath resolution - Add DEB packaging with debconf configuration, systemd service, and FHS layout - Rename binary from main to led-matrix for service ExecStart consistency - Fix matrix plugin loader to check PLUGIN_DIR env var, then dev plugins/ dir, then FHS path (/usr/lib/led-matrix/plugins/) - matches desktop loader - Set WorkingDirectory=/var/lib/led-matrix in systemd unit so images/ working dir resolves under data directory - Fix Shadertoy plugin custom_shader_dir() to use LED_MATRIX_DATA_DIR instead of get_exec_dir() /data/custom_shaders - Remove stale install rule from shared/matrix/CMakeLists.txt that duplicated the root CMakeLists.txt's install to wrong path - Fix Countdown/WeatherOverview plugin font install destinations - Remove dead countdown_font_dir constant with old path - Rewrite update_service.sh for dpkg -i workflow - Rewrite install_led_matrix.sh for .deb download + dpkg install - Delete obsolete scripts/release.ps1
Migrate to DEB packaging with FHS paths and fix plugin loader path resolution
…897→3 files) - WeatherScene.cpp split into WeatherScene.cpp (core dispatch), weather_rendering.cpp (cloud/lightning/fog/aurora/rainbow/utilities), weather_particles.cpp (rain/snow/shooting-stars), weather_cache.cpp (icon download/cache management) - SortingVisualizerScene.cpp split into SortingVisualizerScene.cpp (core+render), sorting_simple.cpp (bubble/selection/insertion/cocktail/gnome), sorting_complex.cpp (shell/quick/heap/comb/merge) - tier2: fix vcpkg_features.cmake JSON parsing (CMake 4.x MEMBER syntax) - fix: add missing consts.h include to update_routes.cpp, pass .get() for shared_ptr in server.cpp - fix: remove fs::create_directories side effect from GET /api/custom-assets/:type - fix: add /uploads/:filename route for uploaded file serving - fix: wire CollectionProvider upload to /pixeljoint/upload_img endpoint
…and docs - build_upload.sh: remove --delete flag, sync to /usr/ not root - canvas.cpp: add LED_MATRIX_SHARE_DIR fallback guard - desktop/main.cpp: update g_should_turn_off_on_exit on toggle, replace std::async with std::thread - PixelJoint ImageScene: convert remaining custom-deleter unique_ptrs - WeatherParser/WeatherScene: convert localtime() to localtime_r() - PropertyDemoScene: replace C-style casts with static_cast - AGENTS.md, README.md: update CI docs, binary name, DEB install instructions
…cate HTTP) - post_processing_routes.cpp: read params from POST body not query string - main.cpp: remove duplicate shutdown_matrix call and stale globals - CMakeLists.txt: remove undefined LIB_NAMES variable - data.h: remove dead from_json declarations for raw pointers
…Layout useMatch, CI glob, dead code
…60 factor without rescaling)
…tains(), dead code, missing locks, type safety
…tale [ ! -f ] guard)
…ardcode, sudo removal, emulator render, scene exclusion, dangling ref, TOCTOU, hue normalization, thread ordering, gitignore, preview_gen install, cmake version consistency
…web, packaging, and CI - Use-after-free: reorder cleanup in main.cpp (delete config after UpdateManager stop) - Update release asset filter from .tar.gz to .deb (update_routes.cpp) - Quote Spotify credentials in debconf-generated EnvironmentFile (postinst) - Guard dpkg -i with DEBIAN_FRONTEND=noninteractive (update_service.sh) - Make debconf config idempotent: skip prompts when answers already exist - Update config.json version field on upgrades via jq (postinst) - Consolidate db_go calls into single call at end (config) - Replace literal 'false' with '' in build-matrix.yml artifact paths - Add RestartSec=5 to systemd unit (led-matrix.service.in) - Guard vcpkg_features.cmake against empty features object - Use specific glob build/led-matrix-*.deb in build_upload.sh - Fix postrm to remove specific files instead of rm -rf whole dir - Fix FallingSandScene random direction bias (== 0 check) - Change showMainWindow to std::atomic<bool> for thread safety - Add try-catch around WebsocketClient::shared_from_this() (bad_weak_ptr) - Flush D-Bus connection after send in single_instance_manager - Disable noUnusedLocals/Parameters in tsconfig.json - Add try/catch + error toast to confirmUpload in AssetManager - Move ErrorBoundary to wrap Layout, add programmatic reset - Remove pre-existing unused imports and dead code references Fixes identified during comprehensive PR review with 4 parallel subagents covering build system, plugins, daemon/desktop/shared libs, and web frontend.
…ns, web, packaging, CI - build: fix CI JSON syntax, upload path literal false, install script DEB regex, vcpkg guards, DEBIAN_FRONTEND for automated updates - daemon: shutdown order (UpdateManager first), .tar.gz+.deb dual matching, POST body+query fallback, separate error_code vars, upload path traversal guard - desktop: restore atomic<bool> for showMainWindow, shared_from_this() try-catch, dbus_connection_flush, General mutex locking, waitpid -1 guard - shared: portable localtime_r/localtime_s for MSVC builds - plugins: PingPongGameScene backward-compat (revert defaults+60f mul), SnakeGameScene div-by-zero guard, WeatherScene div-by-zero guards, weather_particles render pos fix, particle init on empty vector - web: ErrorBoundary wraps Layout, setState reload, restore upload error handling, ESM-compatible __dirname - packaging: restore SPOTIFY_ID quotes, update config.json on upgrade, debconf only ask unset questions, postrm targeted purge
…ing, debconf purge, path fixes, misc
…data races, misc bugs
- Add on_first_frame_ready callback to VideoStreamEngine - Dual-engine architecture on desktop: current (playing) + pending (loading) - Crossfade (500ms default, configurable via ImGui slider) between old and new - Desktop sends status:pending while new engine loads, status:playing on swap - Matrix renders old frames first, overlays loading perimeter trail during pending - render_overlay() skips Fill(0,0,0) so it draws on top of video frames - Lock ordering: engine_mutex_ → track_id_mutex_ to avoid deadlock - Dedup: same track → ignore; same as current with different pending → cancel pending
- WeatherScene: remove dead updateAnimationState, add missing includes (<algorithm>, <ctime>), call updateEnhancedParticles unconditionally - VideoStreamEngine (Windows): CloseHandle pi.hProcess/pi.hThread leak; kill ffmpeg in stop() to prevent deadlock on join - postinst: escape EXTRA_OPTS and Spotify creds for systemd env file parser; quote DATA_DIR in find argument - pr-autoreview-loop: return False on error (not True), fix greedy regex for nested JSON, fix cumulative bug count - FractalScenes/ExampleScenes/GameScenes: multiply hue by 360.0f for color::hsv_to_rgb which expects degrees, not [0,1) - install_led_matrix.sh: add --fail to curl, add trap for temp cleanup - utils.cpp: add missing #include <cerrno> - AssetManager.tsx: add toast error in catch block - weather_rendering.cpp: qualify abs/fmod with std:: - SortingVisualizerScene: fix comb sort highlighting (use contains_index instead of stale j member) - CMakeLists.txt: remove orphaned get_property, stray comment, fix bash -c quoting - config: remove dead db_get call - build_upload.sh: remove redundant systemctl restart - update_service.sh: fix misleading error message
Bugs fixed: - src_matrix/main.cpp: Use-after-free — destroy plugins before globals - src_matrix/canvas.cpp: Transition early-exit loses forced_scene - src_matrix/scene_management.cpp: Silent skip on null get_default() - src_desktop/main.cpp: Duplicate csignal include - VideoStreamEngine.cpp: ffmpeg_pid_ not reset in stop() - WeatherScene.cpp: last_weather_fetch updated before error check - WeatherScene.h: Missing chrono include, unused ANIMATION_INTERVAL - weather_rendering.cpp: Aurora false-positive on N/A data, fog not reset - Constants.h: Dead TINY_FONT declaration - SpotifyMVDesktop.cpp: 7 race/lock/TOCTOU/stol bugs - SpotifyMVScene.cpp: once_flag prevents re-detection - sorting_complex.cpp: Heap sort OOB (-1), comb sort pacing - Transitions.cpp: MorphTransition black pixel blend broken - Countdown.cpp: Duplicate Countdown in font path - Scraper.cpp: Empty shaderIds OOB, redundant fetchInProgress - PI.cmake: DEFINED check always false - postinst: sed escaping, dpkg-query format, heredoc safety - .gitignore: Concatenated entries split - CMakeLists.txt: Literal newline in changelog, project name - pr-autoreview-loop.py: Iteration oscillation, bug count inflation - run_emulator.sh: Unquoted script_dir - install_led_matrix.sh: Trap after mktemp - update_package_version: CONFIGURE_TIME mutation side effect - Home.tsx: POST→GET mismatch, missing res.ok checks - EnumProperty.tsx: Unsafe casts, empty state, useEffect deps
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.
Implements medium and low priority refactoring from plans/tier2-medium.md and plans/tier3-low.md.