Home

dev / openkara

publicthedavidweng/OpenKara· sync paused
Code Branches Pull requestsIssuesInsights
main
Home Code PRsIssues

#389 Integrity cleanup gates its selection reset and skipped count on a report being present

closed

Opened by dev · yesterday

devopened this issueAuthor· yesterday

Line references are pinned to main at afbd7c45.

Pre-existing on main. Not introduced by #374 — the guard and its contents are ported unchanged. Do not bisect to it. Pre-split proof below.

Severity note up front: unlike the other five defects filed alongside this one, I could not find a UI path that reaches the broken state today. See Reachability. This is a latent coupling defect, filed so the structure gets fixed before something makes it reachable, not a bug users are hitting.

Where

src/lib/settings-controller/settings-controller.ts L545-566, inside cleanUpIntegrity (L514-573):

if (report) {
  const keep = (issue: { song_hash: string }) => !deleted.has(issue.song_hash);

  patchIntegrity({
    report: {
      ...report,
      missing_primary_media: report.missing_primary_media.filter(keep),
      empty_primary_media: report.empty_primary_media.filter(keep),
      missing_optional_assets: report.missing_optional_assets.filter(keep),
      empty_optional_assets: report.empty_optional_assets.filter(keep),
    },
    selection: new Set(                                  // <- does not need `report`
      selectedHashes.filter((hash) => !deleted.has(hash)),
    ),
    skippedCount:                                        // <- does not need `report`
      result.skipped_song_hashes.length > 0
        ? result.skipped_song_hashes.length
        : null,
  });
}

What goes wrong

Three independent pieces of post-cleanup state are written inside one guard, but only one of them depends on the guard's condition.

  • Pruning the report genuinely needs report.
  • Resetting selection does not. It drops the hashes the backend just deleted. Skipping it leaves view.integrity.selection holding hashes for songs that no longer exist.
  • Recording skippedCount does not. It is read from result. Skipping it loses the count of entries the backend declined to remove, so a partial cleanup reports as a complete one.

The deletions themselves always apply — the backend call at L529-530 and the store refreshes at L544-545 are outside the guard. So with report === null the library is mutated while the view's record of what was selected and what was skipped is not updated.

Of the two, the stale selection is the more consequential: skippedCount is a missing notice, whereas a selection pointing at deleted songs is state that a subsequent cleanup would act on.

Reachability

I traced every writer of view.integrity:

  • checkIntegrity success (L764-768) sets report and selection together; its failure branch (L769-771) clears both.
  • dismissIntegrityReport (L789-795) clears both.
  • toggleIntegrityEntry (L777-785) mutates selection with no report requirement — this is the one that could desynchronise them.

But toggleIntegrityEntry is only reachable from IntegrityReportModal (src/components/Settings/IntegrityReportModal.tsx L103), and that modal only renders when the report is truthy (src/components/Settings/SettingsLibrarySection.tsx L350). And cleanUpIntegrity early-returns on an empty selection (L518-521), so the dismiss-then-clean race cannot reach the body either.

So the guarded state is currently unreachable through the UI. The controller API can reach it directly, as src/components/Settings/SettingsLibrarySection.interaction.test.tsx L156 does.

Pre-existing proof

32ec36c6:src/components/Settings/settings-overlay.integrity-actions.ts L102-133 — the same if (current.integrityReport) wrapping the same three writes, including integritySelection at L129 and integritySkippedCount at L130.

Fix direction (my view, not the reviewer's)

The suggestion was to hoist the skippedCount patch above the if (report) branch. That is correct as far as it goes, but it stops one field short: selection has the same independence and the same argument applies to it.

Move both out of the guard and leave only the report pruning inside. The guard then reads as what it is — "if there is a report on screen, prune it" — rather than as an accidental gate on everything that happens after a cleanup.

While in there, the view.integrity slice is worth a second look. report, selection and skippedCount are three fields with an undocumented invariant between them (a selection is only meaningful against a report), and this defect exists because that invariant lives in reviewers' heads instead of in the type. Modelling the slice as one value — no report, or a report with its selection — would make the state unrepresentable rather than merely unreachable. That is a larger change than the fix above and should not block it.

Worth adding a controller-level test that runs cleanup with report === null and a non-empty selection, since that is the shape the UI currently prevents but the API allows.

Not fixing here

Filed for tracking only.

Sign in to comment.

Linked pull requests

No linked pull requests yet.