Repository navigation
Conversation
elkampu
left a comment
There was a problem hiding this comment.
Thanks for taking this on—the download action, NDJSON content type, and streaming approach are a good start. I found two blocking issues that need to be addressed before this can merge:
-
The app writes and reads the activity log as
events.jsonlinFrameCatalogService(write, read), but the new endpoint looks forevents.jsonand returns a.jsonattachment (lines 139–160). Real session downloads will therefore return 404. Please useevents.jsonlfor both the stored path and download filename. -
The patch replaces the existing
GET /api/sessions/{id}/eventsendpoint (original mapping) with/events/download(new mapping). That unintentionally removes the existing JSON API and itslimitbehavior. Please keep the original route unchanged and register the download route alongside it.
The success test currently writes events.json directly (lines 45–49), so it mirrors the implementation typo instead of exercising production behavior. Please seed the log through IFrameCatalogService.AppendEventAsync (or at minimum write the actual events.jsonl), assert a .jsonl attachment name, and add a regression test confirming the original /events endpoint still works.
Once those are fixed, I will be happy to re-review. Thank you for the contribution!
|
Hi @elkampu, just following up on the requested changes. The fixes have been pushed, all 48 tests pass locally, and the PR is ready for re-review. The GitHub Actions workflows are also still awaiting maintainer approval. When you have a chance, could you please take another look and approve the workflows? Thank you! |
elkampu
left a comment
There was a problem hiding this comment.
Re-review complete. The endpoint now preserves the existing event API, streams the actual events.jsonl log with the correct content type and attachment name, and covers the regression path. Clean Release build, all 48 tests, and formatting checks pass locally. Thank you for the careful follow-up.

What changed
Closes #9
Verification
dotnet testpassesdotnet format --verify-no-changespassesSecurity impact
The endpoint does not accept a user-controlled filename or path. It resolves the validated session directory through ICaptureSessionService and only accesses the fixed events.json file inside that directory.
The endpoint returns 404 Not Found when the session or file is unavailable. No changes were made to URL validation, network access, credentials, FFmpeg execution, or the application's existing access-control defaults.