-
Notifications
You must be signed in to change notification settings - Fork 589
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
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
formatPrimitive
enhance type safety and improve code clarity. However, there are a few areas that could be further improved:
The use of
// @ts-ignore
comments suggests potential type issues. Consider using type guards or type assertions to handle these cases more safely.For the
DATE_FIELD
andDATE_TIME_FIELD
cases, you could create a type guard to check ifvalue
is 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-ignore
comments 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
isOfDocumentFieldList
selector 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
key
attribute in the mappeddiv
elements 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
key
attribute in thediv
element 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
Primitive
type alias enhances type safety for theformatPrimitive
function. 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
grid
andmodal
fixtures 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
summary
andsummaries
fields 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
verifyObject
method 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 (
Primitive
andSchema
) 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
ScalarDiv
use 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
) toListContainer
improves 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-cy
attribute 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
ListLoadable
component now usesPrimitive[]
instead ofunknown[]
for thedata
type, enhancing type safety. Theformat
function 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
ListLoadable
has been simplified by removing the fragment wrapper around the "No results" message. This improves code readability without affecting functionality.
312-312
: Formatting logic extracted toformat
function.The formatting logic for the slice value has been extracted to the
format
function, which takes theftype
,value
, andtimeZone
as parameters. This improves code organization and reusability.
382-390
: Formatting logic extracted toformat
function and memoized.The formatting logic has been extracted to the
format
function, which takes thefields
,ftype
,timeZone
, andvalue
as parameters. The result is memoized usinguseMemo
to avoid unnecessary re-computations. This improves performance and code organization.
395-395
: Event propagation stopped for better control.The
onKeyDown
event 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_FIELD
to 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
for (const k in obj) { | ||
const v = obj[k]; | ||
const entry = locator.getByTestId(`key-value-${k}-${v}`); |
There was a problem hiding this comment.
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.
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}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! love the tests
export type Primitive = | ||
| number | ||
| null | ||
| string | ||
| undefined | ||
| { datetime: number }; | ||
|
There was a problem hiding this comment.
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)
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?
fiftyone
Python library changesSummary by CodeRabbit
New Features
Bug Fixes
Documentation