Conversation
I want to be clear that I think this is terrible, but I'm pushing the partial commit to demo that when you ask Claude to do X, it will do something like X, but perhaps in a way that makes you want to chuck it into a river.
There was a problem hiding this comment.
Pull request overview
Adds an Airflow FastAPI plugin that serves the Airflow server’s ./public directory under /mokelumne/public, and wires it into the UI via sidebar + DAG run external views. Updates summarise_job to write public output under a run-id-based directory, and adds Playwright e2e coverage for the new viewer.
Changes:
- Add
plugins/public_browser.pyFastAPI app +AirflowPluginregistration and external views. - Update
summarise_joboutput directory naming and HTML link generation for public artifacts. - Add Playwright e2e tests for the public browser endpoints and UI navigation; ignore
uv.lock.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
plugins/public_browser.py |
Implements /mokelumne/public file/directory serving and registers UI external views. |
mokelumne/dags/summarise_job.py |
Writes outputs into a run-id directory and updates index.html links for public browsing. |
mokelumne/dags/notify_user.py |
Removes now-unused public_path_to_url import. |
test/e2e/test_public_browser.py |
Adds request-level e2e tests for auth + attachment behavior of the public browser. |
test/e2e/test_public_browser_nav.py |
Adds UI navigation e2e tests for sidebar and DAG-run tab integration. |
.gitignore |
Ignores uv.lock. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SEEDED_JOB_UUID = "5b5a47d6-18d6-4ca6-aea3-e43396bc587f" | ||
|
|
||
|
|
||
| def test_sidebar_tab_and_job_row_click(page_as_testadmin: Page) -> None: | ||
| page = page_as_testadmin | ||
|
|
||
| # Sidebar button rendered by PluginMenus with topLevel=true. | ||
| sidebar_btn = page.get_by_role("link", name="Batch Image Results").first | ||
| expect(sidebar_btn).to_be_visible() | ||
| sidebar_btn.click() | ||
|
|
||
| # React router navigates to /plugin/<url_route>. | ||
| page.wait_for_url("**/plugin/mokelumne_public_browser") | ||
|
|
||
| # Iframe loads the directory listing. Directory entries render as | ||
| # "<name>/" in the auto-generated <ul>, so match on the trailing slash. | ||
| iframe = page.frame_locator("iframe").first | ||
| job_link = iframe.get_by_role("link", name=f"{SEEDED_JOB_UUID}/") | ||
| expect(job_link).to_be_visible() | ||
|
|
There was a problem hiding this comment.
This UI navigation test also relies on SEEDED_JOB_UUID being present in the public directory listing inside the iframe. Without seeding ./public/<uuid>/ as part of the test setup, this will fail in CI and for new dev checkouts. Consider creating the seed directory/files in a fixture and using that UUID here.
| <dt><a href="./{proc_path}">{proc_count} images processed</a></dt> | ||
| <dd>{proc_count} succeeded, {proc_count - fetch_success} failed</dd> | ||
| <dt><a href="{public_path_to_url(fetched_path)}">{fetch_count} images fetched</a></dt> | ||
| <dt><a href="./{fetched_path}">{fetch_count} images fetched</a></dt> | ||
| <dd>{fetch_success} succeeded, {fetch_failures} failed</dd> | ||
| <dt><a href="{public_path_to_url(skipped_path)}">{skip_count} records skipped</a></dt> | ||
| <dt><a href="./{skipped_path}">{skip_count} records skipped</a></dt> |
There was a problem hiding this comment.
The generated links use ./{proc_path} / ./{fetched_path} / ./{skipped_path}, but those variables are absolute Paths (e.g., /opt/airflow/public/<run_id>/...). This produces broken hrefs and leaks filesystem paths into the rendered HTML. Use paths relative to collated_path (e.g., proc_path.name or proc_path.relative_to(collated_path).as_posix()) when building the links.
| @app.get("/{subpath:path}") | ||
| def any_path(subpath: str) -> Response: | ||
| clean = subpath.rstrip("/") | ||
| return _serve(_resolve(clean), clean) |
There was a problem hiding this comment.
any_path() strips the trailing slash and then serves directory listings without redirecting. If a user hits /mokelumne/public/<dir> (no trailing slash), the listing’s relative links like child/ will resolve to /mokelumne/public/child/ instead of /mokelumne/public/<dir>/child/. Consider preserving the trailing slash or issuing a redirect to the canonical .../<dir>/ URL when the resolved target is a directory.
| * directory with an ``index.html`` → returns that file | ||
| * directory without one → returns an auto-generated ``<ul>`` listing | ||
| * file with a "data" suffix → sent as an attachment (force download) | ||
| * any other file → served inline with FastAPI's default media type |
There was a problem hiding this comment.
Docstring says “file with a "data" suffix → sent as an attachment”, but the implementation uses ATTACHMENT_SUFFIXES (csv/json/tsv). Update the wording to match the actual behavior to avoid confusion.
| * directory with an ``index.html`` → returns that file | |
| * directory without one → returns an auto-generated ``<ul>`` listing | |
| * file with a "data" suffix → sent as an attachment (force download) | |
| * any other file → served inline with FastAPI's default media type | |
| * directory with an ``index.html`` → returns that file | |
| * directory without one → returns an auto-generated ``<ul>`` listing | |
| * file with a ``.csv/.json/.tsv`` suffix → sent as an attachment (force download) | |
| * any other file → served inline with FastAPI's default media type |
| # A summarise_job run left in the local DB from the compose stack's prior | ||
| # execution. The plugin 404s if the per-run public directory is missing, | ||
| # so the test only asserts the tab/iframe wiring, not the iframe content. | ||
| run_id = "asset_triggered__2026-04-17T20:38:36.761076+00:00_ZiSQe0aG" | ||
| page.goto(f"/dags/summarise_job/runs/{run_id}") |
There was a problem hiding this comment.
This test depends on a specific pre-existing DAG run (run_id = "asset_triggered__2026-04-17T20:38:36...") being present in the Airflow metadata DB. In CI the stack starts from a fresh DB, so this will reliably fail. Create the run as part of the test (e.g., via Airflow’s API/CLI) or derive the run_id from a run you trigger within the test.
| # A summarise_job output seeded into ./public/ by running the DAG locally; | ||
| # contents are not committed to the repo. | ||
| SEEDED_JOB_UUID = "5b5a47d6-18d6-4ca6-aea3-e43396bc587f" | ||
|
|
||
|
|
||
| def test_testuser_can_view_browser(page_as_testuser: Page) -> None: | ||
| response = page_as_testuser.request.get(PUBLIC_BROWSER_PATH) | ||
| assert response.status == 200 | ||
| assert SEEDED_JOB_UUID in response.text() | ||
|
|
There was a problem hiding this comment.
These tests assume a specific UUID directory already exists under the mounted ./public volume (“seeded into ./public/ by running the DAG locally; contents are not committed”). In CI (fresh checkout) that directory won’t exist, so the assertions against the directory listing / params.json will fail. Add a test fixture that creates the minimal expected public/<uuid>/ structure (index.html, params.json, etc.) before exercising the endpoints, or explicitly skip the test when the seed data is absent.
NOTE: This is ClaudeSlop at the moment, so hold off on reviewing—it needs some key refactorings (AirFlow 3.x plugin architecture differs from the 2.x implementation here; extracting the view template; questionable ENV choices; …). But this does "work" for some definition of that word, so feel free to pull this down if you want to preview it.