Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lightning bug fixes #3941

Merged
merged 6 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions app/packages/components/src/components/Selector/Selector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import LoadingDots from "../Loading/LoadingDots";
import SearchResults, { UseSearch } from "./SearchResults";
import style from "./Selector.module.css";

export class SelectorValidationError extends Error {}

export interface SelectorProps<T> {
id?: string;
value?: string;
Expand Down Expand Up @@ -69,11 +71,18 @@ function Selector<T>(props: SelectorProps<T>) {
? valuesRef.current[active]
: valuesRef.current.find((v) => toKey(v) === search);

const result = await onSelect(value ? toKey(value) : search, value);
if (result !== undefined) {
local.current = result;
try {
const result = await onSelect(value ? toKey(value) : search, value);
if (result !== undefined) {
local.current = result;
}
setEditing(false);
} catch (error) {
if (error instanceof SelectorValidationError) {
return;
}
throw error;
}
setEditing(false);
};
}, [active, onSelect, toKey, valuesRef]);

Expand Down
4 changes: 2 additions & 2 deletions app/packages/components/src/components/Selector/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
export { default } from "./Selector";
export type { UseSearch } from "./Selector";
export type { UseSearch } from "./SearchResults";
export { SelectorValidationError, default } from "./Selector";
2 changes: 1 addition & 1 deletion app/packages/components/src/components/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export { default as Popout, PopoutDiv } from "./Popout";
export { default as PopoutSectionTitle } from "./PopoutSectionTitle";
export * from "./Selection";
export { default as Selection } from "./Selection";
export { default as Selector } from "./Selector";
export { default as Selector, SelectorValidationError } from "./Selector";
export type { UseSearch } from "./Selector";
export { default as TabOption } from "./TabOption";
export { default as ThemeProvider, useTheme } from "./ThemeProvider";
Expand Down
1 change: 1 addition & 0 deletions app/packages/core/src/components/Actions/Options.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ const Lightning = () => {
options={["disable", "enable"].map((value) => ({
text: value,
title: value,
dataCy: `lightning-mode-${value}`,
onClick: () =>
setThreshold(value === "disable" ? null : config ?? count),
}))}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ const StringFilter = ({
placeholder={`+ ${
isFilterMode ? "filter" : "set visibility"
} by ${name}`}
cy={`sidebar-search-${path}`}
component={ResultComponent}
onSelect={onSelect}
inputStyle={{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import { SelectorValidationError } from "@fiftyone/components";
import { isObjectIdField, snackbarErrors } from "@fiftyone/state";
import { isObjectIdString } from "@fiftyone/utilities";
import { useRef } from "react";
import { RecoilState, useRecoilCallback } from "recoil";
import { Result } from "./Result";
Expand All @@ -12,13 +15,22 @@ export default function (
onSelect: useRecoilCallback(
({ snapshot, set }) =>
async (value: string | null, d?: Result) => {
const isObjectId = await snapshot.getPromise(isObjectIdField(path));
if (isObjectId && (value === null || !isObjectIdString(value))) {
set(snackbarErrors, [
`${value} is not a 24 character hexadecimal string`,
]);
throw new SelectorValidationError();
}

const selected = new Set(await snapshot.getPromise(selectedAtom));
if (d?.value === null) {
value = null;
}
selectedMap.current.set(value, d?.count || null);
selected.add(value);
set(selectedAtom, [...selected].sort());

return "";
},
[modal, path, selectedAtom, selectedMap]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export default ({
return (
<Tooltip text={<AddIndex />} placement="top-center">
<Arrow
data-cy={`sidebar-field-arrow-${id}`}
data-cy={`sidebar-field-arrow-disabled-${id}`}
style={{ margin: 0, color: theme.text.secondary }}
/>
</Tooltip>
Expand All @@ -36,7 +36,7 @@ export default ({

return (
<Arrow
data-cy={`sidebar-field-arrow-${id}`}
data-cy={`sidebar-field-arrow-enabled-${id}`}
style={{
cursor: "pointer",
margin: 0,
Expand Down
24 changes: 24 additions & 0 deletions app/packages/core/src/components/ViewBar/ViewStage/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,27 @@ export const getMatch = (options, value) => {
}
return null;
};

export const cleanCSV = (values: string) => {
return (
values
// replace spaces with a single space (to allow search by words with spaces)
.replace(/[\s\'\"\[\]]+/g, " ")
// replace comma followed by trailing spaces with a single comma
.replace(/,\s*/g, ",")
// remove trailing spaces
.replace(/[ \t]+$/, "")
);
};
Comment on lines +25 to +35
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌


export const getArray = (value: string | unknown[]): unknown[] => {
if (typeof value === "string") {
try {
return JSON.parse(value);
} catch {
return value.replace(/[\s]/g, "").split(",");
}
}

return value;
};
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { v4 as uuid } from "uuid";
import { actions, Machine, sendParent } from "xstate";

import { computeBestMatchString, getMatch } from "./utils";
import { isObjectIdString } from "@fiftyone/utilities";
import { cleanCSV, computeBestMatchString, getArray, getMatch } from "./utils";

const { assign } = actions;

Expand Down Expand Up @@ -85,7 +86,7 @@ export const PARSER = {
castFrom: (value) => value,
castTo: (value) => value,
parse: (value) => value,
validate: (value) => /[0-9A-Fa-f]{24}/g.test(value),
validate: (value) => isObjectIdString(value),
},
int: {
castFrom: (value) => String(value).replace(/\B(?=(\d{3})+(?!\d))/g, ","),
Expand Down Expand Up @@ -113,66 +114,25 @@ export const PARSER = {
"list<field>": {
castFrom: (value) => value.join(","),
castTo: (value) => value.split(","),
parse: (value) => value.replace(/[\s\'\"\[\]]/g, ""),
parse: (value) => cleanCSV(value),
validate: (value, fields) => {
const stripped = value.replace(/[\s]/g, "");
let array = null;
try {
array = JSON.parse(stripped);
} catch {
array = stripped.split(",");
}
return (
typeof value !== "string" &&
Array.isArray(array) &&
array.every((e) => PARSER.field.validate(e, fields))
);
return getArray(value).every((e) => PARSER.field.validate(e, fields));
},
},
"list<id>": {
castFrom: (value) => value.join(","),
castTo: (value) => value.split(","),
parse: (value) => value.replace(/[\s\'\"\[\]]/g, ""),
parse: (value) => cleanCSV(value),
validate: (value) => {
const stripped = value.replace(/[\s]/g, "");
let array = null;
try {
array = JSON.parse(stripped);
} catch {
array = stripped.split(",");
}
return (
typeof value !== "string" &&
Array.isArray(array) &&
array.every((e) => PARSER.id.validate(e))
);
return getArray(value).every((e) => PARSER.id.validate(e));
},
},
"list<str>": {
castFrom: (value) => value.join(","),
castTo: (value) => value.split(","),
parse: (value) =>
value
// replace spaces with a single space (to allow search by words with spaces)
.replace(/[\s\'\"\[\]]+/g, " ")
// replace comma followed by trailing spaces with a single comma
.replace(/,\s*/g, ",")
// remove trailing spaces
.replace(/[ \t]+$/, ""),
parse: (value) => cleanCSV(value),
validate: (value) => {
const stripped = value.replace(/[\s]/g, "");
let array = null;
try {
array = JSON.parse(stripped);
} catch {
// ex: exclude_fields('a, b'). 'a,b' => ['a', 'b']
array = stripped.split(",");
}
return (
typeof array !== "string" &&
Array.isArray(array) &&
array.every((e) => PARSER.str.validate(e))
);
return getArray(value).every((e) => PARSER.str.validate(e));
},
},
NoneType: {
Expand Down
5 changes: 3 additions & 2 deletions app/packages/state/src/recoil/lightning.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ const indexesByPath = selector({
) => {
const projection = frames ? framesProjection : samplesProjection;

fields = fields.map((field) => get(schemaAtoms.dbPath(field)));

if (field === "$**") {
if (!projection) {
return fields;
Expand Down Expand Up @@ -143,8 +145,7 @@ export const pathIndex = selectorFamily({
(path: string) =>
({ get }) => {
const indexes = get(indexesByPath);

return indexes.has(path);
return indexes.has(get(schemaAtoms.dbPath(path)));
},
});

Expand Down
8 changes: 8 additions & 0 deletions app/packages/utilities/src/type-check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@ export function isPrimitiveType(type: string) {
return PRIMITIVE_TYPES.includes(type);
}

export function isHex(value: string) {
return /[0-9a-f]{24}/g.test(value);
}

export function isObjectIdString(value: string) {
return isHex(value) && value.length === 24;
}

export type NumberKeyObjectType<V = unknown> = {
[key: string]: V;
};
Expand Down
6 changes: 6 additions & 0 deletions e2e-pw/src/oss/poms/action-row/display-options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Page } from "src/oss/fixtures";
type SidebarStatisticsMode = "slice" | "group";
type SidebarMode = "fast" | "best" | "all";
type SidebarSortMode = "count" | "value";
type LightningMode = "enable" | "disable";

export class DisplayOptionsPom {
readonly page: Page;
Expand All @@ -11,6 +12,11 @@ export class DisplayOptionsPom {
this.page = page;
}

async setLightningMode(mode: LightningMode) {
const selector = this.page.getByTestId(`lightning-mode-${mode}`);
return selector.click();
}

async setSidebarStatisticsMode(mode: SidebarStatisticsMode) {
const selector = this.page.getByTestId(
`tab-option-View ${mode} sidebar statistics`
Expand Down
42 changes: 40 additions & 2 deletions e2e-pw/src/oss/poms/sidebar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ export class SidebarPom {
.nth(1);
}

fieldContainer(fieldName: string) {
return this.sidebar.getByTestId(`sidebar-field-container-${fieldName}`);
}

fieldArrow(fieldName: string, enabled: boolean) {
return this.fieldContainer(fieldName).getByTestId(
`sidebar-field-arrow-${enabled ? "enabled" : "disabled"}-${fieldName}`
);
}

sidebarEntryDraggableArea(fieldName: string) {
return this.sidebar
.getByTestId(`sidebar-entry-draggable-${fieldName}`)
Expand Down Expand Up @@ -58,7 +68,9 @@ export class SidebarPom {
}

async clickFieldDropdown(field: string) {
const selector = this.sidebar.getByTestId(`sidebar-field-arrow-${field}`);
const selector = this.sidebar.getByTestId(
`sidebar-field-arrow-enabled-${field}`
);
return selector.click();
}

Expand Down Expand Up @@ -92,6 +104,12 @@ export class SidebarPom {
await selectionDiv.click({ force: true });
}

async applySearch(field: string, search: string) {
const input = this.sidebar.getByTestId(`selector-sidebar-search-${field}`);
await input.fill(search);
await input.press("Enter");
}

// apply a filter to a field
async applyLabelFromList(
field: string,
Expand Down Expand Up @@ -137,9 +155,29 @@ class SidebarAsserter {
await expect(this.sb.field(fieldName)).toBeVisible();
}

async assertFieldDisabled(fieldName: string) {
await expect(this.sb.fieldArrow(fieldName, false)).toBeVisible();
}

async assertFieldsDisabled(fieldNames: string[]) {
for (let i = 0; i < fieldNames.length; i++) {
await this.assertFieldDisabled(fieldNames[i]);
}
}

async assertFieldEnabled(fieldName: string) {
await expect(this.sb.fieldArrow(fieldName, true)).toBeVisible();
}

async assertFieldsEnabled(fieldNames: string[]) {
for (let i = 0; i < fieldNames.length; i++) {
await this.assertFieldEnabled(fieldNames[i]);
}
}

async assertFieldsInSidebar(fieldNames: string[]) {
for (let i = 0; i < fieldNames.length; i++) {
await this.sb.asserter.assertFieldInSidebar(fieldNames[i]);
await this.assertFieldInSidebar(fieldNames[i]);
}
}

Expand Down
Loading
Loading