Skip to content

fix(frontend): disable the per-cell result download when result export is switched off - #8539

Draft
yangzhang75 wants to merge 1 commit into
apache:mainfrom
yangzhang75:fix/cell-download-honours-export-switch
Draft

fix(frontend): disable the per-cell result download when result export is switched off#8539
yangzhang75 wants to merge 1 commit into
apache:mainfrom
yangzhang75:fix/cell-download-honours-export-switch

Conversation

@yangzhang75

@yangzhang75 yangzhang75 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

export-execution-result-enabled is a deployment switch (default false in gui.conf; the k8s chart turns it on). The top menu already disables its Export entry on that switch and the context menu hides its entry, but the download button on every result cell rendered regardless. Clicking it opened the export dialog, and every action in the dialog (export to local, save to a dataset, save to a newly created dataset) returned early from WorkflowResultExportService.performExport with no request and no message, after which the dialog closed. On a default local deployment this looks like a broken feature rather than a disabled one.

This PR binds the cell button's disabled state to the switch and explains it in the button's title ("Result export is disabled on this deployment"), the same way the menu entry is disabled. Nothing changes for deployments that have the switch on.

  • result-table-frame.component.ts: a public exportEnabled getter over GuiConfigService.
  • result-table-frame.component.html: [disabled] and [title] on the download button.
  • result-table-frame.component.scss: a disabled look. The stylesheet hardcodes the icon color and cursor, so without this the disabled button was indistinguishable from the live one.

The early return in performExport is left as is; with all three entry points now honouring the switch it is a last line of defence rather than a user-facing path.

Any related issues, documentation, discussions?

Closes #8538 (includes a recording of the before state).

How was this PR tested?

  • Two new unit tests in result-table-frame.component.spec.ts: with the switch on the button is enabled and titled "Download data"; with it off it is disabled and titled "Result export is disabled on this deployment". The switch is driven through MockGuiConfigService.setConfig, which is what the component receives under commonTestProviders. The existing click test now turns the switch on first and asserts it clicks a live button, since the mock ships with the switch off.
  • Mutation check, each restored afterwards: removing [disabled], making the title static, forcing the getter to true, and forcing it to false each turn exactly one named test red; removing the switch from the existing click test turns that test red.
  • Changed lines: 100% statement and function coverage (coverage-final.json, the getter runs 78 times in the spec).
  • ng test --watch=false: 217 files, 5928 passed, 1 skipped (pre-existing), 0 failed. ng build --configuration=production (AOT): clean. eslint on the ts/html/spec and prettier --check on all four files: clean.
  • Not yet exercised in a running instance; the change is a template binding over an existing config value plus a stylesheet rule.

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

Yes. Generated-by: Claude Code (Claude Fable 5.1, Anthropic). Co-authored with Claude; the author reviewed the change before submission.

🤖 Generated with Claude Code

@github-actions github-actions Bot added fix frontend Changes related to the frontend GUI labels Sep 14, 2026
@Yicong-Huang Yicong-Huang added release/v1.3 back porting to release/v1.3 release/v1.2 back porting to release/v1.2 labels Sep 14, 2026
@github-actions
github-actions Bot requested a review from mengw15 September 14, 2026 01:05
@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @juliethecao, @mengw15, @aglinxinyuan
    You can notify them by mentioning @juliethecao, @mengw15, @aglinxinyuan in a comment.

@github-actions
github-actions Bot requested a review from xuang7 September 14, 2026 01:05
@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. A release/* label nominates a backport target; the branch's release manager approving this PR is what sends the fix there. The required Backport Approvals check stays red until every label below is approved, so each manager either approves or removes their own label — which is why the labels left on a merged PR are exactly the branches it reached.

Release branch Analysis
release/v1.3 Already labeled — this fix is queued to backport here. @mengw15 decides: approving sends the fix here, removing this label declines it. The merge waits on one or the other.
release/v1.2 Already labeled — this fix is queued to backport here. @xuang7 decides: approving sends the fix here, removing this label declines it. The merge waits on one or the other.

Auto-label run.

@codecov-commenter

codecov-commenter commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.07%. Comparing base (5042d96) to head (0698dc6).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8539      +/-   ##
============================================
+ Coverage     95.90%   96.07%   +0.16%     
  Complexity      459      459              
============================================
  Files           589      589              
  Lines         33953    37054    +3101     
  Branches       4246     5035     +789     
============================================
+ Hits          32564    35601    +3037     
- Misses          811      847      +36     
- Partials        578      606      +28     
Flag Coverage Δ *Carryforward flag
access-control-service 80.18% <ø> (ø) Carriedforward from 5042d96
agent-service 99.32% <ø> (ø) Carriedforward from 5042d96
amber 90.82% <ø> (ø) Carriedforward from 5042d96
computing-unit-managing-service 77.14% <ø> (ø) Carriedforward from 5042d96
config-service 87.12% <ø> (ø) Carriedforward from 5042d96
file-service 83.65% <ø> (ø) Carriedforward from 5042d96
frontend 96.81% <100.00%> (+0.09%) ⬆️
notebook-migration-service 83.73% <ø> (ø) Carriedforward from 5042d96
pyamber 98.47% <ø> (ø) Carriedforward from 5042d96
workflow-compiling-service 74.09% <ø> (ø) Carriedforward from 5042d96

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

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…t is switched off

`export-execution-result-enabled` is a deployment switch. The top menu and the
context menu already honour it, but the download button on every result cell
rendered regardless, and clicking it returned silently from
WorkflowResultExportService.performExport: no request, no message. A reader
could not tell whether the feature was broken or off.

Bind the button's `disabled` state to the switch and say why in its title, the
same way the menu entry is disabled. The stylesheet hardcodes the icon color
and cursor, so the disabled state also gets its own look; without it the
disabled button was indistinguishable from the live one.

Tests cover both states through the MockGuiConfigService that the component
actually receives, and the existing click test now asserts it clicks a live
button.

Closes apache#8538

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@yangzhang75
yangzhang75 force-pushed the fix/cell-download-honours-export-switch branch from 39e828d to 0698dc6 Compare September 14, 2026 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix frontend Changes related to the frontend GUI release/v1.2 back porting to release/v1.2 release/v1.3 back porting to release/v1.3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Result download button does nothing when export is disabled

3 participants