AP-660: Adds /public viewer by danschmidt5189 · Pull Request #43 · BerkeleyLibrary/mokelumne

danschmidt5189 · 2026-04-18T16:52:59Z

Adds StaticFiles Plugin

Configures FastApi.StaticFiles as a plugin mounted at STATIC_URL_PREFIX (default: /storage) serving files from STATIC_ROOT (default: /opt/airflow/storage).
Adds custom middleware to map responses with a Content Type in STATIC_FORCE_PLAINTEXT to "text/plain", forcing browsers to render them inline. This is necessary for these views to render properly in sandboxed AirFlow iFrames.
Adds a "Files" tab to the DAG Run view showing files at {STATIC_ROOT}/{dag_id}/{run_id}.

Permissions

AirFlow's default "User" role lacks the ability to view Plugins, which prevents them from seeing the routes exposed by this plugin (whether directly or in a DAG Run tab). To get this working, I added Plugins.can_read to the User and Viewer roles. This feels wrong / overly broad so I'm open to a better approach here. (This PR just really dragged on and I wanted to get something reviewable out.)

Download vs. Inline Files

AirFlow's iFrames are sandboxed and don't allow downloads, which causes traditional left-click behavior to fail when trying to view CSVs. The plugin works around this by setting the Content-Type to text/plain for CSVs, causing the browser to render them inline. Users can still download the file via Right Click + Save File As….

Screenshots

Viewing the index.html

If no file is specified (e.g. /public/1/2/ is requested, without a file), then the index.html is rendered if it exists.

Viewing a CSV

The Tab view iFrame blocks downloading, so we have to render the content inline otherwise left-click fails silently. Users can still download the file via Right Click + Save as….

When unauthenticated / unauthorized

Unauthenticated / unauthorized users get a 401/403:

When the DAG run directory is missing / empty

Returns 404 when the DAG run doesn't have public files.

Copilot

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.py FastAPI app + AirflowPlugin registration and external views.
Update summarise_job output 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.

Copilot · 2026-04-22T18:23:13Z

+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()
+


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.

Copilot · 2026-04-22T18:23:14Z

+@app.get("/{subpath:path}")
+def any_path(subpath: str) -> Response:
+    clean = subpath.rstrip("/")
+    return _serve(_resolve(clean), clean)


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.

danschmidt5189 · 2026-04-24T18:31:36Z

-        """Generate a URL-safe directory for collated output."""
-        path = public_dir() / str(uuid4())
-        path.mkdir()  # We don't exist_ok=True because it should be unique.
+    def generate_id(dag: DAG, run_id: str) -> str:


This was a neat find — context variables can be injected by name.

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-24T21:36:21Z

+    run_dir = PUBLIC_HOST_DIR / DAG_ID / RUN_ID
+    run_dir.mkdir(parents=True, exist_ok=True)
+    (run_dir / "index.html").write_text(index_html, encoding="utf-8")
+    (run_dir / SEEDED_CSV_FILENAME).write_text(SEEDED_CSV_TEXT, encoding="utf-8")
+


This fixture creates/overwrites files under the shared ./public/<DAG>/<RUN>/ tree but never removes them. Consider using a yield fixture with teardown to delete run_dir afterwards (or write into a unique per-test subdirectory) to avoid accumulating state across runs.

Copilot · 2026-04-24T21:36:22Z

+def test_unauthenticated_user_cannot_view_files(
+    page: Page,
+    seeded_run: dict[str, str]
+) -> None:
+    """A fresh browser (no login) must get a 401 from the plugin route."""


Role changes in this PR affect both "User" and "Viewer" (Plugins.can_read), but the e2e suite only asserts unauthenticated + user/admin behavior. Add a test covering the viewer/public user fixture (or explicitly assert they should not have access) so the permission intent is locked in.

Copilot · 2026-04-24T21:36:22Z

+    # so left-click behavior works. The user can still right-click/Save As to download it.
+    media_type = mimetypes.guess_type(filepath)[0] or "application/octet-stream"
+    if media_type == "text/csv":
+        media_type = "text/plain; charset=utf-8"
+


The CSV Content-Type rewrite (text/csv -> text/plain; charset=utf-8) is important for iframe behavior, but there’s no automated check that it stays in place. Consider adding a test that requests a seeded .csv and asserts the response Content-Type header so regressions are caught.

Copilot · 2026-04-24T21:36:22Z

+    def generate_id(dag: DAG, run_id: str) -> str:
+        """Creates the local storage path for the current DAG run"""
+        path = public_dir() / dag.dag_id / run_id


generate_id is defined to take (dag: DAG, run_id: str), but the DAG isn’t providing these values via upstream XComs/params. Unless Airflow is injecting these automatically, this task signature will break. Prefer reading dag_id/run_id from get_current_context() inside the task or keep generate_id parameterless.

Suggested change

def generate_id(dag: DAG, run_id: str) -> str:

