Skip to content

Rewrite plugin in TypeScript with rendering performance optimizations#1

Open
kelchm wants to merge 2 commits into
mainfrom
claude/rewrite-typescript-IS3NF
Open

Rewrite plugin in TypeScript with rendering performance optimizations#1
kelchm wants to merge 2 commits into
mainfrom
claude/rewrite-typescript-IS3NF

Conversation

@kelchm

@kelchm kelchm commented Apr 1, 2026

Copy link
Copy Markdown
Owner
  • Migrate from JavaScript to TypeScript with strict mode
  • Cache processed album art buffer (only re-process on track change)
  • Cache text SVG (only rebuild when track name/artist changes)
  • Render only the needed 200x100 dial half instead of full 400x100 then crop
  • Inline progress bar into text area SVG layer (eliminate extra composite)
  • Throttle media event rendering to max once per 500ms
  • Skip keypad re-renders when track info hasn't changed
  • Use JPEG encoding (vs PNG) for LCD output - smaller payloads, faster encode
  • Use Promise.all for parallel action updates
  • Replace file-based debug logging with SDK logger
  • Use class-based SingletonAction registration (proper SDK pattern)
  • Track LCD positions via Map instead of injecting properties on action objects
  • Replace build.js file-copy with tsc
  • Remove test-renderer.js (was a manual smoke test)

Summary by CodeRabbit

  • New Features

    • Plugin migrated to TypeScript with improved stability and performance.
    • Throttled rendering for smoother, less CPU-intensive updates.
    • In-memory caching for album art and track metadata to speed visuals.
    • Enhanced dial rendering with left/right layouts and robust fallback/idle images.
    • Playback interactions: toggle play/pause and seek via dial controls.
  • Chores

    • Build system moved to TypeScript compilation; added clean/rebuild scripts and TypeScript config.

- Migrate from JavaScript to TypeScript with strict mode
- Cache processed album art buffer (only re-process on track change)
- Cache text SVG (only rebuild when track name/artist changes)
- Render only the needed 200x100 dial half instead of full 400x100 then crop
- Inline progress bar into text area SVG layer (eliminate extra composite)
- Throttle media event rendering to max once per 500ms
- Skip keypad re-renders when track info hasn't changed
- Use JPEG encoding (vs PNG) for LCD output - smaller payloads, faster encode
- Use Promise.all for parallel action updates
- Replace file-based debug logging with SDK logger
- Use class-based SingletonAction registration (proper SDK pattern)
- Track LCD positions via Map instead of injecting properties on action objects
- Replace build.js file-copy with tsc
- Remove test-renderer.js (was a manual smoke test)

https://claude.ai/code/session_01X7DW5Xkw2PhRTd3tabdrcj
Copilot AI review requested due to automatic review settings April 1, 2026 23:49
@coderabbitai

coderabbitai Bot commented Apr 1, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Replaced JavaScript implementations and build script with TypeScript equivalents: removed build.js, rewrote src/plugin and src/renderer in TypeScript, added tsconfig.json, updated package.json (version bump and scripts), removed legacy test script, and introduced TypeScript dev dependencies and build lifecycle scripts.

Changes

Cohort / File(s) Summary
Plugin rewrite
src/plugin.ts, src/plugin.js
Added src/plugin.ts implementing Stream Deck actions with a throttled render scheduler, playback controls, and instance lifecycle handling; removed old src/plugin.js.
Renderer rewrite
src/renderer.ts, src/renderer.js
Added src/renderer.ts producing per-dial images with artwork and SVG/text caches and fallback/blank generators; removed old src/renderer.js.
Build & config
build.js, package.json, tsconfig.json
Deleted custom build.js. package.json bumped to v0.2.0, switched build/test scripts to tsc, added clean/rebuild scripts and TypeScript dev dependencies. Added tsconfig.json configuring compilation from src/bin/ with strict options.
Tests / scripts
test-renderer.js
Removed legacy standalone renderer test script.

Sequence Diagram(s)

