From d022167ba2e2812917fa87cc605dce534307baf4 Mon Sep 17 00:00:00 2001 From: Michael Date: Mon, 18 May 2026 20:52:20 +0000 Subject: [PATCH] =?UTF-8?q?fix(home):=20widget's=20"=E2=9C=95"=20Remove=20?= =?UTF-8?q?now=20actually=20removes=20the=20file?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported: on the Home page after uploading data files, the Remove buttons "on the right side" did nothing — the file kept showing up in the list. That was the file_uploader widget's BUILT-IN ✕ icons (the ones inside the uploader's chrome, on the right of each file row), not our custom "Remove" buttons further down — the custom ones have worked correctly since 84e4665. Cause: ``_home_page`` deliberately treated the widget as add-only and never honored widget-side removals. The reasoning, per the prior comment, was that navigation can remount the widget with value ``[]`` — a render-time sync would then wipe ``home_uploads``. Real, but the side effect was that the widget's own ✕ appeared to do nothing: the file vanished from the widget chrome, stayed in ``home_uploads``, and re-rendered immediately in the custom list below. Fix: hook the file_uploader's ``on_change`` callback to reconcile ``home_uploads`` against the widget's current value. Streamlit's ``on_change`` fires ONLY on user-initiated value changes; the remount-induced ``[]`` reset doesn't trigger it, so the stash still survives navigation. Removals from the callback also drop the file's findings entry and clear the singular ``home_uploaded_*`` keys when the active upload was removed — matching the custom-button path. The custom "Remove" buttons further down keep working unchanged; the existing AppTest path through ``_home_remove_`` still removes exactly the file clicked. 2220 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/gui/_home.py | 74 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 23 deletions(-) diff --git a/src/gui/_home.py b/src/gui/_home.py index 325caf4..1df4482 100644 --- a/src/gui/_home.py +++ b/src/gui/_home.py @@ -36,6 +36,43 @@ class _StashedUpload: return self._data +def _sync_uploader_to_home_uploads() -> None: + """``on_change`` callback for the home-page file_uploader. + + Reconciles ``home_uploads`` (our persistent stash) with the widget's + current value: adds newly-uploaded files, and drops files the user + explicitly removed via the widget's built-in "✕" button. Per + Streamlit semantics ``on_change`` only runs for user-initiated + value changes, so the navigation-induced ``[]`` reset never reaches + here — the stash survives intact across page switches. + """ + from src.audit import log_event + + widget_files = st.session_state.get("home_upload") or [] + home_uploads: dict = st.session_state.setdefault("home_uploads", {}) + findings: dict = st.session_state.setdefault("home_findings_by_file", {}) + + widget_names = {f.name for f in widget_files} + + for f in widget_files: + if f.name not in home_uploads: + home_uploads[f.name] = {"bytes": f.getvalue(), "size": f.size} + log_event("upload", f"Uploaded {f.name}", filename=f.name, bytes=f.size) + + for name in list(home_uploads.keys()): + if name not in widget_names: + del home_uploads[name] + findings.pop(name, None) + log_event("upload", f"Removed {name}", filename=name) + if st.session_state.get("home_uploaded_name") == name: + st.session_state.pop("home_uploaded_name", None) + st.session_state.pop("home_uploaded_size", None) + st.session_state.pop("home_uploaded_bytes", None) + + st.session_state["home_uploads"] = home_uploads + st.session_state["home_findings_by_file"] = findings + + def _home_page() -> None: """Render the home page — multi-file upload + per-file analysis. @@ -79,35 +116,26 @@ def _home_page() -> None: # Source of truth for uploaded files. dict[name -> {"bytes", "size"}]. home_uploads: dict = st.session_state.setdefault("home_uploads", {}) - # File uploader — for ADDING new files only. On every render we - # merge widget-returned files INTO home_uploads but never remove - # via the widget. (Widget state can return ``[]`` after navigation, - # which we deliberately don't treat as "user cleared their files".) - new_files = st.file_uploader( + # File uploader — syncs into home_uploads via on_change. We deliberately + # do NOT merge widget state into home_uploads at render time: navigation + # can remount the widget with value ``[]``, and a render-time merge + # would mistakenly leave home_uploads untouched while the user thinks + # they're looking at empty state. + # + # ``on_change`` fires ONLY on user-initiated value changes (uploads + # and the widget's built-in "✕" remove). It does NOT fire on the + # remount-induced reset. That lets us treat the callback as ground + # truth for both adds AND removes — fixing the previous bug where + # the widget's "✕" appeared to do nothing because the file persisted + # in home_uploads and immediately re-rendered in the list below. + st.file_uploader( t("upload.uploader_label_multi"), type=["csv", "tsv", "xlsx", "xls"], accept_multiple_files=True, key="home_upload", help=t("upload.uploader_help"), + on_change=_sync_uploader_to_home_uploads, ) - if new_files: - from src.audit import log_event - changed = False - for f in new_files: - if f.name not in home_uploads: - home_uploads[f.name] = { - "bytes": f.getvalue(), - "size": f.size, - } - changed = True - log_event( - "upload", - f"Uploaded {f.name}", - filename=f.name, - bytes=f.size, - ) - if changed: - st.session_state["home_uploads"] = home_uploads # Persistent file list with per-file remove buttons. We render this # ourselves rather than trusting Streamlit's widget chrome because