"""Creates the local storage path for the current DAG run"""

path = public_dir() / dag.dag_id / run_id

def generate_id() -> str:

"""Creates the local storage path for the current DAG run"""

context = get_current_context()

dag_id = context["dag"].dag_id

run_id = context["run_id"]

path = public_dir() / dag_id / run_id

Copilot · 2026-04-24T21:36:23Z

+    url_base = os.environ.get("MOKELUMNE_PUBLIC_URL", "http://localhost:8080/public/")
    return f"{url_base}{str(location)}"


public_path_to_url() currently concatenates MOKELUMNE_PUBLIC_URL with the public storage relative path but does not incorporate MOKELUMNE_PUBLIC_URL_PREFIX. With example.env setting MOKELUMNE_PUBLIC_URL=http://localhost:8080/, this will generate links that omit /public. Either require MOKELUMNE_PUBLIC_URL to include the prefix (document it) or build URLs from MOKELUMNE_PUBLIC_URL + MOKELUMNE_PUBLIC_URL_PREFIX (normalizing slashes).

Suggested change

url_base = os.environ.get("MOKELUMNE_PUBLIC_URL", "http://localhost:8080/public/")

return f"{url_base}{str(location)}"

url_base = os.environ.get("MOKELUMNE_PUBLIC_URL", "http://localhost:8080/")

url_prefix = os.environ.get("MOKELUMNE_PUBLIC_URL_PREFIX", "/public")

normalized_base = url_base.rstrip("/")

normalized_prefix = url_prefix.strip("/")

normalized_location = location.as_posix().strip("/")

url_parts = [normalized_base]

if normalized_prefix:

url_parts.append(normalized_prefix)

if normalized_location:

url_parts.append(normalized_location)

return "/".join(url_parts)

What Copilot is suggesting is excessive. Try this (presuming an from urllib.parse import urljoin added at the top):

Suggested change

url_base = os.environ.get("MOKELUMNE_PUBLIC_URL", "http://localhost:8080/public/")

return f"{url_base}{str(location)}"

url_base = os.environ.get("MOKELUMNE_PUBLIC_URL", "http://localhost:8080/public/")

return urljoin(url_base, str(location))

Copilot · 2026-04-24T21:36:23Z

 # digging into an auth/config error, but it's *extremely* verbose.
 KC_LOG_LEVEL=INFO

+MOKELUMNE_PUBLIC_STORAGE=/opt/app/storage


MOKELUMNE_PUBLIC_STORAGE is set to /opt/app/storage, but docker-compose bind-mounts ./public into /opt/airflow/public. If someone copies example.env to .env, the plugin/tests will read from a different directory than the mounted ./public volume. Align this value with the compose mount (or update compose accordingly).

Suggested change

MOKELUMNE_PUBLIC_STORAGE=/opt/app/storage

MOKELUMNE_PUBLIC_STORAGE=/opt/airflow/public

Copilot · 2026-04-24T21:36:23Z

 KC_LOG_LEVEL=INFO

+MOKELUMNE_PUBLIC_STORAGE=/opt/app/storage
+MOKELUMNE_PUBLIC_URL=http://localhost:8080/


MOKELUMNE_PUBLIC_URL is set to http://localhost:8080/, but public_path_to_url() currently concatenates this with the public storage relative path (which is {dag_id}/{run_id}/...) and does not add MOKELUMNE_PUBLIC_URL_PREFIX. This will produce links that omit /public. Either set this env var to include the prefix (e.g. .../public/) or adjust URL construction to include MOKELUMNE_PUBLIC_URL_PREFIX.

Suggested change

MOKELUMNE_PUBLIC_URL=http://localhost:8080/

MOKELUMNE_PUBLIC_URL=http://localhost:8080/public/

- Adds an AirFlow plugin that serves files from `/public/{dag_id}/{run_id}`. - Adds Plugins.View permissions to the existing User role. This is necessary for Users to be able to view the routes provided by the plugin. We should be more deliberate about this. - To work with AirFlow's plugin templating mechanism, changes the local storage path for existing DAGs to be `/public/{dag_id}/{run_id}`. The root path and URL path remain configurable via environment variables.

anarchivist

Generally looks good. Given that Dag read permissions are required to view the files (since they're now served through Airflow's apiserver) I'm not sure that "public" is the right nomenclature here. Not sure if that's a blocker as it's a big nit to pick.

anarchivist · 2026-04-29T02:57:32Z

+    url_base = os.environ.get("MOKELUMNE_PUBLIC_URL", "http://localhost:8080/public/")
    return f"{url_base}{str(location)}"


What Copilot is suggesting is excessive. Try this (presuming an from urllib.parse import urljoin added at the top):

Suggested change

url_base = os.environ.get("MOKELUMNE_PUBLIC_URL", "http://localhost:8080/public/")

return f"{url_base}{str(location)}"

url_base = os.environ.get("MOKELUMNE_PUBLIC_URL", "http://localhost:8080/public/")

