Render summary fields in modal sidebar #4851
Conversation
WalkthroughThe changes involve updates to several components and utilities within the application, focusing on enhancing type safety, improving formatting logic, and refining component structure and styling. Key modifications include the introduction of new formatting functions in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
app/packages/utilities/src/index.ts (1)
635-651: Approve changes with suggestion: Improve type handlingThe updates to
formatPrimitiveenhance type safety and improve code clarity. However, there are a few areas that could be further improved:
The use of
// @ts-ignorecomments suggests potential type issues. Consider using type guards or type assertions to handle these cases more safely.For the
DATE_FIELDandDATE_TIME_FIELDcases, you could create a type guard to check ifvalueis of type{ datetime: number }instead of using type assertions.Consider refactoring the function to improve type safety:
function isDateTimeValue(value: Primitive): value is { datetime: number } { return typeof value === 'object' && value !== null && 'datetime' in value; } export const formatPrimitive = ({ ftype, timeZone, value, }: { ftype: string; timeZone: string; value: Primitive; }) => { if (value === null || value === undefined) return null; switch (ftype) { case FRAME_SUPPORT_FIELD: return Array.isArray(value) ? `[${value[0]}, ${value[1]}]` : null; case DATE_FIELD: return isDateTimeValue(value) ? formatDate(value.datetime) : null; case DATE_TIME_FIELD: return isDateTimeValue(value) ? formatDateTime(value.datetime, timeZone) : null; } return prettify(value); };This refactoring eliminates the need for
// @ts-ignorecomments and provides better type safety.app/packages/state/src/recoil/schema.ts (1)
789-793: Improved robustness, but consider a minor enhancementThe changes to the
isOfDocumentFieldListselector improve its robustness by handling the case when there is no parent path. This prevents potential errors and aligns with TypeScript best practices.Consider extracting the parent path calculation to a separate variable for improved readability:
- const parent = path.split(".").slice(0, -1).join("."); - if (!parent) { + const parentPath = path.split(".").slice(0, -1).join("."); + if (!parentPath) { return false; } - const f = get(field(parent)); + const f = get(field(parentPath));This minor change would make the code slightly more self-documenting and easier to understand at a glance.
app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx (2)
289-291: Key attribute usestoString()for better type consistency.The
keyattribute in the mappeddivelements now usesi.toString()instead of justi. This ensures type consistency and avoids potential issues with key uniqueness.
323-323: Key attribute usestoString()for better type consistency.The
keyattribute in thedivelement now usesi.toString()instead of justi. This ensures type consistency and avoids potential issues with key uniqueness.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx (10 hunks)
- app/packages/state/src/recoil/schema.ts (2 hunks)
- app/packages/utilities/src/index.ts (2 hunks)
- e2e-pw/src/oss/poms/modal/modal-sidebar.ts (1 hunks)
- e2e-pw/src/oss/specs/smoke-tests/summary-fields.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/state/src/recoil/schema.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/utilities/src/index.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/poms/modal/modal-sidebar.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/specs/smoke-tests/summary-fields.spec.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (14)
app/packages/utilities/src/index.ts (1)
621-626: LGTM: New Primitive type improves type safetyThe new
Primitivetype alias enhances type safety for theformatPrimitivefunction. It accurately represents the possible types that can be formatted, including date-time values.e2e-pw/src/oss/specs/smoke-tests/summary-fields.spec.ts (2)
6-13: Well-structured extension of test fixturesThe extension of the base test with the
gridandmodalfixtures is correctly implemented. This enhances modularity and reusability, adhering to best practices in test design.
54-57: Accurate verification of summary fieldsThe assertions for verifying the
summaryandsummariesfields are correctly implemented. This ensures that the expected data is rendered in the modal sidebar, aligning with the test objectives.Also applies to: 64-68
e2e-pw/src/oss/poms/modal/modal-sidebar.ts (1)
129-139: LGTM: New method implements object verification correctlyThe
verifyObjectmethod correctly verifies the key-value pairs in the sidebar entries.app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx (10)
3-8: Imports are organized and use proper types.The import statements have been updated to include the necessary types (
PrimitiveandSchema) and constants (EMBEDDED_DOCUMENT_FIELD,formatPrimitive,makePseudoField). This improves type safety and code organization.
59-62: Styling for links is consistent with the theme.The new styles for links within
ScalarDivuse the primary text color from the theme. This ensures consistency in link styling across the application.
128-130: Flexbox properties enhance the layout of the list container.The addition of flexbox properties (
display: flex,flex-direction: column,row-gap: 0.5rem) toListContainerimproves the layout and spacing of list items. This enhances the readability and visual structure of the list.
200-200: Data attribute improves testability.The
data-cyattribute has been added to the arrow icon, enabling more precise targeting of this element in end-to-end tests. This improves the testability and maintainability of the component.
241-257: Type safety and formatting logic improved inListLoadable.The
ListLoadablecomponent now usesPrimitive[]instead ofunknown[]for thedatatype, enhancing type safety. Theformatfunction is used to handle the formatting of values based on their field type, ensuring consistent formatting across different data types.
259-265: Empty state handling simplified inListLoadable.The handling of empty states in
ListLoadablehas been simplified by removing the fragment wrapper around the "No results" message. This improves code readability without affecting functionality.
312-312: Formatting logic extracted toformatfunction.The formatting logic for the slice value has been extracted to the
formatfunction, which takes theftype,value, andtimeZoneas parameters. This improves code organization and reusability.
382-390: Formatting logic extracted toformatfunction and memoized.The formatting logic has been extracted to the
formatfunction, which takes thefields,ftype,timeZone, andvalueas parameters. The result is memoized usinguseMemoto avoid unnecessary re-computations. This improves performance and code organization.
395-395: Event propagation stopped for better control.The
onKeyDownevent handler now stops event propagation usinge.stopPropagation(). This prevents the event from bubbling up to parent elements, providing better control over event handling.
471-545: New formatting functions improve code organization and reusability.Several new formatting functions have been introduced:
format: Handles the formatting of values based on their field type, including a new check forEMBEDDED_DOCUMENT_FIELDto format objects differently.formatPrimitiveOrURL: Manages the rendering of URLs, allowing them to be displayed as clickable links.formatObject: Formats embedded documents by iterating over their entries and applying the appropriate formatting to each key-value pair.These functions improve code organization, reusability, and maintainability by encapsulating specific formatting logic and allowing for easier extension and modification.
| 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() | ||
| `); |
There was a problem hiding this comment.
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; | ||
| await modal.sidebar.assert.verifyObject("summaries", { | ||
| five: "six", | ||
| seven: "eight", | ||
| nine: "ten", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
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()
`);
});| const entryExpandPromise = eventUtils.getEventReceivedPromiseForPredicate( | ||
| "animation-onRest", | ||
| () => true | ||
| ); | ||
| await modal.sidebar.clickFieldDropdown("summaries"); | ||
| await entryExpandPromise; |
There was a problem hiding this comment.
🛠️ 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.
| for (const k in obj) { | ||
| const v = obj[k]; | ||
| const entry = locator.getByTestId(`key-value-${k}-${v}`); |
There was a problem hiding this comment.
🛠️ 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.
| 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}`); |
| export type Primitive = | ||
| | number | ||
| | null | ||
| | string | ||
| | undefined | ||
| | { datetime: number }; | ||
|
|
There was a problem hiding this comment.
👌
(nit: although FoPrimitive would probably make it less confusing elsewhere)
What changes are proposed in this pull request?
Before
After
How is this patch tested? If it is not, please explain why.
e2e spec
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Documentation