sequenceDiagram
    participant StreamDeck as Stream Deck
    participant Plugin as plugin.ts
    participant Scheduler as Render Scheduler
    participant NowPlaying as NowPlaying Service
    participant Renderer as renderer.ts
    participant Actions as Keypad/LCD Actions

    NowPlaying->>Plugin: emit nowPlayingEvent(trackInfo)
    Plugin->>Plugin: update currentTrackInfo
    Plugin->>Scheduler: scheduleRender()
    
    alt Render already scheduled?
        Scheduler->>Scheduler: set queuedUpdate flag
    else First schedule
        Scheduler->>Scheduler: wait RENDER_THROTTLE_MS
    end
    
    Note over Scheduler: Timer fires
    Scheduler->>Actions: iterate keypadContexts
    Actions->>Renderer: render(trackInfo, "left")
    Renderer-->>Actions: base64 image
    Actions->>StreamDeck: updateTitle/updateImage
    
    Scheduler->>Actions: iterate lcdContexts
    Actions->>Renderer: render(trackInfo, "right")
    Renderer-->>Actions: base64 image
    Actions->>StreamDeck: updateImage(position)
    
    alt queuedUpdate flag set?
        Scheduler->>Scheduler: schedule another render
    end
    
    User->>StreamDeck: press keypad / rotate dial
    StreamDeck->>Actions: keydown / dialRotate event
    Actions->>NowPlaying: playPause() / seekTo(offset)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I hopped from JS into TypeScript's glen,

Throttled my renders and cached art again,
Bin cleared and rebuilt, new types in my lair,
Dial spins and keypresses echo with care,
A tiny rabbit applauds this tidy affair.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 57.89% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the two main changes: migrating from JavaScript to TypeScript and implementing rendering performance optimizations (caching, reduced composite steps, JPEG encoding, throttling).
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/rewrite-typescript-IS3NF

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR migrates the Stream Deck Now Playing plugin from JavaScript to strict TypeScript while introducing several rendering and update-path performance optimizations.

Changes:

  • Rewrites the plugin and renderer in TypeScript and switches the build to tsc outputting to bin/.
  • Adds caching/throttling to reduce redundant keypad/LCD updates and optimize rendering work.
  • Updates output encoding and simplifies composition steps to reduce payload/processing overhead.

Reviewed changes

Copilot reviewed 7 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tsconfig.json Adds strict TypeScript compiler configuration targeting ES2022 and emitting to bin/.
src/renderer.ts New Sharp-based renderer with caching, half-width dial rendering, and JPEG output.
src/plugin.ts New TypeScript plugin implementation using SingletonAction pattern, throttled renders, and cached keypad updates.
package.json Switches build to tsc, adds clean/rebuild scripts, and adds TS/@types dev dependencies.
package-lock.json Locks added TypeScript and Node typings dependencies.
src/renderer.js Removes legacy JavaScript renderer implementation.
src/plugin.js Removes legacy JavaScript plugin implementation and file-based debug logging.
build.js Removes file-copy build script in favor of tsc.
test-renderer.js Removes manual renderer smoke test script.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/renderer.ts
Comment on lines +83 to +86
const progressWidth =
duration && duration > 0 && currentTime != null
? Math.round((currentTime / duration) * BAR_WIDTH)
: 0;

Copilot AI Apr 1, 2026

Copy link

Choose a reason for hiding this comment

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

progressWidth can exceed BAR_WIDTH when currentTime > duration (or be NaN if inputs are unexpected), which will draw the fill rect beyond the background bar. Clamp the computed width to [0, BAR_WIDTH] before emitting the fill rectangle to keep rendering stable.

Suggested change
const progressWidth =
duration && duration > 0 && currentTime != null
? Math.round((currentTime / duration) * BAR_WIDTH)
: 0;
let progressWidth = 0;
if (duration && duration > 0 && currentTime != null) {
const rawWidth = Math.round((currentTime / duration) * BAR_WIDTH);
progressWidth = Number.isFinite(rawWidth)
? Math.min(Math.max(rawWidth, 0), BAR_WIDTH)
: 0;
}

Copilot uses AI. Check for mistakes.
Comment thread src/renderer.ts
Comment on lines +160 to +183
// Left dial: album art (100x100) + beginning of text area
const artBuffer = await processArtwork(trackInfo.thumbnail);
if (artBuffer) {
composites.push({ input: artBuffer, left: 0, top: 0 });
}

// Text + progress bar, offset to start after artwork
const title = trackInfo.trackName || "Unknown Track";
const artist = artistString(trackInfo.artist);
const textSvg = getTextSvg(title, artist);
composites.push({ input: Buffer.from(textSvg), left: TEXT_X, top: 0 });

