feat(browser): album artwork with virtualized accessible UI (tr-xaj) by jm2 · Pull Request #171 · jm2/tributary

jm2 · 2026-07-25T20:13:21Z

Implements a first pass at album artwork in the browser's album pane.

Bead: tr-xaj — docs/task.md:994-996, issue #39.

Current status

Draft; returned for redesign after exact-head review of
f5ccd34d0eb1f958c07b8a32e6b0acaa97f9742c.

Hosted CI is largely green on this head, but that does not resolve the
behavioral and lifecycle defects below. This revision must not merge.

Required fixes

Replace the process-global artwork generation counter with independent,
request-local cancellation. As written, visible album thumbnails cancel one
another and interfere with now-playing artwork.
Revoke every outstanding request when a virtualized row is rebound,
including cache-hit and no-art paths, so a late result cannot paint a reused
row.
Bound asynchronous work. Remote requests currently use an unbounded channel
and local artwork can spawn one OS thread per bind.
Preserve retained local-media authority instead of reopening raw paths from
browser candidates.
Preserve the existing album selection handler and active genre/artist
filters when artwork preferences change; rebuilding the pane currently loses
both.
Fix placeholder installation: set_paintable(None) clears the icon that
show_placeholder() just installed.
Scope cache identity to the source/session and cache decoded thumbnails under
an explicit memory budget rather than retaining up to 512 full-size textures.
Add production-path tests for concurrent rows, late completion after
unbind/rebind, source isolation, preference changes, placeholders, retained
authority, and real config serialization. Perform a manual accessibility and
high-contrast smoke test.

The existing branch is intentionally preserved for fix-forward work. A new
exact-head correctness review and fresh CI run are required after the redesign.

gemini-code-assist · 2026-07-25T20:13:24Z

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

codacy-production

Pull Request Overview

The current state of the Pull Request is not up to standards, primarily due to a complete mismatch between the intended functionality and the submitted code. The PR description lists numerous features and bug fixes for the browser UI that are entirely absent from the provided changes, which consist solely of a new CI/CD workflow. Furthermore, Codacy analysis indicates the PR is not up to standards due to security risks in the workflow configuration.

Before this can be considered for merging, the intended UI code must be included, and the identified security issues—specifically regarding credential management and action versioning—must be addressed. No progress can be validated against the specified acceptance criteria for the album artwork feature at this time.

About this PR

Complete scope misalignment: The PR description lists extensive changes to Rust source files (e.g., src/ui/album_pane_art.rs, src/ui/browser.rs) and localization files, but none of these changes are present in the provided diff. The actual implementation of the artwork feature, the LRU cache, and the described bug fixes is entirely missing.

Test suggestions

Missing recommended test scenario: Verify LRU cache eviction and keying logic for album artwork.
Missing recommended test scenario: Verify generation-guarded painting prevents race conditions when UI rows are recycled.
Missing recommended test scenario: Verify artwork resolver error logs do not contain sensitive URLs.
Missing recommended test scenario: Verify UI updates correctly when artwork size preferences are modified.
Missing recommended test scenario: Verify GTK4 image placeholders use the correct size sentinel (-1).

Prompt proposal for missing tests

Consider implementing these tests if applicable:
1. Missing recommended test scenario: Verify LRU cache eviction and keying logic for album artwork.
2. Missing recommended test scenario: Verify generation-guarded painting prevents race conditions when UI rows are recycled.
3. Missing recommended test scenario: Verify artwork resolver error logs do not contain sensitive URLs.
4. Missing recommended test scenario: Verify UI updates correctly when artwork size preferences are modified.
5. Missing recommended test scenario: Verify GTK4 image placeholders use the correct size sentinel (-1).

_{TIP Improve review quality by adding custom instructions}
_{TIP How was this review? Give us feedback}

codacy-production · 2026-07-25T22:55:06Z

+          fetch-depth: 1
+
+      - name: Claude PR review
+        uses: anthropics/claude-code-action@v1


_{🔴 HIGH RISK}

Reference third-party GitHub Actions by their specific commit SHA instead of version tags to ensure the integrity of your CI/CD pipeline. Look up the full commit SHA for 'anthropics/claude-code-action@v1' and 'actions/checkout@v6' and update the 'uses' fields accordingly.

See Issue in Codacy

codacy-production · 2026-07-25T22:55:06Z

+        with:
+          fetch-depth: 1
+
+      - name: Claude PR review


_{🔴 HIGH RISK}

The GitHub CLI commands used in the 'Claude PR review' step will fail without authentication. Add the 'GITHUB_TOKEN' to the environment variables for this step: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}.

codacy-production · 2026-07-25T22:55:06Z

@@ -0,0 +1,73 @@
+name: Claude PR Review


_{🟡 MEDIUM RISK}

The PR title 'feat(browser): album artwork with virtualized accessible UI (tr-xaj)' and the associated description do not match the content of this change, which only adds a GitHub Actions workflow. Please include the missing files related to the UI implementation and LRU caching.

codacy-production · 2026-07-25T22:55:24Z

Not up to standards ⛔

🔴 Issues 3 medium

Alerts:
⚠ 3 issues (≤ 0 issues of at least minor severity)

Results:
3 new issues

Category Results

Complexity 3 medium

View in Codacy

🟢 Metrics 104 complexity · 20 duplication

Metric Results

Complexity 104

Duplication 20

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

_{TIP This summary will be updated as you push new changes.}

codacy-production

Pull Request Overview

This PR is currently not up to standards due to 15 new quality issues and significant logic gaps in the transfer execution engine. Most critically, there is a total misalignment between the PR title and the code contents, which implements a transfer planner and executor for Issue #8.

Technically, the implementation fails to satisfy the 'Roll back committed files if a transfer stage fails' acceptance criterion. The current loop structure uses the '?' operator for error propagation, which causes the executor to exit immediately upon failure, bypassing necessary cleanup. Furthermore, the rollback mechanism is incomplete as it lacks logic to remove directories created during the process. High cyclomatic complexity in the plan method and missing integration tests for failure recovery should also be addressed before merging.

About this PR

Testing Gap: There are no integration tests verifying that the executor rolls back multiple successfully committed files when a subsequent file in the same plan fails. Given the complexity of the rollback logic, these tests are essential.
Major Misalignment: The PR title and description describe 'album artwork' features for the browser UI, but the implementation is entirely focused on 'device file transfer' (Issue #8). Please update the metadata to reflect the actual changes.

Test suggestions

Planner correctly expands directories recursively into creation and copy stages
Planner rejects requests that exceed the specified capacity budget
Executor handles ConflictPolicy::Preserve by generating a disambiguated ' (1)' suffixed filename
Executor performing an atomic commit of a staged file using a rename
Executor rolls back all previously committed files if a stage fails due to I/O error
Executor rolls back committed files upon cooperative cancellation (verification of total rollback)

Prompt proposal for missing tests

Consider implementing these tests if applicable:
1. Executor rolls back all previously committed files if a stage fails due to I/O error
2. Executor rolls back committed files upon cooperative cancellation (verification of total rollback)

Low confidence findings

Unused Logic: The Stage::RemoveFile enum variant is documented as being for rollback but is not utilized by the actual rollback implementation, which uses a private list of paths. This should be reconciled.

_{TIP Improve review quality by adding custom instructions}
_{TIP How was this review? Give us feedback}

codacy-production · 2026-07-25T22:55:47Z

+                    self.execute_create_directory(destination_relative_path)?;
+                }
+                Stage::CopyFile {
+                    source_relative_path,
+                    destination_relative_path,
+                    bytes,
+                    atomic: _,
+                    conflict,
+                } => {
+                    let outcome = self.execute_copy_file(
+                        source_relative_path,
+                        destination_relative_path,
+                        *bytes,
+                        &mut bytes_so_far,
+                        total_bytes,
+                        index as u32,
+                        total_stages,
+                        progress,
+                        cancellation,
+                    )?;
+                    let _ = outcome;
+                    let _ = conflict;
+                    committed_files.push(destination_relative_path.clone());
+                }
+                Stage::RemoveFile { .. } => {
+                    // RemoveFile stages are inserted only by the rollback path
+                    // and never appear in a forward plan. Skip defensively.
+                }
+            }
+            committed_stages = committed_stages.saturating_add(1);
+            progress.on_stage_completed(
+                stage,
+                index as u32,
+                total_stages,
+                bytes_so_far,
+                total_bytes,
+            );
+        }


