From 981a1a9cba9fb81dc4cf57e6316795e574a29e34 Mon Sep 17 00:00:00 2001 From: Michael Date: Wed, 20 May 2026 01:45:51 +0000 Subject: [PATCH] fix(downloads): OneDrive-aware Downloads path + PDF uses html_download_button MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User reported downloads "do nothing on click" in tool pages and "acts like it downloads but no file in the folder" in the PDF tool. Two root causes, two fixes. **Root cause #1 — wrong Downloads folder on Windows.** ``_downloads_dir()`` returned ``Path.home() / "Downloads"`` unconditionally. On Windows machines with OneDrive enabled (very common for business users), the real Downloads folder is redirected to ``C:\Users\\OneDrive\Downloads``. Our helper would write to ``C:\Users\\Downloads`` instead — a folder that may not even exist until ``mkdir`` creates it — and the user, naturally opening their actual OneDrive Downloads, sees no file and concludes nothing happened. Now: on Windows, ``_downloads_dir`` queries the registry key ``Software\Microsoft\Windows\CurrentVersion\Explorer\User Shell Folders`` for FOLDERID_Downloads (GUID ``{374DE290-123F-4565-9164-39C4925E467B}``). This entry returns the redirected path when OneDrive is active, the original ``%USERPROFILE%\Downloads`` otherwise — exactly what the user's File Explorer reads. ``%USERPROFILE%`` expansion is applied via ``os.path.expandvars``. Any registry hiccup falls through to ``Path.home() / "Downloads"`` so the helper never raises. The sanity check (path exists OR parent exists) catches the edge case where the registry points into a deleted OneDrive mount. **Root cause #2 — PDF page used st.download_button.** Every other tool uses the project's ``html_download_button`` helper (which is ``local_download_button`` under the hood — the rename happened in b9147f3). ``st.download_button`` has a long-standing bug where the second-or-later instance in a script pass silently fails to fire. The PDF tool predated the rewrite that switched everyone over and was still using the broken native widget. ``_Logs.py`` had the same problem in two places. Swapped all three call sites to ``html_download_button``. They now save to ``~/Downloads/`` (correctly resolved per fix #1) and show the saved path + "Open Downloads folder" button below the click, matching every other tool in the suite. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/gui/components/_legacy.py | 44 ++++++++++++++++++++++++++++--- src/gui/pages/10_PDF_Extractor.py | 16 ++++++++--- src/gui/pages/_Logs.py | 20 +++++++------- 3 files changed, 64 insertions(+), 16 deletions(-) diff --git a/src/gui/components/_legacy.py b/src/gui/components/_legacy.py index 5719be8..fd1bc68 100644 --- a/src/gui/components/_legacy.py +++ b/src/gui/components/_legacy.py @@ -1391,15 +1391,53 @@ def _farewell_script() -> str: def _downloads_dir() -> "Path": """Return the user's Downloads folder. - Defaults to ``~/Downloads``. Overrideable via the - ``DATATOOLS_DOWNLOADS_DIR`` environment variable so tests can write - to a temp directory instead of polluting the developer's home. + Resolution order: + + 1. ``DATATOOLS_DOWNLOADS_DIR`` env var (tests + power users). + 2. On Windows, the *real* Downloads path from the + ``User Shell Folders`` registry key. This matters because + OneDrive can redirect Downloads to + ``C:\\Users\\\\OneDrive\\Downloads`` — without the + registry lookup we'd write files into the un-redirected + ``C:\\Users\\\\Downloads`` and the user would never + see them in the Downloads they actually open. + 3. ``Path.home() / "Downloads"`` as the final fallback. """ import os + import sys from pathlib import Path + override = os.environ.get("DATATOOLS_DOWNLOADS_DIR") if override: return Path(override) + + if sys.platform == "win32": + try: + import winreg # noqa: PLC0415 + + with winreg.OpenKey( + winreg.HKEY_CURRENT_USER, + r"Software\Microsoft\Windows\CurrentVersion\Explorer" + r"\User Shell Folders", + ) as key: + # GUID for FOLDERID_Downloads. The User Shell + # Folders entry returns the redirected path when + # OneDrive is active, the original ~/Downloads + # otherwise — exactly what we want. + value, _ = winreg.QueryValueEx( + key, "{374DE290-123F-4565-9164-39C4925E467B}", + ) + expanded = os.path.expandvars(value) + resolved = Path(expanded) + # Sanity check: only trust the registry path if it + # exists OR can be created (don't return a path that + # points into a deleted/borked OneDrive mount). + if resolved.exists() or resolved.parent.exists(): + return resolved + except Exception: + # Any registry hiccup — fall through to ``Path.home()``. + pass + return Path.home() / "Downloads" diff --git a/src/gui/pages/10_PDF_Extractor.py b/src/gui/pages/10_PDF_Extractor.py index 8619463..2a6fdf3 100644 --- a/src/gui/pages/10_PDF_Extractor.py +++ b/src/gui/pages/10_PDF_Extractor.py @@ -21,7 +21,11 @@ if str(_project_root) not in sys.path: sys.path.insert(0, str(_project_root)) from src.audit import log_event, log_page_open -from src.gui.components import hide_streamlit_chrome, render_sticky_footer +from src.gui.components import ( + hide_streamlit_chrome, + html_download_button, + render_sticky_footer, +) from src.pdf_extract import ( PdfDependencyMissing, diagnose_pdf_lines, @@ -627,12 +631,16 @@ else: ): export[amt_col] = export[amt_col].map(format_amount) csv_bytes = export.to_csv(index=False).encode("utf-8") - st.download_button( + # Save server-side (consistent with the other tools) — + # writes to the user's Downloads folder and shows the + # exact path. Avoids the st.download_button quirk where + # the second-or-later button in a script pass silently + # fails to fire. + html_download_button( f"Download {len(export):,} rows as CSV", - data=csv_bytes, + csv_bytes, file_name=f"transactions-{ts}.csv", mime="text/csv", - type="primary", ) if not selected.empty: diff --git a/src/gui/pages/_Logs.py b/src/gui/pages/_Logs.py index 2fffdae..2cc2818 100644 --- a/src/gui/pages/_Logs.py +++ b/src/gui/pages/_Logs.py @@ -24,7 +24,11 @@ if str(_project_root) not in sys.path: sys.path.insert(0, str(_project_root)) from src.audit import _RETENTION_DAYS, audit_log_dir, audit_log_path -from src.gui.components import hide_streamlit_chrome, render_sticky_footer +from src.gui.components import ( + hide_streamlit_chrome, + html_download_button, + render_sticky_footer, +) _ICON_PATH = str(Path(__file__).parent.parent / "assets" / "datatools_icon_256.png") st.set_page_config( @@ -54,12 +58,11 @@ if today_path.exists(): st.caption(f"{size:,} bytes") try: data = today_path.read_bytes() - st.download_button( - label=f"Download {today_path.name}", - data=data, + html_download_button( + f"Download {today_path.name}", + data, file_name=today_path.name, mime="application/x-ndjson", - key="dl_today", ) except Exception as e: st.warning(f"Could not read today's log: {type(e).__name__}: {e}") @@ -88,12 +91,11 @@ else: f"{st_mtime:%Y-%m-%d %H:%M} · {size:,} bytes" ) with cols[2]: - st.download_button( - label="Download", - data=p.read_bytes(), + html_download_button( + "Download", + p.read_bytes(), file_name=p.name, mime="application/x-ndjson", - key=f"dl_{p.name}", ) except Exception as e: st.caption(