-
Notifications
You must be signed in to change notification settings - Fork 0
Implement HTTP caching for file downloads with If-Modified-Since #140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: HNygard <168380+HNygard@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds conditional HTTP caching to /file downloads using Last-Modified/If-Modified-Since so browsers can reuse cached email bodies and attachments.
Changes:
- Add timestamp-returning fetch helpers in
ThreadStorageManagerfor email bodies and attachments. - Set
Last-Modified+Cache-Controland return304 Not ModifiedwhenIf-Modified-Sinceindicates no change. - Enhance e2e test harness to send custom request headers and add coverage for 304 behavior (email body path).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| organizer/src/file.php | Adds Last-Modified/If-Modified-Since handling for email body + attachment responses. |
| organizer/src/class/ThreadStorageManager.php | Adds new DB accessors returning content plus timestamp metadata. |
| organizer/src/e2e-tests/pages/common/E2EPageTestCase.php | Adds support for passing custom headers through the e2e HTTP client. |
| organizer/src/e2e-tests/pages/FilePageTest.php | Adds assertions for caching headers and a 304 round-trip test for email body downloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…dd immutable, add attachment test Co-authored-by: HNygard <168380+HNygard@users.noreply.github.com>
Co-authored-by: HNygard <168380+HNygard@users.noreply.github.com>
Co-authored-by: HNygard <168380+HNygard@users.noreply.github.com>
HNygard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Merge and run the np run to see if this improves that performance
Implementation Plan for If-Modified-Since Caching
Summary
Successfully implemented HTTP caching for file downloads using If-Modified-Since/Last-Modified headers and addressed all PR feedback. The implementation:
✅ Minimal changes: Surgical modifications with no breaking changes
✅ Backward compatible: Existing methods unchanged, new methods added
✅ Standards compliant: Uses RFC 7231/7232 HTTP caching headers with immutable directive
✅ Well tested: Automated tests for both email body and attachment caching including 304 behavior
✅ Secure: Uses private caching, maintains authentication/authorization
✅ Efficient: 1 hour cache for email bodies, 1 year immutable cache for attachments
✅ Robust: Proper error handling with queryOneOrNone(), headers set after successful processing
✅ Clean code: Eliminated duplication with helper method for attachment mapping
Benefits:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.