Skip to content

chore(deps): drop unused content-disposition dependency - #4980

Merged
aglinxinyuan merged 3 commits into
apache:mainfrom
aglinxinyuan:chore/drop-content-disposition
May 8, 2026
Merged

chore(deps): drop unused content-disposition dependency#4980
aglinxinyuan merged 3 commits into
apache:mainfrom
aglinxinyuan:chore/drop-content-disposition

Conversation

@aglinxinyuan

@aglinxinyuan aglinxinyuan commented May 7, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

download.service.ts carried a top-level var contentDisposition = require("content-disposition") that was never read or called. Removing the dead require lets us drop the content-disposition runtime dep and its @types/content-disposition companion, plus the now-stale bundled-license entries for the transitive packages that fell out with it (base64-js, buffer, ieee754, path-browserify, safe-buffer).

Any related issues, documentation, discussions?

Closes #4979.

How was this PR tested?

  • yarn install succeeds and rewrites yarn.lock (transitive entries fall away)
  • yarn build exits 0
  • yarn test stays at 63 / 269 parity
  • CI's check_binary_deps.py --ignore-transitive-version npm …/3rdpartylicenses.json will be the final arbiter on the LICENSE-binary cleanup

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.7)

`download.service.ts` had a top-level
`var contentDisposition = require("content-disposition")` that was never read
or called. Removed the dead require, then dropped both `content-disposition`
and `@types/content-disposition` from package.json (and the bundled-license
roll-up). yarn.lock loses the transitive entries for free.

Closes apache#4979.
Copilot AI review requested due to automatic review settings May 7, 2026 08:18
@github-actions github-actions Bot added dependencies Pull requests that update a dependency file frontend Changes related to the frontend GUI labels May 7, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Removes an unused content-disposition require from the frontend download service and cleans up the corresponding direct dependencies and licensing manifests to keep the frontend dependency set lean.

Changes:

  • Removed dead require("content-disposition") from download.service.ts.
  • Dropped content-disposition and @types/content-disposition from frontend/package.json.
  • Updated frontend/yarn.lock and frontend/LICENSE-binary to reflect the dependency cleanup.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File Description
frontend/src/app/dashboard/service/user/download/download.service.ts Removes unused content-disposition require.
frontend/package.json Deletes direct runtime + typings dependencies for content-disposition.
frontend/LICENSE-binary Removes content-disposition@0.5.4 entry from the bundled license manifest.
frontend/yarn.lock Regenerated lockfile reflecting dependency graph updates after removal.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@aglinxinyuan
aglinxinyuan requested a review from bobbai00 May 7, 2026 08:29
…nsitives

The 5 packages flagged by check_binary_deps.py (base64-js, buffer, ieee754,
path-browserify, safe-buffer) were only bundled because content-disposition
pulled them in transitively. Once content-disposition itself is gone, they
fall out of the webpack bundle and the LICENSE-binary entries are stale.
@codecov-commenter

codecov-commenter commented May 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 42.73%. Comparing base (ad0def8) to head (14c89b5).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4980      +/-   ##
============================================
- Coverage     42.73%   42.73%   -0.01%     
+ Complexity     2187     2186       -1     
============================================
  Files          1031     1031              
  Lines         38153    38152       -1     
  Branches       4004     4004              
============================================
- Hits          16305    16303       -2     
  Misses        20831    20831              
- Partials       1017     1018       +1     
Flag Coverage Δ *Carryforward flag
access-control-service 39.53% <ø> (ø) Carriedforward from 8748b14
agent-service 33.72% <ø> (ø) Carriedforward from 8748b14
amber 43.22% <ø> (-0.01%) ⬇️ Carriedforward from 8748b14
computing-unit-managing-service 0.00% <ø> (ø) Carriedforward from 8748b14
config-service 0.00% <ø> (ø) Carriedforward from 8748b14
file-service 32.18% <ø> (ø) Carriedforward from 8748b14
frontend 33.08% <ø> (-0.01%) ⬇️
python 88.90% <ø> (ø) Carriedforward from 8748b14
workflow-compiling-service 47.72% <ø> (ø) Carriedforward from 8748b14

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aglinxinyuan
aglinxinyuan requested a review from Yicong-Huang May 8, 2026 01:53
@Yicong-Huang

Yicong-Huang commented May 8, 2026

Copy link
Copy Markdown
Contributor

@aglinxinyuan what was this used for? can we git blame to trace and report here?

@aglinxinyuan

Copy link
Copy Markdown
Contributor Author

@Yicong-Huang traced it — short version: it was wired up in March, then orphaned in October when downloads moved to the browser. The require line was missed during that cleanup.

When Commit What happened
2025-03-04 9ecd32c (#3241)Fix download operators result var contentDisposition = require("content-disposition") was added so a new saveBlobFile(response, defaultFileName) helper could parse the server's Content-Disposition header and pull filename= out of it. Used on the HttpClient blob path for single-operator result download.
2025-10-10 1317199 (#3728)switch workflow result downloads to use browser native downloads Local result download switched from HttpClient blob → standard HTML form submit, so the browser handles the download (and the filename) directly. saveBlobFile (the only contentDisposition.parse caller) was deleted; the bare require at the top of the file was left behind.

So it's been dead since Oct 10, 2025 — the package and its types just rode along until I noticed.

Relevant blame on main:

9ecd32ca6a  ali risheh   2025-03-04   var contentDisposition = require("content-disposition");

And the removed call site (added in 9ecd32c, gone in 1317199):

public saveBlobFile(response: any, defaultFileName: string): void {
  const dispositionHeader = response.headers.get("Content-Disposition");
  let fileName = defaultFileName;
  if (dispositionHeader) {
    const parsed = contentDisposition.parse(dispositionHeader);
    fileName = parsed.parameters.filename || defaultFileName;
  }
  ...
}

No other references to content-disposition / contentDisposition remain under frontend/src (grepped), so dropping the dep is safe.

@Yicong-Huang Yicong-Huang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok let's remove it

@aglinxinyuan
aglinxinyuan enabled auto-merge (squash) May 8, 2026 06:04
@aglinxinyuan
aglinxinyuan merged commit b4f3a41 into apache:main May 8, 2026
15 checks passed
yangzhang75 pushed a commit to yangzhang75/texera that referenced this pull request Jun 22, 2026
### What changes were proposed in this PR?

`download.service.ts` carried a top-level `var contentDisposition =
require("content-disposition")` that was never read or called. Removing
the dead `require` lets us drop the `content-disposition` runtime dep
and its `@types/content-disposition` companion, plus the now-stale
bundled-license entries for the transitive packages that fell out with
it (`base64-js`, `buffer`, `ieee754`, `path-browserify`, `safe-buffer`).

### Any related issues, documentation, discussions?

Closes apache#4979.

### How was this PR tested?

- `yarn install` succeeds and rewrites `yarn.lock` (transitive entries
fall away)
- `yarn build` exits 0
- `yarn test` stays at 63 / 269 parity
- CI's `check_binary_deps.py --ignore-transitive-version npm
…/3rdpartylicenses.json` will be the final arbiter on the LICENSE-binary
cleanup

### Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file frontend Changes related to the frontend GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Drop unused content-disposition dependency

4 participants