Skip to content

fix: preserve plugin-set key images across profile re-renders#387

Open
atomskz wants to merge 1 commit into
nekename:mainfrom
atomskz:fix/key-image-rerender-desync
Open

fix: preserve plugin-set key images across profile re-renders#387
atomskz wants to merge 1 commit into
nekename:mainfrom
atomskz:fix/key-image-rerender-desync

Conversation

@atomskz

@atomskz atomskz commented Jul 10, 2026

Copy link
Copy Markdown

Preflight checklist

If you remove this checklist, this pull request will be closed without explanation.

  • I understand that if this pull request is about support for non-Elgato or non-Tacto hardware, it will be closed without explanation, as per issue Statement regarding device support #38.
  • I certify the compliance of this pull request with the Jellyfin LLM/"AI" Development Policy adopted by the OpenDeck project.
  • I thoroughly understand the changes that I am proposing in this pull request, and I will be able to respond to questions and review feedback.
  • I reasonably believe that the changes that I am proposing are of production quality, or I am otherwise opening this pull request as a draft.
  • I have thoroughly reviewed the diff of my changes and ensured that I have neither introduced any unrelated additions, nor differences in unmodified code.
  • I have ensured that I have run the appropriate formatter on my changes and that my code produces no linter violations.
  • I will keep "Allow edits from maintainers" enabled for this pull request.

Fixes #386.

Related to #152 — the same symptom (a key showing the wrong image) but a distinct
cause. #152 is a device reconnect (a backend reload from disk); this fixes a purely
frontend re-render and does not address the reconnect case.

Problem

When a plugin updates a key's image via setImage, the update is discarded on the
next profile grid re-render (e.g. after adding or moving any action), and the key
reverts to the image the profile was loaded with. No reconnect is involved.

Cause

slot is a Key's local working copy; inslot is the deliberate one-way downward
channel from profile.keys[] (a plugin's per-session setImage writes only slot,
never profile). On profile = profile, Svelte's safe_not_equal treats an
identical object reference as changed, so the reactive $: update(inslot) re-runs for
every key even when its profile.keys[i] didn't change; update() then
unconditionally does slot = inslot, resetting the key to the loaded instance and
discarding the plugin's image.

Fix

Make update() idempotent: remember the last bound reference and skip the reset when
inslot hasn't actually changed. The one-way inslot/slot split stays intact —
nothing propagates upward into profile — while a genuine change (a fresh instance
from the backend, or the update_state listener) still applies.

 	// One-way binding for slot data.
 	export let inslot: ActionInstance | null;
 	let slot: ActionInstance | null;
+	let lastInslot: ActionInstance | null | undefined;
 	const update = (inslot: ActionInstance | null) => {
+		if (inslot === lastInslot) return;
 		if (inslot && context && inslot.context.split(".")[0] != context.device) return;
+		lastInslot = inslot;
 		slot = inslot;
 	};
 	$: update(inslot);

The latch sits after the device filter, so a filtered entry is re-evaluated on the
next pass exactly as before, and clear()'s upward null still round-trips.

Testing

  • With a plugin that updates a key image via setImage: set an image, restarted so
    the profile loads it, set a different image this session, then added/moved another
    action — the update now persists (previously the key reverted to the loaded image).
  • Keys whose image matches the loaded profile continue to render correctly.
  • deno task check and prettier are clean.

@nekename

Copy link
Copy Markdown
Owner

This is a fix already discovered by @justmangoou, but I'm wary of accepting it because there must have been some reason for separation of inslot and slot - inslot specifically exists so that changes in Key don't propagate upwards.

Anyway, if it turns out that inslot is actually no longer necessary, it should be removed entirely so that the changes made in InstanceEditor are also propagated upwards into the profile so that they are not reset on re-render.

And finally, the ultimate fix is of course to actually discover why other keys are being re-rendered when they're not supposed to. It causes unnecessary churn in the entire frontend and of course causes device updates. If we prevent the unnecessary re-renders of other keys when one key is added/deleted (which seems to be the main trigger of this issue), this won't be necessary at all.

Lastly, I'd rather read issue comments from a human than an LLM, if that's possible.

@atomskz

atomskz commented Jul 13, 2026

Copy link
Copy Markdown
Author

Hi @nekename, thanks for taking the time to review this!

