refactor: code review fixes and maintainability improvements#45
Merged
flexiondotorg merged 32 commits intomainfrom Mar 25, 2026
Merged
refactor: code review fixes and maintainability improvements#45flexiondotorg merged 32 commits intomainfrom
flexiondotorg merged 32 commits intomainfrom
Conversation
- Remove unused disable() function from MPRIS integration - Remove redundant autoUpdater.autoDownload assignment in win32 branch - Remove unused UPDATE_DOWNLOADING_TEXT and UPDATE_FAILED_TEXT i18n records - Remove unused downloading/failed fields from getAutoUpdateStrings() - Update i18n-consistency tests to match removed records Signed-off-by: Martin Wimpress <code@wimpress.io>
- Extract _exec() helper in MPRIS to consolidate 9 executeJavaScript+catch blocks - Define NowPlayingPayload interface in player.ts, replace duplicate type casts - Export PlaybackState constants, replace magic numbers 2/3/4 in 3 files - Add errorMessage() utility, replace duplicate error.instanceof guard - Extract refresh() closure in tray menu to consolidate 16 identical rebuild calls - Create src/utils.ts for shared error handling logic Signed-off-by: Martin Wimpress <code@wimpress.io>
- main: splash/content/update timing and window dimensions (8 constants) - tray: about window dimensions (2 constants) - update: semver component count and timeout (2 constants) - notifications: debounce delay and artwork download timeout (2 constants) - mpris: volume echo tolerance and ms-to-microsecond conversion (2 constants) - Total: 16 constants introduced across 5 files for improved maintainability and clarity Signed-off-by: Martin Wimpress <code@wimpress.io>
…ants - Add utils.test.ts: 6 cases for errorMessage() handling Error, primitives, null, undefined, custom toString - Add player.test.ts: 3 cases verifying PlaybackState enum values (Playing=2, Paused=3, Stopped=4) Signed-off-by: Martin Wimpress <code@wimpress.io>
- Inline enable() function into init() in MPRIS integration, remove bridging variables - Remove CATPPUCCIN_TEXT and STYLE_APPLE_MUSIC_TEXT identity translations from i18n - Remove pkg require from i18n.ts, use app.getName() instead of pkg.build?.productName - Use errorMessage() utility in update.ts instead of inline duplicate - Remove duplicate detail field in autoUpdate dialog.showMessageBox call - Extract STYLE_FIX_CSS inline string to assets/styleFix.css, add to asarUnpack - Update i18n consistency tests to reflect removed translation records - Total: 127 tests passing (down 8 from removed consistency assertions) Signed-off-by: Martin Wimpress <code@wimpress.io>
- Extract storefront logic to src/storefront.ts (buildAppleMusicURL, extractStorefrontFromURL, handleStorefrontNavigation) - Extract theme management to src/theme.ts (initCatppuccinCSS, state) - Add getProductInfo() to src/paths.ts to encapsulate package.json reads - Remove pkg require from tray.ts, replace with getProductInfo() - Reduce setupContentHandlers parameter list from 6 to 4 by passing assets object - Update test import for extracted URL functions - Fixes inappropriate intimacy, divergent change, and long parameter list smells Signed-off-by: Martin Wimpress <code@wimpress.io>
…it states - Replace untyped EventEmitter with generic TypedEmitter<Events> class - Define PlayerEvents interface mapping event names to typed payload tuples - Expand PlaybackState constants from 3 to all 10 MusicKit states (0-9) - Add PlaybackStatePayload type for strongly-typed state values - Remove payload: unknown and inline 'as' casts from all event handlers - Add unit tests for all 10 PlaybackState values - Update player.test.ts with type-level assertions for PlayerEvents Signed-off-by: Martin Wimpress <code@wimpress.io>
- Remove all 15 'payload as X' casts from event handlers - Import typed PayloadState and NowPlayingPayload from player.ts - Update handler signatures in wedgeDetector, discord-presence, notifications, mpris - Replace 'payload: unknown' parameters with strongly-typed payload types - Remove redundant type guards (now enforced by TypeScript) - Preserve MPRIS named function declarations for removeListener identity - All consumers now receive compiler-verified event shapes - Fixes Primitive Obsession smell: no more unsafe casts across the event pipeline Signed-off-by: Martin Wimpress <code@wimpress.io>
- Define IntegrationContext interface in src/player.ts - Update all 5 integration init sites to accept context object - Replaces (player, getMainWindow) positional pairs with single parameter - Enables adding shared resources without touching every integration signature - Add runtime guards in wedgeDetector and MPRIS for required getMainWindow - Notifications falls back to () => null for optional window access - Fixes Data Clumps smell: cohesive context object instead of scattered args Signed-off-by: Martin Wimpress <code@wimpress.io>
- Add 6 update methods encapsulating field mutation logic (updatePlaybackStatus, updateNowPlaying, updateRepeatMode, updateShuffleMode, updateVolume, updatePosition) - Add cleanup() method replacing module-level cleanupState() - Move all state into the class: seek detection, volume suppression, debounce timing - Remove playerIfaceRef module variable and all module-level state variables - Rewire 6 event handlers to delegate to class methods via arrow-function wrappers - Make _playbackStatus, _loopStatus, _shuffle, _metadata, _volume, _position, _currentTrackId truly private - Preserve removeListener stability via const arrow functions in init() closure - D-Bus property emissions and semantics unchanged - Fixes Inappropriate Intimacy smell: class now enforces its own invariants Signed-off-by: Martin Wimpress <code@wimpress.io>
…rtup Hook now fires volumeDidChange unconditionally when it first attaches to MusicKit, before registering the change listener. This ensures MPRIS receives the actual volume immediately instead of staying at the 1.0 default when volume is static. Remove broken executeJavaScript seed from init() which was silently falling back to 1.0 because MusicKit was not yet available at that point in the startup sequence. Signed-off-by: Martin Wimpress <code@wimpress.io>
Add test/storefront.test.ts: 7 tests for handleStorefrontNavigation covering persistence logic, null fallbacks, language override, and state retention. Add test/player.test.ts: 6 tests verifying that Player handler methods correctly emit typed events (playbackStateDidChange, nowPlayingItemDidChange, etc.). Signed-off-by: Martin Wimpress <code@wimpress.io>
- Add Vitest to technology stack - Update project structure with new modules: storefront.ts, theme.ts, utils.ts - Document Player class enhancements: TypedEmitter, PlayerEvents, IntegrationContext - Add test/ directory listing (7 test files, 149 tests) - Update i18n record list: now 22 records (was 12), remove identity translations - Add getProductInfo() to paths.ts description Signed-off-by: Martin Wimpress <code@wimpress.io>
- Create src/types/electron.d.ts with augmentations for
App.setDesktopName() and getPath('cache')
- Remove redundant type cast in src/main.ts
- Remove redundant type cast in src/integrations/notifications/index.ts
Eliminates type-safety violations whilst maintaining runtime behaviour.
Signed-off-by: Martin Wimpress <code@wimpress.io>
Replace unsafe `(bus as any)` cast with `DbusMessageBusInternals` interface that documents the internal structure of dbus-next's MessageBus object. The interface captures the `_connection?.stream?.destroy` access path with optional chaining, and includes a version-pinning comment referencing @holusion/dbus-next 0.11.2. This improves type safety whilst documenting the intentional reliance on internal APIs. Signed-off-by: Martin Wimpress <code@wimpress.io>
…ndler pattern - Extract CSS/hook injection into local injectContent() helper - Register single did-finish-load handler for predictable execution order - Use initialized flag to ensure integration init runs only on first load - Simplify event handler registration and eliminate self-nullifying side effects This replaces the self-nullifying initIntegrationsOnce closure with explicit event handler registration, improving code clarity and maintainability. The initialized flag guarantees integration setup occurs only once, preserving the original ordering guarantee that injection completes before init runs. Signed-off-by: Martin Wimpress <code@wimpress.io>
…ystem
- Replace toggleCatppuccin(enabled) with applyTheme(name: ThemeName)
- Introduce ThemeName union type ('apple-music' | 'catppuccin')
- Add extensible themeCssMap for mapping theme names to CSS files
- Make apple-music a first-class theme option (no override CSS)
- Update config to store theme name instead of boolean flag
- Update all call sites in main.ts and tray.ts
Enables straightforward addition of future themes: add name to ThemeName
union, register in themeCssMap, include CSS file, and add radio option.
Signed-off-by: Martin Wimpress <code@wimpress.io>
Extract 11 identical vi.mock() blocks from url.test.ts and storefront.test.ts into shared fixture at test/mocks/storefront-deps.ts. Each test file previously contained a 56-line mock declaration block for application modules (config, i18n, paths, storefront, etc.). Consolidate into single source of truth with corrected module resolution paths (../../src/) for the deeper fixture location. - Create test/mocks/storefront-deps.ts with shared vi.mock() declarations - Replace inline mocks in test/url.test.ts with fixture import - Replace inline mocks in test/storefront.test.ts with fixture import Signed-off-by: Martin Wimpress <code@wimpress.io>
- Add src/types/electron.d.ts to project structure - Reorganise src/theme.ts documentation with implementation details - Add test/mocks/storefront-deps.ts with scope notes - Update configuration table: remove implicit catppuccin.enabled, add theme: ThemeName - Document six architecture patterns: CastLabs type augmentation, D-Bus typing with version pin, event handler initialization flag, theme extensibility union, test mock fixture paths, promise queue for theme application - Correct theming persistence options: apple-music, catppuccin (remove default) - Expand theming implementation with applyTheme(), themeCssMap, and theme extension recipe Signed-off-by: Martin Wimpress <code@wimpress.io>
…nt Norwegian entries - Add RESTART_NOW_TEXT and LATER_TEXT to all 32 language records for autoUpdate dialog - Replace hardcoded button labels in autoUpdate.ts with localised strings - Remove redundant 'no' (Norwegian) entries from all 24 translation records; 'no' is non-standard BCP 47 and unreachable due to locale fallback resolution - All translation records now contain 32 language keys (down from 33) - Update AGENTS.md: remove stale integration.ts reference, correct record count to 24 - Update README.md: correct supported language count to 32 Signed-off-by: Martin Wimpress <code@wimpress.io>
Establishes comprehensive test coverage for existing modules before source code changes: - config.test.ts: 18 type assertion tests for StoreSchema keys, 8 runtime behaviour tests - wedgeDetector.test.ts: 9 tests for stall detection logic, thresholds, skip attempts, reset behaviour, track changes - player.test.ts: 11 skipped payload validation tests for Phase 2 implementation Establishes the defence-in-depth test foundation to validate existing behaviour before source code changes in future development. Signed-off-by: Martin Wimpress <code@wimpress.io>
Harden IPC boundaries with type-narrowing validation at the message handler layer. Each of the six handle* methods now validates incoming payload shapes before processing: - playbackStateDidChange: validates status (boolean) and state (number) - nowPlayingItemDidChange: validates NowPlayingPayload shape - playbackTimeDidChange: validates finite number - repeatModeDidChange: validates number or null - shuffleModeDidChange: validates number or null - volumeDidChange: validates number or null Invalid payloads log a warning via playerLog.warn and return early, preventing malformed data from propagating to the event emitter. Unskipped 11 validation tests in test/player.test.ts (all now pass). This addresses the IPC validation gap identified in the security audit. The preload layer already allowlists channels; the main process now validates payload shapes at runtime for defence-in-depth hardening. Signed-off-by: Martin Wimpress <code@wimpress.io>
Add structured IPC receive channels to the preload script, eliminating string interpolation-based command execution. Implements 9 player command channels with runtime argument validation: - player:play, player:pause, player:playPause - player:next, player:previous - player:seek (validated as finite number) - player:setVolume (validated as finite number) - player:setRepeat, player:setShuffle (validated against allowed modes) Follows the existing SEND_CHANNELS allowlist pattern for consistency. Each handler validates arguments before dispatching to window.__sidra.* methods, ensuring type safety across the IPC boundary. Foundational infrastructure which will migrate MPRIS and wedgeDetector to use webContents.send() instead of executeJavaScript, improving overall IPC architecture. Signed-off-by: Martin Wimpress <code@wimpress.io>
…r commands Fixes MPRIS write operations (pause, next, stop, shuffle) which failed due to contextIsolation boundary. The preload runs in an isolated world and could not directly call window.__sidra methods in the main world where musicKitHook.js sets them up. Solution: preload uses window.postMessage() to cross the isolation boundary. musicKitHook.js listens for 'sidra:command' messages and dispatches to the corresponding window.__sidra method. Maintains structured IPC without string interpolation. - src/preload.ts: receive handlers now postMessage instead of direct __sidra calls - assets/musicKitHook.js: added message listener for sidra:command events - AGENTS.md: updated architecture note to document the postMessage bridge pattern Signed-off-by: Martin Wimpress <code@wimpress.io>
…ue-tracking Volume echo suppression now tracks the pending value directly instead of using a fixed 500ms timer. When the echo arrives (matching value within tolerance), suppression clears immediately. A 2000ms safety timeout prevents permanent suppression if the echo never arrives (e.g. renderer crash). This improves accuracy: genuine volume changes within 500ms of an MPRIS volume set are no longer incorrectly suppressed, and echo suppression does not overstay its welcome if the round-trip time exceeds 500ms. - Replaced _volumeSuppressionMs timer with _volumeSafetyMs safety timeout - updateVolume clears _pendingVolume on echo match (value within tolerance) - Safety timer at 2000ms prevents permanent suppression - VOLUME_ECHO_TOLERANCE remains 0.01 Signed-off-by: Martin Wimpress <code@wimpress.io>
Separates 850 lines of translation data from 150 lines of logic in src/i18n.ts. Translation records now live in assets/locales/ as JSON: - assets/locales/loading.json: LOADING_TEXT - assets/locales/tray.json: 14 tray menu records - assets/locales/about.json: 4 about dialog records - assets/locales/update.json: 5 auto-update records src/i18n.ts reduced from 998 lines to 179. Loads JSON via fs.readFileSync + getAssetPath() and re-exports all 24 named records, preserving import compatibility. Updated AGENTS.md project structure, i18n.ts description, and "Adding translations" section to reference JSON files. Updated docs/SPECIFICATION.md similarly. All 4 locale JSON files added to asarUnpack in package.json (explicit entries, no globs). Signed-off-by: Martin Wimpress <code@wimpress.io>
Aligns the TypeScript type with fields actually sent by musicKitHook.js in the nowPlayingItemDidChange payload. Both fields are optional. - audioTraits?: string[] - audio quality indicators (e.g. "Lossless", "Hi-Res Lossless") - targetBitrate?: number - the audio bitrate in kbps These fields enable upcoming feature work for displaying audio quality information in Discord Rich Presence and other integrations. Previously, these fields were silently dropped after arriving via IPC. Now they flow through to the Player EventEmitter and are available to integrations that need them. Signed-off-by: Martin Wimpress <code@wimpress.io>
- Rename logger scopes to match their modules (theme, storefront) - Load musicKitHook.js once in loadAssets() instead of per page load - Derive PLAYBACK_STATES from PlaybackState constant - Extract TrayStrings named interface from inline type definition - Consolidate duplicate language guard in storefront - Extract buildTrackId() helper to eliminate duplication in mpris - Document require() calls with rationale comments - Fix: About window now displays "Sidra" using pkg.build.productName - Fix: Tray "About Sidra" menu item uses getProductInfo().productName Signed-off-by: Martin Wimpress <code@wimpress.io>
Contributor
There was a problem hiding this comment.
5 issues found across 35 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/storefront.ts">
<violation number="1" location="src/storefront.ts:78">
P2: Do not coerce missing language to `null`; preserve `undefined` so URLs without `?l=` are treated as no language change.
(Based on your team's feedback about not clearing/persisting language when Apple Music omits the `l` parameter.) [FEEDBACK_USED]</violation>
</file>
<file name="src/theme.ts">
<violation number="1" location="src/theme.ts:23">
P1: `applyTheme()` can throw before `initThemeCSS()` runs because `applyThemeCSSInternal` is uninitialized.</violation>
</file>
<file name="test/storefront.test.ts">
<violation number="1" location="test/storefront.test.ts:32">
P1: This test asserts incorrect behavior for URLs without `l`: it expects `setLanguage(null)`, but absence of `l` should mean no language change (including when current language is undefined).
(Based on your team's feedback about preserving language when `l` is missing.) [FEEDBACK_USED]</violation>
</file>
<file name="test/config.test.ts">
<violation number="1" location="test/config.test.ts:9">
P2: The test mocks `../src/config` itself, so runtime assertions only test the mock and not the real config implementation.</violation>
</file>
<file name="test/i18n-consistency.test.ts">
<violation number="1" location="test/i18n-consistency.test.ts:12">
P2: This list now omits `RESTART_NOW_TEXT` and `LATER_TEXT`, so i18n consistency checks no longer cover all exported auto-update translation records.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Static HTML files (about.html, splash.html) are renderer assets with no build step. Co-locating them with other static assets (JS, CSS) improves project organisation and simplifies asset handling via getAssetPath(). Signed-off-by: Martin Wimpress <code@wimpress.io>
- Guard against init-order changes in theme initialization - Prevent spurious setLanguage(null) calls when ?l= parameter absent - Add test assertion for language preservation without parameter - Expand i18n consistency test coverage Signed-off-by: Martin Wimpress <code@wimpress.io>
- Update about.html and splash.html references from src/ to assets/ - Line 25: about.html → assets/about.html - Priority matrix row 4: add assets/about.html context Reflects earlier file reorganisation moving static HTML assets to assets/ directory. Signed-off-by: Martin Wimpress <code@wimpress.io>
Add RESTART_NOW_TEXT and LATER_TEXT to the TRANSLATION_RECORDS array to ensure all translation keys are validated in the i18n consistency test. Signed-off-by: Martin Wimpress <code@wimpress.io>
Contributor
There was a problem hiding this comment.
0 issues found across 10 files (changes from recent commits).
Requires human review: This is a massive refactor (4400+ lines) involving core business logic reorganization, architectural changes, and D-Bus integration updates, which requires human domain knowledge.
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
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
This branch consolidates the results of a comprehensive code review by Donatello (verdict: "Impressed") and implements seven maintainability refactors across the Electron/TypeScript codebase. The work eliminates code duplication, improves asset loading efficiency, enforces naming consistency, extracts interfaces for type safety, and addresses identified concerns. All 195 tests pass.
Changes
Testing