Add Rating Keyboard Shortcuts for Images - #7088
Conversation
DogmaDragon
left a comment
There was a problem hiding this comment.
Documentation check passed.
…happ#7088) Convert delete (stashapp#7022) to the void/delete branch and rebuild void/global-counter on top of it, so the open lightbox PRs can stack cleanly without pulling develop: gallery -> delete -> global-counter -> page-boundary -> rating. page-boundary (stashapp#7083) and rating (stashapp#7088) become void/page-boundary and void/rating branches. Resolves the global-counter<->page-boundary dependency-array conflict and adds the rating keyboard shortcuts (stashapp#7088, issue stashapp#5616) to the build.
Gykes
left a comment
There was a problem hiding this comment.
Static review only, most of this is cleanup/pre existing issues
| return () => { | ||
| Mousetrap.unbind("r"); | ||
| mousetrap.unbind("r"); | ||
| }; |
There was a problem hiding this comment.
The restart timer is never cleared when isVisable goes false. If the user presses r then closes the lightbox within 1s the timeout still fires. Not a massive deal as it probably wont be happening very often but still good to clean it up.
There was a problem hiding this comment.
Confirmed — worse than a wasted timer, actually: the lightbox doesn't unmount on close (isVisible just toggles an early return null), and its scoped Mousetrap instance is never paused (only the global singleton is, by design). So r 3 followed by closing within 1s left the digit/backtick keys bound on that instance for up to a second after close — a stray 3 keypress anywhere afterward would silently re-fire setRating.
Fixed in bd4e5c5: the effect's cleanup now cancels the pending timeout and runs the queued unbind immediately, so nothing outlives the effect.
There was a problem hiding this comment.
This is preexisting:
This effect doesnt seem to have a dep array. It seems to unbind/remount on every render. Just another small clean up while we are working on this. Could do : [isVisible, ratingSystem, setRating, mousetrap]
There was a problem hiding this comment.
Hmm, this seems to have used the original page rather than the new page. It is supposed to be Keybinds.ts:82
There was a problem hiding this comment.
Fixed in bd4e5c5, but not with the literal array you listed — running it through this repo's biome useExhaustiveDependencies rule flagged that as wrong: the effect doesn't call setRating directly, it calls handleStarRatingKeybinds/handleDecimalKeybinds (which close over it), so biome wants those two in the deps instead of setRating. Wrapped both handlers (and the timer helper) in useCallback and depend on those; biome lint and tsc --noEmit are both clean now.
One caveat: this doesn't fully stop the effect from re-running on every render for the six existing scene/image/etc. callers — their setRating is a plain function redefined every render there too, not useCallback-wrapped, so it still gets a new identity each time. That's a pre-existing pattern across all six of those files and out of scope for this PR; happy to file it separately if useful.
Verified against a locally-built matching backend with the Playwright e2e suite (lightbox rating stars/decimal, the rapid r-sequence boundary regression, d d, arrow/Escape, and the scene/image detail-page rating shortcuts) — all green.
Addresses review feedback from Gykes on stashapp#7088: - The "r" effect's cleanup only unbound "r" itself, leaving any in-flight sequence's digit/backtick bindings and unbind timer live past the effect's lifetime (e.g. closing the lightbox mid-sequence). Cleanup now cancels the pending timeout and runs the queued unbind immediately. - The effect had no dependency array and reran on every render. Wrapped the star/decimal handlers in useCallback (deps: mousetrap, setRating) and gave the effect a proper dependency array, satisfying this repo's biome useExhaustiveDependencies rule.
Gykes
left a comment
There was a problem hiding this comment.
Tested and it worked as expected. No obvious regressions were found.
From the actual code itself I think how this was done isn't 100% the best way. Now I'm not a TS or front end guy so I could be wrong. I think if we decouple the cancel timer and unbind now from the every render it would be better. I'm not 100% sure on that and I think this is "good enough".
|
@void-function865 Just now looking it seems like the merge conflict is from more upstream changes to the lightbox. Can you please take a look at those? |
Implement the `r` + digit rating shortcuts (issue stashapp#5616) while viewing images in the lightbox, honoring the configured Stars/Decimal system. The lightbox pauses the global Mousetrap singleton while open, so it now owns a separate (non-paused) Mousetrap instance — mousetrap-pause tracks `paused` per instance — and reuses the existing useRatingKeybinds hook (now accepting an optional instance) rather than duplicating its logic. The hand-rolled `d d` delete double-tap is replaced with the standard Mousetrap "d d" sequence binding on that instance. Also fix a pre-existing bug in useRatingKeybinds: each `r` scheduled an independent unbind timer, so a quick second sequence (e.g. "r 3" then "r 4") could have its digits unbound mid-way by the earlier timer and the keypress dropped. The unbind timer is now cancelled and rescheduled per sequence (and the decimal buffer is reset at the start of each sequence). Document the lightbox rating shortcuts in the keyboard shortcuts manual.
Addresses review feedback from Gykes on stashapp#7088. The previous fix cancelled the pending unbind timer in the "r" effect's cleanup, but that cleanup ran on every render (the effect's deps included useCallback-wrapped handlers that depend on setRating, which every caller recreates each render). Cleanup running on every render meant it could flush the pending unbind mid-sequence on any unrelated re-render, unbinding the digit/backtick keys before the 1s window elapsed and dropping keystrokes -- a variant of the same class of bug this PR originally fixed. Root cause: the hook coupled the digit/backtick bindings' lifetime to a dynamic bind/unbind cycle tied to render-driven effect churn. Rewritten so "r", the digits, and "`" are bound once for isVisible's lifetime, and a ref-tracked sequence state (idle/star/decimal) plus a single timeout gate their behavior instead. setRating/ratingSystem are read through refs updated every render, so the bind effect only depends on isVisible/mousetrap and no longer reruns on every render.
bd4e5c5 to
d1257d8
Compare
A fix to the "r 3 then r 4" overlapping-timeout bug (stashapp#7088 review feedback) cancelled a sequence's pending digit-unbind in the "r"-binding effect's cleanup, but that effect reran on every lightbox render (its deps included handlers closing over setRating, which the lightbox recreates every render). So any unrelated re-render mid-sequence -- not just a second "r" press -- flushed the pending unbind and dropped the digit keys early. Verified this spec actually catches it: it fails against that intermediate version (rating stays unset, digits dropped after toggling the options panel mid-sequence) and passes against the state-machine rewrite that replaced it.
|
Pushed a rewrite addressing the design concern from the last review round. What changed in Also rebased onto current Manually verified in a running instance, including the specific regression this addresses (an unrelated re-render — e.g. toggling the lightbox options panel, nothing to do with rating — landing mid-sequence no longer drops the pending digit). |
Description
This implements the rating keyboard shortcuts (
rthen a digit) for images in the lightbox, honoring whichever rating system is configured and matching the scene and image detail pages:r 1–r 5set the rating,r 0clears it.r {0-9}{0-9}sets the rating (r 0 0for10.0),r `clears it.Key design choices:
useRatingKeybindsvia a lightbox-scoped Mousetrap instance. The lightbox callsMousetrap.pause()while open to suppress global shortcuts behind the overlay, which is why the existing Mousetrap-based hook never fired there. Rather than re-implement the rating logic by hand, the lightbox now owns its ownnew Mousetrap()— themousetrap-pauseplugin tracks the paused flag per instance, so a separate instance keeps firing while the global singleton stays paused. The hook gains an optional Mousetrap-instance parameter that defaults to the global singleton, so the scene and image detail pages are unchanged, and the lightbox reuses the hook verbatim instead of duplicating its logic.d ddelete onto the same shared mechanism. The delete shortcut now uses the standardMousetrap.bind("d d", …)sequence used everywhere else in the app, on the lightbox's own instance, replacing the bespoke timestamp-based double-tap detector that only existed because the global Mousetrap was paused. I added that detector in New: Add delete image shortcut and button to lightbox #7022; folding its removal in here keeps both lightbox sequence shortcuts on the shared mechanism rather than bespoke code, and only the activation glue stays per-consumer (the global hook binds digit keys dynamically through Mousetrap; the lightbox feeds the same hook through its scoped instance).useRatingKeybindsthat the reuse surfaced. Everyrpress scheduled its own independent timer to unbind the digit keys after one second but never cancelled the previous one, so a quick second sequence (for exampler 3thenr 4) whose digit landed after the first timer fired had its keys unbound mid-sequence and the keypress dropped — the rating did not always update. The timer is now cancelled and rescheduled per sequence, and the decimal entry buffer is reset at the start of each sequence. This fix also benefits the scene and image detail pages, which share the hook.Related Issue
Closes #5616.
Testing
Verified against a local instance with both rating systems, plus an automated Playwright suite run in my fork (driving a real browser against a dev build of this branch):
r 3sets three stars and persists across closing and reopening the image;r 0clears it.r 3 5sets 3.5 and persists;r `clears it.r 3immediately followed byr 4ends on four stars (this deterministically failed before the timer fix, with the4dropped, and passes after it).Escapeto close,d dto open the delete dialog, and global shortcuts (such asg s) staying inert while the lightbox is open.Screenshots
No new UI.
Checklist
AI Usage Disclosure
I used Claude Code (Anthropic) to assist with this PR. It helped explore the lightbox and
useRatingKeybindsimplementations, propose and implement the approach (the scoped Mousetrap instance, the hook parameter, thed ddedup, and the unbind-timer fix), and author the Playwright end-to-end tests used to verify it. I directed the design decisions, reviewed all changes, and validated the behavior manually and through the automated tests before submitting.Additional Context
This is a front-end-only change with no schema or generated-binding changes. The keyboard shortcuts are also documented in the keyboard shortcuts manual (
KeyboardShortcuts.md).