This is an automated email from the ASF dual-hosted git repository. aglinxinyuan pushed a commit to branch backport/7806-sort-cache-size-ascending-drop-the-broke-v1.2 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 2e9f478bae644e875268918409b6b8cead917a03 Author: Xinyuan Lin <[email protected]> AuthorDate: Thu Aug 20 20:01:00 2026 +0000 fix(frontend): sort cache size ascending, drop the broken preset updater (#7806) ### What changes were proposed in this PR? Three frontend defects. **5 files, +14 / −60.** **1. `sortBySize` now matches the `NzTableSortFn` contract.** It returned `b - a`, but ng-zorro uses the comparator's result as-is for `ascend` and negates it for `descend` (`ng-zorro-antd-table.mjs:822`), with `nzSortDirections` defaulting to `['ascend','descend',null]` (`:1063`) — which this header does not override. So the first click **lit the up-caret while rendering largest-first**. `admin-user.component.ts:268` already uses the contract-correct form, so this was a one-off, not a convention. **2. `PresetService.updatePreset` removed — dead *and* broken.** No caller repo-wide outside its own spec, and it could not have worked: `indexOf(presets, originalPreset)` is lodash **reference** equality against a freshly `JSON.parse`d array. Reproduced by execution: ``` contains(presets, originalPreset) = true (the guard passes) indexOf(presets, originalPreset) = -1 splice branch -> [{"p":"a"},{"p":"b"}] // deleted the LAST preset, not the target assign branch -> [{"p":"a"},{"p":"b"},{"p":"c"}] // the replacement was dropped entirely ``` Its sibling `updateOrCreatePreset` carries the explanatory comment and uses `findIndex(isEqual)` — the fix was applied there and not here. Removed the method, its doc block, the now-unused `indexOf` import, and the one spec test that existed solely for it. **3. An unreachable template fallback simplified.** `{{ displayFileName || "Selected image" }}` → `{{ displayFileName }}`. Guard dominance confirmed: the enclosing div is `*ngIf="previewSrc"`, `previewSrc` returns `hasImage ? formControl.value : ""`, so a truthy `previewSrc` implies `hasImage`, and `displayFileName` then returns a non-empty filename or `"Uploaded image"` — never empty. No subclass, single template consumer. The spec already asserted the span reads `"Uploaded image"` in the no-filename case, which is direct evidence the fallback was dead. ### The sort fix is pinned in both directions The spec already held a **characterization** test recording the broken caret/order pairing, with a comment saying a fix "is meant to flip both halves". Both halves are now flipped to assert correct behaviour, and verified with the production file reverted and restored: | | production reverted | with fix | |---|---|---| | `user-quota.component.spec.ts` | **2 failed, 23 passed** | **25 passed** | The failure that matters: ``` AssertionError: expected [ '30', '10', '20' ] to deeply equal [ '20', '10', '30' ] ``` And the detail that makes this a real user-facing bug rather than a comparator nit: in the before-state the **caret assertions passed** (`caretActive("up") === true`) while the rows came out largest-first. That is exactly what a user sees. Asserting row order alone would still pass against a comparator inverted the other way, so the test pins the caret and the order together, in both directions. ### Verification - `user-quota.component.spec.ts` + `preset.service.spec.ts` + `hugging-face-image-upload.component.spec.ts`: **124 passed (124)**. - `npx ng build`: **exit 0**, zero errors and zero NG####/TS diagnostics. Worth running because `ng test` and `tsc --noEmit` both miss Angular template errors — and the bundle was grepped to confirm the template change was genuinely AOT-compiled: `"Uploaded image"` appears in 2 chunks, `"Selected image"` in **0**. - `yarn format:ci`: exit 0. ### A follow-up worth filing separately The same defect class appears six more times in `admin-user.component.ts`: `sortByName`, `sortByEmail`, `sortByComment`, `sortByRole`, `sortByAffiliation` and `sortByJoiningReason` all use `b.localeCompare(a)`, and their headers set `nzSortDirections="['ascend','descend']"`, so they also start at `ascend` and render reversed. Not touched here — different component, and it deserves its own change with its own tests. ### Any related issues, documentation, discussions? Closes #7805 ### How was this PR tested? ``` npx ng test --watch=false --include="**/user-quota.component.spec.ts" --include="**/preset.service.spec.ts" --include="**/hugging-face-image-upload.component.spec.ts" ``` ``` Test Files 3 passed (3) Tests 124 passed (124) ``` ### Was this PR authored or co-authored using generative AI tooling? (backported from commit ff5bd7f8604366feda569ab29917bdb30a460266) Generated-by: Claude Code (Opus 5) --- .../user/user-quota/user-quota.component.spec.ts | 32 +++++++++++++++++++++ .../user/user-quota/user-quota.component.ts | 2 +- .../service/preset/preset.service.spec.ts | 12 -------- .../app/workspace/service/preset/preset.service.ts | 33 ---------------------- 4 files changed, 33 insertions(+), 46 deletions(-) diff --git a/frontend/src/app/dashboard/component/user/user-quota/user-quota.component.spec.ts b/frontend/src/app/dashboard/component/user/user-quota/user-quota.component.spec.ts index 1190a562e9..98f42aa34e 100644 --- a/frontend/src/app/dashboard/component/user/user-quota/user-quota.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/user-quota/user-quota.component.spec.ts @@ -24,6 +24,26 @@ import { HttpClientTestingModule } from "@angular/common/http/testing"; import { commonTestProviders } from "../../../../common/testing/test-utils"; import { of } from "rxjs"; import type { Mocked } from "vitest"; +import { ExecutionQuota } from "../../../../common/type/user"; + +function execution(eid: number, result: number, runtime: number, log: number): ExecutionQuota { + return { + eid, + workflowId: 1, + workflowName: "wf-1", + resultBytes: result, + runTimeStatsBytes: runtime, + logBytes: log, + }; +} + +// Fixtures for the Cache Size comparator. The three byte triples are picked so that ordering by +// any single count — or by the sum with any one of the three terms dropped — yields a different +// sequence than the full sum does, and so that no two of the six orderings share a tie. +const SIZE_SORT_BIG = execution(30, 1, 450, 452); // 903 +const SIZE_SORT_MIDDLE = execution(10, 800, 1, 1); // 802 +const SIZE_SORT_SMALL = execution(20, 2, 700, 2); // 704 + describe("UserQuotaComponent", () => { let component: UserQuotaComponent; let fixture: ComponentFixture<UserQuotaComponent>; @@ -55,4 +75,16 @@ describe("UserQuotaComponent", () => { fixture.detectChanges(); expect(component).toBeTruthy(); }); + + describe("sortBySize", () => { + // Ascending, as NzTableSortFn requires (`a - b`): nz-table takes the comparator's result as-is + // for 'ascend' and negates it for 'descend', so a `b - a` comparator lights the up caret while + // rendering largest-first. nzSortDirections defaults to ['ascend', 'descend', null] and the + // Cache Size header does not override it, so the first click is 'ascend'. + it("orders executions by total cache size, smallest first", () => { + const scrambled = [SIZE_SORT_MIDDLE, SIZE_SORT_SMALL, SIZE_SORT_BIG]; + + expect([...scrambled].sort(component.sortBySize).map(e => e.eid)).toEqual([20, 10, 30]); + }); + }); }); diff --git a/frontend/src/app/dashboard/component/user/user-quota/user-quota.component.ts b/frontend/src/app/dashboard/component/user/user-quota/user-quota.component.ts index ae04467a80..cf2201be6a 100644 --- a/frontend/src/app/dashboard/component/user/user-quota/user-quota.component.ts +++ b/frontend/src/app/dashboard/component/user/user-quota/user-quota.component.ts @@ -342,5 +342,5 @@ export class UserQuotaComponent implements OnInit { formatSize = formatSize; public sortBySize: NzTableSortFn<ExecutionQuota> = (a: ExecutionQuota, b: ExecutionQuota) => - b.resultBytes + b.logBytes + b.runTimeStatsBytes - a.resultBytes - a.logBytes - a.runTimeStatsBytes; + a.resultBytes + a.logBytes + a.runTimeStatsBytes - (b.resultBytes + b.logBytes + b.runTimeStatsBytes); } diff --git a/frontend/src/app/workspace/service/preset/preset.service.spec.ts b/frontend/src/app/workspace/service/preset/preset.service.spec.ts index 4095418195..e7c8703223 100644 --- a/frontend/src/app/workspace/service/preset/preset.service.spec.ts +++ b/frontend/src/app/workspace/service/preset/preset.service.spec.ts @@ -188,18 +188,6 @@ describe("PresetService", () => { expect((errors[0] as Error).message).toMatch(/already exists/); }); - it("updatePreset does not write the preset back when the original preset is missing", async () => { - userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify([{ presetProperty: "v1" }]))); - - const errors = await captureRxjsUnhandled(() => - presetService.updatePreset(presetType, presetTarget, { presetProperty: "missing" }, { presetProperty: "v3" }) - ); - - expect(userConfigStub.set).not.toHaveBeenCalled(); - expect(errors).toHaveLength(1); - expect((errors[0] as Error).message).toMatch(/doesn't exist/); - }); - it("deletePreset removes the matching preset via savePresets", () => { const a: Preset = { presetProperty: "v1" }; const b: Preset = { presetProperty: "v2" }; diff --git a/frontend/src/app/workspace/service/preset/preset.service.ts b/frontend/src/app/workspace/service/preset/preset.service.ts index 9ba5c2a318..5361ec3484 100644 --- a/frontend/src/app/workspace/service/preset/preset.service.ts +++ b/frontend/src/app/workspace/service/preset/preset.service.ts @@ -158,39 +158,6 @@ export class PresetService { }); } - /** - * broadcast savePresets event and also save preset to presetDict, which is a *view* (in the database sense) of DictionaryService's dictionary that only stores presets - * @param type string, usually "operator" - * @param target string, usualy operatorType - * @param presets Preset[] - * @param displayMessage message to display when saving presets - * @param messageType see AlertMessageType, determines icon used in popup message - */ - public updatePreset( - type: string, - target: string, - originalPreset: Preset, - replacementPreset: Preset, - displayMessage?: string | null, - messageType: AlertMessageType = "success" - ) { - this.userConfigService - .fetchKey(`${type}-${target}`) - .pipe(first()) - .subscribe(presetsString => { - let presets = JSON.parse(presetsString ?? "[]") as Preset[]; - if (!contains(presets, originalPreset)) { - throw new Error("attempting to update preset that doesn't exist"); - } else if (contains(presets, replacementPreset)) { - // implicit deletion by replacing original with existing preset - presets.splice(indexOf(presets, originalPreset), 1); - } else { - presets[indexOf(presets, originalPreset)] = replacementPreset; - } - this.savePresets(type, target, presets, displayMessage, messageType); - }); - } - /** * broadcast savePresets event and also save preset to presetDict, which is a *view* (in the database sense) of DictionaryService's dictionary that only stores presets * @param type string, usually "operator"
