Repository navigation
Conversation
 [Low impact] The PR appears safe to merge; no blocking code issues were found. SummaryFixes nested Node Buffer fields losing their parent names during automatic URL-encoded serialization.
Reviews (6) · Last reviewed commit: "test(form): cover empty and nested Buffe..." · Reviewed by Greptile |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply to a comment to ask cubic a question or push back. It learns from your replies.
Re-trigger cubic
d0c8f53 to
2eab971
Compare
jasonsaayman
added
the
ai-assisted
jasonsaayman
left a comment
jasonsaayman
left a comment
There was a problem hiding this comment.
PR #11290 - fix(form): preserve nested URL-encoded Buffer paths
Repo: axios/axios Author: @imharjot Base: v1.x Files: 4 **+101/-2
CI: Unavailable: no checks returned; no build/test workflow result is present in this snapshot. Mergeability: conflicting. Reviewed head: 2eab971f49cb0d5a686c4b30c61cb4411132b88f.
Repo Profile
Axios is an npm JavaScript HTTP client for browsers and Node, with XHR/Fetch/HTTP adapters, default and named exports, interceptors, headers/errors, and paired ESM/CJS declarations. The target uses Vitest plus browser and package/module tests; v1 packaging covers older Node CJS runtimes as well as current ESM, Bun and Deno. Contributions preserve transport/security boundaries, add regression coverage and prerelease records, and defer unreleased user docs to PRE_RELEASE_DOCS.md. This review uses the actual target-branch guidance and files, not the separate local v0 checkout.
Verdict
Request changes — No concrete source defect found in this review; the missing latest-head validation and merge conflicts prevent merge approval.
Posted as a comment review for the validation/integration hold; no formal code-change request is implied beyond the specific items below.
Change Explanation And User Impact
The URL-encoded serializer special-cased Buffer values by appending only their leaf key, bypassing the normal path renderer. Nested objects therefore lost their ancestry and could collide. This head keeps base64 encoding but sends the string through defaultVisitor with the original path and options. Executed with {a:{file:Buffer.from("abc")}, b:{file:Buffer.from("xyz")}}: the target produced file=YWJj&file=eHl6; this head produced a%5Bfile%5D=YWJj&b%5Bfile%5D=eHl6. Root Buffers keep their base64 representation, while array/dot paths follow the ordinary serializer.
Validation scope: source/diff and regression-test review. Examples explicitly marked executed used isolated source snapshots on Node 24.18.0 with the workspace dependency installation; these were focused checks, not a clean install, full CI matrix, or full test-suite run. Author-reported test results were not treated as independently executed.
Competing PRs
Search: axios/axios open PRs, including drafts; full open-list scan plus toFormData in:title,body, URL encoded Buffer in:title,body, 11289 in:title,body. Complementary searches were run across related APIs/issues; no query reached the 100-result cap and no search failed. The five explicitly excluded PRs were not reviewed or modified in this batch.
#11317 and #11290 make the same nested Buffer path correction, but #11317 has useful empty/binary/nested-array coverage and #11290 has an end-to-end custom-visitor override case. Keep both pending consolidation with attribution. #11264 fixes typed-array bytes, #11256 normalizes nested key segments, and #11279 isolates recursive visitor state; these are complementary.
Preferred PR: No final winner until the useful test cases from both equivalent implementations are consolidated.
Unique contributions: Credit #11317 for empty/binary/nested Buffer-array coverage and #11290 for the end-to-end custom-visitor override regression.
Action: Keep both open pending consolidation; neither is discarded merely because its runtime hunk is equivalent.
Project Fit
North star: Reliable HTTP transport and configuration across supported JavaScript environments, without embedding application-specific policy.
Benefits: Preserves nested Buffer paths and custom-visitor behavior.
Tradeoffs: Reusing defaultVisitor removes a separate key renderer and its drift, but visitor overrides must remain in control.
Fit: Strong — The underlying problem belongs in the transport, serializer, declarations or their maintenance tests.
Adversarial Review
Strongest objection: Reusing defaultVisitor removes a separate key renderer and its drift, but visitor overrides must remain in control. Latest-head integration evidence is still needed.
Abuse/failure modes checked: Inspected custom visitors, dots/index options, root/deep Buffers and real request regressions. Ran the nested collision example against both proposals.
Result: No concrete source defect established in the examined cases; validation and integration limits remain below.
Disposition
Continue
Address the specific blockers below; no unrelated scope or complementary PR should be closed.
Semver
patch — Corrects existing behavior or strengthens internal tooling/tests without adding a public API.
Breaking Changes
None intended beyond the bug correction described above; no public API removal or new runtime requirement is introduced by this diff.
Documentation
PRE_RELEASE_CHANGELOG.md records the change. PRE_RELEASE_DOCS.md also captures the relevant deferred documentation.
Performance And Regression Risk
Hot path touched: Yes — request/response processing or its shared helpers.
Performance concern: No unbounded new work identified in the changed path. Reusing defaultVisitor removes a separate key renderer and its drift, but visitor overrides must remain in control.
Regression risk: low — the changed boundary needs regression and integration coverage, particularly after any rebase.
Evidence requested: The latest-head checks listed below; no extra benchmark requested for this scoped change.
Blocking Issues
- Latest-head build/test validation is unavailable in the returned GitHub checks. Run or attach the repository build, relevant regression suite and required package/type/browser checks before merge; review/security bots alone do not establish that they pass. This is a validation hold, not an invented source defect.
- GitHub reports merge conflicts with the current target. Resolve them and re-run relevant checks; the reviewed source snapshot is not a tested merge result.
Non-Blocking Suggestions
- Consolidate the two proposals with attribution: preserve #11317's empty/binary and nested Buffer-array cases and #11290's end-to-end custom-visitor override test.
Looks Good
- Inspected custom visitors, dots/index options, root/deep Buffers and real request regressions. Ran the nested collision example against both proposals.
2eab971 to
43e54ab
Compare
|
Thanks for the detailed review. I've rebased onto e6b79d9 and added HTTP coverage for the empty/binary and nested Buffer-array cases from @syedryaaan's #11317, with attribution in the commit and description. The end-to-end custom-visitor case is retained. Both PRs remain open as requested. The focused suite passes all 35 tests. The packed CJS/ESM module and type suites pass (8 and 9 tests), as do the CJS/ESM smoke suites (72 and 78) and Chromium/WebKit (710 passed, two skipped). The complete unit rerun has 1,706 passes and the same inherited Symbol.iterator header failure as unchanged upstream (1,699 passes). Firefox still exits before tests with 'Could not find profile folder', even after reinstalling and retrying with a writable workspace temp directory. Those limits are recorded in the description. The new workflows require maintainer approval; could you approve them so the remote matrix can check this head? |
|
The two remaining local validation blockers are resolved at 43e54ab. The full unit suite passes all 1,707 tests on Node 24.22.1 and 26.12.1. Chromium, Firefox and WebKit pass all 1,065 tests with three existing skips on both versions. Lint, build, and the packed CJS/ESM module, type and smoke suites also pass. I traced the header failure to native Fetch on Node 24.11.1: it reproduces without Axios and passes on the current Node versions. Firefox needed a separate disposable test home on macOS ( I've updated the description with the commands, runtime versions and results. This head is still based on current v1.x. Could a maintainer approve the pending workflows so the remote matrix can run? |
Add end-to-end HTTP regression test for custom form visitor overrides adapted from PR axios#11290 by @imharjot.
43e54ab to
82583b4
Compare
|
Rebased onto current v1.x ( Local checks pass: lint/build, 1,707 unit tests, 1,065 browser tests with three existing skips, and the installed-tarball CJS/ESM module, type and smoke suites. The five failing upstream Buffer cases still pass with the fix. An initial Firefox polling failure did not recur in complete upstream and patched reruns; that result is recorded in the description, with no assertion or timeout changes. GitHub now reports no merge conflicts. The four new test/build/security workflows still require maintainer approval. |
|
Thanks for the update. Since the local validation checks are passing and GitHub reports no merge conflicts, could a maintainer please review and approve the four pending test/build/security workflows? Once those workflows are approved and complete successfully, the PR should be ready for the final merge review, assuming no other issues are identified. |
|
Thanks for the update. Since local validation checks (npm run lint, npm run build, and npm run test:vitest:unit) are passing and GitHub reports no merge conflicts, could a maintainer please review and approve the pending test/build/security workflows? Once those workflows are approved and complete successfully, the PR should be ready for final merge review, assuming no other issues are identified. (Note: If not already added, please also ensure the ai-assisted label is applied per repo guidelines.) |
|
Hi maintainers, I’d like to take ownership of investigating and addressing this issue. I’m particularly interested in the edge cases around nested URL-encoded serialization in Node.js, especially preserving My proposed approach is to:
I have experience working through complex debugging and regression-testing workflows, and I’m prepared to investigate this systematically rather than introduce a narrowly scoped fix that misses related cases. Would you be open to assigning this issue to me or allowing me to submit a PR for review? I’ll coordinate with any existing work on the same issue to avoid duplication and keep the proposed change focused. Thank you for your time and for maintaining Axios. |
Add HTTP coverage for the empty/binary Buffer and nested Buffer-array cases contributed in axios#11317 by @syedryaaan, retaining the custom-visitor regression.
82583b4 to
c2d5925
Compare
Summary
Nested Buffer values lose their parent paths when Axios automatically serializes a Node request as URL-encoded data. Pass the existing base64 string through the default form visitor so it keeps the same bracket, dot and array-index formatting as other fields.
Linked issue
Fixes #11289. Related: #11317.
Changes
v1.xat3226b1d2826647f83f6eaee283fc61877196b512. Kept the Buffer entries alongside the newer upstream prerelease notes. The runtime fix and all seven regression cases are unchanged.Validation
Checked on 3 November 2026 on macOS with Node 26.12.1 (head
c2d5925c7181b80befc4fe745f8600240477c610).npm ci --ignore-scripts,npm run lint,npm run build, and Playwright installation passed.npm run test:vitest:unit -- --maxWorkers=2: 1712 passed (1712).npm run test:vitest:browser:headless -- --maxWorkers=1 --no-file-parallelism: 1065 passed | 3 skipped (1068) across Chromium, Firefox and WebKit. I stopped the first attempt after it ran unusually long without reporting results. The serial run kept the same assertions and timeouts.git diff --checkpassed.GitHub Actions results are separate from these local checks. Local checks do not cover the entire remote runtime matrix.
Compatibility and documentation
Patch bug fix limited to the existing Node Buffer branch. No public API or declaration changes, so new ESM/CJS type declarations are not needed; the existing package/type suites were still checked. Base64 values, root field names, ordinary nested values and custom visitors retain their behavior. The prerelease changelog entry and deferred documentation note are included. No dependency, workflow, generated bundle or release-owned documentation changes are part of the PR.
Checklist
The repository's required
ai-assistedlabel is present.Summary by cubic
Description
Fixes nested Buffer values in Node's automatic URL-encoded serialization so they no longer lose their parent paths, closing #11289. Previously
{ first: { file: ... }, second: { file: ... } }was sent as twofilefields; now it is sent asfirst[file]andsecond[file], including dot notation whenformSerializer.dotsis enabled.The fix passes the existing base64 string to the default form visitor, which already formats parent paths, so root encoding, non-Buffer fields, arrays, and custom visitors behave as before.
Docs
Records the fix in
PRE_RELEASE_CHANGELOG.mdand adds a pending entry inPRE_RELEASE_DOCS.md; suggests updating the README's automatic serialization section and the corresponding docs page.Testing
Adds seven regression tests in
tests/unit/helpers/toURLEncodedForm.test.jsrun throughaxios.postagainst a real HTTP echo server. They cover root encoding, nested and deep parent paths, arrays, dot notation, empty and binary Buffers, and custom visitors; the empty/binary and nested array cases came from #11317. The full unit suite still has one pre-existing failure in the fetch adapter unrelated to this change.Semantic version impact
Patch bug fix scoped to the Node Buffer branch; the public API and declarations are unchanged, so no breaking or minor version change.
Written for commit c2d5925. Summary will update on new commits.