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

Render summary fields in modal sidebar #4851

Merged
merged 5 commits into from
Sep 27, 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
139 changes: 122 additions & 17 deletions app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import { LoadingDots, useTheme } from "@fiftyone/components";
import * as fos from "@fiftyone/state";
import { formatPrimitive, makePseudoField } from "@fiftyone/utilities";
import type { Primitive, Schema } from "@fiftyone/utilities";
import {
EMBEDDED_DOCUMENT_FIELD,
formatPrimitive,
makePseudoField,
} from "@fiftyone/utilities";
import { KeyboardArrowDown, KeyboardArrowUp } from "@mui/icons-material";
import { useSpring } from "@react-spring/core";
import React, { Suspense, useMemo, useState } from "react";
Expand Down Expand Up @@ -51,6 +56,10 @@ const ScalarDiv = styled.div`
&.expanded > div {
white-space: unset;
}

& a {
color: ${({ theme }) => theme.text.primary};
}
`;

const ScalarValueEntry = ({
Expand Down Expand Up @@ -116,9 +125,11 @@ const ListContainer = styled(ScalarDiv)`
color: ${({ theme }) => theme.text.secondary};
margin-top: 0.25rem;
padding: 0.25rem 0.5rem;
display: flex;
flex-direction: column;
row-gap: 0.5rem;

& > div {
margin-bottom: 0.5rem;
white-space: unset;
}
`;
Expand Down Expand Up @@ -186,6 +197,7 @@ const ListValueEntry = ({
</span>
<Arrow
key="arrow"
data-cy={`sidebar-field-arrow-enabled-${path}`}
style={{ cursor: "pointer", margin: 0 }}
onClick={(event) => {
event.preventDefault();
Expand Down Expand Up @@ -226,20 +238,31 @@ const LengthLoadable = ({ path }: { path: string }) => {
};

const ListLoadable = ({ path }: { path: string }) => {
const data = useData<unknown[]>(path);
const data = useData<Primitive[]>(path);
const { fields, ftype, subfield } = fos.useAssertedRecoilValue(
fos.field(path)
);
const timeZone = useRecoilValue(fos.timeZone);

const field = subfield || ftype;
if (!field) {
throw new Error(`expected an ftype for ${path}`);
}

const values = useMemo(() => {
return data
? Array.from(data).map((value) => prettify(value as string))
: [];
}, [data]);
return Array.from(data || []).map((value) =>
format({ fields, ftype: field, value, timeZone })
);
}, [data, field, fields, timeZone]);

return (
<ListContainer>
<ListContainer data-cy={`sidebar-entry-${path}`}>
{values.map((v, i) => (
<div key={i} title={typeof v === "string" ? v : undefined}>
{v}
<div key={i.toString()} title={typeof v === "string" ? v : undefined}>
{v === null ? "None" : v}
</div>
))}
{values.length === 0 && <>No results</>}
{values.length === 0 && "No results"}
</ListContainer>
);
};
Expand All @@ -263,9 +286,9 @@ const SlicesListLoadable = ({ path }: { path: string }) => {
{slice}
</div>
{(data || []).map((value, i) => (
<div key={i}>{prettify(value as string)}</div>
<div key={i.toString()}>{prettify(value as string)}</div>
))}
{(!data || !data.length) && <>No results</>}
{(!data || !data.length) && "No results"}
</ListContainer>
);
})}
Expand All @@ -286,7 +309,7 @@ const SlicesLoadable = ({ path }: { path: string }) => {
<>
{Object.entries(values).map(([slice, value], i) => {
const none = value === null || value === undefined;
const formatted = formatPrimitive({ ftype, value, timeZone });
const formatted = format({ ftype, value, timeZone });

const add = none ? { color } : {};
return (
Expand All @@ -297,7 +320,7 @@ const SlicesLoadable = ({ path }: { path: string }) => {
columnGap: "0.5rem",
marginBottom: "0.5rem",
}}
key={i}
key={i.toString()}
>
<div style={{ color: theme.text.secondary }}>{slice}</div>
<div
Expand Down Expand Up @@ -356,14 +379,20 @@ const useSlicesData = <T,>(path: string) => {
const Loadable = ({ path }: { path: string }) => {
const value = useData<string | number | null>(path);
const none = value === null || value === undefined;
const { ftype } = useRecoilValue(fos.field(path)) ?? makePseudoField(path);
const { fields, ftype } =
useRecoilValue(fos.field(path)) ?? makePseudoField(path);
const color = useRecoilValue(fos.pathColor(path));
const timeZone = useRecoilValue(fos.timeZone);
const formatted = formatPrimitive({ ftype, value, timeZone });

const formatted = useMemo(
() => format({ fields, ftype, timeZone, value }),
[fields, ftype, timeZone, value]
);

return (
<div
data-cy={`sidebar-entry-${path}`}
onKeyDown={(e) => e.stopPropagation()}
onClick={(e) => e.stopPropagation()}
style={none ? { color } : {}}
title={typeof formatted === "string" ? formatted : undefined}
Expand Down Expand Up @@ -439,4 +468,80 @@ const PathValueEntry = ({
);
};

interface PrimitivesObject {
[key: string]: Primitive;
}

type Primitives = Primitive | PrimitivesObject;

const format = ({
fields,
ftype,
timeZone,
value,
}: {
fields?: Schema;
ftype: string;
timeZone: string;
value: Primitives;
}) => {
if (ftype === EMBEDDED_DOCUMENT_FIELD && typeof value === "object") {
return formatObject({ fields, timeZone, value: value as object });
}

return formatPrimitiveOrURL({ ftype, value: value as Primitive, timeZone });
};

const formatPrimitiveOrURL = (params: {
fields?: Schema;
ftype: string;
timeZone: string;
value: Primitive;
}) => {
const result = formatPrimitive(params);

return result instanceof URL ? (
<a href={result.toString()} target="_blank" rel="noreferrer">
{result.toString()}
</a>
) : (
result
);
};

const formatObject = ({
fields,
timeZone,
value,
}: {
fields?: Schema;
timeZone: string;
value: object;
}) => {
return Object.entries(value)
.map(([k, v]) => {
if (!fields?.[k]?.ftype) {
return null;
}

const text = formatPrimitiveOrURL({
ftype: fields?.[k]?.ftype,
timeZone,
value: v,
});

return (
<div
data-cy={`key-value-${k}-${text}`}
key={k}
style={{ display: "flex", justifyContent: "space-between" }}
>
<span data-cy={`key-${k}`}>{k}</span>
<span data-cy={`value-${text}`}>{text}</span>
</div>
);
})
.filter((entry) => Boolean(entry));
};

export default React.memo(PathValueEntry);
10 changes: 7 additions & 3 deletions app/packages/state/src/recoil/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ import {
LABEL_LISTS,
LABEL_LISTS_MAP,
LIST_FIELD,
meetsFieldType,
OBJECT_ID_FIELD,
STRING_FIELD,
Schema,
StrictField,
STRING_FIELD,
VALID_NUMERIC_TYPES,
VALID_PRIMITIVE_TYPES,
meetsFieldType,
withPath,
} from "@fiftyone/utilities";
import { RecoilState, selector, selectorFamily } from "recoil";
Expand Down Expand Up @@ -786,7 +786,11 @@ export const isOfDocumentFieldList = selectorFamily({
get:
(path: string) =>
({ get }) => {
const f = get(field(path.split(".")[0]));
const parent = path.split(".").slice(0, -1).join(".");
if (!parent) {
return false;
}
const f = get(field(parent));

return [
DYNAMIC_EMBEDDED_DOCUMENT_FIELD,
Expand Down
24 changes: 15 additions & 9 deletions app/packages/utilities/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ import _ from "lodash";
import mime from "mime";
import { Field } from "./schema";

export * from "./Resource";
export * from "./buffer-manager";
export * from "./color";
export * from "./errors";
export * from "./fetch";
export * from "./order";
export * from "./paths";
export * from "./Resource";
export * from "./schema";
export * as styles from "./styles";
export * from "./type-check";
Expand Down Expand Up @@ -618,31 +618,37 @@ export const formatDate = (timeStamp: number): string => {
.replaceAll("/", "-");
};

export type Primitive =
| number
| null
| string
| undefined
| { datetime: number };

Comment on lines +621 to +627
Copy link
Contributor

Choose a reason for hiding this comment

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

👌
(nit: although FoPrimitive would probably make it less confusing elsewhere)

export const formatPrimitive = ({
ftype,
timeZone,
value,
}: {
ftype: string;
timeZone: string;
value: unknown;
value: Primitive;
}) => {
if (value === null || value === undefined) return undefined;
if (value === null || value === undefined) return null;

switch (ftype) {
case FRAME_SUPPORT_FIELD:
value = `[${value[0]}, ${value[1]}]`;
break;
return `[${value[0]}, ${value[1]}]`;
case DATE_FIELD:
// @ts-ignore
value = formatDate(value.datetime as number);
break;
return formatDate(value?.datetime as number);
case DATE_TIME_FIELD:
// @ts-ignore
value = formatDateTime(value.datetime as number, timeZone);
return formatDateTime(value?.datetime as number, timeZone);
}

return prettify(String(value));
// @ts-ignore
return prettify(value);
};

export const makePseudoField = (path: string): Field => ({
Expand Down
12 changes: 12 additions & 0 deletions e2e-pw/src/oss/poms/modal/modal-sidebar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,18 @@ class SidebarAsserter {
);
}

async verifyObject(key: string, obj: { [key: string]: string }) {
const locator = this.modalSidebarPom.getSidebarEntry(key);

for (const k in obj) {
const v = obj[k];
const entry = locator.getByTestId(`key-value-${k}-${v}`);
Comment on lines +132 to +134
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use Object.entries with for...of to iterate over object properties

Using for...in can iterate over inherited enumerable properties, which might not be desired. It's safer to use Object.entries(obj) with a for...of loop to iterate over the object's own properties.

Apply this diff to implement the change:

-    for (const k in obj) {
-      const v = obj[k];
+    for (const [k, v] of Object.entries(obj)) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for (const k in obj) {
const v = obj[k];
const entry = locator.getByTestId(`key-value-${k}-${v}`);
for (const [k, v] of Object.entries(obj)) {
const entry = locator.getByTestId(`key-value-${k}-${v}`);


await expect(entry.getByTestId(`key-${k}`)).toHaveText(k);
await expect(entry.getByTestId(`value-${v}`)).toHaveText(v);
}
}

async verifyLabelTagCount(count: number) {
await this.modalSidebarPom.page.waitForFunction(
(count_) => {
Expand Down
70 changes: 70 additions & 0 deletions e2e-pw/src/oss/specs/smoke-tests/summary-fields.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import { test as base } from "src/oss/fixtures";
import { GridPom } from "src/oss/poms/grid";
import { ModalPom } from "src/oss/poms/modal";
import { getUniqueDatasetNameWithPrefix } from "src/oss/utils";

const test = base.extend<{ grid: GridPom; modal: ModalPom }>({
grid: async ({ page, eventUtils }, use) => {
await use(new GridPom(page, eventUtils));
},
modal: async ({ page, eventUtils }, use) => {
await use(new ModalPom(page, eventUtils));
},
});

const datasetName = getUniqueDatasetNameWithPrefix("summary-fields");

test.describe("summary fields", () => {
test.beforeAll(async ({ fiftyoneLoader }) => {
await fiftyoneLoader.executePythonCode(`
import fiftyone as fo

dataset = fo.Dataset("${datasetName}")
dataset.persistent = True
dataset.add_sample(
fo.Sample(
filepath=f"image.png",
summary=fo.DynamicEmbeddedDocument(one="two", three="four"),
summaries=[
fo.DynamicEmbeddedDocument(five="six", seven="eight"),
fo.DynamicEmbeddedDocument(nine="ten"),
],
)
)
dataset.app_config.sidebar_groups = [
fo.SidebarGroupDocument(
name="summaries", paths=["summary", "summaries"], expanded=True
)
]
dataset.save()
dataset.add_dynamic_sample_fields()
`);
Comment on lines +19 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure safe interpolation of datasetName in Python code

While interpolating datasetName into the Python code string, there's a minimal risk of code injection if datasetName contains unintended characters. Although datasetName is generated internally, it's good practice to sanitize or validate variables before injecting them into executable code to prevent potential security issues.

Consider explicitly validating or sanitizing datasetName before interpolation:

const sanitizedDatasetName = datasetName.replace(/[^a-zA-Z0-9_\-]/g, "");

And then use sanitizedDatasetName in your Python code.

});

test("modal sidebar summary fields render", async ({
eventUtils,
fiftyoneLoader,
grid,
modal,
page,
}) => {
await fiftyoneLoader.waitUntilGridVisible(page, datasetName);
await grid.openFirstSample();
await modal.waitForSampleLoadDomAttribute(true);
await modal.sidebar.assert.verifyObject("summary", {
one: "two",
three: "four",
});
const entryExpandPromise = eventUtils.getEventReceivedPromiseForPredicate(
"animation-onRest",
() => true
);
await modal.sidebar.clickFieldDropdown("summaries");
await entryExpandPromise;
Comment on lines +58 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refine event predicate to prevent flaky tests

The predicate function () => true used in getEventReceivedPromiseForPredicate will resolve on any animation-onRest event, which may not be specific to the desired interaction. This could cause the test to pass prematurely if multiple animations occur.

Refine the predicate to ensure it matches the specific element or context:

const entryExpandPromise = eventUtils.getEventReceivedPromiseForPredicate(
  "animation-onRest",
  (event) => event.target === expectedTargetElement
);

Replace expectedTargetElement with a reference to the element associated with the animation you intend to wait for.

await modal.sidebar.assert.verifyObject("summaries", {
five: "six",
seven: "eight",
nine: "ten",
});
});
Comment on lines +44 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add cleanup logic to remove the dataset after tests

The test creates a persistent dataset in the beforeAll hook but does not remove it after execution. This could lead to leftover test data interfering with other tests or cluttering the environment.

Implement an afterAll hook to delete the dataset post-testing:

test.afterAll(async ({ fiftyoneLoader }) => {
  await fiftyoneLoader.executePythonCode(`
    import fiftyone as fo
    if fo.dataset_exists("${datasetName}"):
        dataset = fo.load_dataset("${datasetName}")
        dataset.delete()
  `);
});

});
Loading