Skip to content

feat: expose the response's Content-Type on FileData - #198

Merged
mmktomato merged 2 commits into
masterfrom
feat/file-data-content-type
Sep 1, 2026
Merged

mmktomato merged 2 commits into
masterfrom
feat/file-data-content-type

Conversation

@katayama8000

Copy link
Copy Markdown
Collaborator

Closes #197.

Based on #196, not master — both change parseFileData, and stacking keeps this diff to the three lines that are actually about Content-Type. Retarget to master once #196 lands.

The problem

FileData carried body, url and filename. The response's Content-Type was read past and discarded, so a caller that needed the media type of a download had to re-derive it from the filename extension.

Backlog does send a correct one. Measured against a live space:

uploaded as Content-Type
shot.png image/png
図面.png image/png
data.csv text/csv
shot.jpg (bytes are a PNG) image/jpeg

nulab/backlog-mcp-server#187 carries a hand-written MIME_TYPES table for exactly this, returning application/octet-stream for any extension not in it — while the server's own answer was on the response.

The change

const contentType = response.headers.get("Content-Type") ?? "";

added to both halves of the union. The browser half could have gone without it, since blob() carries the same value in its type, but a field only one branch has is a field callers have to guard before reading. "" when the header is absent, matching filename.

Not solved, worth knowing

Backlog derives Content-Type from the extension too — the shot.jpg row above is a PNG that Backlog labels image/jpeg. So this is the server's claim, not a sniff. It is still a better source than a caller repeating the same guess with a smaller table, but a consumer that must be right about the format still has to check the bytes.

Verified

Three tests, all failing without the change: the two content types above, and "" when the header is absent. 62 tests, oxlint, oxfmt --check, tsc --noEmit and npm run build all clean.

🤖 Generated with Claude Code

`parseFileData` read one header and dropped the rest, so a caller that needed
the media type of a download had to re-derive it from the filename extension
— lossy for anything not in its table, and wrong for a file saved under the
wrong extension. Backlog does send a correct `Content-Type`; it was being
thrown away.

Added to both halves of the union, so `FileData.contentType` is reachable
without narrowing first. The browser half could have gone without it —
`blob()` carries the same value in its `type` — but a field only one branch
has is a field callers have to guard.

`""` when the header is absent, matching how `filename` already behaves.

Closes #197

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@katayama8000

Copy link
Copy Markdown
Collaborator Author

Two things from re-reading this, one measured and one I should have put in the description.

Content-Type carries no parameters, so the value is directly comparable

I wrote the field as "the response's Content-Type" without checking whether Backlog appends a charset, which would make contentType === "text/csv" fail for a caller doing the obvious thing. Measured on four more attachments:

uploaded as Content-Type
note.txt text/plain
page.html text/html
d.json application/json
blob.bin application/octet-stream

Bare media types, no charset even on text/*. Nothing to change — recording it because the field is only useful as-is if that holds.

This is a type-level breaking change, which the description does not say

contentType is required on both interfaces, so anything that constructs a FileData — a test double, a fake, a hand-built fixture — stops compiling until it adds the field. Reading is unaffected.

I argued for putting it on both halves of the union so callers do not have to narrow before reading it, and I still think that is right, but the cost is this, and I left it out. The alternative is contentType?: string, which avoids the break and hands every caller a guard for a value the library always produces — worse, I think, for a 0.x library where this is the moment to get the shape right. Happy to switch if you would rather not take the break.

@katayama8000

Copy link
Copy Markdown
Collaborator Author

Closing a gap in how I verified this, prompted by the question of whether the header is really there.

The earlier measurement did not test the code path this PR changes. I read the header with curl -D -, which does not follow redirects, while parseFileData reads response.headers from fetch, which does. Had the attachment endpoint redirected to object storage, curl would have shown me Backlog's response and the client would have been reading a different one. I was inferring from a neighbouring observation, not from the thing itself.

Checked, and it does not redirect — HTTP/2 200 direct, num_redirects: 0 with -L. So the two views coincide, but that was luck rather than method.

Then ran this branch against a live space, which is the check I should have run first:

shot.png
   FileData keys : ["body","url","filename","contentType"]
   filename      : "shot.png"
   contentType   : "image/png"
data.csv
   FileData keys : ["body","url","filename","contentType"]
   filename      : "data.csv"
   contentType   : "text/csv"

Backlog#getIssueAttachment with the change applied, no mocks anywhere. The header reaches the caller through fetch, and contentType is populated from it. Test issues cleaned up.

Base automatically changed from fix/content-disposition-filename to master September 1, 2026 08:02

@mmktomato mmktomato left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@katayama8000
I added a comment 🙏

Comment thread test/test.ts Outdated
expect(data).toHaveProperty("filename", expected);
});

// Backlog does send a correct Content-Type; before this it was read off the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't make sense after merge.
(Same here: #196 (comment) )

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, and it is the same mistake you already caught on #196 — I fixed it there and then wrote it again here. Fixed in the head commit.

Replaced with what the tests actually pin: Backlog sends a bare media type with no charset parameter, so the value compares directly. That holds whenever someone reads it, rather than only while this PR is open.

I also swept both branches for anything else phrased against the previous behaviour. The remaining "before"s are positional — "sanitise it before using it as a path", "before the unquoted form" — not temporal.

62 tests, lint, format and tsc still pass.

"before this it was read off the response and thrown away" is the same mistake
already corrected in #196: it describes the change, and after the merge there
is no before for a reader to compare against.

Replaced with the fact the tests are actually pinning — Backlog sends a bare
media type, no charset parameter, so the value compares directly. That is
worth knowing whenever it is read, not just while reviewing this.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@katayama8000

Copy link
Copy Markdown
Collaborator Author

@mmktomato fixed

@mmktomato mmktomato left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@katayama8000
LGTM!!!

@mmktomato
mmktomato merged commit 7f11c3a into master Sep 1, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

parseFileData drops the Content-Type Backlog sends, so callers have to guess it from the filename

2 participants