fix: derive fetch url truncation label - #1017
Conversation
No changeset foundThis PR does not add a changeset, so it will not appear in the changelog or trigger a release. If the change is user-facing, add one: pnpm changesetPick a bump (patch / minor / major) and write the changelog entry in our usual voice ("Added X... Thanks to @you. Closes #123."), then commit the generated If this PR is docs-only or a chore that needs no release note, you can ignore this - or run |
will-lamerton
left a comment
There was a problem hiding this comment.
Thanks for this. The instinct is right, the label shouldn't be hardcoded separately from the constant, but the chosen formatter changes the user-visible string in a way I don't think we want.
1. The rendered label becomes 97.7 KB
MAX_URL_CONTENT_BYTES is 100_000 (decimal, per the // ~100 KB comment), but formatSize divides by 1024, so the warning now reads ⚠ Content was truncated to 97.7 KB. Users never configured 97.7 KB, and the one-decimal figure implies a precision we don't have: the truncation at fetch-url.tsx:39 compares content.length (UTF-16 code units), not bytes.
Two options that both keep the value derived from the constant:
- Say what the code actually measures:
Content was truncated to ${MAX_URL_CONTENT_BYTES.toLocaleString()} characters. This also lines up with the handler's own note (original size was ${content.length} characters), which currently disagrees with the formatter's "KB" wording. - Or keep KB with the decimal divisor the constant was written in:
${MAX_URL_CONTENT_BYTES / 1000}KB, which renders the same100KBas today.
2. The updated test no longer asserts anything
const expectedLabel = `Content was truncated to ${formatSize(MAX_URL_CONTENT_BYTES)}`;
t.regex(output!, new RegExp(expectedLabel.replace('.', '\\.')));This builds the expectation from the same constant and the same function the source uses, so it passes for any value of either, including a broken formatSize. The old hardcoded assertion was the stronger test because it pinned the user-visible output. Please assert the literal expected string so a change to the constant surfaces as a failure someone has to consciously accept.
Also, String.replace with a string pattern only replaces the first match, so the escaping is incomplete by construction (it works here only because there's exactly one dot). Since no pattern matching is needed, t.true(output!.includes(expectedLabel)) is simpler and correct.
3. Missing changeset
The PR body has the changeset box checked, but the diff is only the two source files. This is a user-visible string change, so it needs one.
Happy to merge once the label stays round and honest about its unit, the test pins the literal output, and the changeset is added.
Description
Brief description of what this PR does
Fix the hardcoded
100KBtruncation warning infetch-urlby deriving the displayed size fromMAX_URL_CONTENT_BYTESusing the existingformatSizeutility. Also updates the related formatter test to verify the dynamic value.Changeset
pnpm changeset) describing this change for the changelogDocs-only or internal chores need no changeset (or run
pnpm changeset --emptyto note that intentionally).Testing
Automated Tests
.spec.ts/tsxfilespnpm test:allcompletes successfully)Manual Testing
Checklist