Copilot commented on code in PR #6298: URL: https://github.com/apache/texera/pull/6298#discussion_r3555623751
########## frontend/src/app/workspace/component/dataset-selection-modal/dataset-search.util.ts: ########## @@ -0,0 +1,53 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { NzSelectItemInterface } from "ng-zorro-antd/select"; +import { DashboardDataset } from "../../../dashboard/type/dashboard-dataset.interface"; + +/** + * Whether a dataset matches a type-to-search query in the dataset picker. + * + * Matches against both the dataset name and its numeric id (shown as `#<id>` in the + * dropdown), case-insensitively, so typing either `iris` or `17` finds `#17 iris` + * (per the design decision on the proposal). An empty/whitespace query matches + * everything. + */ +export function datasetMatchesQuery( + name: string | null | undefined, + did: number | null | undefined, + query: string +): boolean { + const q = query.trim().toLowerCase(); + if (q === "") { + return true; + } + const nameMatches = (name ?? "").toLowerCase().includes(q); + const idMatches = did !== null && did !== undefined && String(did).includes(q); + return nameMatches || idMatches; Review Comment: The dataset id is displayed with a leading "#" in the option label (e.g. "#17"), but the filter currently only checks String(did). This makes queries like "#17" not match even though users may type what they see. Consider matching against the displayed token ("#<id>") so both "17" and "#17" work. ########## frontend/src/app/workspace/component/dataset-selection-modal/dataset-search.util.spec.ts: ########## @@ -0,0 +1,94 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { NzSelectItemInterface } from "ng-zorro-antd/select"; +import { datasetMatchesQuery, filterDatasetOption } from "./dataset-search.util"; + +describe("datasetMatchesQuery", () => { + it("matches by name, case-insensitively", () => { + expect(datasetMatchesQuery("iris", 17, "iris")).toBe(true); + expect(datasetMatchesQuery("iris", 17, "IR")).toBe(true); + expect(datasetMatchesQuery("iris", 17, "ris")).toBe(true); // substring + }); + + it("matches by numeric id", () => { + expect(datasetMatchesQuery("iris", 17, "17")).toBe(true); + expect(datasetMatchesQuery("iris", 17, "1")).toBe(true); // substring of id + }); Review Comment: If the dropdown shows dataset ids as "#<id>", it’s worth covering that common query form in the unit tests (e.g. searching "#17"). This also protects against regressions if the match logic is updated to include the "#" prefix. ########## frontend/src/app/workspace/component/dataset-selection-modal/dataset-search.util.spec.ts: ########## @@ -0,0 +1,94 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { NzSelectItemInterface } from "ng-zorro-antd/select"; +import { datasetMatchesQuery, filterDatasetOption } from "./dataset-search.util"; + +describe("datasetMatchesQuery", () => { + it("matches by name, case-insensitively", () => { + expect(datasetMatchesQuery("iris", 17, "iris")).toBe(true); + expect(datasetMatchesQuery("iris", 17, "IR")).toBe(true); + expect(datasetMatchesQuery("iris", 17, "ris")).toBe(true); // substring + }); + + it("matches by numeric id", () => { + expect(datasetMatchesQuery("iris", 17, "17")).toBe(true); + expect(datasetMatchesQuery("iris", 17, "1")).toBe(true); // substring of id + }); + + it("does not match when neither name nor id contains the query", () => { + expect(datasetMatchesQuery("iris", 17, "test")).toBe(false); + expect(datasetMatchesQuery("iris", 17, "99")).toBe(false); + }); + + it("treats an empty or whitespace query as matching everything", () => { + expect(datasetMatchesQuery("iris", 17, "")).toBe(true); + expect(datasetMatchesQuery("iris", 17, " ")).toBe(true); + }); + + it("trims and lowercases the query", () => { + expect(datasetMatchesQuery("Iris", 17, " IRIS ")).toBe(true); + }); + + it("handles missing name or id safely", () => { + expect(datasetMatchesQuery(null, 17, "17")).toBe(true); + expect(datasetMatchesQuery(undefined, null, "iris")).toBe(false); + expect(datasetMatchesQuery("iris", null, "iris")).toBe(true); + expect(datasetMatchesQuery("iris", undefined, "3")).toBe(false); + }); + + it("matches when the query appears in the name but not the id (and vice versa)", () => { + expect(datasetMatchesQuery("customers", 42, "cust")).toBe(true); // name only + expect(datasetMatchesQuery("customers", 42, "42")).toBe(true); // id only + }); + + it("does not partially match across the name/id boundary", () => { + // "s4" is not a substring of the name "iris" nor of the id "3" + expect(datasetMatchesQuery("iris", 3, "s4")).toBe(false); + }); +}); + +describe("filterDatasetOption", () => { + // Build an nz-select option carrying a DashboardDataset-shaped value, like the real + // dropdown does. Only the fields the filter reads (name, did) matter here. + const optionFor = (name: string, did: number): NzSelectItemInterface => + ({ nzLabel: name, nzValue: { dataset: { name, did } } }) as unknown as NzSelectItemInterface; + + it("matches by dataset name pulled from the option value", () => { + expect(filterDatasetOption("iris", optionFor("iris", 17))).toBe(true); + expect(filterDatasetOption("IR", optionFor("iris", 17))).toBe(true); + expect(filterDatasetOption("test", optionFor("iris", 17))).toBe(false); + }); + + it("matches by dataset #id pulled from the option value", () => { + expect(filterDatasetOption("17", optionFor("iris", 17))).toBe(true); + expect(filterDatasetOption("99", optionFor("iris", 17))).toBe(false); + }); Review Comment: Similarly, the nzFilterOption adapter should be tested for "#<id>" queries since users may type the leading "#" from the UI label. -- 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]
