perf(mister): zero-RPC browsing via direct database reads - #360
Draft
giancarloerra wants to merge 24 commits into
Draft
perf(mister): zero-RPC browsing via direct database reads#360giancarloerra wants to merge 24 commits into
giancarloerra wants to merge 24 commits into
Conversation
On MiSTer, a cover miss now resolves the artwork file path Core already persisted in its media database and reads the image straight off the card, ahead of the media.image RPC. Core serializes image lookups through a single-slot semaphore and ships covers base64-encoded over the socket, so first views paid 100 ms+ per cover, strictly one at a time; the direct lookup is a ~7 ms indexed query plus a file read, and whole sessions complete without a single image RPC. The hole in the API abstraction is kept as small as agreed: MiSTer only (the module activates only when the MiSTer data directory exists), images only, strictly read-only, no second caches or thumbnail stores. A schema guard verifies the four tables consulted and disables the layer loudly if Core ever renames them, leaving the RPC path the sole source exactly as before. ZAPAROO_FRONTEND_DB_ART=0 disables it at runtime. Candidates are ranked (requested type, media-level before title-level, keyed row before path twins) and tried in order, so a stale database path degrades to the next candidate, and the v2.16 arcade classification systems inherit artwork from the row their file shares with the primary system. Fetch workers widen from 2 to 8 for the local path while a semaphore holds media.image RPC concurrency at the original 2, so Core sees no added pressure.
With covers loading locally in milliseconds, gating the detail pane on the second press hid speed the frontend now has: a quick double-tap skip paid an artificial wait. Taps engage rapid mode from the fifth press of a burst, held-key repeat still forces it through its own path, and the quiet window drops from 260 ms to 120 ms — above the 90 ms repeat tick, so a held key cannot flicker the pane.
…C-2026-0202 The lockfile gains the rusqlite tree added by the direct-read module, and cxx plus cxx-gen move to 1.0.195/0.7.195 together (the bridge ABI requires the pair to match): 1.0.194's let_cxx_string! is not exception safe (RUSTSEC-2026-0202), which cargo-deny rightly blocks. The advisory predates this branch — it is in the existing cxx-qt dependency chain — so main gets the fix as a side effect. The forced bump moves cxx to syn 3 while serde_derive and the cxx-qt macros stay on syn 2; the duplicate pair joins the documented skip list.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds read-only SQLite media lookups with RPC fallbacks. It integrates local artwork, browse, favorites, history, and metadata loading. It also updates browse rendering, prefetching, rapid navigation, page turbo, tests, and translation metadata. ChangesLocal media database integration
Browse and navigation performance
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GamesModel
participant MediaBrowseDb
participant MediaImageCache
participant MediaArtDb
participant Core
GamesModel->>MediaBrowseDb: browse_folder(path, systems)
MediaBrowseDb-->>GamesModel: return direct folder results
MediaImageCache->>MediaArtDb: resolve_art(media key)
MediaArtDb-->>MediaImageCache: return ranked artwork paths
MediaImageCache->>Core: request media.image on local miss
Core-->>MediaImageCache: return image response
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
|
cheers man! really appreciate it. i'll start getting your prs in soon just trying to get a patch out of core first EXACTLY TWO CONDITIONS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
src/ui/app/Main.qml (1)
2727-2730: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one activation predicate for both rapid-navigation flags.
The condition on Lines 2727 and 2729 is identical. Compute it once so
rapidNavigationActiveandrapidNavigationIndicatorActivecannot receive different thresholds later.Proposed refactor
- if (forceActive || root._rapidNavigationTapCount >= root._rapidNavigationTapThreshold) - root.rapidNavigationActive = true; - if (forceActive || root._rapidNavigationTapCount >= root._rapidNavigationTapThreshold) - root.rapidNavigationIndicatorActive = true; + const shouldActivate = forceActive || root._rapidNavigationTapCount >= root._rapidNavigationTapThreshold; + if (shouldActivate) { + root.rapidNavigationActive = true; + root.rapidNavigationIndicatorActive = true; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/app/Main.qml` around lines 2727 - 2730, In the rapid-navigation handler, compute the shared activation predicate from forceActive and the tap threshold once, then use that predicate to update both rapidNavigationActive and rapidNavigationIndicatorActive. Keep both flags driven by the same existing condition.tests/ui/tst_navigation.qml (1)
463-475: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPin the five-press and indicator contracts in this test.
The loop uses
main._rapidNavigationTapThresholdas its oracle. If the implementation changes from five presses to three or four, this test still passes. Assert that the property equals5and use a test constant for the expected boundary.The changed
src/ui/app/Main.qmlcode also updatesrapidNavigationIndicatorActive, but this test never verifies that it stays false below the threshold or becomes true on the fifth press.Proposed test update
function test_rapid_navigation_taps_activate_at_threshold(): void { + const expectedThreshold = 5; + compare(main._rapidNavigationTapThreshold, expectedThreshold); main._noteRapidNavigationAction("down", false); compare(main.rapidNavigationAction, "down", "rapid action tracks latest rapid input even before active mode"); - for (var i = 2; i < main._rapidNavigationTapThreshold; i++) { + compare(main.rapidNavigationIndicatorActive, false); + for (var i = 2; i < expectedThreshold; i++) { main._noteRapidNavigationAction("down", false); compare(main.rapidNavigationActive, false, "press " + i + " of a burst below the threshold must not enter rapid mode"); + compare(main.rapidNavigationIndicatorActive, false); } main._noteRapidNavigationAction("down", false); compare(main.rapidNavigationActive, true, "threshold press enters rapid mode"); + compare(main.rapidNavigationIndicatorActive, true, "threshold press enables the rapid indicator");As per coding guidelines, after editing QML, run
just lint; runjust testwhen the change can affect runtime behavior. Do not leave lint warnings or failing tests behind.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ui/tst_navigation.qml` around lines 463 - 475, Update test_rapid_navigation_taps_activate_at_threshold to assert main._rapidNavigationTapThreshold equals 5, then use a fixed expected threshold constant for the loop and boundary checks instead of the implementation property. Also verify rapidNavigationIndicatorActive remains false for presses below the threshold and becomes true on the fifth press, alongside the existing rapidNavigationActive assertions.Source: Coding guidelines
rust/frontend/src/media_image_cache.rs (2)
168-170: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueA panic in the blocking task is discarded silently.
.await.ok()??maps aJoinErrortoNone, so a panic inside the blocking closure looks identical to a clean miss and the code falls through to RPC. Log the join error atwarnso a repeated panic is visible in the diagnostics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/frontend/src/media_image_cache.rs` around lines 168 - 170, Update the blocking-task result handling around the `.await` chain to preserve the existing clean-miss behavior while logging any `JoinError` at warn level before returning None. Do not silently convert a panic in the blocking closure into a cache miss; use the surrounding media image cache logging context.
1801-1806: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
db_art_lookupfilter logic.The new local path adds three untested branches: the
wantedtype rejection, the extension rejection, and the dead-path retry loop. None of the tests in this module reach them. Extract the per-candidate decision into a pure helper that takes anArtHitand thewantedlist, then test that helper directly. Runjust testfor this change, because it affects runtime behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/frontend/src/media_image_cache.rs` around lines 1801 - 1806, The db_art_lookup candidate filtering lacks coverage for wanted-type rejection, extension rejection, and dead-path retry behavior. Extract the per-candidate decision logic into a pure helper near db_art_lookup, accepting an ArtHit and wanted list, then add focused tests for each branch and run just test to verify the runtime behavior.Source: Coding guidelines
rust/frontend/src/media_art_db.rs (4)
152-164: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffOne shared connection serializes every lookup across the eight workers.
db.connis a singleMutex<Option<Connection>>. All eight fetch workers contend on it, so the database stage runs strictly serially and only the file read parallelizes. That still beats the RPC path, so this is not a defect. If measured queue wait on the lookup stage grows, give each blocking worker its own read-only connection (for example athread_local!connection or a small pool) instead of widening the worker count further.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/frontend/src/media_art_db.rs` around lines 152 - 164, The shared db.conn mutex serializes database lookups across workers. If lookup-stage queue wait is confirmed to be a bottleneck, replace the single shared connection in the media-art database lookup flow with per-worker read-only connections or a small connection pool, while preserving the existing reopen and empty-result handling around open_checked.
281-288: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider containing resolved art paths under the card root.
absolute_art_pathpasses any absolute value through unchanged, and a relative value with..segments can escape/media/fat. The caller then reads that file and serves the bytes as a cover. The database is local and Core-owned, so this is a posture gap and not an exploit path today. Normalizing the joined path and rejecting results outside/media/fatremoves the class entirely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/frontend/src/media_art_db.rs` around lines 281 - 288, Update absolute_art_path to normalize the resolved path and reject or otherwise prevent any path that escapes the /media/fat root, including absolute inputs and relative paths containing .. segments. Ensure callers cannot read or serve art files outside that card root, while preserving valid in-root paths.
93-96: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe layer stays off for the process lifetime if the database appears later.
ART_DBis aOnceLock, so a missing file at the first lookup cachesNonepermanently. On a first-boot card, Core createsmedia.dbduring the initial scrape, and covers then use RPC until the frontend restarts. If that case matters, retry the open on a later lookup instead of caching the absence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/frontend/src/media_art_db.rs` around lines 93 - 96, Update the ART_DB initialization path around the db_path existence check so a missing media database does not permanently cache None in the OnceLock. Retry database discovery and opening on later lookups, while retaining the existing initialization behavior once the file becomes available.
52-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the default art-type order instead of duplicating it.
DEFAULT_TYPE_ORDERduplicatesCORE_DEFAULT_IMAGE_TYPESand is used separately in local artwork sorting. Use the image cache constant here, or add an assertion that both ordered lists match so changes cannot drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/frontend/src/media_art_db.rs` around lines 52 - 66, Update DEFAULT_TYPE_ORDER to reuse the image cache’s CORE_DEFAULT_IMAGE_TYPES constant rather than maintaining a duplicate list, or add an explicit assertion that both ordered lists remain identical if direct reuse is unavailable. Keep the existing local artwork sorting behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/frontend/src/media_art_db.rs`:
- Around line 117-124: Update open_checked to detect the
SQLITE_READONLY_CANTINIT error from Connection::open_with_flags and surface a
MiSTer-visible permission warning before the existing RPC fallback occurs.
Preserve the current behavior for other open failures, and use the existing
user-facing warning mechanism if one is available in the surrounding media
database code.
In `@rust/frontend/src/media_image_cache.rs`:
- Around line 140-170: Update the local lookup around db_art_lookup and its
spawn_blocking closure to honor the max_size budget computed by fetch_one. Pass
the budget into the local path and reject or downscale any candidate exceeding
it before returning bytes, allowing oversized candidates to fall through to RPC
while preserving the existing candidate iteration and unreadable-file behavior.
- Around line 1453-1464: Move the `fetch_started` initialization in the
`rpc_gate()` acquisition block so it starts immediately after `_permit` is
acquired, preserving `fetch_ms` as Core round-trip time only. Keep the timer
available to the existing `debug!` logging path, and preserve the current
semaphore and `store.client().media_image(params)` behavior.
---
Nitpick comments:
In `@rust/frontend/src/media_art_db.rs`:
- Around line 152-164: The shared db.conn mutex serializes database lookups
across workers. If lookup-stage queue wait is confirmed to be a bottleneck,
replace the single shared connection in the media-art database lookup flow with
per-worker read-only connections or a small connection pool, while preserving
the existing reopen and empty-result handling around open_checked.
- Around line 281-288: Update absolute_art_path to normalize the resolved path
and reject or otherwise prevent any path that escapes the /media/fat root,
including absolute inputs and relative paths containing .. segments. Ensure
callers cannot read or serve art files outside that card root, while preserving
valid in-root paths.
- Around line 93-96: Update the ART_DB initialization path around the db_path
existence check so a missing media database does not permanently cache None in
the OnceLock. Retry database discovery and opening on later lookups, while
retaining the existing initialization behavior once the file becomes available.
- Around line 52-66: Update DEFAULT_TYPE_ORDER to reuse the image cache’s
CORE_DEFAULT_IMAGE_TYPES constant rather than maintaining a duplicate list, or
add an explicit assertion that both ordered lists remain identical if direct
reuse is unavailable. Keep the existing local artwork sorting behavior
unchanged.
In `@rust/frontend/src/media_image_cache.rs`:
- Around line 168-170: Update the blocking-task result handling around the
`.await` chain to preserve the existing clean-miss behavior while logging any
`JoinError` at warn level before returning None. Do not silently convert a panic
in the blocking closure into a cache miss; use the surrounding media image cache
logging context.
- Around line 1801-1806: The db_art_lookup candidate filtering lacks coverage
for wanted-type rejection, extension rejection, and dead-path retry behavior.
Extract the per-candidate decision logic into a pure helper near db_art_lookup,
accepting an ArtHit and wanted list, then add focused tests for each branch and
run just test to verify the runtime behavior.
In `@src/ui/app/Main.qml`:
- Around line 2727-2730: In the rapid-navigation handler, compute the shared
activation predicate from forceActive and the tap threshold once, then use that
predicate to update both rapidNavigationActive and
rapidNavigationIndicatorActive. Keep both flags driven by the same existing
condition.
In `@tests/ui/tst_navigation.qml`:
- Around line 463-475: Update test_rapid_navigation_taps_activate_at_threshold
to assert main._rapidNavigationTapThreshold equals 5, then use a fixed expected
threshold constant for the loop and boundary checks instead of the
implementation property. Also verify rapidNavigationIndicatorActive remains
false for presses below the threshold and becomes true on the fifth press,
alongside the existing rapidNavigationActive assertions.
🪄 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 Plus
Run ID: 06b06851-602f-4a96-8879-ece521721281
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
rust/Cargo.tomlrust/deny.tomlrust/frontend/Cargo.tomlrust/frontend/src/lib.rsrust/frontend/src/media_art_db.rsrust/frontend/src/media_image_cache.rssrc/ui/app/Main.qmltests/ui/tst_navigation.qml
open_checked now identifies SQLITE_READONLY_CANTINIT — the read-only WAL sidecar permission failure — in its warning so a permissions problem on the card is diagnosable from the log. The RPC gate wait is timed as its own gate_ms field so fetch_ms keeps meaning the Core round trip alone and stays comparable with earlier logs.
The dependency graph resolves a single unicode-width version now, so both skip entries match nothing and cargo-deny warns about the 0.2.2 one on every run. Removing them restores a warning-free gate; if a duplicate ever returns, cargo-deny will fail loudly and the skip can be re-added deliberately.
The local art read returns the original file instead of honoring the RPC max_size downscale. Resizing locally would need a re-encode or a thumbnail store, both excluded from this path by design, and views decode at their snapped sourceSize tier so the larger payload only affects the encoded bytes held in RAM, not decode work. Also drops a stale disk-cache mention from the same comment.
media.tags.update blanket-invalidated every cached media.browse listing, so a single favorite toggle forced the next entry into any folder back through a full browse round trip - seconds on large folders. The toggling view already patches the row's heart in place, so drop the browse invalidation and accept a cosmetically stale heart in other cached folders holding the same title until their next natural refetch. Favorites and search invalidation are unchanged. Also widen rapid paging: the rapid fetch chunk rises to Core's max_results ceiling and the list grid looks eight pages ahead during rapid navigation, so sustained paging no longer catches the loaded edge and stalls on 'Loading more'.
The paged games model only ever fetched near or past the loaded edge, so paging raced a media.browse round trip and lost routinely, blocking on the Loading-more banner. Once a folder's first page is up, a timer now pulls gentle 100-row chunks back to back until the whole folder is local, paced so other requests interleave; leaving the screen stops it and the model's browse generation discards stale responses on folder switches. The list tail prefetch keeps a five-page window as a backstop for users outrunning the fill. The rapid chunk drops to 300: measured on device, max-size responses cost several seconds while small ones return in well under one, so a blocked pager should never wait on a huge answer. The Loading-more banner now shows only when the user is actually near the loaded edge, since the background fill would otherwise keep it lit for the whole first seconds of a large folder. A repeat Accept for a system whose entry is already in flight is now absorbed instead of restarting the load from scratch, which doubled the visible loading time for users who press again when a load feels slow. Translation catalogs refreshed for shifted source lines.
Gamepad input stacks (MiSTer's pad-to-key translation among them) deliver a held dpad as separate press/release pairs at their own cadence rather than one held key with auto-repeat. Those repeats only counted through the tap path, and at cadences slower than the 120ms quiet window the burst count reset on every press, so rapid mode never engaged during a pad hold and cover work churned through it, degrading input latency exactly while the user paged hardest. The quiet timer now arms a wider 300ms chain window for discrete taps, while notes from an established QML hold-repeat keep the tight window so rapid still exits quickly after a genuine held key is released. Covered by a new navigation test driving spaced discrete repeats to the threshold. Translation catalogs refreshed for shifted source lines.
Extends the direct-read fast path from artwork to folder listings: on MiSTer, entering a folder runs one read-only SQLite query against the tables Core already maintains for its own browse cache (BrowseDirs and BrowseDirCounts for the directory tree, Media.ParentDir and SortName behind idx_media_browse_sort for files, the MediaTags projection for the favorite heart, image property tags for hasCover) and hands the complete listing to the existing Ready pipeline in a single apply. A full local listing has no next page, so the paged fetch machinery, the background fill, and the append trickle that contended with input on the UI thread all stay idle for folders served this way. Same guard rails as the artwork path: read-only WAL-friendly open, schema guard that disables the layer loudly on drift, the same BrowseIndexVersion serveability gate Core uses, and RPC fallback for anything the database cannot answer with confidence (roots, letter index, meta, and all writes always stay on the RPC). Known accepted divergences are documented in the module header. Disabled at runtime with ZAPAROO_FRONTEND_DB_BROWSE=0 for A/B comparison. Also fixes the whitespace-mangled WAL diagnostic string in media_art_db.
A model reset costs the UI thread roughly linear time in row count, so handing a complete direct listing to the initial apply froze folder entry for seconds on large folders while the database read itself took a fraction of that. The reset now carries only the first page and the remainder lands as bulk inserts queued behind the paint, with loading_more held and a synthetic has-next flag so the restore chase and every fetch path treat the tail window as an in-flight fetch.
The platform pad bridge delivers held page buttons as discrete press/release pairs at its own autofire rate, so held paging crawled at bridge speed and never engaged rapid mode, leaving cover work running through the scroll. The third chained press now hands paging to a self-driven turbo tick that forces rapid mode and absorbs the bridge presses; press silence stops it, since bridge releases carry no lift information. Translation catalogs refreshed for shifted lines.
The paged grid materialised one cell per model row even while hidden behind the list layout, costing roughly three milliseconds per row on the software renderer: large-folder resets and bulk inserts blocked the UI thread for seconds and every cursor landing re-evaluated selection bindings across the whole folder. The grid now suspends its delegates in list layout and serves itemCount from the model's own count, keeping the cursor and page math intact with zero delegates alive. List rows also drop their decoration (heart, tag layout) while rapid mode drives, mirroring the grid's rapidRenderMode.
The RPC flow gets its Pending status from the store's status stream; the direct flow never said it, so a re-entered folder showed a bare empty view for the query duration instead of the loading cue.
The list layout routes Left/Right through the page path, so a held Left/Right is a held page button there and must feed the turbo like the dedicated page keys; previously only page_prev/page_next qualified and held paging stayed at the pad bridge's autofire beat.
giancarloerra
changed the title
giancarloerra
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/app/Main.qml (1)
2736-2738: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWire list-layout Left and Right actions before enabling their turbo path.
_pageTurboEligible()acceptsleftandrightfor Games list layout.MediaListScreen.handleAction()only callslistLeftActionorlistRightActionin that layout.GamesScreen.qmlbinds neither callback. These presses and turbo ticks do not page.
_isRapidNavigationAction()also excludes these actions. Turbo therefore does not pause cover work for held Left or Right input.Bind the two callbacks in
GamesScreen.qml. Normalize these turbo actions topage_prevandpage_nextbefore calling_noteRapidNavigationAction(). Add coverage for held Left and Right list paging.Proposed fix
*** src/ui/app/Main.qml @@ - root._noteRapidNavigationAction(action, true); + const rapidAction = action === "left" ? "page_prev" + : action === "right" ? "page_next" : action; + root._noteRapidNavigationAction(rapidAction, true); *** src/ui/screens/GamesScreen.qml @@ pageAction: delta => games._performPage(delta) + listLeftAction: () => games._performPage(-1) + listRightAction: () => games._performPage(1)Also applies to: 2814-2821, 2851-2854
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/app/Main.qml` around lines 2736 - 2738, Update GamesScreen’s list-layout action wiring to bind listLeftAction and listRightAction so Left and Right presses page the list. Extend _isRapidNavigationAction() and the _noteRapidNavigationAction() call path to recognize these actions, normalizing left/right to page_prev/page_next before recording turbo navigation. Add coverage verifying held Left and Right inputs page correctly and pause cover work during turbo ticks.
🧹 Nitpick comments (2)
rust/frontend/src/media_browse_db.rs (2)
636-655: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the schema guard and the directory system filter.
Two behaviors the module depends on are untested:
open_checkedrefuses to serve when a required table is missing. The fixture creates exactly the 11 names inREQUIRED_TABLES, so a later rename in Core would not fail these tests.dir_entriesapplies thesystemsfilter.system_filter_excludes_other_systemsonly asserts on media rows.Also note the guidelines: run
just lintandjust testafter this change.Based on coding guidelines: "After editing C++, Rust, or QML, run
just lint; runjust testwhen the change can affect runtime behavior."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/frontend/src/media_browse_db.rs` around lines 636 - 655, Add tests covering both missing required tables and directory system filtering. Extend the schema fixture tests around open_checked to remove or rename each required table, including all entries from REQUIRED_TABLES, and assert the connection is rejected; add assertions in the system_filter_excludes_other_systems coverage to verify dir_entries excludes directory entries from other systems. Run just lint and just test after the changes.Source: Coding guidelines
75-103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider retrying initialization when
media.dbappears later.
BROWSE_DBcaches the init result for the whole process. Ifmedia.dbdoes not exist at the first call (line 85), the layer stays disabled until the frontend restarts. Core creates the database on its first scan, so a first-boot device keeps using the RPC paging path for the rest of the session. Correctness is preserved by the fallback, but the optimization never engages.A bounded retry keeps the fast path reachable: store an
Option<BrowseDb>behind the existingMutexpattern, or gate re-initialization on a coarse time interval so a missing file is re-checked at most once per N seconds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/frontend/src/media_browse_db.rs` around lines 75 - 103, Change browse_db initialization so a missing media.db does not permanently cache None for the process. Preserve the existing disabled-environment behavior, but use the existing Mutex-based state or a coarse retry interval to re-check the database path at bounded intervals and initialize BrowseDb once the file appears, while retaining RPC fallback when unavailable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/frontend/src/models/games.rs`:
- Around line 1516-1524: Update start_initial_browse and the browse_folder call
path to coalesce stale direct database reads using a bounded latest-request-wins
worker or cooperative cancellation. Ensure rapid path changes do not leave one
detached spawn_blocking task per enabled non-root path, and make stale requests
stop before the full-folder read completes while preserving the existing
current-ticket behavior.
In `@src/ui/app/Main.qml`:
- Around line 2869-2878: Update the page-turbo input flow around handleKey(),
_notePageTurboPress(), and handleAction() so transition or modal input gating
occurs before any turbo state changes; when blocked, call _stopPageTurbo() and
prevent pageTurboTick from continuing. Apply the same stop behavior when the
window loses focus, and add a regression test covering a pending transition with
discrete page-button press pairs.
In `@src/ui/components/PagedGrid.qml`:
- Around line 48-57: Use American English in the comments: in
src/ui/components/PagedGrid.qml lines 48-57, replace “materialises” and
“materialisation” with “materializes” and “materialization”; in
src/ui/screens/MediaListScreen.qml lines 506-509, replace “materialisation” with
“materialization”; and in tests/ui/tst_paged_grid.qml lines 104-107, replace
“materialised” with “materialized”.
- Around line 48-57: The suspendDelegates implementation in
src/ui/components/PagedGrid.qml:48-57 and its transition logic at
src/ui/components/PagedGrid.qml:630 must avoid assigning the full model to
itemRepeater when leaving list layout; retain itemCount, pagination, and
currentIndex from independent model-visible state while materialising delegates
incrementally or only within the rendered retention window. Update the
corresponding layout transition in src/ui/screens/MediaListScreen.qml:501-509 to
use this staged or windowed activation behavior.
In `@src/ui/screens/GamesScreen.qml`:
- Around line 338-346: Update the backgroundFolderFill Timer’s running condition
and trigger guard to require games._listLayout, so background pagination only
runs for list layout while preserving the existing loading and has_next_page
checks.
In `@tests/ui/tst_navigation.qml`:
- Around line 528-562: Update the page-turbo tests around handleKey() so every
simulated PageUp/PageDown press is followed by handleKeyRelease(), matching the
bridge’s press/release behavior. In
test_page_turbo_engages_on_third_chained_press, also assert that normal repeat
is inactive when pageTurboRunning becomes true, using the existing repeat state
symbol.
---
Outside diff comments:
In `@src/ui/app/Main.qml`:
- Around line 2736-2738: Update GamesScreen’s list-layout action wiring to bind
listLeftAction and listRightAction so Left and Right presses page the list.
Extend _isRapidNavigationAction() and the _noteRapidNavigationAction() call path
to recognize these actions, normalizing left/right to page_prev/page_next before
recording turbo navigation. Add coverage verifying held Left and Right inputs
page correctly and pause cover work during turbo ticks.
---
Nitpick comments:
In `@rust/frontend/src/media_browse_db.rs`:
- Around line 636-655: Add tests covering both missing required tables and
directory system filtering. Extend the schema fixture tests around open_checked
to remove or rename each required table, including all entries from
REQUIRED_TABLES, and assert the connection is rejected; add assertions in the
system_filter_excludes_other_systems coverage to verify dir_entries excludes
directory entries from other systems. Run just lint and just test after the
changes.
- Around line 75-103: Change browse_db initialization so a missing media.db does
not permanently cache None for the process. Preserve the existing
disabled-environment behavior, but use the existing Mutex-based state or a
coarse retry interval to re-check the database path at bounded intervals and
initialize BrowseDb once the file appears, while retaining RPC fallback when
unavailable.
🪄 Autofix
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 Plus
Run ID: 1db0f6c5-ac86-4a4b-8895-908501a52175
📒 Files selected for processing (29)
rust/frontend/src/lib.rsrust/frontend/src/media_art_db.rsrust/frontend/src/media_browse_db.rsrust/frontend/src/models/games.rsrust/zaparoo-core/src/endpoints/media_tags_update.rssrc/ui/app/Main.qmlsrc/ui/components/BrowseList.qmlsrc/ui/components/BrowseListDetailView.qmlsrc/ui/components/PagedGrid.qmlsrc/ui/screens/GamesScreen.qmlsrc/ui/screens/MediaListScreen.qmlsrc/ui/translations/frontend_ar.tssrc/ui/translations/frontend_de.tssrc/ui/translations/frontend_el.tssrc/ui/translations/frontend_en.tssrc/ui/translations/frontend_es.tssrc/ui/translations/frontend_eu.tssrc/ui/translations/frontend_he.tssrc/ui/translations/frontend_hi.tssrc/ui/translations/frontend_it.tssrc/ui/translations/frontend_ja.tssrc/ui/translations/frontend_ko.tssrc/ui/translations/frontend_nl.tssrc/ui/translations/frontend_ro.tssrc/ui/translations/frontend_sk.tssrc/ui/translations/frontend_uk.tssrc/ui/translations/frontend_zh_CN.tstests/ui/tst_navigation.qmltests/ui/tst_paged_grid.qml
🚧 Files skipped from review as they are similar to previous changes (2)
- rust/frontend/src/lib.rs
- rust/frontend/src/media_art_db.rs
Direct folder reads now take a cancellation probe polled at each query stage and periodically inside the row loop, so a superseded browse stops consuming its blocking worker instead of running the full read to completion. The page turbo observes the same gates handleAction applies (transition in flight, cue visible, modal open, window inactive), so swallowed presses can no longer start or sustain a tick that would page the newly ready screen. The background folder fill is limited to list layout, where grid delegates cannot grow from idle appends. Turbo tests now model the bridge's press/release pairs and assert hold-repeat stays disarmed, with a regression test for gated input; comments move to American English.
Singleton state survives across TestCases in the ui binary; a modal left open by an earlier suite trips the page turbo's input gate and fails the paging tests for reasons unrelated to navigation.
Contributor
Author
|
If the direction in this PR is good, I would like to also propose the complete What else would move to direct reads
What stays on CoreAll writes (favourite toggles, launches, scraping, settings), the The refactorAt a third reader, the right direction stops being per-feature modules: one How I would do itI am happy to do this, measured step by step the same way as End state if the full refactor is approved: favourites, recents, and the detail pane Or we keep the boundary at the two instances already shipped (artwork and listings) |
The favorites screen paged media.search twenty-five rows at a time; the same rows live in indexed tables, so one local query returns the complete favorite set in tens of milliseconds. The screen paints direct-first on cold entry, every store Ready re-reads locally, and the RPC page applies only when the local read cannot answer, keeping toggles and reconnects on the store's freshness signal. Same contract as the other direct modules: read-only, schema-guarded, kill switch ZAPAROO_FRONTEND_DB_FAVORITES, unit tests on a synthetic schema.
Recents paged media.history twenty-five raw events at a time and deduplicated client-side; one local window query over Core's user database returns the complete deduplicated list, newest first, with media ids resolved by attaching the media database read-only and timestamps in the wire's RFC3339 form so Resume semantics are untouched (open sessions keep endedAt null). Direct-first paint with the store as freshness signal, same contract as the other modules, kill switch ZAPAROO_FRONTEND_DB_HISTORY.
Tags, scraped properties, the title record, and available image types come from indexed point lookups tried before every media.meta RPC, including the prefetch cache; the RPC remains the fallback for any miss. Measured a couple of milliseconds against tens over the wire, which lets the detail settle debounce drop from 220ms to 40ms when the layer is active (exposed to QML as AppStatus.direct_meta), so the detail pane tracks the cursor essentially live. Kill switch ZAPAROO_FRONTEND_DB_META; unit tests on a synthetic schema.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rust/frontend/src/models/favorites.rs (1)
443-496: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the direct-read helpers shared by Favorites and Recents.
spawn_direct_ready,spawn_direct_early_paint, and the surrounding ticket handling are duplicated in both models and differ only in the database module and the entry type. The copies have already begun to diverge: Recents carries an extra upgrade path that Favorites does not. A shared generic helper that takes the read function and the paint callback keeps the ticket, cancellation, and cover-gate rules in one place. This matches the shared read layer described in the PR objectives.
rust/frontend/src/models/favorites.rs#L443-L496: replace the two spawn helpers with calls into the shared helper, passingmedia_favorites_db::favoritesandapply_direct_full_paint.rust/frontend/src/models/recents.rs#L498-L551: replace the two spawn helpers with calls into the same shared helper, passingmedia_history_db::historyandapply_direct_full_paint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/frontend/src/models/favorites.rs` around lines 443 - 496, Extract the duplicated ticket, cancellation, blocking-read, and Qt-queue logic from spawn_direct_ready and spawn_direct_early_paint into one shared generic helper. Update rust/frontend/src/models/favorites.rs lines 443-496 to call it with media_favorites_db::favorites and apply_direct_full_paint, and update rust/frontend/src/models/recents.rs lines 498-551 to call the same helper with media_history_db::history and apply_direct_full_paint; preserve each model’s entry type, cover-gate behavior, and Recents upgrade path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/frontend/src/media_history_db.rs`:
- Around line 192-212: Update the SQL construction in the media-history query to
remove the mediadb.Media and mediadb.Systems joins and resolve media_id_col with
a correlated scalar subquery matching both h.MediaPath and h.SystemID. Preserve
NULL when resolve_media is false, and ensure the outer query continues returning
at most one row per deduplicated history event.
In `@rust/frontend/src/models/recents.rs`:
- Around line 348-411: Remove the redundant direct-history upgrade path by
deleting spawn_direct_history_upgrade and apply_direct_history. Update
finish_ready_pagination to gate the fetch_more look-ahead on the actual
direct-read outcome rather than only media_history_db::enabled(), while
preserving the existing direct-first flow through spawn_direct_ready and
apply_direct_full_paint.
---
Nitpick comments:
In `@rust/frontend/src/models/favorites.rs`:
- Around line 443-496: Extract the duplicated ticket, cancellation,
blocking-read, and Qt-queue logic from spawn_direct_ready and
spawn_direct_early_paint into one shared generic helper. Update
rust/frontend/src/models/favorites.rs lines 443-496 to call it with
media_favorites_db::favorites and apply_direct_full_paint, and update
rust/frontend/src/models/recents.rs lines 498-551 to call the same helper with
media_history_db::history and apply_direct_full_paint; preserve each model’s
entry type, cover-gate behavior, and Recents upgrade path.
🪄 Autofix
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 Plus
Run ID: 6c6e928a-6d27-4cdf-a694-78346d09d2b6
📒 Files selected for processing (35)
rust/frontend/src/lib.rsrust/frontend/src/media_browse_db.rsrust/frontend/src/media_favorites_db.rsrust/frontend/src/media_history_db.rsrust/frontend/src/media_meta_cache.rsrust/frontend/src/media_meta_db.rsrust/frontend/src/models/alternate_versions.rsrust/frontend/src/models/app_status.rsrust/frontend/src/models/favorites.rsrust/frontend/src/models/game_info.rsrust/frontend/src/models/games.rsrust/frontend/src/models/recents.rssrc/ui/app/Main.qmlsrc/ui/components/FocusedMediaDetailController.qmlsrc/ui/components/PagedGrid.qmlsrc/ui/screens/GamesScreen.qmlsrc/ui/screens/MediaListScreen.qmlsrc/ui/translations/frontend_ar.tssrc/ui/translations/frontend_de.tssrc/ui/translations/frontend_el.tssrc/ui/translations/frontend_en.tssrc/ui/translations/frontend_es.tssrc/ui/translations/frontend_eu.tssrc/ui/translations/frontend_he.tssrc/ui/translations/frontend_hi.tssrc/ui/translations/frontend_it.tssrc/ui/translations/frontend_ja.tssrc/ui/translations/frontend_ko.tssrc/ui/translations/frontend_nl.tssrc/ui/translations/frontend_ro.tssrc/ui/translations/frontend_sk.tssrc/ui/translations/frontend_uk.tssrc/ui/translations/frontend_zh_CN.tstests/ui/tst_navigation.qmltests/ui/tst_paged_grid.qml
🚧 Files skipped from review as they are similar to previous changes (6)
- src/ui/screens/MediaListScreen.qml
- tests/ui/tst_paged_grid.qml
- src/ui/screens/GamesScreen.qml
- src/ui/components/PagedGrid.qml
- src/ui/app/Main.qml
- rust/frontend/src/models/games.rs
Contributor
Author
|
Following up my earlier comment with results instead of plans: I went What the new phase adds, same contract as before (read-only,
Measured on device across the set: folder entry ~9x faster, a visible One coordination note: my favorites PR (#348) rewrites the favorites As stated before, I still think this should be one named boundary, not I am happy to add either, both, or neither, and the offer to fold all five |
…e path The media id now resolves through a correlated scalar subquery: the former joins duplicated a history row whenever its path is indexed under two systems (arcade classification twins), breaking the deduplicated contract; a regression test pins the twin case. The pre-direct-first upgrade path is gone with its apply variant, and the post-Ready look-ahead warms unconditionally, since that code is now reached only when the direct read could not answer and the paged chain must continue.
giancarloerra
changed the title
One cached pixmap per row is within the rapid frame budget, and hiding the heart made rows appear to change state while scrolling.
A genuinely held Left/Right on the games list repeats whole pages, and the page repaint is the expensive unit, so the repeat timer ticks at the page-turbo cadence there instead of the 90 ms row cadence. Both hold shapes the input stack can produce (a real held key, and press/release pairs through the turbo ticker) now travel at the same rhythm. Row actions and other contexts keep the row cadence; a pure-helper test pins the cadence table.
Contributor
Author
|
@coderabbitai full review |
Contributor
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
wizzomafizzo
added a commit
to ZaparooProject/zaparoo-core
that referenced
this pull request
Aug 13, 2026
* feat(api): add local-path image delivery Let clients request Core-owned cached thumbnail paths while preserving inline delivery as the default. Validate returned cache artifacts and fall back to inline data when materialization fails. Inspired by Giancarlo Erra's MiSTer artwork performance work and measurements in ZaparooProject/zaparoo-frontend#360. Ref ZaparooProject/zaparoo-frontend#360 Co-authored-by: Giancarlo Erra <giancarlo@widescreen.studio> * feat(api): add distinct media history Let clients request newest-session-per-media history directly from Core while preserving existing paginated history behavior. Group by system and media path before cursor filtering so older sessions cannot reappear on later pages. Inspired by Giancarlo Erra's recents performance work and measurements in ZaparooProject/zaparoo-frontend#360. Ref ZaparooProject/zaparoo-frontend#360 Co-authored-by: Giancarlo Erra <giancarlo@widescreen.studio> * chore(api): add request transport timings Correlate queue, handler, response, marshal, payload, encryption, and HTTP timing logs by request ID. This makes target-device API latency attributable without changing response contracts. Motivated by MiSTer performance measurements from ZaparooProject/zaparoo-frontend#360. Ref ZaparooProject/zaparoo-frontend#360 * perf(database): cache cover availability Build an exact in-memory index of media and title artwork properties while retaining a bounded title-first SQL fallback during cold start. Invalidate the index on image-property and database lifecycle changes so Core can serve cover status without exposing its schema. Inspired by Giancarlo Erra's MiSTer artwork and browse performance work and measurements in ZaparooProject/zaparoo-frontend#360. Ref ZaparooProject/zaparoo-frontend#360 Co-authored-by: Giancarlo Erra <giancarlo@widescreen.studio> * feat(api): expose media cover availability Return an always-present hasCover flag from media.search and media.history using Core's batched media/title artwork lookup. This lets clients plan artwork requests without reading Core-owned database tables. Inspired by Giancarlo Erra's artwork, browse, and recents performance work in ZaparooProject/zaparoo-frontend#360. Ref ZaparooProject/zaparoo-frontend#360 Co-authored-by: Giancarlo Erra <giancarlo@widescreen.studio> * chore(api): add media search stage timings Measure semaphore wait, database search, cover lookup, response construction, system metadata, ZapScript, and relative-path stages independently. This makes remaining target-device search latency attributable without changing API behavior. Motivated by MiSTer performance measurements from ZaparooProject/zaparoo-frontend#360. Ref ZaparooProject/zaparoo-frontend#360 * perf(database): reuse fetched search tags Carry title IDs through search results and derive disambiguating ZapScript tags from media-scope rows already returned by the full tag query. Keep a small-page tag preflight while avoiding a redundant query for larger tagged result sets. Developed while attributing MiSTer search latency reported through ZaparooProject/zaparoo-frontend#360. Ref ZaparooProject/zaparoo-frontend#360 * test(database): update search mocks for title IDs Update existing scoped and multi-variant search mocks for the MediaTitleDBID column and shared tag lookup introduced by search enrichment. * perf(database): stream dense search candidates Stream default-order media rows by DBID and filter large cached title candidate sets in memory, stopping once the page fills. Cache scoped media bounds, support up to four systems, and fall back to grouped SQL when a bounded window is sparse or the request is unsupported. Developed from target-device latency analysis prompted by ZaparooProject/zaparoo-frontend#360. Ref ZaparooProject/zaparoo-frontend#360 * chore(ui): document search parameter lint exceptions * fix(api): harden media browsing fallbacks --------- Co-authored-by: Giancarlo Erra <giancarlo@widescreen.studio>
This was referenced Aug 16, 2026
wizzomafizzo
marked this pull request as draft
August 17, 2026 23:37
wizzomafizzo
changed the title
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
2 participants
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
Implements and extends the direction agreed in #359: on MiSTer, the
frontend reads state Core already maintains directly from the media
database instead of paying a round trip per request. The first phase
(previously reviewed here) did it for cover artwork; the second phase
extends the same contract to folder listings and makes the model cheap
enough to receive them, which together take entering a large folder
from ~4.5 s to ~0.5 s and remove the "Loading more…" interruption from
fast scrolling entirely.
For review orientation: the artwork phase is the commit set up to
f0e88e0, which was already reviewed; the listings phase is everycommit after it.
Still one boundary (hole in the abstraction)
The artwork phase shipped under the two conditions from #359:
data directory exists; desktop and every other platform behave as
before. It queries image properties and nothing else, read-only
(
SQLITE_OPEN_READ_ONLY), and a schema guard verifies the four tablesit touches at open — if a future Core renames them the layer disables
itself loudly and every cover goes through the RPC path exactly as
today. A runtime kill switch (
ZAPAROO_FRONTEND_DB_ART=0) is includedfor A/B comparison.
The existing in-memory cache remains the only cache; the direct read is
a resolution fast path, not a store.
Working with that boundary since, I think what actually makes it sound
was never the media type. It was the direction of the operation:
same SD card. For pure reads of state Core already maintains, the
frontend consumes the database in place: read-only connection, WAL
reader, schema-guarded, no second cache, no derived storage.
launching, history, configuration: everything that changes state goes
through the API, without exception. Core stays the single writer and
the single owner of the business logic.
The listings phase does not introduce a second hole through the
abstraction. It asks to name the one hole that already exists
precisely: on MiSTer, the frontend is a read-only consumer of the
database Core maintains. Artwork was the first instance of that rule;
listings are the second instance of the same rule. The practical
benefit of naming it is that it stops the drift toward per-feature
carve-outs: any future read view either fits the stated contract
(read-only, schema-guarded, RPC fallback, Core remains authoritative)
or it does not go in, and no write path ever qualifies.
Phase 1: artwork (previously reviewed)
media_art_db: resolves a cover key (media id, else canonical path) toranked artwork-file candidates: requested type first, media-level before
title-level, the keyed row before rows sharing the same file path. The
caller tries candidates in order, so a database row pointing at a file
that no longer exists degrades to the next candidate instead of a miss.
The path-twin step also gives the v2.16 arcade classification systems
(CPS1/CPS2/Neo Geo MVS/...) working artwork: Core indexes the same file
under a second system with no art bound, and the gamelist scraper has no
folder mapping for those systems, so borrowing from the twin is the only
place the art can come from today.
the original limit of 2 concurrent
media.imageRPCs, so Core sees noadditional pressure.
rather than the 2nd press, and its quiet window drops from 260 ms to
120 ms (still above the 90 ms held-key repeat tick, so holds cannot
flicker). With covers loading in milliseconds the old gate was hiding
speed the frontend now has: a quick double-tap skip pays no artificial
delay, and held-key scrolling keeps its suppression via the forced path.
Artwork measurements (MiSTer, Cyclone V, debug-log timings): cover
fetch went from 100 ms+ per cover through Core's serialised image RPC to
a ~7 ms median database lookup plus a ~26 ms file read, workers in
parallel. Sessions run with zero image RPCs.
Phase 2: folder listings
Entering a folder now runs one read-only SQLite query against Core's
own tables and hands the complete listing to the model: a small
first-page apply paints immediately, the remainder lands as bulk
inserts behind the paint, and the whole folder is local moments later.
The RPC path is untouched and remains the only path everywhere else,
and on MiSTer whenever the direct read cannot answer with full
confidence.
Core already maintains everything a listing needs as indexed tables:
BrowseDirs/BrowseDirCounts(the pre-computed directory tree behindthe browse cache),
Media.ParentDir/SortName/IsMissingbehindidx_media_browse_sort, theuser:favoriteprojection inMediaTags,and the image property tags that back
hasCover. The queries mirrorsql_browse.go's cache path, including theBrowseIndexVersionserveability gate: if Core would not serve from its own cache, neither
do we, and the RPC answers.
Listings measurements (same device and folder shapes, before/after
logs available; "before" is the current release behavior over the RPC):
Entering a large folder (roughly two thousand entries):
The direct read itself returns the complete listing in under 200 ms
warm. The rest of the win came from making delivery cheap: the paged
grid used to materialize one delegate per model row even while hidden
behind the list layout, which cost roughly 3 ms per row on the software
renderer, so bulk inserts of a thousand rows blocked the UI thread for
about 3 seconds. With delegates suspended in list layout, the same
inserts take 5 to 7 ms, several hundred times less, and folder
entry no longer starves the cover gate.
Fast scrolling ("Loading more…" and paging):
media.browseround trip and lostroutinely: a page past the loaded edge blocked on "Loading more…"
for ~0.5 s per small chunk, and multi-second for large ones, and
chunk cost grows superlinearly with size, so bigger requests made
stalls worse, not better.
browsing: the "Loading more…" interruption is gone in list
browsing, along with the background fill churn that used to compete
with input on the UI thread.
(~2 pages per second, fixed, regardless of how fast the user
pressed). A press-stream turbo now detects held page buttons,
including Left/Right where the list layout routes them to paging,
drives paging at its own tick with rapid mode engaged (cover work
paused), and sustains roughly 66 rows per second of travel,
about six times the held line-scroll rate.
Known remaining limit: the visual beat of a full-page flip is ~3
per second on the software renderer, as ten rows re-bind and re-layout
per landing. Throughput is no longer the bottleneck; a pre-laid-out
page swap could close the remaining gap to raw-framebuffer menus and
is left as follow-up work.
Phase 3: favorites, recents, and detail metadata
Completes the read map under the same boundary: three more read-only
consumers of the databases Core maintains, each with the contract the
first two phases established (read-only connection, schema guard that
disables the layer loudly on any mismatch, per-domain kill switch, RPC
fallback on any miss). Writes still never come near any of this.
media_favorites_db,ZAPAROO_FRONTEND_DB_FAVORITES):the complete favorite set in one media.db query, tens of milliseconds
for a few hundred entries. The screen paints direct-first on cold
entry; every store Ready triggers a local re-read whose result
replaces the page content, so the endpoint subscription remains the
freshness signal and favorite toggles keep refetching exactly as
today. The paged default view and the full-set scoped views stop
being different worlds, which also dissolves the "Show: All loads a
few pages then more on scroll" report at the root.
media_history_db,ZAPAROO_FRONTEND_DB_HISTORY): thededuplicated play history from
user.db, the same contract appliedto Core's second database file. Latest event per media path via a
window query, timestamps in the wire's RFC3339 form, open sessions
keeping
endedAtnull so Resume semantics are untouched. Media idsresolve through a correlated scalar subquery against the attached
media database, keyed on path plus system so classification twins
cannot duplicate an entry (regression-tested).
media_meta_db,ZAPAROO_FRONTEND_DB_META):tags, scraped properties, the title record, and available image
types from indexed point lookups, tried before every
media.metaRPC including the prefetch cache. With the round trip gone, the
detail settle debounce drops from 220 ms to 40 ms (exposed to QML as
AppStatus.direct_meta), so the detail pane tracks the cursoressentially live.
Phase 3 measurements (same device and method as above):
With phase 3 in place, steady-state browsing performs zero RPCs:
the WebSocket carries writes, launches, and notifications.
Known divergences for this phase, same spirit as the listings list:
zapScriptis not populated on direct favorites entries (it is notstored in the database; launching uses the media path, which is the
primary launch text, and portable card-write text falls back to the
path), tag labels are empty (they come from Core's tag registry; every
consumer falls back to the tag value), and direct favorites order by
SortNamerather than Core's search order, which the view's own sortmodes sit on top of either way.
For review orientation: phase 3 is the last four commits (the three
feat(mister)reads plus the review-round fix).Listings guard rails (same contract as the artwork path)
SQLITE_OPEN_READ_ONLY, short busy timeout, nowrites, no checkpoints, no pragma tuning. WAL readers do not block
Core's writer.
place, per the condition agreed for the artwork path.
any mismatch disables the layer loudly, leaving the RPC as the sole
source.
launcher routes), the letter index, search, meta, and all writes
(favorites) remain on the RPC.
ZAPAROO_FRONTEND_DB_BROWSE=0disables the layer at runtime; theartwork layer has the equivalent switch. The A/B numbers above were
taken with it.
Known divergences from
media.browsedeemed acceptableZipsAsDirs): affectedfolders render as plain directories; drilling in shows the single
item. Could be added later with the same resolution query Core uses.
default
SortNameorder. Could be ported if wanted.disambiguatingTagsvariant badges at browse level; the detailpane still gets full tags via
media.meta.live-refresh that folder (the RPC path's store subscription does);
re-entering the folder re-reads fresh data.
Each of these trades a rarely-visible feature for the elimination of
every listing round trip.
Why making
media.browsefaster in Core is not the real solutionWorth doing too, and it would help every client. But even an instant
server still pays serialization, transport, client deserialize, and
per-page model delivery. For the frontend that lives on the same SD
card as the database, one indexed query straight into the model is a
structurally lower floor, in the same way the artwork direct read was.
The delivery-side fixes in this PR (split apply, delegate suspension,
the paging turbo) stand on their own and benefit the RPC path too.
Motivation (speed and responsiveness)
Newer frontends, Console Mode being the most visible example, feel instant
because they read local data directly. Zaparoo is the better product by
a wide margin, and after the artwork fast path the listings were the last
place the difference showed. The aim of this PR is simply that
Zaparoo Frontend feels fast during use on MiSTer, with Core keeping full
ownership of everything that writes.
Testing
against a synthetic schema: dirs-before-files ordering, per-system
filtering, favorite and has-cover flags (media- and title-level),
SortNamefallback, missing-row exclusion, cache-gate refusal,unknown-path refusal.
from the model with zero delegates) and the paging turbo state
machine (threshold engagement, absorption, direction flip, stop on
other input).
direct, from the same binary and debug log.
bindings, and RPC fallback verified on device.
Summary by CodeRabbit
New Features
Performance
Bug Fixes
Localization