Skip to content

refactor: code review fixes and maintainability improvements#45

Merged
flexiondotorg merged 32 commits intomainfrom
cleanup
Mar 25, 2026
Merged

refactor: code review fixes and maintainability improvements#45
flexiondotorg merged 32 commits intomainfrom
cleanup

Conversation

@flexiondotorg
Copy link
Copy Markdown
Member

@flexiondotorg flexiondotorg commented Mar 25, 2026

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

  • Code review resolution: Addressed 10 documented concerns from codebase audit
  • DRY violation removal: Eliminated duplication in integrations, utilities, and type handling
  • Type safety improvements: Added Electron module augmentation, typed D-Bus interfaces, typed event payloads
  • Pattern upgrades: Replaced boolean toggles with extensible named themes, self-nullifying closures with event handlers, positional arguments with context objects
  • Concern extraction: Separated storefront, theme, and product info into dedicated modules
  • Test infrastructure: Added Phase 1 test foundation with unit tests for config, wedgeDetector, player, storefront, and utilities
  • Bug fixes: Corrected app name casing in about window and tray menu, fixed MPRIS volume initialization
  • Documentation: Updated AGENTS.md and SPECIFICATION.md with refactoring learnings
  • i18n refactor: Extracted translation records to JSON files for maintainability
  • Dead code removal: Cleaned up obsolete code paths and simplifications

Testing

  • All 195 existing tests pass
  • Added 21 new test cases covering config, wedgeDetector, player, storefront, and utilities
  • Manual validation of UI changes (app name, theme system, tray menu)
  • IPC bridge and player command handlers verified with new receive channel tests

- 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>
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@flexiondotorg flexiondotorg merged commit dcfee6b into main Mar 25, 2026
12 checks passed
@flexiondotorg flexiondotorg deleted the cleanup branch March 25, 2026 02:39
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