On your suspicion — you're right, and I checked the history to be sure. The inslot/slot split landed in b2c5014 ("Implement user state modification") together with the "One-way binding for slot data" comment, so the one-way design is clearly deliberate: slot is the local working copy, inslot is the downward channel from profile.keys[], and the only intended upward write is the explicit inslot = slot in clear(). A plugin's per-session setImage is runtime state, not configuration, so it has no business leaking into profile. That invariant is worth keeping — which is why I now agree the PR as submitted is wrong. (For what it's worth, the revert has been possible ever since that split landed: update_state has only ever written to slot, so any grid re-render could clobber it.)

Why the write-back is incorrect. inslot = payload.contents does the one thing the split exists to prevent: it pushes transient plugin state upward into profile.keys[]. It's also inconsistent — InstanceEditor still writes only slot, as you pointed out — and it doesn't reduce any churn: every other key still re-renders and re-pushes its image to the device on each add/move; the write-back just makes the re-pushed value happen to be right. It patches the symptom while breaking the design.

The replacement. I've reworked the change to leave the one-way design untouched and instead make update() idempotent — skip the reset when the bound reference didn't change:

+ let lastInslot: ActionInstance | null | undefined;
const update = (inslot: ActionInstance | null) => {
+	if (inslot === lastInslot) return;
	if (inslot && context && inslot.context.split(".")[0] != context.device) return;
+	lastInslot = inslot;
	slot = inslot;
};

The mechanism: on profile = profile, Svelte's safe_not_equal treats an identical object reference as changed, so $: update(inslot) re-runs for every key even when its profile.keys[i] didn't change, and the unconditional slot = inslot is what clobbers the plugin-set image. With the guard, an untouched key early-returns: the image survives and nothing propagates upward. The reference check is safe because a real change always arrives either as a fresh object from the backend (create_instance / move_instance / paste responses, profile refetches — I went through every write to profile.keys[] / sliders[] / infobars[]) or directly through the update_state listener. clear()'s upward null still round-trips correctly, and the latch sits after the device filter, so a filtered entry gets re-evaluated on the next pass exactly like upstream. deno task check and prettier are clean (the two Timeout errors in Key.svelte are pre-existing).

Compared to the write-back: same bug fixed, but nothing flows upward, the InstanceEditor inconsistency isn't made worse, and the runtime-vs-configured divergence stays where the design puts it — in slot.

On the re-renders themselves — I dug into why other keys re-render at all, and there are two layers. The slot clobbering is what the guard fixes. But the frontend/device churn you describe has a second, independent cause: DeviceView passes context={{ device: device.id, ... }} as an inline literal, so every profile = profile hands every Key a brand-new context object, and the render block depends on context — so all keys still re-render and re-push to the device even with the guard in place. Fixing that would mean hoisting/memoizing the per-slot context objects in DeviceView (their contents only change on device/profile switch). Happy to take that on as a follow-up — with both pieces in place, an add/move would touch only the affected key, which I believe is the "ultimate fix" you described.

If you'd rather go the remove-inslot route instead, or you and @justmangoou already have something in flight, no hard feelings — I'll close. Otherwise I'll update the PR to the guard version.

Lastly, I'd rather read issue comments from a human than an LLM, if that's possible.

Understood, and fair enough. I use models to format and translate my comments into English, since it isn't my native language.

Key.svelte kept a plugin-set image only in the component-local `slot`, while the
bound `inslot` stayed on the profile's loaded instance. A grid re-render
(`profile = profile` after create/move/paste in DeviceView) re-runs
`$: update(inslot)` for every key -- safe_not_equal treats an identical object
reference as changed -- and the unconditional `slot = inslot` reset the key back
to the loaded image.

Guard `update()` so it only resets `slot` when the bound reference actually
changed, keeping the one-way `inslot`/`slot` design intact: an untouched key
early-returns and keeps its `slot`, while a genuine change (a fresh instance in
the profile, or the update_state listener) still applies.
@atomskz
atomskz force-pushed the fix/key-image-rerender-desync branch from 93e70f6 to 2c8fb68 Compare July 13, 2026 15:20
@nekename

Copy link
Copy Markdown
Owner

Excellent work, thank you. That sounds like an acceptable fix that doesn't require changes in DeviceView. I'm away for a few more days this month but I'd be happy to test and merge this when I get back.

As for LLM usage for translation, that's perfectly OK but you should just mention that you're using it for translation so that it doesn't come across the wrong way.

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.

Plugin-set key image is reverted to the profile's loaded value when another action is added or moved

2 participants