const progressSvg = buildProgressBarSvg(
trackInfo.trackDuration,
trackInfo.trackProgress,
);
composites.push({
input: Buffer.from(progressSvg),
left: TEXT_X,
top: 0,
});
} else {
// Right dial: continuation of text area (already handled by left in the
// two-dial layout). Show a simplified view with just track info.

Copilot AI Apr 1, 2026

Copy link

Choose a reason for hiding this comment

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

For the left dial, the text/progress SVG (width TEXT_AREA_WIDTH = 192) is composited at left: TEXT_X (108) onto a 200px-wide canvas, so only ~92px of the text area is visible and most track text will be clipped. If the intent is “left dial = album art, right dial = track info” (per manifest tooltip), consider not rendering text on the left dial and instead render the full text/progress only on the right dial (or, if you want a continuous 2-dial layout, render the same full-width text layer but offset it by -DIAL_WIDTH for the right dial).

Suggested change
// Left dial: album art (100x100) + beginning of text area
const artBuffer = await processArtwork(trackInfo.thumbnail);
if (artBuffer) {
composites.push({ input: artBuffer, left: 0, top: 0 });
}
// Text + progress bar, offset to start after artwork
const title = trackInfo.trackName || "Unknown Track";
const artist = artistString(trackInfo.artist);
const textSvg = getTextSvg(title, artist);
composites.push({ input: Buffer.from(textSvg), left: TEXT_X, top: 0 });
const progressSvg = buildProgressBarSvg(
trackInfo.trackDuration,
trackInfo.trackProgress,
);
composites.push({
input: Buffer.from(progressSvg),
left: TEXT_X,
top: 0,
});
} else {
// Right dial: continuation of text area (already handled by left in the
// two-dial layout). Show a simplified view with just track info.
// Left dial: album art only (per tooltip: album art on left dial)
const artBuffer = await processArtwork(trackInfo.thumbnail);
if (artBuffer) {
composites.push({ input: artBuffer, left: 0, top: 0 });
}
} else {
// Right dial: full track info (text + progress bar)

Copilot uses AI. Check for mistakes.
Comment thread src/plugin.ts
Comment on lines +164 to +185
function updateAllActions(): void {
// Keypads: skip if nothing visually changed
const newKeypadKey = keypadCacheKey(currentTrackInfo);
if (newKeypadKey !== lastKeypadCacheKey) {
lastKeypadCacheKey = newKeypadKey;
const keypadUpdates = Array.from(keypadContexts).map((action) =>
updateKeypadAction(action).catch((err) =>
logger.error("Keypad update error:", err),
),
);
Promise.all(keypadUpdates);
}

// LCDs: always update (progress bar changes)
const lcdUpdates = Array.from(lcdContexts).map((action) => {
const position = lcdPositions.get(action.id) ?? "left";
return updateLCDAction(action, position).catch((err) =>
logger.error("LCD update error:", err),
);
});
Promise.all(lcdUpdates);
}

Copilot AI Apr 1, 2026

Copy link

Choose a reason for hiding this comment

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

Promise.all(...) is called but not awaited/returned in updateAllActions(), so it doesn’t provide backpressure and makes it easy to accidentally introduce unhandled rejections later (and concurrent render passes can overlap). Consider making updateAllActions async and awaiting the Promise.all calls (and calling it with void updateAllActions() from scheduleRender), or drop Promise.all and explicitly fire-and-forget with void for clarity.

Copilot uses AI. Check for mistakes.
Comment thread package.json
"sharp": "^0.34.5"
},
"devDependencies": {
"@types/node": "^25.5.0",

Copilot AI Apr 1, 2026

Copy link

Choose a reason for hiding this comment

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

The plugin runtime is Node.js 20 (manifest.json Nodejs.Version), but the PR adds @types/node v25.x. Using a types major that’s newer than the runtime can allow APIs in type-checking that don’t exist at runtime. Consider pinning @types/node to a 20.x version (or matching the declared runtime) to keep types aligned with production behavior.

Suggested change
"@types/node": "^25.5.0",
"@types/node": "^20.0.0",

Copilot uses AI. Check for mistakes.
Comment thread package.json
Comment on lines +7 to +9
"build": "tsc",
"clean": "rm -rf bin",
"rebuild": "npm run clean && npm run build"

Copilot AI Apr 1, 2026

Copy link

Choose a reason for hiding this comment

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

clean uses rm -rf bin, which won’t work in default Windows shells. Since this repo targets Windows/macOS, consider a cross-platform approach (e.g., rimraf, del-cli, or a small Node script) so contributors can run npm run clean/rebuild everywhere.

Copilot uses AI. Check for mistakes.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (4)
src/renderer.ts (2)

28-35: Global caches assume single-track rendering scenario.

The module-level caches for thumbnail and text SVG work well for the current single-track-at-a-time use case. However, if the plugin ever needs to render different tracks concurrently (e.g., multiple media sources), this would cause cache thrashing. This is fine for now but worth documenting.

Consider adding a brief comment:

 // --- Caches ---
+// NOTE: Single-track caching assumes one active media source at a time.
+// For multi-source support, consider keyed caches (Map) instead.

 /** Cache key for the last rendered album art thumbnail. */
 let cachedThumbnailSrc: string | undefined;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer.ts` around lines 28 - 35, The module-level caches
(cachedThumbnailSrc, cachedArtBuffer, cachedTextKey, cachedTextSvg) assume only
one track is rendered at a time and can cause cache thrashing if multiple
different tracks are rendered concurrently; add a concise comment above these
declarations stating this single-track assumption, warn that
concurrent/multi-source rendering would require per-track keys or a map-based
cache (e.g., Map<string, Buffer|string> keyed by track id or
"trackName\0artist"), and mention that changing to per-track caches is the
intended mitigation if concurrent rendering is needed in the future.

54-59: Edge case: malformed data URLs could cause issues.

The split(",")[1] approach assumes exactly one comma separating the MIME header from the base64 data. While rare, malformed data URLs could produce unexpected results. Consider a more defensive approach if robustness is a concern.

♻️ More defensive extraction
 function extractImageBuffer(dataUrl: string): Buffer {
   if (dataUrl.startsWith("data:")) {
-    return Buffer.from(dataUrl.split(",")[1], "base64");
+    const commaIndex = dataUrl.indexOf(",");
+    if (commaIndex === -1) {
+      throw new Error("Invalid data URL: missing comma separator");
+    }
+    return Buffer.from(dataUrl.substring(commaIndex + 1), "base64");
   }
   return Buffer.from(dataUrl, "base64");
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer.ts` around lines 54 - 59, The extractImageBuffer function
assumes dataUrl.split(",")[1] exists; make it defensive by checking that dataUrl
starts with "data:" and contains a comma or a "base64," marker before slicing;
if the separator is missing or the base64 payload is empty, throw or return a
clear error. Update extractImageBuffer to locate the first comma via indexOf or
match a /^data:.*;base64,/ regex, slice the substring after that separator,
validate it's non-empty, and then call Buffer.from(payload, "base64") so
malformed data URLs don't produce undefined behavior.
src/plugin.ts (1)

244-254: Seek direction is based on tick sign, but magnitude is ignored.

The dial rotation handler uses a fixed 5-second seek regardless of how fast/far the user rotates the dial. The ticks value indicates rotation magnitude, but only its sign is used. This may feel unresponsive for quick rotations.

♻️ Consider scaling seek amount by tick count
   override async onDialRotate(
     ev: DialRotateEvent<LcdSettings>,
   ): Promise<void> {
     if (!nowPlayingInstance) return;
     const ticks = ev.payload.ticks;
-    const seekMs = ticks > 0 ? 5000 : -5000;
+    const seekMs = ticks * 2000; // 2 seconds per tick, direction preserved
     const current = currentTrackInfo?.trackProgress ?? 0;
     await nowPlayingInstance.seekTo(current + seekMs).catch((err) => {
       logger.error("Error seeking:", err);
     });
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/plugin.ts` around lines 244 - 254, The onDialRotate handler currently
uses only the sign of ev.payload.ticks and always seeks ±5000ms; change it to
scale the seek amount by the tick magnitude (e.g., seekMs = ev.payload.ticks *
5000) so fast rotations jump farther, ensure you handle zero ticks (no-op) and
negative ticks correctly, call nowPlayingInstance.seekTo(current + seekMs) as
before, and keep the existing catch/logger.error behavior (referencing
onDialRotate, DialRotateEvent, ev.payload.ticks, seekMs,
nowPlayingInstance.seekTo, and currentTrackInfo).
package.json (1)

8-8: Cross-platform compatibility: rm -rf won't work on Windows.

The clean script uses Unix-specific rm -rf which will fail on Windows. Consider using a cross-platform solution.

♻️ Suggested fix using rimraf or shx

Option 1: Use rimraf (add as devDependency):

-    "clean": "rm -rf bin",
+    "clean": "rimraf bin",

Option 2: Use Node.js built-in (no extra dependency):

-    "clean": "rm -rf bin",
+    "clean": "node -e \"fs.rmSync('bin',{recursive:true,force:true})\"",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@package.json` at line 8, The "clean" npm script currently uses the Unix-only
command "rm -rf" and will fail on Windows; update the package.json "clean"
script to a cross-platform approach (e.g., replace the value for the "clean"
script with a call to "rimraf bin" after adding rimraf as a devDependency, or
use "shx rm -rf bin" with shx as a devDependency, or use a Node.js one-liner
that removes the "bin" directory), and ensure the chosen tool (rimraf or shx) is
added to devDependencies and package.json scripts updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/plugin.ts`:
- Around line 164-185: The updateAllActions function launches keypad and LCD
update promises but does not await the Promise.all calls, which can hide errors
and cause race conditions; make updateAllActions async and await both
Promise.all results (for the keypadUpdates and lcdUpdates), keeping the existing
per-promise .catch handlers or alternatively remove per-promise .catch and let
Promise.all reject so callers can observe failures; locate the logic around
keypadCacheKey/lastKeypadCacheKey, keypadContexts, updateKeypadAction and
lcdContexts/updateLCDAction/lcdPositions and add the awaits there (or add a
clear comment if intentional fire-and-forget behavior is required).

---

Nitpick comments:
In `@package.json`:
- Line 8: The "clean" npm script currently uses the Unix-only command "rm -rf"
and will fail on Windows; update the package.json "clean" script to a
cross-platform approach (e.g., replace the value for the "clean" script with a
call to "rimraf bin" after adding rimraf as a devDependency, or use "shx rm -rf
bin" with shx as a devDependency, or use a Node.js one-liner that removes the
"bin" directory), and ensure the chosen tool (rimraf or shx) is added to
devDependencies and package.json scripts updated accordingly.

In `@src/plugin.ts`:
- Around line 244-254: The onDialRotate handler currently uses only the sign of
ev.payload.ticks and always seeks ±5000ms; change it to scale the seek amount by
the tick magnitude (e.g., seekMs = ev.payload.ticks * 5000) so fast rotations
jump farther, ensure you handle zero ticks (no-op) and negative ticks correctly,
call nowPlayingInstance.seekTo(current + seekMs) as before, and keep the
existing catch/logger.error behavior (referencing onDialRotate, DialRotateEvent,
ev.payload.ticks, seekMs, nowPlayingInstance.seekTo, and currentTrackInfo).

In `@src/renderer.ts`:
- Around line 28-35: The module-level caches (cachedThumbnailSrc,
cachedArtBuffer, cachedTextKey, cachedTextSvg) assume only one track is rendered
at a time and can cause cache thrashing if multiple different tracks are
rendered concurrently; add a concise comment above these declarations stating
this single-track assumption, warn that concurrent/multi-source rendering would
require per-track keys or a map-based cache (e.g., Map<string, Buffer|string>
keyed by track id or "trackName\0artist"), and mention that changing to
per-track caches is the intended mitigation if concurrent rendering is needed in
the future.
- Around line 54-59: The extractImageBuffer function assumes
dataUrl.split(",")[1] exists; make it defensive by checking that dataUrl starts
with "data:" and contains a comma or a "base64," marker before slicing; if the
separator is missing or the base64 payload is empty, throw or return a clear
error. Update extractImageBuffer to locate the first comma via indexOf or match
a /^data:.*;base64,/ regex, slice the substring after that separator, validate
it's non-empty, and then call Buffer.from(payload, "base64") so malformed data
URLs don't produce undefined behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ff2f800d-861f-4106-b5dc-5dee2ee5184b

📥 Commits

Reviewing files that changed from the base of the PR and between 3147d31 and 33a45a3.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (8)
  • build.js
  • package.json
  • src/plugin.js
  • src/plugin.ts
  • src/renderer.js
  • src/renderer.ts
  • test-renderer.js
  • tsconfig.json
💤 Files with no reviewable changes (4)
  • build.js
  • test-renderer.js
  • src/renderer.js
  • src/plugin.js

Comment thread src/plugin.ts
Comment on lines +164 to +185
function updateAllActions(): void {
// Keypads: skip if nothing visually changed
const newKeypadKey = keypadCacheKey(currentTrackInfo);
if (newKeypadKey !== lastKeypadCacheKey) {
lastKeypadCacheKey = newKeypadKey;
const keypadUpdates = Array.from(keypadContexts).map((action) =>
updateKeypadAction(action).catch((err) =>
logger.error("Keypad update error:", err),
),
);
Promise.all(keypadUpdates);
}

// LCDs: always update (progress bar changes)
const lcdUpdates = Array.from(lcdContexts).map((action) => {
const position = lcdPositions.get(action.id) ?? "left";
return updateLCDAction(action, position).catch((err) =>
logger.error("LCD update error:", err),
);
});
Promise.all(lcdUpdates);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Missing await on Promise.all calls may hide errors and cause timing issues.

The Promise.all calls on lines 174 and 184 are not awaited, meaning updateAllActions returns immediately while updates run in the background. While the individual promises have .catch() handlers, the function signature and behavior suggest these should complete before returning. This could cause subtle race conditions if updateAllActions is called rapidly.

🔧 Proposed fix to await the promises
-function updateAllActions(): void {
+async function updateAllActions(): Promise<void> {
   // Keypads: skip if nothing visually changed
   const newKeypadKey = keypadCacheKey(currentTrackInfo);
   if (newKeypadKey !== lastKeypadCacheKey) {
     lastKeypadCacheKey = newKeypadKey;
     const keypadUpdates = Array.from(keypadContexts).map((action) =>
       updateKeypadAction(action).catch((err) =>
         logger.error("Keypad update error:", err),
       ),
     );
-    Promise.all(keypadUpdates);
+    await Promise.all(keypadUpdates);
   }

   // LCDs: always update (progress bar changes)
   const lcdUpdates = Array.from(lcdContexts).map((action) => {
     const position = lcdPositions.get(action.id) ?? "left";
     return updateLCDAction(action, position).catch((err) =>
       logger.error("LCD update error:", err),
     );
   });
-  Promise.all(lcdUpdates);
+  await Promise.all(lcdUpdates);
 }

Note: If the fire-and-forget behavior is intentional for performance reasons (to not block the throttle timer), consider adding a comment explaining this design choice and ensure the .catch() handlers are sufficient for error handling.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function updateAllActions(): void {
// Keypads: skip if nothing visually changed
const newKeypadKey = keypadCacheKey(currentTrackInfo);
if (newKeypadKey !== lastKeypadCacheKey) {
lastKeypadCacheKey = newKeypadKey;
const keypadUpdates = Array.from(keypadContexts).map((action) =>
updateKeypadAction(action).catch((err) =>
logger.error("Keypad update error:", err),
),
);
Promise.all(keypadUpdates);
}
// LCDs: always update (progress bar changes)
const lcdUpdates = Array.from(lcdContexts).map((action) => {
const position = lcdPositions.get(action.id) ?? "left";
return updateLCDAction(action, position).catch((err) =>
logger.error("LCD update error:", err),
);
});
Promise.all(lcdUpdates);
}
async function updateAllActions(): Promise<void> {
// Keypads: skip if nothing visually changed
const newKeypadKey = keypadCacheKey(currentTrackInfo);
if (newKeypadKey !== lastKeypadCacheKey) {
lastKeypadCacheKey = newKeypadKey;
const keypadUpdates = Array.from(keypadContexts).map((action) =>
updateKeypadAction(action).catch((err) =>
logger.error("Keypad update error:", err),
),
);
await Promise.all(keypadUpdates);
}
// LCDs: always update (progress bar changes)
const lcdUpdates = Array.from(lcdContexts).map((action) => {
const position = lcdPositions.get(action.id) ?? "left";
return updateLCDAction(action, position).catch((err) =>
logger.error("LCD update error:", err),
);
});
await Promise.all(lcdUpdates);
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/plugin.ts` around lines 164 - 185, The updateAllActions function launches
keypad and LCD update promises but does not await the Promise.all calls, which
can hide errors and cause race conditions; make updateAllActions async and await
both Promise.all results (for the keypadUpdates and lcdUpdates), keeping the
existing per-promise .catch handlers or alternatively remove per-promise .catch
and let Promise.all reject so callers can observe failures; locate the logic
around keypadCacheKey/lastKeypadCacheKey, keypadContexts, updateKeypadAction and
lcdContexts/updateLCDAction/lcdPositions and add the awaits there (or add a
clear comment if intentional fire-and-forget behavior is required).

500ms made the progress bar noticeably choppy. 250ms keeps the render
rate reasonable while maintaining smooth visual updates.

https://claude.ai/code/session_01X7DW5Xkw2PhRTd3tabdrcj
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.

3 participants