Yicong-Huang commented on code in PR #7463:
URL: https://github.com/apache/texera/pull/7463#discussion_r3885535325
##########
frontend/src/app/dashboard/component/user/search-bar/search-bar.component.ts:
##########
@@ -143,8 +142,6 @@ export class SearchBarComponent {
convertToName(resultItem: SearchResultItem): string {
if (resultItem.workflow) {
return new DashboardEntry(resultItem.workflow).name;
Review Comment:
Dropping the `project` arm makes a leftover project row fatal rather than
ignored.
`SearchService.search` still asks for the all-resource-type response, and
`searchAllResources` still unions `ProjectSearchQueryBuilder` into it. With no
arm to match, the chain reaches the throw at :150 — inside a `switchMap` whose
`subscribe` at :89 has no error handler. The autocomplete subscription
terminates, and every later keystroke is dead with only a console error.
The builder joins `PROJECT_USER_ACCESS.UID.eq(uid)`, so any logged-in user
with a surviving project matching their query hits it, on the search bar every
page renders. That refutes the description's "the `/project` endpoints simply
stop being called": the break is not on a `/project` endpoint.
Suggested fix in one place — guard in `SearchService.search`, the funnel
both consumers call, dropping items with no client-modelled payload. Stale rows
are then ignored, and future server-side types behave the same.
Worth a test too: `search.service.spec.ts` deleted `makeProjectItem`, so
nothing observes this. `search-bar.component.spec.ts` pins the throw with
`resourceType: "computing-unit"` — a value `/dashboard/search` never emits —
leaving the reachable case unguarded.
##########
frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts:
##########
@@ -162,14 +155,13 @@ export class UserWorkflowComponent implements
AfterViewInit, OnDestroy {
constructor(
private userService: UserService,
private workflowPersistService: WorkflowPersistService,
- private userProjectService: UserProjectService,
private notificationService: NotificationService,
- private modalService: NzModalService,
private router: Router,
private downloadService: DownloadService,
private searchService: SearchService,
private config: GuiConfigService,
- private notebookMigrationService: NotebookMigrationService
+ private notebookMigrationService: NotebookMigrationService,
+ private modalService: NzModalService
Review Comment:
`private modalService: NzModalService` moves from the fifth constructor
position to the last. Angular resolves by type and the dependency is still used
at :285, so this is behaviour-neutral — but it is churn unrelated to the
removal. Restoring its original position keeps the diff to the deletion.
##########
frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.spec.ts:
##########
@@ -283,29 +261,22 @@ describe("SavedWorkflowSectionComponent", () => {
});
it("searchByManyParameters", async () => {
- // Apply the project, ID, owner, and operator filter all at once.
+ // Apply the ID, owner, and operator filter all at once.
Review Comment:
Three filters, so the noun needs the plural.
```suggestion
// Apply the ID, owner, and operator filters all at once.
```
##########
frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts:
##########
@@ -542,7 +542,7 @@ describe("AdminSettingsComponent", () => {
});
/**
* The settings form is four near-identical Save/Reset cards, three
near-identical upload blocks and
- * twelve switches whose keys include two confusable singular/plural pairs
(workflow_enabled vs
+ * eleven switches whose keys include two confusable singular/plural pairs
(workflow_enabled vs
Review Comment:
The count is off by one. This line says "eleven switches", three lines above
a `SWITCH_KEYS` array with twelve entries.
The 12→11 arithmetic was right against the pre-merge branch. But the merge
base already carried thirteen keys — `main` added `models_enabled` without
updating this prose — so the real change is 13→12. The PR description needs the
same correction.
This suite exists to catch cross-wiring among near-identical switches, so a
count contradicting the list below it misleads the next reader.
```suggestion
* twelve switches whose keys include two confusable singular/plural pairs
(workflow_enabled vs
```
##########
frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts:
##########
@@ -71,7 +67,7 @@ import { FormsModule } from "@angular/forms";
/**
* Saved-workflow-section component contains information and functionality
- * of the saved workflows section and is re-used in the user projects section
when a project is clicked
+ * of the saved workflows section
Review Comment:
With the project clause trimmed, this line restates the class name and
nothing else. Naming what the component is for keeps the docblock earning its
place.
```suggestion
* of the saved workflows section: the list of workflows the user owns or
has access to
```
--
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]
