fix(home): make per-file Remove button reliable
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) <noreply@anthropic.com>
This commit is contained in:
@@ -101,10 +101,28 @@ def _home_page() -> None:
|
|||||||
# ourselves rather than trusting Streamlit's widget chrome because
|
# ourselves rather than trusting Streamlit's widget chrome because
|
||||||
# the widget's "✕" only mutates widget-state, leaving home_uploads
|
# the widget's "✕" only mutates widget-state, leaving home_uploads
|
||||||
# out of sync.
|
# 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:
|
if home_uploads:
|
||||||
|
import hashlib
|
||||||
st.markdown("**Uploaded files**")
|
st.markdown("**Uploaded files**")
|
||||||
|
to_remove: str | None = None
|
||||||
for name in list(home_uploads.keys()):
|
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(
|
col_file.markdown(
|
||||||
f"📄 `{name}` "
|
f"📄 `{name}` "
|
||||||
f"<span style='opacity:0.6'>"
|
f"<span style='opacity:0.6'>"
|
||||||
@@ -112,23 +130,28 @@ def _home_page() -> None:
|
|||||||
unsafe_allow_html=True,
|
unsafe_allow_html=True,
|
||||||
)
|
)
|
||||||
if col_remove.button(
|
if col_remove.button(
|
||||||
"✕",
|
"Remove",
|
||||||
key=f"_home_remove_{name}",
|
key=f"_home_remove_{digest}",
|
||||||
help=f"Remove {name}",
|
help=f"Remove {name}",
|
||||||
|
type="secondary",
|
||||||
|
use_container_width=True,
|
||||||
):
|
):
|
||||||
del home_uploads[name]
|
to_remove = name
|
||||||
|
|
||||||
|
if to_remove is not None:
|
||||||
|
del home_uploads[to_remove]
|
||||||
# Drop any findings/results tied to the removed file.
|
# Drop any findings/results tied to the removed file.
|
||||||
findings_by_file_drop = st.session_state.get(
|
findings_by_file_drop = st.session_state.get(
|
||||||
"home_findings_by_file", {}
|
"home_findings_by_file", {}
|
||||||
)
|
)
|
||||||
findings_by_file_drop.pop(name, None)
|
findings_by_file_drop.pop(to_remove, None)
|
||||||
st.session_state["home_uploads"] = home_uploads
|
st.session_state["home_uploads"] = home_uploads
|
||||||
st.session_state["home_findings_by_file"] = findings_by_file_drop
|
st.session_state["home_findings_by_file"] = findings_by_file_drop
|
||||||
# If we just removed the active upload, also clear the
|
# If we just removed the active upload, also clear the
|
||||||
# singular ``home_uploaded_*`` keys so tool pages don't
|
# singular ``home_uploaded_*`` keys so tool pages don't
|
||||||
# pick up stale bytes; the next render will repopulate
|
# pick up stale bytes; the next render will repopulate
|
||||||
# them from whatever file is now first.
|
# them from whatever file is now first.
|
||||||
if st.session_state.get("home_uploaded_name") == name:
|
if st.session_state.get("home_uploaded_name") == to_remove:
|
||||||
st.session_state.pop("home_uploaded_name", None)
|
st.session_state.pop("home_uploaded_name", None)
|
||||||
st.session_state.pop("home_uploaded_size", None)
|
st.session_state.pop("home_uploaded_size", None)
|
||||||
st.session_state.pop("home_uploaded_bytes", None)
|
st.session_state.pop("home_uploaded_bytes", None)
|
||||||
|
|||||||
Reference in New Issue
Block a user