_{🔴 HIGH RISK}

Failure to roll back committed files on stage error. Early returns via '?' (here and at line 729) skip the mandatory cleanup of previously committed files in the plan. Wrap the execution loop in a closure or capture the result in a variable to ensure rollback() is called if an error occurs.

codacy-production · 2026-07-25T22:55:47Z

+    fn rollback(&self, committed_files: &mut Vec<PathBuf>) -> io::Result<()> {
+        while let Some(relative) = committed_files.pop() {
+            self.request
+                .destination
+                .validate()
+                .map_err(|error| {
+                    TransferError::authority(format!("destination not current: {error}"))
+                })
+                .map_err(|error| io::Error::other(format!("{error:?}")))?;
+            self.request.destination.remove_relative_file(&relative)?;
+        }
+        Ok(())
+    }
+}


_{🟡 MEDIUM RISK}

Rollback should track and remove directories created during execution. Currently, it only processes committed_files. Consider maintaining a stack of all successful stages and reversing them using remove_relative_directory for directory stages.

codacy-production · 2026-07-25T22:55:47Z

+    }
+
+    #[allow(clippy::too_many_arguments)]
+    fn execute_copy_file(


_{🟡 MEDIUM RISK}

This method has 10 parameters, making the call site difficult to manage. Consider bundling the progress-related arguments (bytes_so_far, total_bytes, stage_index, total_stages, progress, cancellation) into a single ProgressContext struct.

See Issue in Codacy

codacy-production · 2026-07-25T22:55:47Z

+    /// resolve conflict policy; the resolved policy is recorded on every
+    /// copy stage so the executor never re-decides a conflict.
+    #[allow(clippy::unused_self)]
+    pub fn plan(&self, request: &TransferRequest) -> Result<TransferPlan, TransferError> {


_{🟡 MEDIUM RISK}

The plan method is currently handling too many responsibilities. Consider extracting the logic for processing individual transfer items into a dedicated private method like fn plan_item(...) to improve maintainability and reduce cyclomatic complexity.

See Issue in Codacy
See Issue in Codacy

codacy-production · 2026-07-25T22:55:47Z

+                    .map_err(|error| TransferError::io("failed to flush staged file", error))
+                    .map_err(io::Error::other)?;
+
+                if declared_bytes != 0 && copied != declared_bytes {


_{⚪ LOW RISK}

Remove the zero-check to ensure that files that were planned as empty are still verified at execution time if they were modified on disk.

codacy-production · 2026-07-25T22:55:47Z

+    }
+
+    /// Remove a regular file atomically through the retained authority.
+    pub fn remove_relative_file(&self, relative: &Path) -> io::Result<()> {


_{⚪ LOW RISK}

Suggestion: Duplicate logic detected between remove_relative_file and remove_relative_directory. Extract a private helper method to handle the shared path assembly and metadata validation logic.

See Clone in Codacy

codacy-production · 2026-07-25T22:55:47Z

+            request.source.validate().map_err(|error| {
+                TransferError::io("source authority failed pre-plan validation", error)
+            })?;
+            request.destination.validate().map_err(|error| {
+                TransferError::io("destination authority failed pre-plan validation", error)
+            })?;


_{⚪ LOW RISK}

Nitpick: Move the authority validation calls outside of the for item in &request.items loop. Validation at the start of the plan method is sufficient.

claude · 2026-07-26T18:54:31Z

Claude finished @jm2's task in 4m 50s —— View job

Claude's review

Todo list

Gather context (PR body, diff, changed files)
Read repo CLAUDE.md for repo-specific conventions
Review src/ui/album_pane_art.rs (new, cache/generation-guard/placeholder logic)
Review src/ui/browser.rs changes (bind factory, rebuild_album_pane)
Review src/ui/preferences.rs, src/ui/window.rs, locales
Review src/ui/objects/browser_item.rs and mod.rs changes
Check test coverage for the new module
Post inline findings and final review

No CLAUDE.md was found in the repo, so I reviewed against general Rust/GTK4 conventions and the patterns already established elsewhere in this file (e.g. set_album_artist_grouping, rebuild_browser_data).

Findings (posted inline)

[High] rebuild_album_pane drops the album pane's selection-changed wiring (src/ui/browser.rs, around lines 357–396 / 561). build_album_pane never calls connect_selection_changed; that's wired once in build_browser onto the original pane's SingleSelection. rebuild_album_pane swaps in a brand-new pane with a fresh SingleSelection that has no listeners. Practical effect: after the first toggle of the album-artwork checkbox or a size change in Preferences, clicking an album row silently stops cross-filtering Genre/Artist and stops calling on_filter_changed for the rest of the session. This is the same class of regression the PR description says was already fixed twice (pane blanking, inert size pref) — the safer existing pattern (set_album_artist_grouping/rebuild_browser_data) repopulates the store in place instead of replacing the pane, and doesn't have this problem.
[Medium] AlbumArtSize::as_token/from_token doc claims a migration guarantee that isn't wired to actual persistence (src/ui/preferences.rs, lines 113–133). Config is persisted via serde_json using the enum's derived Serialize/Deserialize (no rename_all), so the JSON on disk stores "Small"/"Medium"/"Large", not the lowercase tokens these functions produce. Both functions are #[allow(dead_code)] and used only by this module's own test. So today there's no real protection against a future variant rename breaking existing users' config.json (a failed deserialize resets all of AppConfig to defaults, not just this field) — worth either wiring the tokens into a real Serialize/Deserialize impl (or #[serde(rename_all = "lowercase")]) or dropping the dead code/misleading comment.

Other observations (not inline)

Testing gap: browser.rs has no unit tests at all (consistent with the rest of the file — GTK widgets need a main thread CI doesn't reliably provide), but that means the exact widget-tree logic in rebuild_album_pane where finding USB device support (drag and drop) #1 lives is untested and unverifiable by cargo test. Given this PR's own history of pane-rebuild regressions, a manual/GTK-inspector smoke test of "toggle artwork → click an album row → confirm Genre/Artist panes still filter" would have caught this.
src/ui/album_pane_art.rs itself is solid: the bounded LRU cache, generation-guarded async fetch (BindGeneration), and placeholder/pixel-size-sentinel (-1 vs 0) handling all look correct, and the unit tests directly exercise the cache eviction/promotion and generation-monotonicity invariants they claim to guarantee. The #[cfg(test)] use crate::ui::preferences::AlbumArtSize plus a handful of #[allow(dead_code)] public methods (build_binder without size, AlbumArtController::attach_pixel_size, AlbumArtCellState::reset/row) are currently unused outside tests — minor surface-area cleanup opportunity, not a correctness issue.
Credential-boundary claim in the PR body checks out for the code actually touched here: resolve_kind's error-path logging (album_pane_art.rs) only includes error, source_id, and track_id, no URLs — consistent with the stated constraint. I did not re-audit the pre-existing SourceRegistry::resolve_artwork implementation itself, since it's outside this diff.
Note: two of the Codacy bot review comments on this PR (referencing src/device/transfer.rs, a "device file transfer" feature, and claiming the PR "only adds a CI/CD workflow") describe content that does not match this PR's actual diff. I disregarded those as not applicable to PR feat(browser): album artwork with virtualized accessible UI (tr-xaj) #171 and reviewed the real album-artwork changes instead.

coderabbitai · 2026-07-26T18:54:35Z

📝 Walkthrough

Walkthrough

Changes

Album-pane artwork support is added across preferences, album-item metadata, asynchronous thumbnail rendering, bounded caching, browser pane rebuilding, and window-level preference/source-registry wiring.

Album pane artwork

Layer / File(s)	Summary
Artwork preferences and item contracts `locales/en.yml`, `src/ui/preferences.rs`, `src/ui/objects/*`	Adds persisted artwork visibility and size settings, preference controls and translations, plus album artwork candidates and row-state accessors.
Artwork cell, cache, and fetch pipeline `src/ui/album_pane_art.rs`, `src/ui/mod.rs`	Adds reusable GTK artwork cells, generation-safe async binding, bounded pixel-size-aware caching, artwork resolution paths, and unit tests.
Browser album-pane integration `src/ui/browser.rs`	Carries artwork inputs through track snapshots, creates candidate-backed album rows, selects thumbnail or text binders, and rebuilds the pane when artwork settings change.
Window and preference wiring `src/ui/window.rs`	Loads persisted artwork settings, attaches the source registry, and connects preference callbacks to browser artwork updates.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Preferences
  participant Window
  participant Browser
  participant AlbumArtController
  participant AlbumArtCache
  Preferences->>Window: change artwork toggle or size
  Window->>Browser: update album pane settings
  Browser->>AlbumArtCache: clear cached textures
  Browser->>AlbumArtController: build artwork binder
  AlbumArtController->>AlbumArtCache: read or store artwork by album and size

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: adding album artwork support in the browser with a virtualized UI.

✨ 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 polecat/tr-xaj

_{Comment @coderabbitai help to get the list of available commands.}

claude · 2026-07-26T18:59:03Z

+fn rebuild_album_pane(browser_box: &gtk::Box, state: &BrowserState) {
+    let panes_box = browser_box
+        .last_child()
+        .and_then(|w| w.downcast::<gtk::Box>().ok());
+    let Some(panes_box) = panes_box else {
+        return;
+    };
+
+    let mut child = panes_box.first_child();
+    let mut panes = Vec::new();
+    while let Some(widget) = child {
+        if let Some(pane) = widget.downcast_ref::<gtk::Box>() {
+            panes.push(pane.clone());
+        }
+        child = widget.next_sibling();
+    }
+
+    if panes.len() < 3 {
+        return;
+    }
+
+    // Album pane is the 3rd child (index 2). Replace it.
+    let old_pane = panes[2].clone();
+    let album_store =
+        album_store_from_pane(&old_pane).unwrap_or_else(gio::ListStore::new::<BrowserItem>);
+
+    // Clear the cache so the new bind factory doesn't serve stale
+    // textures from before the layout change — they're decoded at the
+    // old size, and a stale hit would bypass the new bind path entirely.
+    state.album_art_cache.inner.borrow_mut().entries.clear();
+    state.album_art_cache.inner.borrow_mut().order.clear();
+
+    let new_pane = build_album_pane(
+        &album_store,
+        state.album_art_controller.clone(),
+        state.album_pane_artwork.clone(),
+        state.album_pane_artwork_size.clone(),
+    );
+    panes_box.remove(&old_pane);
+    panes_box.append(&new_pane);


High risk: rebuild_album_pane silently drops the album pane's selection-changed handler.

build_album_pane (line 561) only wires up widgets — it never calls connect_selection_changed on the pane's SingleSelection. That wiring is installed exactly once, in build_browser, on the original album pane's selection object (let sel = get_selection(&album_pane); sel.connect_selection_changed(...) around line 212).

rebuild_album_pane builds a brand-new pane via build_album_pane (line 389) with a fresh gtk::SingleSelection (line 576) and swaps it into panes_box (line 396). That new selection object has no listeners at all, so after the very first call to set_album_pane_artwork or set_album_pane_artwork_size (i.e. the first time a user toggles the artwork checkbox or picks a size in Preferences), clicking an album row stops cross-filtering the Genre/Artist panes and stops invoking on_filter_changed for the rest of the session — silently, with no error.

Contrast with the established, safer pattern already used for the same kind of preference change: set_album_artist_grouping (line 859) and rebuild_browser_data (line 806) only repopulate the existing gio::ListStore in place via populate_*, and deliberately keep the original pane/SingleSelection/listener alive. rebuild_album_pane is the only one of the three that replaces the pane wholesale, and it's exactly this class of bug the PR's own description calls out fixing twice already (pane blanking, inert size preference).

A fix that preserves the selection object would sidestep this: extract list_view.set_factory(&new_factory) on the existing ListView/SingleSelection instead of rebuilding the whole pane box, or re-attach connect_selection_changed (with the same closure body used at pane-construction time) to the new pane's selection before appending it.

Fix this →

claude · 2026-07-26T18:59:09Z

+    /// Stable persistence token. New variants must keep older strings
+    /// recognized for in-place config migration.
+    #[allow(dead_code)]
+    pub const fn as_token(self) -> &'static str {
+        match self {
+            Self::Small => "small",
+            Self::Medium => "medium",
+            Self::Large => "large",
+        }
+    }
+
+    /// Parse a previously-persisted token. Returns `None` for unknown
+    /// values so callers can fall back rather than reject a config file.
+    #[allow(dead_code)]
+    pub fn from_token(token: &str) -> Option<Self> {
+        match token {
+            "small" => Some(Self::Small),
+            "medium" => Some(Self::Medium),
+            "large" => Some(Self::Large),
+            _ => None,
+        }


Medium: doc comment describes a persistence mechanism that isn't actually wired up.

as_token/from_token are documented as "Stable persistence token[s]" that "must keep older strings recognized for in-place config migration," but AlbumArtSize derives Serialize/Deserialize with no #[serde(rename_all = ...)] or custom impl — so serde_json (confirmed as the config format via save_config/load_config) actually persists/reads the Rust variant names directly ("Small"/"Medium"/"Large"), not the lowercase tokens these functions produce/parse. Grepping the tree, as_token/from_token are called nowhere outside this module's own test (album_pane_art.rs:840) and are both marked #[allow(dead_code)].

Net effect: the actual persisted representation has no migration protection at all (renaming an enum variant would break serde_json::from_value for the whole AppConfig, which falls back to all-defaults in load_config, not just this field), while the code that looks like it provides that protection is unused. Worth either wiring as_token/from_token into custom Serialize/Deserialize impls (or #[serde(rename_all = "lowercase")] on the enum) so the documented guarantee is real, or dropping the dead functions and the misleading comment.

coderabbitai

Actionable comments posted: 3

🧹 Nitpick comments (3)

src/ui/preferences.rs (1)
113-134: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

as_token/from_token don't match what actually gets persisted.

The derived Serialize/Deserialize writes variant names ("Small"/"Medium"/"Large"), not these lowercase tokens, so the "stable persistence token" contract is only exercised by tests. Either drive serde through the tokens (#[serde(rename_all = "lowercase")]) or drop the doc claim so a future migration doesn't rely on a mapping that was never written to disk.
♻️ Align serde with the token strings
-#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)]
+#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)]
+#[serde(rename_all = "lowercase")]
 pub enum AlbumArtSize {
Note this changes the on-disk representation; combine with #[serde(default)] (already present) and an alias if configs with capitalized names already exist in the wild.
🤖 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/preferences.rs` around lines 113 - 134, Align serde persistence for
the size enum with the tokens returned by as_token and accepted by from_token by
applying lowercase variant renaming and preserving compatibility with existing
capitalized serialized names through serde aliases. Keep the existing
#[serde(default)] behavior and token conversion methods unchanged.
src/ui/album_pane_art.rs (1)
776-826: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

These three tests assert BindGeneration::next() against itself, not the cancellation contract.

bind_generation_advances_on_reset and bind_generation_late_result_is_dropped never touch AlbumArtCellState or reset, so they'd still pass if the bind/unbind paths stopped bumping the generation. AlbumArtCellState::reset/current_generation are GTK-free apart from show_placeholder; splitting the generation bump into a widget-free method would let these tests cover the real invariant without a main thread.
🤖 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/album_pane_art.rs` around lines 776 - 826, Update the tests to
exercise AlbumArtCellState’s actual generation lifecycle rather than calling
BindGeneration::next() directly. Add or use a GTK-free reset/generation-bump
method in AlbumArtCellState, have reset invoke it while preserving
show_placeholder behavior, and revise bind_generation_advances_on_reset and
bind_generation_late_result_is_dropped to verify current_generation changes
across reset and rejects a captured prior generation.
src/ui/objects/browser_item.rs (1)
20-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

is_all_row is dead state unless the binder uses it. The “All” row currently skips artwork because BrowserItem::new leaves artwork_candidate empty; AlbumArtBinder::bind_fn doesn’t read is_all_row. Either thread the flag into the binder or trim the field/comment so the invariant stays accurate.
🤖 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/objects/browser_item.rs` around lines 20 - 23, Remove the unused
is_all_row field and its associated documentation from BrowserItem unless the
binder is updated to consume it. If retaining the field, update
AlbumArtBinder::bind_fn to read it and explicitly skip artwork fetching for the
“All” row while preserving the existing text label behavior.

🤖 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 `@src/ui/album_pane_art.rs`:
- Around line 425-433: Prevent stale album-art cell state by adding a teardown
closure in build_binder_with_size_internal that removes each ListItem pointer
key from cell_states. In src/ui/album_pane_art.rs#L425-433, return the new
teardown alongside the existing factory callbacks; in
src/ui/browser.rs#L561-633, destructure it and register it with
factory.connect_teardown(teardown) next to the setup, bind, and unbind wiring.
- Around line 583-607: Move the install_cache_probe call to before the match
dispatches update_direct_file_album_art, fetch_remote_album_art, or
fetch_resolved_album_art, while preserving the existing cache, image, album_key,
pixel_size, cell_state, and generation arguments. Keep the artwork resolution
branches unchanged so synchronous paintable updates are observed by the probe.

In `@src/ui/browser.rs`:
- Around line 357-411: The rebuilt pane in rebuild_album_pane must preserve
album behavior and filtering state. Move the album selection wiring currently
created in build_browser—selected_genre, selected_artist, and
on_filter_changed—into BrowserState or an equivalent shared AlbumPaneWiring,
reconnect the new pane’s SingleSelection callback after build_album_pane, and
pass the live genre/artist filters plus current album selection to
populate_albums instead of &None values.

---

Nitpick comments:
In `@src/ui/album_pane_art.rs`:
- Around line 776-826: Update the tests to exercise AlbumArtCellState’s actual
generation lifecycle rather than calling BindGeneration::next() directly. Add or
use a GTK-free reset/generation-bump method in AlbumArtCellState, have reset
invoke it while preserving show_placeholder behavior, and revise
bind_generation_advances_on_reset and bind_generation_late_result_is_dropped to
verify current_generation changes across reset and rejects a captured prior
generation.

In `@src/ui/objects/browser_item.rs`:
- Around line 20-23: Remove the unused is_all_row field and its associated
documentation from BrowserItem unless the binder is updated to consume it. If
retaining the field, update AlbumArtBinder::bind_fn to read it and explicitly
skip artwork fetching for the “All” row while preserving the existing text label
behavior.

In `@src/ui/preferences.rs`:
- Around line 113-134: Align serde persistence for the size enum with the tokens
returned by as_token and accepted by from_token by applying lowercase variant
renaming and preserving compatibility with existing capitalized serialized names
through serde aliases. Keep the existing #[serde(default)] behavior and token
conversion methods unchanged.

🪄 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: 532a8ebb-d5dc-4123-8f03-08a8cd2fa5b5

📥 Commits

Reviewing files that changed from the base of the PR and between 7e0aad8 and 23830ad.

📒 Files selected for processing (8)

locales/en.yml
src/ui/album_pane_art.rs
src/ui/browser.rs
src/ui/mod.rs
src/ui/objects/browser_item.rs
src/ui/objects/mod.rs
src/ui/preferences.rs
src/ui/window.rs

coderabbitai · 2026-07-26T19:01:46Z

+        let setup = move |_factory: &gtk::SignalListItemFactory, list_item: &glib::Object| {
+            let list_item = list_item.downcast_ref::<gtk::ListItem>().expect("ListItem");
+            let cell_state = AlbumArtCellState::new(AlbumArtCell::new(placeholder_icon));
+            let row_widget = cell_state.cell.row.clone();
+            cell_states
+                .borrow_mut()
+                .insert(list_item.as_ptr() as usize, cell_state);
+            list_item.set_child(Some(&row_widget));
+        };


🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Missing teardown handling for the album-art factory. The setup closure registers a cell_states entry per ListItem and no code path removes it, so map entries and their widget trees outlive the list view — notably across every artwork toggle/size rebuild.

src/ui/album_pane_art.rs#L425-L433: return a teardown closure from build_binder_with_size_internal that removes list_item.as_ptr() as usize from cell_states.

src/ui/browser.rs#L561-L633: destructure the new closure and call factory.connect_teardown(teardown) alongside the existing setup/bind/unbind wiring.

📍 Affects 2 files

src/ui/album_pane_art.rs#L425-L433 (this comment)

src/ui/browser.rs#L561-L633

🤖 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/album_pane_art.rs` around lines 425 - 433, Prevent stale album-art cell state by adding a teardown closure in build_binder_with_size_internal that removes each ListItem pointer key from cell_states. In src/ui/album_pane_art.rs#L425-433, return the new teardown alongside the existing factory callbacks; in src/ui/browser.rs#L561-633, destructure it and register it with factory.connect_teardown(teardown) next to the setup, bind, and unbind wiring.

coderabbitai · 2026-07-26T19:01:46Z

+            match resolved {
+                ResolvedArtKind::NoArtwork => {
+                    // Leave the placeholder visible.
+                }
+                ResolvedArtKind::DirectFile { uri } => {
+                    // Embedded extraction goes through the album-art
+                    // worker; the worker's own generation check prevents
+                    // late results from racing newer rows.
+                    album_art::update_direct_file_album_art(&image, &uri);
+                }
+                ResolvedArtKind::DirectUrl { url } => {
+                    album_art::fetch_remote_album_art(&image, &url);
+                }
+                ResolvedArtKind::ResolvedRequest(request) => {
+                    let gen = album_art::begin_remote_album_art(&image);
+                    album_art::fetch_resolved_album_art(&image, *request, gen);
+                }
+            }
+
+            // Cache the texture only once the worker publishes it. The
+            // worker delivers bytes through `gdk::Texture::from_bytes`
+            // synchronously on the GTK main loop, so we listen for the
+            // resulting `paintable` property change.
+            install_cache_probe(cache, image, album_key, pixel_size, cell_state, generation);
+        });


🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win

The cache probe is installed after the fetch is dispatched, so synchronous results are never cached.

update_direct_file_album_art / fetch_remote_album_art can set the paintable before install_cache_probe connects the notify::paintable listener (e.g. a warm worker cache or an already-decoded local file). Those rows then miss the cache on every rebind and re-fetch forever. Connect the probe first, then dispatch.

🐛 Install the probe before dispatching

+ install_cache_probe( + cache, + image.clone(), + album_key, + pixel_size, + cell_state, + generation, + ); + match resolved { ResolvedArtKind::NoArtwork => { // Leave the placeholder visible. } @@ ResolvedArtKind::ResolvedRequest(request) => { let gen = album_art::begin_remote_album_art(&image); album_art::fetch_resolved_album_art(&image, *request, gen); } } - - // Cache the texture only once the worker publishes it. The - // worker delivers bytes through `gdk::Texture::from_bytes` - // synchronously on the GTK main loop, so we listen for the - // resulting `paintable` property change. - install_cache_probe(cache, image, album_key, pixel_size, cell_state, generation);

📝 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

match resolved {

ResolvedArtKind::NoArtwork => {

// Leave the placeholder visible.

}

ResolvedArtKind::DirectFile { uri } => {

// Embedded extraction goes through the album-art

// worker; the worker's own generation check prevents

// late results from racing newer rows.

album_art::update_direct_file_album_art(&image, &uri);

}

ResolvedArtKind::DirectUrl { url } => {

album_art::fetch_remote_album_art(&image, &url);

}

ResolvedArtKind::ResolvedRequest(request) => {

let gen = album_art::begin_remote_album_art(&image);

album_art::fetch_resolved_album_art(&image, *request, gen);

}

}

// Cache the texture only once the worker publishes it. The

// worker delivers bytes through `gdk::Texture::from_bytes`

// synchronously on the GTK main loop, so we listen for the

// resulting `paintable` property change.

install_cache_probe(cache, image, album_key, pixel_size, cell_state, generation);

});

install_cache_probe(

cache,

image.clone(),

album_key,

pixel_size,

cell_state,

generation,

);

match resolved {

ResolvedArtKind::NoArtwork => {

// Leave the placeholder visible.

}

ResolvedArtKind::DirectFile { uri } => {

// Embedded extraction goes through the album-art

// worker; the worker's own generation check prevents

// late results from racing newer rows.

album_art::update_direct_file_album_art(&image, &uri);

}

ResolvedArtKind::DirectUrl { url } => {

album_art::fetch_remote_album_art(&image, &url);

}

ResolvedArtKind::ResolvedRequest(request) => {

let gen = album_art::begin_remote_album_art(&image);

album_art::fetch_resolved_album_art(&image, *request, gen);

}

}

});

🤖 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/album_pane_art.rs` around lines 583 - 607, Move the install_cache_probe call to before the match dispatches update_direct_file_album_art, fetch_remote_album_art, or fetch_resolved_album_art, while preserving the existing cache, image, album_key, pixel_size, cell_state, and generation arguments. Keep the artwork resolution branches unchanged so synchronous paintable updates are observed by the probe.

coderabbitai · 2026-07-26T19:01:46Z

+fn rebuild_album_pane(browser_box: &gtk::Box, state: &BrowserState) {
+    let panes_box = browser_box
+        .last_child()
+        .and_then(|w| w.downcast::<gtk::Box>().ok());
+    let Some(panes_box) = panes_box else {
+        return;
+    };
+
+    let mut child = panes_box.first_child();
+    let mut panes = Vec::new();
+    while let Some(widget) = child {
+        if let Some(pane) = widget.downcast_ref::<gtk::Box>() {
+            panes.push(pane.clone());
+        }
+        child = widget.next_sibling();
+    }
+
+    if panes.len() < 3 {
+        return;
+    }
+
+    // Album pane is the 3rd child (index 2). Replace it.
+    let old_pane = panes[2].clone();
+    let album_store =
+        album_store_from_pane(&old_pane).unwrap_or_else(gio::ListStore::new::<BrowserItem>);
+
+    // Clear the cache so the new bind factory doesn't serve stale
+    // textures from before the layout change — they're decoded at the
+    // old size, and a stale hit would bypass the new bind path entirely.
+    state.album_art_cache.inner.borrow_mut().entries.clear();
+    state.album_art_cache.inner.borrow_mut().order.clear();
+
+    let new_pane = build_album_pane(
+        &album_store,
+        state.album_art_controller.clone(),
+        state.album_pane_artwork.clone(),
+        state.album_pane_artwork_size.clone(),
+    );
+    panes_box.remove(&old_pane);
+    panes_box.append(&new_pane);
+
+    // The new album pane keeps the same `gio::ListStore` as the old one,
+    // so the album rows survive the swap. Repopulate the store from the
+    // current snapshot so the BrowserItem's artwork candidates are
+    // refreshed against the latest library state — but do NOT call
+    // `rebuild_browser_data` here: that helper would replace
+    // `state.tracks` with whatever slice it is handed, and the call site
+    // would have to pass the live master track list to avoid blanking
+    // the genre and artist panes. Toggling the artwork checkbox or
+    // changing the size is a layout event, not a library sync, so the
+    // snapshot must stay put.
+    let borrowed = state.tracks.borrow();
+    let use_aa = state.use_album_artist.get();
+    populate_albums(&album_store, &borrowed, &None, &None, use_aa);
+}


🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift

The rebuilt album pane loses its selection_changed wiring — the album pane goes inert after any artwork toggle/size change.

build_browser connects the album selection handler (cross-filtering genre/artist and firing on_filter_changed) on the original pane's SingleSelection. rebuild_album_pane constructs a brand-new pane/selection and never reconnects it, so after the user flips the artwork checkbox or changes the size, clicking an album no longer filters the tracklist.

Same root cause affects state: the repopulate at Line 410 passes &None, &None, discarding the active genre/artist filters and the current album selection, so the pane also silently widens back to the full album list.

The selection callback and the filter state (selected_genre, selected_artist, on_filter_changed) are local to build_browser today; they need to be moved into BrowserState (or a small AlbumPaneWiring struct) so rebuild_album_pane can both reconnect the handler and repopulate with the live filters.

🤖 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/browser.rs` around lines 357 - 411, The rebuilt pane in rebuild_album_pane must preserve album behavior and filtering state. Move the album selection wiring currently created in build_browser—selected_genre, selected_artist, and on_filter_changed—into BrowserState or an equivalent shared AlbumPaneWiring, reconnect the new pane’s SingleSelection callback after build_album_pane, and pass the live genre/artist filters plus current album selection to populate_albums instead of &None values.

claude · 2026-07-27T13:22:25Z

Claude finished @jm2's task in 2m 24s —— View job

Review of the latest push (`a851cde`, `a7a5a01`)

Todo list

Fetch latest origin/main and diff the newly pushed commits
Check whether the critical rebuild_album_pane selection-wiring regression was fixed
Check whether the cache-probe-installed-after-dispatch issue was fixed
Check whether the factory teardown / cell_states leak issue was fixed
Check whether the AlbumArtSize serde token issue was addressed
Review the two new commits directly
Post final review

What's genuinely fixed by these two commits

The three defects the PR body describes are correctly resolved in a851cde:

Pane blanking: set_album_pane_artwork() no longer calls rebuild_browser_data(..., &[]); the snapshot in state.tracks is preserved and only the album store is repopulated (src/ui/browser.rs:408-410).
Inert size preference: the size Cell is now threaded into the controller via build_binder_with_size (src/ui/browser.rs:583) and read every bind through current_pixel_size() (src/ui/album_pane_art.rs:482, 552), and window.rs:3493 calls the new set_album_pane_artwork_size(browser_box, state, ...) signature correctly.
Zero-sized placeholder: AlbumArtCell::PLACEHOLDER_PIXEL_SIZE = -1 (src/ui/album_pane_art.rs:78) replaces the old .pixel_size(0), confirmed by the rewritten test at line 915.

a7a5a01's test rewrite (dropping gtk::test_synced for bind_generation_advances_on_reset and asserting PLACEHOLDER_PIXEL_SIZE as a plain constant) is a reasonable fix for the cross-platform GTK-init-on-worker-thread failure it describes — both rewritten tests now assert the same contract without constructing a widget.

Previously-flagged issues that are still open in the current HEAD

I checked each finding from the last review round against the code at a7a5a01 — none of these were touched by the two new commits, since they fixed a different set of defects than the ones described in the PR body:

[Critical, still open] rebuild_album_pane still drops the album pane's selection-changed wiring and discards active filters. src/ui/browser.rs:357-411 still calls build_album_pane, which constructs a brand-new gtk::SingleSelection (src/ui/browser.rs:576) with no listener — the only place connect_selection_changed is wired for the album pane is in build_browser (src/ui/browser.rs:226), once, on the original selection object. After the very first artwork toggle or size change, clicking an album row stops cross-filtering Genre/Artist and stops firing on_filter_changed. Compounding this, rebuild_album_pane line 410 calls populate_albums(&album_store, &borrowed, &None, &None, use_aa) — hardcoded Nones instead of the live selected_genre/selected_artist — so any active filter is silently dropped on every toggle/resize, in addition to the dead selection handler.
[Major, still open] Cache probe installed after dispatch. src/ui/album_pane_art.rs:583-606: install_cache_probe is still called after update_direct_file_album_art / fetch_remote_album_art / fetch_resolved_album_art are invoked. A synchronous/already-warm result sets the paintable before the notify::paintable listener is connected, so that row never gets cached and re-fetches on every rebind.
[Major, still open] No factory teardown — cell_states map leaks. src/ui/album_pane_art.rs:425-438 (build_binder_with_size_internal) registers a cell_states entry per ListItem in setup but nothing removes it; src/ui/browser.rs:583-586 only wires connect_setup/connect_bind/connect_unbind, no connect_teardown. Every artwork toggle/size rebuild (which recreates the factory) leaves the old map's entries (and their widget trees) unreachable-but-referenced until the whole AlbumArtBinder drops.
[Medium, still open] AlbumArtSize::as_token/from_token doc claims a migration guarantee not actually wired to persistence. src/ui/preferences.rs:95-134: still plain #[derive(..., Serialize, Deserialize)] with no rename_all or custom impl, so serde_json persists the Rust variant names ("Small"/"Medium"/"Large"), not the lowercase tokens as_token/from_token produce — those functions remain #[allow(dead_code)] and exercised only by their own test.

None of these four are new — they were raised independently by both the earlier Claude review and CodeRabbit against this same commit (a7a5a01) — but since they weren't part of the three defects this push targeted, they're still live and worth fixing before merge, particularly #1, which regresses existing cross-filtering behavior on every artwork preference change.

Fix this →

coderabbitai

♻️ Duplicate comments (3)

src/ui/album_pane_art.rs (2)
425-433: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

No teardown removes the cell_states entry. Each setup inserts a AlbumArtCellState keyed by list_item.as_ptr() as usize and nothing ever removes it, so entries (and their widget trees) accumulate across every artwork toggle/size rebuild, and a recycled ListItem allocation at the same address would resolve to a stale cell. Return a teardown closure that removes the key and wire it via factory.connect_teardown in src/ui/browser.rs's build_album_pane.
🤖 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/album_pane_art.rs` around lines 425 - 433, Add a teardown closure
alongside the setup closure in the album art factory that downcasts the
ListItem, removes its as_ptr() as usize key from cell_states, and releases the
associated AlbumArtCellState. Connect this closure with factory.connect_teardown
in build_album_pane, preserving the existing setup behavior.
583-607: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win

Probe is installed after the dispatch, so synchronous paintable updates are missed. update_direct_file_album_art / fetch_remote_album_art may set the paintable before install_cache_probe connects the notify::paintable listener (warm worker cache, already-decoded local file), so those rows never populate the cache and refetch on every rebind. Connect the probe first, then dispatch.
🤖 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/album_pane_art.rs` around lines 583 - 607, The cache probe is
installed after album-art dispatch, allowing synchronous paintable updates to be
missed. In the flow containing install_cache_probe, move that call before the
match that invokes update_direct_file_album_art, fetch_remote_album_art, or
fetch_resolved_album_art, while preserving the existing cache parameters and
dispatch behavior.
src/ui/browser.rs (1)
357-411: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift

The rebuilt album pane is inert: no selection_changed handler and filters are dropped. build_browser connects the album selection callback on the original pane's SingleSelection; rebuild_album_pane creates a new pane/selection and never reconnects it, so after an artwork toggle or size change clicking an album stops cross-filtering. The repopulate at Line 410 also passes &None, &None, discarding the active genre/artist filters, so the pane widens back to the full album list. Move the selection wiring and filter state (selected_genre, selected_artist, on_filter_changed) into BrowserState so the rebuild can reconnect and repopulate with live filters.
🤖 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/browser.rs` around lines 357 - 411, Update BrowserState to retain
selected_genre, selected_artist, and the on_filter_changed callback, then have
rebuild_album_pane reconnect the new album selection’s selection_changed handler
using that stored state, matching the wiring in build_browser. Replace the &None
filter arguments in populate_albums with the live stored genre and artist
filters so rebuilding preserves the active filtered album list.

🤖 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.

Duplicate comments:
In `@src/ui/album_pane_art.rs`:
- Around line 425-433: Add a teardown closure alongside the setup closure in the
album art factory that downcasts the ListItem, removes its as_ptr() as usize key
from cell_states, and releases the associated AlbumArtCellState. Connect this
closure with factory.connect_teardown in build_album_pane, preserving the
existing setup behavior.
- Around line 583-607: The cache probe is installed after album-art dispatch,
allowing synchronous paintable updates to be missed. In the flow containing
install_cache_probe, move that call before the match that invokes
update_direct_file_album_art, fetch_remote_album_art, or
fetch_resolved_album_art, while preserving the existing cache parameters and
dispatch behavior.

In `@src/ui/browser.rs`:
- Around line 357-411: Update BrowserState to retain selected_genre,
selected_artist, and the on_filter_changed callback, then have
rebuild_album_pane reconnect the new album selection’s selection_changed handler
using that stored state, matching the wiring in build_browser. Replace the &None
filter arguments in populate_albums with the live stored genre and artist
filters so rebuilding preserves the active filtered album list.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 97276b2d-7a21-4241-b152-09403209f095

📥 Commits

Reviewing files that changed from the base of the PR and between 23830ad and a7a5a01.

📒 Files selected for processing (8)

locales/en.yml
src/ui/album_pane_art.rs
src/ui/browser.rs
src/ui/mod.rs
src/ui/objects/browser_item.rs
src/ui/objects/mod.rs
src/ui/preferences.rs
src/ui/window.rs

🚧 Files skipped from review as they are similar to previous changes (6)

src/ui/mod.rs
locales/en.yml
src/ui/objects/mod.rs
src/ui/window.rs
src/ui/objects/browser_item.rs
src/ui/preferences.rs

Adds album artwork thumbnails to the browser Album pane as a virtualized, accessible column that fetches lazily and cancels in-flight work when rows scroll out of view. The Album pane already grouped tracks by album name in a GTK `ListView`; each row showed only a label. This commit decorates every non-"All" row with a thumbnail next to its label. The thumbnail is decoded on demand inside the same `SignalListItemFactory` GTK already uses to virtualize the list — so a 10 000-album library still allocates widgets for only the rows on screen, and the per-row GObject/CSS cost is paid only for visible rows. The implementation lives in a new `src/ui/album_pane_art` module: * `AlbumArtCache` is a bounded FIFO-with-recency cache keyed by `(album_key, pixel_size)`. The cap is `MAX_CACHED_ALBUM_ARTS` (512 entries); a library exceeding the cap evicts the oldest entry, a hit promotes it back to the most-recent position. The cap exists so a misbehaving Subsonic peer or a malicious catalogue cannot inflate the UI's working set through the album pane. * `AlbumArtController` owns the cache plus a `SourceRegistry` handle for credential-isolated remote artwork. The bind factory mints a fresh `BindGeneration` for every bind, then `spawn_fetch` runs the resolver (remote → direct URL → embedded file) on a `glib::MainContext` future. The resolver follows the same lease-isolated path the playback header-bar image already uses — `fetch_resolved_album_art` for remote, `fetch_remote_album_art` for legacy direct URLs, and `update_direct_file_album_art` for `file://` rows. None of these branches invent new credential-isolation seams; they reuse the existing per-row `ResolvedHttpRequest` flow. * Cancellation: `unbind` bumps the row's generation token and disconnects the `paintable`-notify handler that the bind phase installed for cache probing. A late async result observes the bumped generation and paints nothing. A test (`bind_generation_late_result_is_dropped`) pins the same predicate the production path uses. * Accessibility: the row's `gtk::Image` gets an accessible role (`Img`) and a per-row label matching the album name; a screen reader announces the same text it announces for the plain-label bind path. * Placeholder: while a fetch is in flight (or when the album has no artwork) the cell paints `audio-x-generic-symbolic`. No empty box, no row-flicker when the cache is warm. The preferences dialog gains two new controls under the existing Browser Views group: * `Album pane artwork` checkbox — toggles the bind factory between the artwork and the plain-label paths. Switching the factory requires rebuilding the album pane (GTK's `SignalListItemFactory` does not expose a clean in-place replace), so the change goes through a new `browser::set_album_pane_artwork` helper that swaps the third pane child and clears the cache so the new bind path doesn't serve stale textures from before the layout change. * `Small` / `Medium` / `Large` radios — choose the thumbnail side length (`32` / `48` / `72` device pixels). The size is persisted via a new `AlbumArtSize` enum on `AppConfig`; serde's `#[default]` on `Medium` keeps older configs reading cleanly. The album pane's `populate_albums` now records one representative `AlbumArtCandidate` per album (the first track whose source identity the UI knows how to resolve). Each `BrowserItem` carries the candidate inline so a bind never has to reach back into the snapshot map. `TrackSnapshot` grew source-identity fields (`track_id`, `uri`, `cover_art_url`, `source_id`, `source_session_epoch`) so the representative can be selected without a second pass over the master track list. Nine new unit tests cover the cache bounded-eviction, hit-promotion, pixel-size keying, generation monotonicity, late-result drop, default pixel-size parity with `AlbumArtSize::Medium`, and the `AlbumArtSize` token round-trip. The shape of every gate the rig's CI runs (`cargo fmt --check`, `cargo clippy --all-targets -- -D warnings`, `cargo clippy --release -- -D warnings`, `cargo test --all-targets`) is preserved: 1707 tests pass, including the 9 new ones; fmt and clippy are clean.

…n toggle (tr-xaj) The previous attempt at tr-xaj shipped three defects that the refinery flagged in review. Each one breaks a piece of the album-pane artwork acceptance criteria; this commit lands the focused fixes without re-architecting the virtualization or cache work that already passed review. (1) REGRESSION — set_album_pane_artwork() ended with rebuild_browser_data(browser_box, state, &[]). That helper unconditionally replaces *state.tracks with the slice it is handed, so the empty slice wiped the shared track snapshot the genre/artist selection handlers filter over, and the empty populate_* calls repopulated all three stores from it. Toggling the new "Album pane artwork" checkbox therefore blanked every browser pane. The fix is to keep the snapshot and only repopulate the album store (the only store whose contents depend on the artwork-toggling bind factory). The window callback that toggles album-artist grouping already shows the correct pattern: read the live master track list and pass the real slice, not an empty one. (2) ACCEPTANCE UNMET — set_album_pane_artwork_size() only wrote BrowserState::album_pane_artwork_size; no code anywhere read that Cell. The render path used the hardcoded AlbumArtController::default_pixel_size()=48, so Small/Large persisted to AppConfig and changed nothing on screen. The fix threads the BrowserState's size Cell into the AlbumArtController at bind-factory build time (build_binder_with_size) and reads it on every bind via current_pixel_size(). The fetch path uses the same accessor, so the cache key tracks the live knob. set_album_pane_artwork_size now also rebuilds the album pane and clears the cache: the cache is keyed by (album_key, pixel_size), so every entry was decoded at the previous size and would never match a new lookup, and leaving them in would still consume the bounded-memory budget. (3) Supporting — AlbumArtCell::new built the gtk::Image with .pixel_size(0), which is a literal 0×0 request, not GTK4's "use icon-theme default" sentinel (that is -1). The bind factory never called set_pixel_size again, so the audio-x-generic-symbolic placeholder rendered at 0 pixels. The cell constructor now uses .pixel_size(-1), and the bind factory applies controller.current_pixel_size() on every bind so both the placeholder and the eventual texture are sized consistently. The two existing tests that touch gtk::Image (one new, one pre-existing in the module) are routed through gtk::test_synced, which dispatches to GTK's exclusive test-thread pool. The pre-existing test's docstring already warned this serialisation was needed; the parallel test runner was previously getting away with one of the two landing on the GTK-init-compatible main thread, but a new test changes the scheduling and reliably loses. gtk::test_synced is the supported gtk-rs entry point for GTK-touching tests and avoids the dev-dep. Validation: cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test --release all green (1710 unit + 14 packaging, 0 failures). Local gates match the prior passing run; this commit addresses the diff-review rejection directly. Refs: tr-xaj refinery review_defect (1)(2)(3).

…I (tr-xaj) Gate 2 (hosted CI) on the tr-xaj refinery submission rejected on three matrices (macOS aarch64, Linux aarch64, Coverage Linux x86_64) for one cause: the two tests in src/ui/album_pane_art.rs that construct a real gtk::Image were never given an initialized GTK. The previous fix-forward commit had routed both tests through gtk::test_synced, but that helper dispatches onto GTK's test thread pool without initializing GTK -- so the first widget call inside the closure still hit the uninitialized-GTK assertion. On macOS the situation is worse: GTK panics on Quartz rather than erroring if init is attempted off the main thread, so no dispatch-based wrapper can rescue it. The reviewer's correction is unambiguous: do not call GTK init off the main thread at all, and prefer testing the pure logic where the assertion does not actually need a widget. Local green is worth nothing for this class; the same test must hold on macOS and aarch64, where the GTK-init semantics differ. This commit makes both tests exercise the pure logic instead of the widget tree: (1) bind_generation_advances_on_reset tested that AlbumArtCellState's generation advances on reset(). The bump is a plain Cell<BindGeneration> write, totally independent of the gtk::Image the cell otherwise holds. The new body asserts the same pure contract against a fresh Rc<Cell<BindGeneration>> (the same Rc<Cell> shape the production code uses at album_pane_art.rs:256), without constructing a widget. The companion tests bind_generation_monotonic_across_rebinds and bind_generation_late_result_is_dropped already cover the binding trait and the late-result predicate; this one pins the bump. (2) cell_pixel_size_starts_at_icon_theme_sentinel tested that AlbumArtCell::new builds a gtk::Image with pixel_size(-1). The sentinel is now exposed as the public associated constant AlbumArtCell::PLACEHOLDER_PIXEL_SIZE, and AlbumArtCell::new uses it. The test asserts the constant -- same contract, no widget construction, holds on every matrix. Both tests now run on the local cargo test thread directly, with no gtk::test_synced, no gtk::Image, no GTK-thread-pool dispatch. The gate-1 fix (the three production defects from bc7cbc4) is untouched; this commit only changes tests and the placeholder-size literal the tests were guarding. Validation: cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo clippy --release -- -D warnings, cargo test --release, and cargo test --all-targets all green on 1746e1b + this commit (1710 passed in release, 1710 + 14 packaging in --all-targets, 0 failures). The two rewritten tests are visible in the test list as pure-logic assertions; the rest of the module's test surface is unchanged. Refs: tr-xaj refinery rejection on 1746e1b (CI Gate 2 FAIL, bind_generation_advances_on_reset and cell_pixel_size_starts_at_icon_theme_sentinel).

claude · 2026-07-27T16:50:06Z

Claude finished @jm2's task in 2m 20s —— View job

Review of the latest push (HEAD `f5ccd34`, branch rebased/restacked to 3 commits: `329c1f3` → `69cf7a2` → `f5ccd34`)

Todo list

Fetch latest commits and diff since last review
Check whether rebuild_album_pane selection-wiring/filter-drop regression was fixed
Check whether the cache-probe-after-dispatch ordering was fixed
Check whether the cell_states teardown leak was fixed
Check whether the AlbumArtSize serde token issue was addressed
Post final review

Note on branch history

The SHAs referenced in the last review round (a7a5a01, a851cde, 23830ad) no longer exist on this branch — it's been rebased/restacked down to three commits (329c1f3 feat, 69cf7a2 fix pane-blanking + size, f5ccd34 fix CI test flakiness). I reviewed the current tree directly rather than diffing against the missing SHAs.

What `f5ccd34` actually changed

Only src/ui/album_pane_art.rs, and only tests + a doc/const extraction (AlbumArtCell::PLACEHOLDER_PIXEL_SIZE). It rewrites bind_generation_advances_on_reset and cell_pixel_size_starts_at_icon_theme_sentinel to avoid constructing a gtk::Image off the main thread (the prior gtk::test_synced wrapper dispatches to a thread pool without initializing GTK, which was failing on macOS/aarch64 CI). This is a reasonable, narrowly-scoped CI fix. One tradeoff worth naming: bind_generation_advances_on_reset no longer calls AlbumArtCellState::reset() at all — it now asserts a bare Cell<BindGeneration> increment, so it no longer exercises the actual reset() code path, only the primitive it's built from. This was already flagged as a nitpick by CodeRabbit against the prior revision; not a new issue, but worth knowing it's a slightly weaker regression guard than the name implies.

Previously-flagged issues — still open in current HEAD

I re-verified each of these directly against the code on disk (not from memory of the earlier review threads); all four are still present and none were touched by this push:

[Critical] rebuild_album_pane still drops album-pane selection wiring and active filters. src/ui/browser.rs:357-411. rebuild_album_pane calls build_album_pane (browser.rs:561), which constructs a brand-new gtk::SingleSelection (browser.rs:575) and never calls connect_selection_changed on it — that wiring exists only once, in build_browser, attached to the original pane's selection object (browser.rs:226). BrowserState (browser.rs:27-49) has no field for selected_genre/selected_artist/on_filter_changed, so rebuild_album_pane has no way to reconnect it even if it wanted to. It also repopulates with hardcoded &None, &None (browser.rs:410) instead of the live filters. Net effect, confirmed by reading the current code: after the first artwork toggle or size change, clicking an album row stops cross-filtering Genre/Artist and stops firing on_filter_changed for the rest of the session, and any active genre/artist filter is silently dropped from the album list.
[Major] Cache probe still installed after dispatch. src/ui/album_pane_art.rs:583-606. install_cache_probe is still called after the match resolved { ... } block that invokes update_direct_file_album_art / fetch_remote_album_art / fetch_resolved_album_art. A synchronous or already-warm result can set the paintable before the notify::paintable listener is connected, so that row's texture never gets cached and the row re-fetches on every rebind.
[Major] No factory teardown — cell_states still leaks. src/ui/album_pane_art.rs:425-433 inserts a cell_states entry per ListItem in setup, and nothing removes it. build_binder_with_size_internal returns only (setup, bind, unbind, binder) — no teardown closure — and src/ui/browser.rs:580-585 only wires connect_setup/connect_bind/connect_unbind. Every artwork toggle/size rebuild recreates the factory, leaving the previous map's entries (and their widget trees) referenced-but-unreachable until the whole AlbumArtBinder drops.
[Medium] AlbumArtSize::as_token/from_token still don't match what's persisted. src/ui/preferences.rs:94-134. The enum still derives plain Serialize/Deserialize with no #[serde(rename_all = ...)], so serde_json persists the Rust variant names ("Small"/"Medium"/"Large"), not the lowercase tokens as_token/from_token produce. Both remain #[allow(dead_code)] and are exercised only by this module's own test.

None of these are new findings — all four were raised independently by both a prior Claude review round and CodeRabbit against this same code — but since this push targeted a different problem (CI test flakiness), they're all still live. #1 is the one I'd block merge on: it's a regression in existing, load-bearing cross-filtering behavior that fires on every single artwork preference change, with no test coverage catching it (album pane wiring is GTK-widget-tree logic with no cargo test coverage in this file).

Fix this →

Other observations (unchanged from prior rounds, not blocking)

is_all_row on BrowserItem (src/ui/objects/browser_item.rs:20-23) is still set but never read by AlbumArtBinder::bind_fn — dead state unless intentionally reserved for a future change.
Credential-boundary claim holds for the code in this diff: resolve_kind's error logging in album_pane_art.rs includes only error, source_id, track_id — no URLs.
src/ui/album_pane_art.rs's bounded LRU cache, generation-guard, and placeholder-sentinel logic remain correct and are the best-tested part of this change.

codacy-production Bot reviewed Jul 25, 2026

View reviewed changes

claude Bot reviewed Jul 26, 2026

View reviewed changes

coderabbitai Bot reviewed Jul 26, 2026

View reviewed changes

jm2 force-pushed the polecat/tr-xaj branch from 23830ad to a7a5a01 Compare July 27, 2026 13:22

coderabbitai Bot reviewed Jul 27, 2026

View reviewed changes

jm2 and others added 3 commits July 27, 2026 12:13

jm2 force-pushed the polecat/tr-xaj branch from a7a5a01 to f5ccd34 Compare July 27, 2026 16:13

jm2 marked this pull request as draft July 27, 2026 17:25

coderabbitai Bot mentioned this pull request Jul 28, 2026

style(ui): re-evaluate separator, count-opacity and alignment against GNOME HIG (tr-ewq) #179

Draft

Conversation

jm2 commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current status

Required fixes

Uh oh!

gemini-code-assist Bot commented Jul 25, 2026

Uh oh!

codacy-production Bot left a comment

Choose a reason for hiding this comment

Pull Request Overview

About this PR

Test suggestions

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Not up to standards ⛔

Uh oh!

codacy-production Bot left a comment

Choose a reason for hiding this comment

Pull Request Overview

About this PR

Test suggestions

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude's review

Findings (posted inline)

Other observations (not inline)

Uh oh!

coderabbitai Bot commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

claude Bot Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 26, 2026

Choose a reason for hiding this comment

jm2 commented Jul 25, 2026 •

edited

Loading

codacy-production Bot commented Jul 25, 2026 •

edited

Loading

claude Bot commented Jul 26, 2026 •

edited

Loading

coderabbitai Bot commented Jul 26, 2026 •

edited

Loading

claude Bot commented Jul 27, 2026 •

edited

Loading

Review of the latest push (`a851cde`, `a7a5a01`)

claude Bot commented Jul 27, 2026 •

edited

Loading

Review of the latest push (HEAD `f5ccd34`, branch rebased/restacked to 3 commits: `329c1f3` → `69cf7a2` → `f5ccd34`)

What `f5ccd34` actually changed