ref: extract serve_file_variant for static file handlers

[tipogi]

Closes #331

Summary

Extracts shared static file serving logic into serve_file_variant and completes the remaining polish from the avatar endpoint work in #322.

Checklist status

Item	Status
OpenAPI Axum handler (unified pattern)	Already was Done
Mock user + avatar file in repo	Already was Done
Integration test	Already was Done
Reuse #165 file-processing code	Done in that PR
/avatar benchmark	Done in that PR

Changes

• Add serve_file_variant helper in serve_dir.rs for variant generation, disk serving, and cache headers
• Refactor avatar.rs and files.rs to use the shared helper
• files.rs keeps variant validation and optional ?dl= download header

Pre-submission Checklist

• I manually reviewed the PR
• I asked one or more LLMs to review the PR
• I asked one or more LLMs to check if this PR can be simplified ¹
• If appropriate, I added tests for the changes in this PR
• If appropriate, I added performance benchmarks for the APIs added in this PR ²

Footnotes

1. Sample prompt: "Can this be simplified? Can the code, comments, or logic introduced by these changes be simplfied, clarified or otherwise made more terse, concise and understandable, without affecting functionality?" ↩

2. cargo bench -p nexus-webapi ↩

[greptile-apps]

Greptile Summary

This PR extracts the repeated variant-generation + disk-serving + cache-header logic from avatar.rs and files.rs into a single serve_file_variant helper in serve_dir.rs, and adds a Criterion benchmark for the avatar handler with warm and cold (variant-missing) scenarios.

• serve_dir.rs gains serve_file_variant, which calls Blob::get_by_id to ensure the variant exists, delegates to PubkyServeDir::try_call to stream the file, then stamps Cache-Control: public, max-age=3600 and, when download=true, Content-Disposition: attachment.
• avatar.rs / files.rs are each reduced to a single serve_file_variant call; files.rs retains its variant-validation guard and ?dl= logic before the call.
• benches/avatar.rs benchmarks the full handler chain (no HTTP server) in both warm-cache and cold-cache states, using a TempDir-backed AppState.

Confidence Score: 4/5

Safe to merge; the refactor is mechanically correct and the two inline suggestions are polish items rather than blocking issues.

The owner_id parameter on serve_file_variant is redundant with file.owner_id, creating a latent mismatch risk, and the Content-Disposition filename is not RFC 6266-escaped. Both are edge-case quality issues rather than present-day breakage. The core extraction is sound and the benchmark is a useful addition.

nexus-webapi/src/routes/static/serve_dir.rs — the two inline findings live here.

Important Files Changed

Filename	Overview
nexus-webapi/src/routes/static/serve_dir.rs	Core change: adds the serve_file_variant helper that consolidates variant generation, disk serving, and cache/download headers. Minor design issue: the owner_id parameter is redundant with file.owner_id, and the filename in Content-Disposition is inserted as-is without RFC 6266 encoding.
nexus-webapi/src/routes/static/avatar.rs	Cleanly refactored to delegate to serve_file_variant; the switch from user_id to owner_id (extracted from the file URI) for the disk path is actually more correct for cross-user avatar scenarios.
nexus-webapi/src/routes/static/files.rs	Simplified to delegate to serve_file_variant; variant validation and ?dl= download logic are preserved correctly.
nexus-webapi/benches/avatar.rs	New benchmark for the avatar handler with warm and cold (variant regeneration) scenarios. Correctly acknowledges the OnceLock constraint.
nexus-webapi/Cargo.toml	Adds http-body-util as a dev-dependency for BodyExt in the benchmark, and registers the new avatar bench target.
nexus-common/src/models/user/mod.rs	Re-exports CACHE_USER_RECOMMENDED_KEY_PARTS alongside existing stream exports; unrelated to the main refactor.

Sequence Diagram

sequenceDiagram
    participant C as Client
    participant AH as user_avatar_handler
    participant FH as static_files_handler
    participant SFV as serve_file_variant
    participant Blob as Blob get_by_id
    participant PSD as PubkyServeDir

    C->>AH: GET /avatar/:user_id
    AH->>AH: Lookup UserDetails and FileDetails
    AH->>SFV: serve_file_variant(req, file, owner_id, Small, path, false)
    SFV->>Blob: ensure/create variant on disk
    Blob-->>SFV: content_type
    SFV->>PSD: try_call(req, disk path, content_type, path)
    PSD-->>SFV: Response
    SFV->>SFV: set Cache-Control header
    SFV-->>AH: Response
    AH-->>C: 200 + avatar bytes

    C->>FH: GET /static/files/:owner/:file/:variant
    FH->>FH: Lookup FileDetails and validate variant
    FH->>SFV: serve_file_variant(req, file, owner_id, variant, path, dl)
    SFV->>Blob: ensure/create variant on disk
    Blob-->>SFV: content_type
    SFV->>PSD: try_call(req, disk path, content_type, path)
    PSD-->>SFV: Response
    SFV->>SFV: set Cache-Control header
    SFV->>SFV: optionally set Content-Disposition attachment
    SFV-->>FH: Response
    FH-->>C: 200 + file bytes

Loading

_{Reviews (1): Last reviewed commit: "fix: serve avatar files from URI owner" | Re-trigger Greptile}

[greptile-apps]

The owner_id parameter is already available as file.owner_id, so callers must keep both in sync manually. Since Blob::get_by_id looks up the file with file.owner_id, using a separately-passed owner_id to build the disk path creates a potential divergence — if they ever differ (e.g. from a stale cache entry), the variant is written under file.owner_id but served from a different path. Removing the extra parameter eliminates that class of bug entirely.

Suggested change

	pub async fn serve_file_variant(
	request: Request<Body>,
	file: &FileDetails,
	owner_id: &str,
	variant: &FileVariant,
	files_path: PathBuf,
	download: bool,
	) -> Result<Response<ServeFileSystemResponseBody>> {
	let content_type = Blob::get_by_id(file, variant, files_path.clone()).await?;
	let disk_path = format!("/{owner_id}/{}/{variant}", file.id);
	pub async fn serve_file_variant(
	request: Request<Body>,
	file: &FileDetails,
	variant: &FileVariant,
	files_path: PathBuf,
	download: bool,
	) -> Result<Response<ServeFileSystemResponseBody>> {
	let content_type = Blob::get_by_id(file, variant, files_path.clone()).await?;
	let disk_path = format!("/{}/{}/{variant}", file.owner_id, file.id);

[greptile-apps]

Unescaped filename in Content-Disposition header

The filename value is interpolated directly into the attachment; filename="..." string without any RFC 6266 escaping. A filename containing " or \ (e.g. my"file.txt) produces a structurally malformed header that clients will interpret incorrectly — the closing quote appears early and the rest of the name leaks into the directive. HeaderValue::from_str will accept this silently because those bytes are in the valid visible-ASCII range. The safe fix is to percent-encode the filename or use the filename* (RFC 5987) form: attachment; filename*=UTF-8''<percent-encoded>.

[tipogi]

check pubky/pubky-app-specs#132

[ok300]

Looks good, just a few NITs that would simplify the code but keep the logic unchanged.

[ok300]

This fix (user_id → file.owner_id) is a behavior change.

The tests in tests/user/avatar.rs and the new benchmark both use the same-owner case (USER_PUBKY owns FILE_ID), so the exact scenario the fix repairs is untested.

NIT: would it make sense to add a test? For example, one where a user's image points to a file owned by a different PK would lock in the fix and prevent regression.