From c0287bee2b239d346be555fe71c99be388e28370 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sun, 20 Sep 2026 09:10:29 -0400 Subject: [PATCH 1/3] fix: name ffmpeg as the cause when videos are skipped at index time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a platform with no bundled ffmpeg binary (win_arm64, musl/Alpine, linux armv7 — imageio-ffmpeg's sdist ships no binary), every video fails to yield a frame and is dropped from the index. The completion notice said only "N files could not be read and were skipped", which sends the user hunting for corrupt files that are in fact fine. Count the videos among bad_files and, when ffmpeg is genuinely absent, say so. A mixed run keeps the generic count and appends the cause, since the images failed for their own reasons. ffmpeg_exe() is guarded behind the video count because it re-probes by spawning `ffmpeg -version` whenever it has no binary to report; a photo-only album must not pay for that. Co-Authored-By: Claude Opus 5 (1M context) --- photomap/backend/embeddings.py | 32 +++++++-- tests/backend/test_video_guards.py | 106 ++++++++++++++++++++++++++++- 2 files changed, 131 insertions(+), 7 deletions(-) diff --git a/photomap/backend/embeddings.py b/photomap/backend/embeddings.py index af661f4e..fee57fae 100644 --- a/photomap/backend/embeddings.py +++ b/photomap/backend/embeddings.py @@ -50,7 +50,7 @@ from .thumbnail_cache import keep_hashes_for, thumbnail_dir from .thumbnail_cache import prune as prune_thumbnails from .util import atomic_savez -from .video import VIDEO_METADATA_KEY, extract_video_frame +from .video import VIDEO_METADATA_KEY, extract_video_frame, ffmpeg_exe from .video_cache import VideoFrameCache from .video_transcode import TranscodeCache @@ -1349,10 +1349,32 @@ def _register_unreadable_files_warning( return count = len(result.bad_files) noun, verb = ("file", "was") if count == 1 else ("files", "were") - progress_tracker.add_completion_warning( - album_key, - f"{count} {noun} could not be read and {verb} skipped.", - ) + message = f"{count} {noun} could not be read and {verb} skipped." + + # On a platform with no ffmpeg binary *every* video fails, and the + # generic notice above sends the user hunting for corrupt files that + # are in fact fine. Naming the cause is the difference between "my + # videos are broken" and "this machine needs ffmpeg". + # + # Probing here is cheap: a successful probe is memoized, so by the + # time a run has indexed anything this is a cached read. Only the + # None case re-probes, and only on a run that already had failures. + videos = sum(1 for path in result.bad_files if is_video(path)) + if videos and ffmpeg_exe() is None: + video_noun = "video" if videos == 1 else "videos" + if videos == count: + message = ( + f"{count} {video_noun} could not be indexed and {verb} " + "skipped: no ffmpeg binary is available on this system." + ) + else: + message = ( + f"{count} {noun} could not be read and {verb} skipped, " + f"including {videos} {video_noun} that need ffmpeg, which " + "is not available on this system." + ) + + progress_tracker.add_completion_warning(album_key, message) logger.warning( f"Skipped {count} unreadable {noun} in album '{album_key}': " + ", ".join(p.name for p in result.bad_files[:5]) diff --git a/tests/backend/test_video_guards.py b/tests/backend/test_video_guards.py index 7a1f1398..6cd6bc4a 100644 --- a/tests/backend/test_video_guards.py +++ b/tests/backend/test_video_guards.py @@ -20,8 +20,8 @@ from fixtures import media_fixture_path from PIL import Image -from photomap.backend.embeddings import Embeddings, _open_npz_file -from photomap.backend.progress import ProgressTracker +from photomap.backend.embeddings import Embeddings, IndexResult, _open_npz_file +from photomap.backend.progress import ProgressTracker, progress_tracker from photomap.backend.util import atomic_savez ENCODER_SPEC = "openai-clip:ViT-B/32" @@ -440,6 +440,108 @@ def test_adding_an_empty_warning_is_a_noop(): assert tracker.get_progress("album").warning_message == "kept" +# -------------------------------------------------------------------------- +# Naming ffmpeg as the cause of a skip +# -------------------------------------------------------------------------- + + +def _skip_result(*names: str) -> IndexResult: + """An IndexResult whose only content is a list of files that failed.""" + return IndexResult( + embeddings=np.zeros((0, EMBEDDING_DIM), dtype=np.float32), + filenames=np.array([], dtype=object), + modification_times=np.array([], dtype=float), + metadata=np.array([], dtype=object), + bad_files=[Path(name) for name in names], + ) + + +def _registered_warning(monkeypatch, result: IndexResult, exe: str | None) -> str: + """Run the notice builder with ffmpeg forced present/absent.""" + monkeypatch.setattr("photomap.backend.embeddings.ffmpeg_exe", lambda: exe) + key = f"warning-test-{id(result)}" + captured: list[str] = [] + monkeypatch.setattr( + progress_tracker, + "add_completion_warning", + lambda album_key, message: captured.append(message), + ) + Embeddings._register_unreadable_files_warning(key, result) + assert len(captured) == 1 + return captured[0] + + +def test_missing_ffmpeg_is_named_when_only_videos_failed(monkeypatch): + """Otherwise a platform with no ffmpeg wheel (win_arm64, musl, armv7) + reports every video as unreadable and sends the user hunting for corrupt + files that are perfectly fine.""" + warning = _registered_warning( + monkeypatch, _skip_result("a.mp4", "b.mov", "c.mkv"), exe=None + ) + + assert warning == ( + "3 videos could not be indexed and were skipped: no ffmpeg binary is " + "available on this system." + ) + + +def test_missing_ffmpeg_notice_agrees_for_a_single_video(monkeypatch): + warning = _registered_warning(monkeypatch, _skip_result("only.mp4"), exe=None) + + assert warning == ( + "1 video could not be indexed and was skipped: no ffmpeg binary is " + "available on this system." + ) + + +def test_a_mixed_failure_keeps_the_generic_count_and_adds_the_cause(monkeypatch): + """The images failed for their own reasons, so the notice must not claim + ffmpeg explains all four.""" + warning = _registered_warning( + monkeypatch, + _skip_result("a.mp4", "b.mov", "broken.jpg", "truncated.png"), + exe=None, + ) + + assert warning == ( + "4 files could not be read and were skipped, including 2 videos that " + "need ffmpeg, which is not available on this system." + ) + + +def test_wording_is_unchanged_when_ffmpeg_is_present(monkeypatch): + """A truncated clip on a working install is not an ffmpeg problem, and + saying so would be actively misleading.""" + warning = _registered_warning( + monkeypatch, _skip_result("broken.mp4"), exe="/usr/bin/ffmpeg" + ) + + assert warning == "1 file could not be read and was skipped." + + +def test_image_only_failures_never_mention_ffmpeg(monkeypatch): + """ffmpeg_exe() must not even be consulted: it re-probes on every call + when it has no binary to report, so a photo-only album would pay a + process spawn for nothing.""" + + def _fail(): + raise AssertionError("ffmpeg must not be probed for an image-only skip") + + monkeypatch.setattr("photomap.backend.embeddings.ffmpeg_exe", _fail) + captured: list[str] = [] + monkeypatch.setattr( + progress_tracker, + "add_completion_warning", + lambda album_key, message: captured.append(message), + ) + + Embeddings._register_unreadable_files_warning( + "image-only", _skip_result("broken.jpg", "truncated.png") + ) + + assert captured == ["2 files could not be read and were skipped."] + + def test_set_completion_warning_still_replaces(): tracker = ProgressTracker() tracker.set_completion_warning("album", "first") From e11cdd558e79f7e2e3bb05ddf5c38efe8f9c011f Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sun, 20 Sep 2026 09:27:28 -0400 Subject: [PATCH 2/3] fix: correct verb agreement and keep the ffmpeg check off the event loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of the previous commit turned up three defects: - "1 video that need ffmpeg" — the mixed branch singularised the noun but hard-coded the plural verb. The mixed branch always has count >= 2, so the outer verb is always plural and the bug could only show through the inner one; the videos == 1 case was also the one hole in the new tests. - ffmpeg_exe() was called from the asyncio event loop: both callers of _register_unreadable_files_warning are async, while every pre-existing caller is on a worker thread. It stats whatever $IMAGEIO_FFMPEG_EXE names (accepted unchecked by imageio, so possibly a hung network mount) and can spawn an untimed `ffmpeg -version`. Added ffmpeg_known_unavailable(), which reports the probe every video failure already performed on a worker thread and does no work of its own. - The notice asserted "no ffmpeg binary is available on this system". imageio memoizes its own negative result in an lru_cache, and _is_valid_exe swallows OSError, so a binary that exists but failed to exec once (an AV scanner holding a freshly unpacked ffmpeg.exe) stays unavailable for the process lifetime. Reworded to state what this process observed rather than a claim about the machine. Co-Authored-By: Claude Opus 5 (1M context) --- photomap/backend/embeddings.py | 31 ++++++++---- photomap/backend/video.py | 27 ++++++++++- tests/backend/test_video_guards.py | 60 +++++++++++++++-------- tests/backend/test_video_probe.py | 76 ++++++++++++++++++++++++++++++ 4 files changed, 165 insertions(+), 29 deletions(-) diff --git a/photomap/backend/embeddings.py b/photomap/backend/embeddings.py index fee57fae..deabfba3 100644 --- a/photomap/backend/embeddings.py +++ b/photomap/backend/embeddings.py @@ -50,7 +50,11 @@ from .thumbnail_cache import keep_hashes_for, thumbnail_dir from .thumbnail_cache import prune as prune_thumbnails from .util import atomic_savez -from .video import VIDEO_METADATA_KEY, extract_video_frame, ffmpeg_exe +from .video import ( + VIDEO_METADATA_KEY, + extract_video_frame, + ffmpeg_known_unavailable, +) from .video_cache import VideoFrameCache from .video_transcode import TranscodeCache @@ -1356,22 +1360,33 @@ def _register_unreadable_files_warning( # are in fact fine. Naming the cause is the difference between "my # videos are broken" and "this machine needs ffmpeg". # - # Probing here is cheap: a successful probe is memoized, so by the - # time a run has indexed anything this is a cached read. Only the - # None case re-probes, and only on a run that already had failures. + # ``ffmpeg_known_unavailable`` rather than ``ffmpeg_exe()``: this runs + # on the asyncio event loop (both callers are ``async def``), and + # ffmpeg_exe can stat a hung network mount or spawn an untimed + # ``ffmpeg -version``. It only reports the probe every video failure + # already performed on a worker thread. + # + # Worded as what this process observed, not as a claim about the + # machine: imageio memoizes its own negative, so a binary that exists + # but could not be executed once (an AV scanner holding a freshly + # unpacked ffmpeg.exe) stays unavailable for the process lifetime, and + # "there is no ffmpeg here" would be a lie to someone who can see it. videos = sum(1 for path in result.bad_files if is_video(path)) - if videos and ffmpeg_exe() is None: + if videos and ffmpeg_known_unavailable(): video_noun = "video" if videos == 1 else "videos" if videos == count: message = ( f"{count} {video_noun} could not be indexed and {verb} " - "skipped: no ffmpeg binary is available on this system." + "skipped: PhotoMapAI could not find a working ffmpeg." ) else: + # Plural here is independent of ``verb``: the mixed branch + # always has count >= 2, but ``videos`` can still be 1. + needs = "needs" if videos == 1 else "need" message = ( f"{count} {noun} could not be read and {verb} skipped, " - f"including {videos} {video_noun} that need ffmpeg, which " - "is not available on this system." + f"including {videos} {video_noun} that {needs} ffmpeg, " + "which PhotoMapAI could not find." ) progress_tracker.add_completion_warning(album_key, message) diff --git a/photomap/backend/video.py b/photomap/backend/video.py index 0ae92d46..ad3e1b1b 100644 --- a/photomap/backend/video.py +++ b/photomap/backend/video.py @@ -247,9 +247,28 @@ def _has_decodable_video_stream(report: str) -> bool: _ffmpeg_exe_cache: str | None = None _ffmpeg_exe_probed = False +_ffmpeg_missing = False _ffmpeg_exe_lock = threading.Lock() +def ffmpeg_known_unavailable() -> bool: + """Did the most recent probe in this process fail to find a binary? + + Reports what already happened instead of asking again. Callers on the + asyncio event loop must not trigger :func:`ffmpeg_exe`'s work: it stats + whatever ``$IMAGEIO_FFMPEG_EXE`` names, which is accepted unchecked by + imageio and may be a hung network mount, and the underlying probe spawns + ``ffmpeg -version`` with no timeout. Every video failure has already + driven a real probe on a worker thread (``_load_video`` -> + ``extract_video_frame`` -> :func:`_run_ffmpeg`), so by the time a run + reports its skipped files this is an answer rather than a guess. + + ``False`` until something has probed: absence of evidence has to read as + "do not blame ffmpeg", or a caller would invent a cause it never observed. + """ + return _ffmpeg_missing + + def ffmpeg_exe() -> str | None: """A runnable ffmpeg, or ``None`` if this platform has none. @@ -270,7 +289,7 @@ def ffmpeg_exe() -> str | None: name ``"ffmpeg"`` for a PATH lookup, so a non-``None`` answer is not on its own evidence that anything is executable. """ - global _ffmpeg_exe_cache, _ffmpeg_exe_probed + global _ffmpeg_exe_cache, _ffmpeg_exe_probed, _ffmpeg_missing if _ffmpeg_exe_probed: return _ffmpeg_exe_cache @@ -286,6 +305,7 @@ def ffmpeg_exe() -> str | None: logger.warning( f"No usable ffmpeg binary found ({e}); video files will be skipped." ) + _ffmpeg_missing = True return None # not memoized — the next call retries resolved = candidate if os.path.isabs(candidate) else shutil.which(candidate) @@ -294,19 +314,22 @@ def ffmpeg_exe() -> str | None: f"ffmpeg reported as {candidate!r} but is not executable; " "video files will be skipped." ) + _ffmpeg_missing = True return None _ffmpeg_exe_cache = resolved _ffmpeg_exe_probed = True + _ffmpeg_missing = False return _ffmpeg_exe_cache def _reset_ffmpeg_exe_cache() -> None: """Test seam: forget the probed binary so the next call re-resolves.""" - global _ffmpeg_exe_cache, _ffmpeg_exe_probed + global _ffmpeg_exe_cache, _ffmpeg_exe_probed, _ffmpeg_missing with _ffmpeg_exe_lock: _ffmpeg_exe_cache = None _ffmpeg_exe_probed = False + _ffmpeg_missing = False def _parse_ffmpeg_banner(stderr: str) -> dict[str, object]: diff --git a/tests/backend/test_video_guards.py b/tests/backend/test_video_guards.py index 6cd6bc4a..d46a237d 100644 --- a/tests/backend/test_video_guards.py +++ b/tests/backend/test_video_guards.py @@ -456,17 +456,18 @@ def _skip_result(*names: str) -> IndexResult: ) -def _registered_warning(monkeypatch, result: IndexResult, exe: str | None) -> str: - """Run the notice builder with ffmpeg forced present/absent.""" - monkeypatch.setattr("photomap.backend.embeddings.ffmpeg_exe", lambda: exe) - key = f"warning-test-{id(result)}" +def _registered_warning(monkeypatch, result: IndexResult, missing: bool) -> str: + """Run the notice builder with ffmpeg observed present/absent.""" + monkeypatch.setattr( + "photomap.backend.embeddings.ffmpeg_known_unavailable", lambda: missing + ) captured: list[str] = [] monkeypatch.setattr( progress_tracker, "add_completion_warning", lambda album_key, message: captured.append(message), ) - Embeddings._register_unreadable_files_warning(key, result) + Embeddings._register_unreadable_files_warning("warning-test", result) assert len(captured) == 1 return captured[0] @@ -476,21 +477,21 @@ def test_missing_ffmpeg_is_named_when_only_videos_failed(monkeypatch): reports every video as unreadable and sends the user hunting for corrupt files that are perfectly fine.""" warning = _registered_warning( - monkeypatch, _skip_result("a.mp4", "b.mov", "c.mkv"), exe=None + monkeypatch, _skip_result("a.mp4", "b.mov", "c.mkv"), missing=True ) assert warning == ( - "3 videos could not be indexed and were skipped: no ffmpeg binary is " - "available on this system." + "3 videos could not be indexed and were skipped: PhotoMapAI could " + "not find a working ffmpeg." ) def test_missing_ffmpeg_notice_agrees_for_a_single_video(monkeypatch): - warning = _registered_warning(monkeypatch, _skip_result("only.mp4"), exe=None) + warning = _registered_warning(monkeypatch, _skip_result("only.mp4"), missing=True) assert warning == ( - "1 video could not be indexed and was skipped: no ffmpeg binary is " - "available on this system." + "1 video could not be indexed and was skipped: PhotoMapAI could not " + "find a working ffmpeg." ) @@ -500,12 +501,28 @@ def test_a_mixed_failure_keeps_the_generic_count_and_adds_the_cause(monkeypatch) warning = _registered_warning( monkeypatch, _skip_result("a.mp4", "b.mov", "broken.jpg", "truncated.png"), - exe=None, + missing=True, ) assert warning == ( "4 files could not be read and were skipped, including 2 videos that " - "need ffmpeg, which is not available on this system." + "need ffmpeg, which PhotoMapAI could not find." + ) + + +def test_a_mixed_failure_with_one_video_still_agrees(monkeypatch): + """The mixed branch always has count >= 2, so ``verb`` is always plural + there — but ``videos`` can be 1, which needs its own verb. One video plus + some unreadable photos is the likeliest mixed shape in the wild, and + pinning only the videos == 2 case let "1 video that need ffmpeg" through. + """ + warning = _registered_warning( + monkeypatch, _skip_result("a.mp4", "broken.jpg"), missing=True + ) + + assert warning == ( + "2 files could not be read and were skipped, including 1 video that " + "needs ffmpeg, which PhotoMapAI could not find." ) @@ -513,21 +530,26 @@ def test_wording_is_unchanged_when_ffmpeg_is_present(monkeypatch): """A truncated clip on a working install is not an ffmpeg problem, and saying so would be actively misleading.""" warning = _registered_warning( - monkeypatch, _skip_result("broken.mp4"), exe="/usr/bin/ffmpeg" + monkeypatch, _skip_result("broken.mp4"), missing=False ) assert warning == "1 file could not be read and was skipped." def test_image_only_failures_never_mention_ffmpeg(monkeypatch): - """ffmpeg_exe() must not even be consulted: it re-probes on every call - when it has no binary to report, so a photo-only album would pay a - process spawn for nothing.""" + """ffmpeg must not be consulted at all when no video failed. + + The observation is process-wide, so a machine with no ffmpeg would + otherwise let an all-photos album inherit "could not find ffmpeg" as the + explanation for two corrupt JPEGs. + """ def _fail(): - raise AssertionError("ffmpeg must not be probed for an image-only skip") + raise AssertionError("ffmpeg must not be consulted for an image-only skip") - monkeypatch.setattr("photomap.backend.embeddings.ffmpeg_exe", _fail) + monkeypatch.setattr( + "photomap.backend.embeddings.ffmpeg_known_unavailable", _fail + ) captured: list[str] = [] monkeypatch.setattr( progress_tracker, diff --git a/tests/backend/test_video_probe.py b/tests/backend/test_video_probe.py index 0fee25fc..b00108dc 100644 --- a/tests/backend/test_video_probe.py +++ b/tests/backend/test_video_probe.py @@ -401,6 +401,82 @@ def test_ffmpeg_exe_points_at_a_real_binary(): assert Path(ffmpeg_exe()).exists() +def test_known_unavailable_tracks_the_last_probe(monkeypatch, tmp_path): + """The indexing completion notice reads this instead of calling + ``ffmpeg_exe`` — it runs on the asyncio event loop, where a stat of a hung + ``$IMAGEIO_FFMPEG_EXE`` mount or an untimed ``ffmpeg -version`` spawn would + freeze every request. So it has to follow the probe in both directions, + including back to False once a binary is found. + """ + video_module._reset_ffmpeg_exe_cache() + try: + # Nothing probed yet: no evidence must not read as "ffmpeg is missing". + assert video_module.ffmpeg_known_unavailable() is False + + stand_in = tmp_path / "ffmpeg" + stand_in.write_text("") + calls = [] + + def flaky(): + calls.append(1) + if len(calls) == 1: + raise OSError("Cannot allocate memory") + return str(stand_in) + + import imageio_ffmpeg + + monkeypatch.setattr(imageio_ffmpeg, "get_ffmpeg_exe", flaky) + + assert video_module.ffmpeg_exe() is None + assert video_module.ffmpeg_known_unavailable() is True + + assert video_module.ffmpeg_exe() == str(stand_in) + assert video_module.ffmpeg_known_unavailable() is False + finally: + video_module._reset_ffmpeg_exe_cache() + + +def test_known_unavailable_is_set_when_the_candidate_is_not_executable( + monkeypatch, tmp_path +): + """The other ``return None`` path: imageio hands back a name that does not + resolve to anything runnable.""" + video_module._reset_ffmpeg_exe_cache() + try: + import imageio_ffmpeg + + monkeypatch.setattr( + imageio_ffmpeg, + "get_ffmpeg_exe", + lambda: str(tmp_path / "definitely-not-here"), + ) + + assert video_module.ffmpeg_exe() is None + assert video_module.ffmpeg_known_unavailable() is True + finally: + video_module._reset_ffmpeg_exe_cache() + + +def test_resetting_the_cache_also_clears_the_unavailable_flag(monkeypatch): + """Otherwise the flag outlives the probe it describes, and the next test + (or a re-probe after the condition clears) inherits a stale verdict.""" + video_module._reset_ffmpeg_exe_cache() + try: + import imageio_ffmpeg + + monkeypatch.setattr( + imageio_ffmpeg, + "get_ffmpeg_exe", + lambda: (_ for _ in ()).throw(OSError("boom")), + ) + assert video_module.ffmpeg_exe() is None + assert video_module.ffmpeg_known_unavailable() is True + finally: + video_module._reset_ffmpeg_exe_cache() + + assert video_module.ffmpeg_known_unavailable() is False + + # -------------------------------------------------------------------------- # Banner parsing against hostile input # -------------------------------------------------------------------------- From 44686c54170921e46ffbb8299db664c2df5642a5 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sun, 20 Sep 2026 09:54:00 -0400 Subject: [PATCH 3/3] fix: paint a completion warning orange instead of success-green MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `.index-status.completed { color: green !important }` is an author !important declaration, and those outrank *normal* inline declarations — so `status.style.color = "#ff9800"` never reached the screen and every completion-with-a-warning rendered identically to a clean success. That is the one state where the colour carries the message. Drive it from a `with-warning` class instead, whose rule out-specifies the base one (three classes to two; both !important, so only specificity can break the tie), and clear the inline colour that earlier poll ticks leave behind. The two existing Jest assertions were checking `status.style.color`, which jsdom happily records even when a real engine discards it — they passed throughout. Re-pinned on the class, plus coverage for a stale with-warning surviving into a later clean run and for the inline colour left by the scanning branch. Co-Authored-By: Claude Opus 5 (1M context) --- .../frontend/static/css/album-manager.css | 6 ++++ .../static/javascript/album-manager.js | 23 ++++++++----- tests/frontend/album-manager-progress.test.js | 32 +++++++++++++++++-- 3 files changed, 50 insertions(+), 11 deletions(-) diff --git a/photomap/frontend/static/css/album-manager.css b/photomap/frontend/static/css/album-manager.css index 42075325..7cbf2586 100644 --- a/photomap/frontend/static/css/album-manager.css +++ b/photomap/frontend/static/css/album-manager.css @@ -256,6 +256,12 @@ .index-status.completed { color: green !important; } +/* Completed, but something was skipped. Needs both the !important and the + extra class: .index-status.completed is also !important, so only higher + specificity can beat it — an inline style cannot. */ +.index-status.completed.with-warning { + color: #ff9800 !important; +} .index-status.error { color: #b00020 !important; } diff --git a/photomap/frontend/static/javascript/album-manager.js b/photomap/frontend/static/javascript/album-manager.js index 896f3cf4..f28da170 100644 --- a/photomap/frontend/static/javascript/album-manager.js +++ b/photomap/frontend/static/javascript/album-manager.js @@ -1952,14 +1952,21 @@ export class AlbumManager { updateProgressStatus(status, progress, estimatedTime) { if (progress.status === "completed") { - status.className = AlbumManager.STATUS_CLASSES.COMPLETED; - if (progress.warning_message) { - status.textContent = `Indexing completed — ${progress.warning_message}`; - status.style.color = "#ff9800"; // Orange: completed, but with a caveat - } else { - status.textContent = "Indexing completed successfully"; - status.style.color = "green"; - } + // The colour has to come from the stylesheet, not from style.color: + // `.index-status.completed` carries an author !important, which outranks + // a *normal* inline declaration, so the orange set here was silently + // discarded and every warning rendered in success-green — the one state + // where the colour is carrying the message. + const hasWarning = Boolean(progress.warning_message); + status.className = hasWarning + ? `${AlbumManager.STATUS_CLASSES.COMPLETED} with-warning` + : AlbumManager.STATUS_CLASSES.COMPLETED; + status.textContent = hasWarning + ? `Indexing completed — ${progress.warning_message}` + : "Indexing completed successfully"; + // Clear any inline colour a previous poll tick left behind (the branches + // below still set one), so the rule that applies is this state's. + status.style.color = ""; estimatedTime.textContent = ""; } else if (progress.status === "error") { status.className = AlbumManager.STATUS_CLASSES.ERROR; diff --git a/tests/frontend/album-manager-progress.test.js b/tests/frontend/album-manager-progress.test.js index 34bcf930..76f53c62 100644 --- a/tests/frontend/album-manager-progress.test.js +++ b/tests/frontend/album-manager-progress.test.js @@ -123,7 +123,7 @@ describe("AlbumManager completed status", () => { expect(status.textContent).toBe("Indexing completed successfully"); expect(status.className).toBe(AlbumManager.STATUS_CLASSES.COMPLETED); - expect(status.style.color).toBe("green"); + expect(status.classList.contains("with-warning")).toBe(false); }); test("surfaces a non-fatal warning_message alongside completion", () => { @@ -140,9 +140,35 @@ describe("AlbumManager completed status", () => { expect(status.textContent).toContain("Indexing completed"); expect(status.textContent).toContain("not found on disk"); - expect(status.className).toBe(AlbumManager.STATUS_CLASSES.COMPLETED); // Completion-with-a-caveat is coloured differently from a clean success. - expect(status.style.color).not.toBe("green"); + // Asserted on the class, not style.color: `.index-status.completed` is an + // author !important rule, so an inline colour never reaches the screen. + // The old assertion passed in jsdom (which happily records the ignored + // inline value) while the real page painted the warning success-green. + expect(status.classList.contains("completed")).toBe(true); + expect(status.classList.contains("with-warning")).toBe(true); + }); + + test("drops the warning class when a later run completes cleanly", () => { + const { status, estimatedTime } = makeElements(); + + callUpdate(status, { status: "completed", warning_message: "1 file was skipped." }, estimatedTime); + callUpdate(status, { status: "completed" }, estimatedTime); + + // className is reassigned wholesale, but pin it: a stale with-warning + // would paint a clean run orange for the rest of the session. + expect(status.classList.contains("with-warning")).toBe(false); + }); + + test("does not leave an inline colour that outlives the state that set it", () => { + const { status, estimatedTime } = makeElements(); + + // Scanning sets an inline orange; completing must not inherit it, since + // inline beats the non-!important base `.index-status` rule. + callUpdate(status, { status: "scanning" }, estimatedTime); + callUpdate(status, { status: "completed" }, estimatedTime); + + expect(status.style.color).toBe(""); }); });