kunwp1 commented on code in PR #8539:
URL: https://github.com/apache/texera/pull/8539#discussion_r4055094329
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.html:
##########
@@ -193,7 +193,11 @@ <h5 class="rightAlign"><span
[innerHTML]="compare(column.header, 'other')"></spa
<ng-container *ngSwitchDefault>{{ column.getCell(row)
}}</ng-container>
</ng-container>
</span>
+ <!-- Not rendered when result export is switched off for the
deployment: every action
+ behind it returns without sending a request, so the button
would be there and do
+ nothing. The top menu and the context menu already honour the
same switch. -->
<button
+ *ngIf="guiConfigService.env.exportExecutionResultEnabled"
(click)="downloadData(currentResult[i][column.columnDef], i,
columnIndex, column.columnDef); $event.stopPropagation()"
Review Comment:
**`currentResult[i][column.columnDef]` is computed and then thrown away.**
`downloadData(data, rowIndex, columnIndex, columnName)` never reads `data` —
the export addresses the cell by row and column index, not by value. Dropping
the first parameter here and in the method signature costs nothing and removes
the only place in this template that indexes `currentResult` by the page-local
`*ngFor` index, which is exactly the mismatch the `cellMediaTypes` comment on
line 118 of the component warns about.
Since you are already editing this function's signature area, this is the
moment to do it. The three spec calls to `downloadData` would drop their first
argument too.
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.html:
##########
@@ -193,7 +193,11 @@ <h5 class="rightAlign"><span
[innerHTML]="compare(column.header, 'other')"></spa
<ng-container *ngSwitchDefault>{{ column.getCell(row)
}}</ng-container>
</ng-container>
</span>
+ <!-- Not rendered when result export is switched off for the
deployment: every action
+ behind it returns without sending a request, so the button
would be there and do
+ nothing. The top menu and the context menu already honour the
same switch. -->
<button
+ *ngIf="guiConfigService.env.exportExecutionResultEnabled"
Review Comment:
**This is a second, independent behaviour change, and it is in neither the
title nor the description.**
Hiding the button when `exportExecutionResultEnabled` is off is a reasonable
change and your reasoning for it is correct. It is also unrelated to "make a
result cell's download work off the canvas": a deployment with the switch off
is precisely the one case this PR's actual fix cannot reach.
Two AGENTS.md rules apply:
- "Narrowly scoped changes. No unrelated rewrites or cross-service moves."
- "**Frontend PRs**: any visible UI change requires screenshots / GIF,
**before / after** side by side."
The button is hover-revealed and absolutely positioned, so nothing reflows —
a before/after of one hovered row with the switch off would satisfy the second
rule. Otherwise this hunk deserves its own issue and PR.
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.ts:
##########
@@ -464,17 +464,27 @@ export class ResultTableFrameComponent implements OnInit,
OnChanges {
}
downloadData(data: any, rowIndex: number, columnIndex: number, columnName:
string): void {
+ // A cell belongs to the operator whose results this frame is showing.
Without one there is
+ // nothing to scope an export to, and the dialog would open only to export
nothing.
+ if (!this.operatorId) {
+ return;
+ }
const realRowNumber = (this.currentPageIndex - 1) * this.pageSize +
rowIndex;
const defaultFileName = `${columnName}_${realRowNumber}`;
const modal = this.modalService.create({
Review Comment:
`modal` is assigned and never read.
```suggestion
this.modalService.create({
```
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.ts:
##########
@@ -130,7 +130,7 @@ export class ResultTableFrameComponent implements OnInit,
OnChanges {
private changeDetectorRef: ChangeDetectorRef,
private sanitizer: DomSanitizer,
private workflowStatusService: WorkflowStatusService,
- private guiConfigService: GuiConfigService
+ public guiConfigService: GuiConfigService
Review Comment:
**`protected` is enough here, and it matches the sibling component.**
`context-menu.component.ts` declares `protected config: GuiConfigService`
for exactly this purpose — a template-only read of the same switch. Angular
templates can reach protected members, so `public` widens the component's API
without buying anything; nothing outside the class touches this field,
including the specs, which reach the mock through
`TestBed.inject(GuiConfigService)`.
```suggestion
protected guiConfigService: GuiConfigService
```
##########
frontend/src/app/workspace/component/result-exportation/result-exportation.component.ts:
##########
@@ -103,15 +106,21 @@ export class ResultExportationComponent implements OnInit
{
filteredUserAccessibleDatasets: DashboardDataset[] = [];
/**
- * Gets the operator IDs to check for restrictions based on the source
trigger.
- * Menu: all operators, Context menu: highlighted operators only
+ * The operators this export covers, which everything below reads: what may
be exported, what
+ * a blocking dataset blocks, and what kind of output the dialog is offering.
+ *
+ * A caller that named them wins. The two fallbacks each belong to a caller
reading the
+ * canvas -- the menu exports the whole workflow, the context menu exports
the selection --
+ * and a result cell is neither: it belongs to one operator, whoever is
selected. On the Form
+ * View nothing is selected until the user clicks a step, so this answered
"no operators", and
+ * with nothing in scope a blocked operator went unreported.
*/
private getOperatorIdsToCheck(): readonly string[] {
+ if (this.operatorIds.length > 0) {
+ return this.operatorIds;
+ }
if (this.sourceTriggered === "menu") {
Review Comment:
**Two mechanisms now answer one question.**
The caller-named `operatorIds` is the better one: it states exactly what the
export covers. `sourceTriggered` is the weaker one: a string the dialog matches
in order to guess a scope from the canvas. Keeping both means the rule lives in
this method *and* in the callers, and the two agree only for as long as nobody
edits one of them — which is the objection your own comment on the service
signature raises.
The deeper fix is to let every caller name its operators, since both already
can:
| Caller | Passes |
| --- | --- |
| `menu.component.ts` | `getTexeraGraph().getAllOperatorIDs()` |
| `context-menu.component.ts` | `getCurrentHighlightedOperatorIDs()` |
| `result-table-frame.component.ts` | `[operatorId]` (already done) |
Then `sourceTriggered`, both canvas fallbacks, and the
`WorkflowActionService` injection all leave this component, and
`getOperatorIdsToCheck()` collapses to reading a field.
##########
frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts:
##########
@@ -197,12 +197,18 @@ export class WorkflowResultExportService {
rowIndex: number,
columnIndex: number,
filename: string,
- exportAll: boolean = false, // if the user click export button on the top
bar (a.k.a menu),
- // we should export all operators, otherwise, only highlighted ones
- // which means export button is selected from context-menu
- destination: "dataset" | "local" = "dataset", // default to dataset
- unit: DashboardWorkflowComputingUnit | null // computing unit for cluster
setting
+ destination: "dataset" | "local",
+ unit: DashboardWorkflowComputingUnit | null, // computing unit for cluster
setting
+ // The operators this export covers. The caller resolves them: the dialog
already works out
+ // its own scope in order to report what a blocking dataset blocks, so it
says so here rather
+ // than leaving the scope to be worked out a second time, separately, from
a flag and the
+ // canvas -- two answers to one question that agree only for as long as
nobody edits one.
+ operatorIds: readonly string[]
Review Comment:
**The PR description no longer describes this signature.**
It promises "a trailing optional `requestedOperatorIds`". What is here is a
**required** ninth parameter named `operatorIds`, and `exportAll` is
**deleted** rather than kept beside it. The description also says "Ten lines of
logic in three files" and "Nothing changes for the menu or the context menu",
while the second commit touches four production files, changes how the service
resolves scope for every caller, fixes `getWorkflowMetadata` being read without
being called, and gates the cell button on a config switch.
To be clear, the shipped design is the better one — a single caller-supplied
scope beats a flag plus a second read of the canvas, and the snapshot copy on
the next line closes a real hole. The description just needs to catch up so a
reviewer reads about the change that is actually here.
##########
frontend/src/app/workspace/component/result-exportation/result-exportation.component.ts:
##########
@@ -103,15 +106,21 @@ export class ResultExportationComponent implements OnInit
{
filteredUserAccessibleDatasets: DashboardDataset[] = [];
/**
- * Gets the operator IDs to check for restrictions based on the source
trigger.
- * Menu: all operators, Context menu: highlighted operators only
+ * The operators this export covers, which everything below reads: what may
be exported, what
+ * a blocking dataset blocks, and what kind of output the dialog is offering.
+ *
+ * A caller that named them wins. The two fallbacks each belong to a caller
reading the
+ * canvas -- the menu exports the whole workflow, the context menu exports
the selection --
+ * and a result cell is neither: it belongs to one operator, whoever is
selected. On the Form
+ * View nothing is selected until the user clicks a step, so this answered
"no operators", and
+ * with nothing in scope a blocked operator went unreported.
*/
private getOperatorIdsToCheck(): readonly string[] {
Review Comment:
**This runs four to five times per change-detection cycle.**
The template binds `hasPartialNonDownloadable` (which reads both
`exportableOperatorIds` and `blockedOperatorIds`), `blockingDatasetLabels`, and
`isExportRestricted` (which reads `exportableOperatorIds` again). Each of those
getters calls this method and re-runs a filter over the result. On the menu
path, every one of those calls also allocates a fresh array of every operator
ID in the workflow.
The scope cannot change while the dialog is open — the modal's mask covers
the canvas, and `updateOutputType()` already assumes a stable scope by running
exactly once. Resolving the scope once into a field and having the getters read
that field would remove the repeated work and put the precedence rule in one
place.
(The `getAllOperators().map(...)` → `getAllOperatorIDs()` swap already made
each call cheaper. This is about the number of calls.)
##########
frontend/src/app/workspace/component/result-exportation/result-exportation.component.ts:
##########
@@ -103,15 +106,21 @@ export class ResultExportationComponent implements OnInit
{
filteredUserAccessibleDatasets: DashboardDataset[] = [];
/**
- * Gets the operator IDs to check for restrictions based on the source
trigger.
- * Menu: all operators, Context menu: highlighted operators only
+ * The operators this export covers, which everything below reads: what may
be exported, what
+ * a blocking dataset blocks, and what kind of output the dialog is offering.
+ *
+ * A caller that named them wins. The two fallbacks each belong to a caller
reading the
+ * canvas -- the menu exports the whole workflow, the context menu exports
the selection --
+ * and a result cell is neither: it belongs to one operator, whoever is
selected. On the Form
+ * View nothing is selected until the user clicks a step, so this answered
"no operators", and
+ * with nothing in scope a blocked operator went unreported.
*/
private getOperatorIdsToCheck(): readonly string[] {
+ if (this.operatorIds.length > 0) {
+ return this.operatorIds;
Review Comment:
**The dialog now opens showing the red "Export unavailable" alert.**
`downloadability` stays `undefined` until `computeRestrictionAnalysis()`'s
HTTP response lands, so `exportableOperatorIds` returns `[]` (line 133). With a
named scope, `operatorIds.length > 0` is true from the very first render, so
`isExportRestricted` (line 148) is `true` and the template renders the
`#restrictedExport` branch for the whole round trip before flipping to the form.
Before this PR, the cell path on the Form View had an empty scope, so the
flag was `false` and the user saw the normal form — it just exported nothing,
which is the bug you are fixing. The fix moves an error state onto the path it
just repaired.
I confirmed it against this head: with the analysis held open,
`isExportRestricted === true` and the rendered text contains "Export
unavailable"; once the response arrives it is `false`.
The fix is one line in `isExportRestricted` — treat "not analysed yet" as
"not restricted":
```ts
get isExportRestricted(): boolean {
if (!this.downloadability) return false;
const operatorIds = this.getOperatorIdsToCheck();
return this.exportableOperatorIds.length === 0 && operatorIds.length > 0;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
