Improve handling of CardJson accept header when non-card paths are specified #3892
Conversation
lukemelia
force-pushed
the
card-json-cleanup
branch
from
e484998 to
6c5773e
Compare
Preview deployments |
There was a problem hiding this comment.
Pull request overview
This pull request improves the handling of CardJson accept headers when non-card paths (plain files) are specified. The implementation moves from a centralized pre-routing check to per-handler checks, making the error handling more explicit and appropriate.
Changes:
- Introduced a new
nonJsonFileExistshelper method to determine if a path refers to a non-JSON-backed file - Added specific checks in GET, PATCH, and DELETE handlers to reject CardJson requests for plain files with a 415 status
- Consolidated and reorganized file URL tests for better clarity
- Improved test adapter's
existsmethod to not auto-create directories - Fixed test setup ordering to start Matrix after realm initialization
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime-common/error.ts | Added notAcceptable function to return 415 errors for unsupported media type requests |
| packages/runtime-common/realm.ts | Removed centralized Accept header checks, added nonJsonFileExists helper, and integrated checks into GET/PATCH/DELETE handlers |
| packages/realm-server/tests/card-endpoints-test.ts | Consolidated scattered file URL tests into a single module and updated test descriptions |
| packages/host/tests/helpers/index.gts | Reordered test setup to start Matrix after realm initialization |
| packages/host/tests/helpers/adapter.ts | Simplified exists method and added #traverseExisting to avoid auto-creating directories |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I noticed we got rid of the public writable tests--is that because that's an unrealistic scenario? |
| if (startMatrix) { | ||
| await mockMatrixUtils.start(); | ||
| } |
There was a problem hiding this comment.
what was the reason for moving this? was it causing test flakiness?
There was a problem hiding this comment.
I think AI thought it would help. I doubt it matters one way or the other.
The only thing in that module was a test for a non-card URL, and I reorganized to put all the non-card URLs together under one module. |
No description provided.