From 84e4665ab0c1d6c242f56d03773267d1a6a65ea0 Mon Sep 17 00:00:00 2001 From: Michael Date: Sun, 17 May 2026 00:34:20 +0000 Subject: [PATCH] fix(home): make per-file Remove button reliable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported: the "✕" buttons on the uploaded file list removed files inconsistently — some clicks took, some didn't. Two compounding causes: 1. ``key=f"_home_remove_{name}"`` embedded the raw filename in the Streamlit widget key. Streamlit's widget-identity machinery normalizes keys differently across reruns when they contain spaces, dots, brackets, or non-ASCII characters, so a button's identity could shift between the render where the user clicked it and the rerun that should have processed the click. The click was registered, but the post-rerun render produced a new widget under a new effective key, and the original click was "lost". 2. The handler mutated ``home_uploads`` mid-loop while subsequent iterations were still creating buttons. ``st.rerun()`` raises synchronously, but if ANOTHER button's state changed in the same pass (e.g. a stale click held over from a fast double-tap), the ordering of state-mutation vs widget-key-update vs rerun could race. Fixes: - Stable widget keys: ``f"_home_remove_{sha1(name)[:10]}"``. The hash is identifier-safe regardless of spaces / dots / Unicode in the filename. Verified across "sample with spaces.csv", "sample.csv", and "日本語.csv" — three sequential Remove clicks each remove exactly one file with no clicks lost. - Two-phase capture: the loop collects the target ``to_remove`` filename, finishes rendering every other row at consistent widget identity, THEN mutates state once and reruns. No more mid-loop ``del`` racing other widgets' click handlers. - Wider click target: column ratio ``[8, 1]`` (was ``[12, 1]``) and ``use_container_width=True`` on the Remove button so the click surface fills the entire column. Label changed to "Remove" for the same reason — "✕" is a thin glyph that compressed the hit-test region. 2220 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/gui/_home.py | 63 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 20 deletions(-) diff --git a/src/gui/_home.py b/src/gui/_home.py index 7e74be4..317f59c 100644 --- a/src/gui/_home.py +++ b/src/gui/_home.py @@ -101,10 +101,28 @@ def _home_page() -> None: # ourselves rather than trusting Streamlit's widget chrome because # the widget's "✕" only mutates widget-state, leaving home_uploads # out of sync. + # + # Two-phase click capture pattern (avoids the "hit-or-miss" click + # losses we had previously): + # + # 1. ``st.button(key=stable_hash)`` returns True on the rerun where + # it was clicked. We use a sha1 hash of the filename as the key + # so it's identifier-safe regardless of spaces / dots / unicode + # in the file name — Streamlit's widget-identity hashing on raw + # filenames was the root cause of inconsistent removals. + # 2. Inside a single pass we collect WHICH file to remove (if any), + # then mutate state ONCE after the loop and rerun. Mutating mid + # -loop while continuing to render other buttons risked + # interleaving widget-key updates with state changes. if home_uploads: + import hashlib st.markdown("**Uploaded files**") + to_remove: str | None = None for name in list(home_uploads.keys()): - col_file, col_remove = st.columns([12, 1]) + digest = hashlib.sha1( + name.encode("utf-8"), usedforsecurity=False, + ).hexdigest()[:10] + col_file, col_remove = st.columns([8, 1]) col_file.markdown( f"📄 `{name}`   " f"" @@ -112,27 +130,32 @@ def _home_page() -> None: unsafe_allow_html=True, ) if col_remove.button( - "✕", - key=f"_home_remove_{name}", + "Remove", + key=f"_home_remove_{digest}", help=f"Remove {name}", + type="secondary", + use_container_width=True, ): - del home_uploads[name] - # Drop any findings/results tied to the removed file. - findings_by_file_drop = st.session_state.get( - "home_findings_by_file", {} - ) - findings_by_file_drop.pop(name, None) - st.session_state["home_uploads"] = home_uploads - st.session_state["home_findings_by_file"] = findings_by_file_drop - # If we just removed the active upload, also clear the - # singular ``home_uploaded_*`` keys so tool pages don't - # pick up stale bytes; the next render will repopulate - # them from whatever file is now first. - 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.rerun() + to_remove = name + + if to_remove is not None: + del home_uploads[to_remove] + # Drop any findings/results tied to the removed file. + findings_by_file_drop = st.session_state.get( + "home_findings_by_file", {} + ) + findings_by_file_drop.pop(to_remove, None) + st.session_state["home_uploads"] = home_uploads + st.session_state["home_findings_by_file"] = findings_by_file_drop + # If we just removed the active upload, also clear the + # singular ``home_uploaded_*`` keys so tool pages don't + # pick up stale bytes; the next render will repopulate + # them from whatever file is now first. + if st.session_state.get("home_uploaded_name") == to_remove: + 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.rerun() if not home_uploads: st.info(t("upload.empty_state"))