return urljoin(url_base, str(location))

anarchivist · 2026-04-29T03:00:38Z

+
+Serves files from `/{public_dir}/{dag_id}/{run_id}`
+
+Users must have permissions to view the given DAG in order to view its files.


This makes me wonder if /public and similarly named methods are the right naming convention here. If users have permissions to view the given Dag, then it's not truly public, right?

anarchivist

r+ with a clean rebase and tests passing. just trying to clarify the multiple env variables that seem to have the same value.

anarchivist · 2026-04-30T18:49:06Z

+FILESERVER_FORCE_PLAINTEXT=text/csv
+FILESERVER_ROOT=/opt/airflow/storage
+FILESERVER_URL_PREFIX=/storage
+


is this redundant to FILESERVER_ROOT? (might just be alittle slow this morning)

danschmidt5189 force-pushed the AP-660 branch from e6be80a to e89ca23 Compare April 21, 2026 19:55

danschmidt5189 requested a review from Copilot April 22, 2026 18:18

Copilot started reviewing on behalf of danschmidt5189 April 22, 2026 18:18 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

danschmidt5189 force-pushed the AP-660 branch 2 times, most recently from d4292a7 to 1824f70 Compare April 24, 2026 18:19

danschmidt5189 commented Apr 24, 2026

View reviewed changes

danschmidt5189 force-pushed the AP-660 branch from 0ad73eb to 16bba4b Compare April 24, 2026 19:16

danschmidt5189 commented Apr 24, 2026

View reviewed changes

Comment thread plugins/oidc_security_manager.py

danschmidt5189 force-pushed the AP-660 branch from a7b3080 to 660f8b4 Compare April 24, 2026 21:26

danschmidt5189 marked this pull request as ready for review April 24, 2026 21:31

danschmidt5189 requested review from anarchivist, Copilot and jason-raitz April 24, 2026 21:31

Copilot started reviewing on behalf of danschmidt5189 April 24, 2026 21:31 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

danschmidt5189 added 5 commits April 28, 2026 09:28

Run Actions tests in the api service so files are present

a65b7a6

Use a public volume in CI so files are shared across tests

b8ff8a0

chown the public volume to airflow

3b14341

Run tests as root just to see...

d163a09

danschmidt5189 force-pushed the AP-660 branch from 888bf69 to d163a09 Compare April 28, 2026 16:29

anarchivist reviewed Apr 29, 2026

View reviewed changes

Refactor into "static files" plugin

1dabffe

anarchivist approved these changes Apr 30, 2026

View reviewed changes

-    def generate_id(dag: DAG, run_id: str) -> str:
-        """Creates the local storage path for the current DAG run"""
-        path = public_dir() / dag.dag_id / run_id
+    def generate_id() -> str:
+        """Creates the local storage path for the current DAG run"""
+        context = get_current_context()
+        dag_id = context["dag"].dag_id
+        run_id = context["run_id"]
+        path = public_dir() / dag_id / run_id

		url_base = os.environ.get("MOKELUMNE_PUBLIC_URL", "http://localhost:8080/public/")
		return f"{url_base}{str(location)}"

-    url_base = os.environ.get("MOKELUMNE_PUBLIC_URL", "http://localhost:8080/public/")
-    return f"{url_base}{str(location)}"
+    url_base = os.environ.get("MOKELUMNE_PUBLIC_URL", "http://localhost:8080/")
+    url_prefix = os.environ.get("MOKELUMNE_PUBLIC_URL_PREFIX", "/public")
+    normalized_base = url_base.rstrip("/")
+    normalized_prefix = url_prefix.strip("/")
+    normalized_location = location.as_posix().strip("/")
+    url_parts = [normalized_base]
+    if normalized_prefix:
+        url_parts.append(normalized_prefix)
+    if normalized_location:
+        url_parts.append(normalized_location)
+    return "/".join(url_parts)

	MOKELUMNE_PUBLIC_STORAGE=/opt/app/storage
	MOKELUMNE_PUBLIC_STORAGE=/opt/airflow/public

	MOKELUMNE_PUBLIC_URL=http://localhost:8080/
	MOKELUMNE_PUBLIC_URL=http://localhost:8080/public/


		Serves files from `/{public_dir}/{dag_id}/{run_id}`

		Users must have permissions to view the given DAG in order to view its files.

Conversation

danschmidt5189 commented Apr 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Adds StaticFiles Plugin

Permissions

Download vs. Inline Files

Screenshots

Viewing the index.html

Viewing a CSV

When unauthenticated / unauthorized

When the DAG run directory is missing / empty

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danschmidt5189 Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

anarchivist Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

anarchivist left a comment

Choose a reason for hiding this comment

Uh oh!

anarchivist Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

anarchivist Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

anarchivist left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anarchivist Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

danschmidt5189 commented Apr 18, 2026 •

edited

Loading

anarchivist left a comment •

edited

Loading