Fix dynamic groups modal shortcuts + dynamic groups refactoring to use schema #4510
Conversation
WalkthroughThese updates streamline event handling, enhance user interactions, and improve code maintainability across several components and hooks. Major changes include refinements in hover event handling, removal of redundant functions like Changes
Poem
Tip AI model upgrade
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
app/packages/state/src/recoil/utils.ts (1)
Line range hint
145-155: Replacing.map().flat()with.flatMap()simplifies the code and improves performance by reducing the complexity of the operation.- .map((parent) => - fieldsMatcher( - parent.fields || [], - labelsMatcher(parent), - undefined, - `${prefix}${parent.name}.` - ) - ) - .flat(); + .flatMap((parent) => + fieldsMatcher( + parent.fields || [], + labelsMatcher(parent), + undefined, + `${prefix}${parent.name}.` + ) + );app/packages/state/src/hooks/useCreateLooker.ts (1)
Line range hint
81-81: Avoid shadowing the globalconstructorproperty to prevent confusion and potential errors.- let constructor: + let lookerConstructor:
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- app/packages/core/src/components/Modal/Sample.tsx (1 hunks)
- app/packages/state/src/hooks/useCreateLooker.ts (2 hunks)
- app/packages/state/src/hooks/useExpandSample.ts (2 hunks)
- app/packages/state/src/hooks/useHoveredSample.ts (1 hunks)
- app/packages/state/src/recoil/dynamicGroups.ts (3 hunks)
- app/packages/state/src/recoil/utils.ts (1 hunks)
Additional context used
Path-based instructions (6)
app/packages/state/src/hooks/useHoveredSample.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/core/src/components/Modal/Sample.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/utils.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/state/src/hooks/useExpandSample.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/state/src/recoil/dynamicGroups.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/state/src/hooks/useCreateLooker.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.
Biome
app/packages/state/src/hooks/useHoveredSample.ts
[error] 12-12: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 16-16: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 20-20: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
app/packages/state/src/recoil/utils.ts
[error] 145-155: The call chain .map().flat() can be replaced with a single .flatMap() call. (lint/complexity/useFlatMap)
Safe fix: Replace the chain with .flatMap().
app/packages/state/src/recoil/dynamicGroups.ts
[error] 238-238: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
app/packages/state/src/hooks/useCreateLooker.ts
[error] 81-81: Do not shadow the global "constructor" property. (lint/suspicious/noShadowRestrictedNames)
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
[error] 241-241: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (8)
app/packages/state/src/hooks/useHoveredSample.ts (2)
7-7: Refactoring to use object destructuring for function parameters enhances clarity and maintainability.
Line range hint
7-20: The logic for handling mouse events and managing hover state is correctly implemented.Tools
Biome
[error] 12-12: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
app/packages/core/src/components/Modal/Sample.tsx (2)
44-44: The update to use the destructuredhoverEventHandlersfromuseHoveredSamplealigns with the new hook signature and improves code clarity.
44-62: Proper use of React hooks likeuseState,useRef, anduseCallbackensures efficient state management and component re-rendering.app/packages/state/src/hooks/useExpandSample.ts (1)
141-141: Direct usage ofgroupByfromdynamicGroupParameterssimplifies the code and removes unnecessary abstraction layers.app/packages/state/src/recoil/dynamicGroups.ts (1)
159-159: Consider using optional chaining for improved safety and readability.- expr: `$${groupBy}`, + expr: `$${groupBy}?`,Likely invalid or redundant comment.
app/packages/state/src/hooks/useCreateLooker.ts (2)
179-179: Implement optional chaining for safer access to properties.- const groupByFieldValue = get(sample, groupBy); + const groupByFieldValue = get(sample, groupBy)?;Likely invalid or redundant comment.
34-34: Ensure all necessary imports are present and correct given the removal ofgetSanitizedGroupByExpression.Verification successful
The verification confirms that no other files are attempting to import the removed function
getSanitizedGroupByExpression.
- No occurrences of
getSanitizedGroupByExpressionwere found in the TypeScript files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no other files are trying to import the removed function `getSanitizedGroupByExpression`. # Test: Search for the function usage. Expect: No occurrences outside of already modified files. rg --type typescript $'getSanitizedGroupByExpression'Length of output: 87
Script:
#!/bin/bash # Description: Verify that no other files are trying to import the removed function `getSanitizedGroupByExpression`. # Test: Search for the function usage. Expect: No occurrences outside of already modified files. rg --glob '*.ts' 'getSanitizedGroupByExpression'Length of output: 48
|
Looks like e2e is failing |
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/packages/state/src/hooks/useHoveredSample.ts (1 hunks)
Additional context used
Path-based instructions (1)
app/packages/state/src/hooks/useHoveredSample.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.
Biome
app/packages/state/src/hooks/useHoveredSample.ts
[error] 12-12: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 16-16: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 20-20: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (1)
app/packages/state/src/hooks/useHoveredSample.ts (1)
7-7: Refactor: Destructure function parameters for clarity.The change to destructure
updateandclearfunctions in the function parameters enhances readability and makes the function usage more explicit.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/packages/state/src/hooks/useHoveredSample.ts (1 hunks)
Additional context used
Path-based instructions (1)
app/packages/state/src/hooks/useHoveredSample.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.
Biome
app/packages/state/src/hooks/useHoveredSample.ts
[error] 12-12: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 16-16: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 20-20: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
| } | ||
| function onMouseLeave() { | ||
| setSample(null); | ||
| clear && clear(); | ||
| args?.clear && args.clear(); |
There was a problem hiding this comment.
Implement optional chaining for safety.
Similar to the previous comment, implement optional chaining for args.clear to prevent runtime errors when clear is undefined.
- args?.clear && args.clear();
+ args?.clear?.();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.
| args?.clear && args.clear(); | |
| args?.clear?.(); |
Tools
Biome
[error] 16-16: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
| function onMouseEnter() { | ||
| setSample(sample); | ||
| update && update(); | ||
| args?.update && args.update(); |
There was a problem hiding this comment.
Implement optional chaining for safety.
To ensure robustness, especially when args.update might be undefined, use optional chaining.
- args?.update && args.update();
+ args?.update?.();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.
| args?.update && args.update(); | |
| args?.update?.(); |
Tools
Biome
[error] 12-12: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
| } | ||
| function onMouseMove() { | ||
| setSample(sample); | ||
| update && update(); | ||
| args?.update && args.update(); |
There was a problem hiding this comment.
Consistency in optional chaining.
For consistency and to avoid potential errors, apply optional chaining to args.update in the onMouseMove handler as well.
- args?.update && args.update();
+ args?.update?.();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.
| args?.update && args.update(); | |
| args?.update?.(); |
Tools
Biome
[error] 20-20: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
There was a problem hiding this comment.
@sashankaryal I think _sample_id needs to be added back. Or more generally, if the field has a db_field defined in its schema, it should use that. Happy to take offline
Dynamic video is working with the below on develop, but not on this branch.
import fiftyone.zoo
dataset = foz.load_zoo_dataset("quickstart-video")
frames = dataset.to_frames(sample_frames=True).clone("quickstart-frames")
video = frames.group_by("sample_id", order_by="frame_number")
session = fo.launch_app(video)
sashankaryal
force-pushed
the
bugfix/dynamic-groups-modal-keys
branch
from
687745b to
0bf5a77
Compare
sashankaryal
force-pushed
the
bugfix/dynamic-groups-modal-keys
branch
from
0bf5a77 to
aa34ca6
Compare
sashankaryal
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
app/packages/state/src/hooks/useCreateLooker.ts (2)
Line range hint
82-82: Rename theconstructorvariable to avoid shadowing.Using
constructoras a variable name can lead to confusion due to its special meaning in JavaScript. Consider renaming it to something more specific likelookerConstructor.- let constructor: + let lookerConstructor:
Line range hint
246-246: Use optional chaining for safer property access.Implementing optional chaining here will prevent runtime errors if
sample.groupis undefined.- if (sample.group.name) { + if (sample.group?.name) {
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- app/packages/core/src/components/Modal/Sample.tsx (1 hunks)
- app/packages/state/src/hooks/useCreateLooker.ts (3 hunks)
- app/packages/state/src/hooks/useExpandSample.ts (3 hunks)
- app/packages/state/src/hooks/useHoveredSample.ts (1 hunks)
- app/packages/state/src/recoil/dynamicGroups.ts (4 hunks)
- app/packages/state/src/recoil/utils.ts (1 hunks)
- app/packages/utilities/src/schema.ts (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- app/packages/core/src/components/Modal/Sample.tsx
- app/packages/state/src/hooks/useExpandSample.ts
- app/packages/state/src/hooks/useHoveredSample.ts
- app/packages/state/src/recoil/utils.ts
Additional context used
Path-based instructions (3)
app/packages/utilities/src/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/state/src/recoil/dynamicGroups.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/state/src/hooks/useCreateLooker.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.
Biome
app/packages/state/src/recoil/dynamicGroups.ts
[error] 242-242: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/packages/state/src/hooks/useCreateLooker.ts
[error] 82-82: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (3)
app/packages/utilities/src/schema.ts (1)
4-4: Add documentation forpathWithDbField.The new
pathWithDbFieldproperty in theFieldinterface is a welcome addition for better handling of database fields. Consider adding documentation to explain its usage and significance.app/packages/state/src/recoil/dynamicGroups.ts (1)
7-7: Check for potential impacts due to removal ofgetSanitizedGroupByExpression.The removal of
getSanitizedGroupByExpressionand the addition ofgetFieldInfoare in line with the PR's objectives. However, ensure that all parts of the application that previously depended on the sanitized expressions are updated to work with the directgroupByusage.Verification successful
Check for potential impacts due to removal of
getSanitizedGroupByExpression.The removal of
getSanitizedGroupByExpressionand the addition ofgetFieldInfoare in line with the PR's objectives. The verification confirms that there are no remaining references togetSanitizedGroupByExpressionin the codebase.
- No further action is needed as the transition has been successfully completed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all parts of the application have transitioned from `getSanitizedGroupByExpression` to `getFieldInfo`. # Test: Search for any remaining references to `getSanitizedGroupByExpression`. Expect: None. rg --type typescript $'getSanitizedGroupByExpression'Length of output: 87
Script:
#!/bin/bash # Description: Verify that all parts of the application have transitioned from `getSanitizedGroupByExpression` to `getFieldInfo`. # Test: Search for any remaining references to `getSanitizedGroupByExpression`. Expect: None. rg --type ts 'getSanitizedGroupByExpression'Length of output: 44
app/packages/state/src/hooks/useCreateLooker.ts (1)
16-16: Ensure complete removal of dependencies ongetSanitizedGroupByExpression.The removal of
getSanitizedGroupByExpressionshould simplify the logic. Ensure that no residual code depends on this function, potentially leading to errors.Verification successful
Verified complete removal of
getSanitizedGroupByExpression.The search confirmed that there are no remaining references to
getSanitizedGroupByExpressionin the codebase, ensuring the logic simplification is fully implemented.
- No further action required.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the complete removal of `getSanitizedGroupByExpression` and its dependencies. # Test: Search for any remaining references to `getSanitizedGroupByExpression`. Expect: None. rg --type typescript $'getSanitizedGroupByExpression'Length of output: 87
Script:
#!/bin/bash # Description: Verify the complete removal of `getSanitizedGroupByExpression` and its dependencies. # Test: Search for any remaining references to `getSanitizedGroupByExpression`. Expect: None. rg --type ts 'getSanitizedGroupByExpression'Length of output: 44
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/packages/utilities/src/schema.test.ts (3 hunks)
- app/packages/utilities/src/schema.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/packages/utilities/src/schema.ts
Additional context used
Path-based instructions (1)
app/packages/utilities/src/schema.test.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 not posted (2)
app/packages/utilities/src/schema.test.ts (2)
38-61: Ensure Consistency in Field PropertiesThe
embeddedWithDbFieldsfield is added correctly. However, ensure that all nested properties, such asdbField,pathWithDbField, and others, are consistently applied across similar fields in the schema.
84-87: Ensure Correctness ofpathWithDbFieldPropertyThe test cases for
getFieldInfofunction are updated to includepathWithDbField. Verify that the expected values forpathWithDbFieldare correct and consistent with the schema's structure.Also applies to: 91-94
Verification successful
Ensure Correctness of
pathWithDbFieldPropertyThe test cases for the
getFieldInfofunction correctly reflect the assignment logic ofpathWithDbField. The property is assigned by joining non-nulldbFieldvalues, and the test cases expectpathWithDbFieldto be an empty string when nodbFieldvalues are present.
app/packages/utilities/src/schema.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correctness of `pathWithDbField` property in the schema. # Test: Search for all occurrences of `pathWithDbField` in the schema file. Expect: Correct and consistent values. rg --type json --json-path '$..pathWithDbField'Length of output: 485
Script:
#!/bin/bash # Description: Verify correctness of `pathWithDbField` property in the schema. # Search for all occurrences of `pathWithDbField` in the codebase. rg 'pathWithDbField'Length of output: 949
Script:
#!/bin/bash # Description: Extract and review the relevant code sections in `schema.ts` where `pathWithDbField` is assigned and used. # Extract lines around the assignment of `pathWithDbField` in `schema.ts`. rg -A 5 -B 5 'pathWithDbField' app/packages/utilities/src/schema.tsLength of output: 540
| it("should return correct pathWithDbField", () => { | ||
| const field = schema.getFieldInfo( | ||
| "embeddedWithDbFields.sample_id", | ||
| SCHEMA | ||
| ); | ||
| console.log("field is ", JSON.stringify(field, null, 2)); | ||
| expect(field?.pathWithDbField).toBe("embeddedWithDbFields._sample_id"); | ||
| }); |
There was a problem hiding this comment.
Remove Debugging Code
The console.log statement is used for debugging purposes and should be removed in the final code.
- console.log("field is ", JSON.stringify(field, null, 2));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.
| it("should return correct pathWithDbField", () => { | |
| const field = schema.getFieldInfo( | |
| "embeddedWithDbFields.sample_id", | |
| SCHEMA | |
| ); | |
| console.log("field is ", JSON.stringify(field, null, 2)); | |
| expect(field?.pathWithDbField).toBe("embeddedWithDbFields._sample_id"); | |
| }); | |
| it("should return correct pathWithDbField", () => { | |
| const field = schema.getFieldInfo( | |
| "embeddedWithDbFields.sample_id", | |
| SCHEMA | |
| ); | |
| expect(field?.pathWithDbField).toBe("embeddedWithDbFields._sample_id"); | |
| }); |
There was a problem hiding this comment.
@sashankaryal I agree with 🐇 if you do
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| it("should return correct pathWithDbField", () => { | ||
| const field = schema.getFieldInfo( | ||
| "embeddedWithDbFields.sample_id", | ||
| SCHEMA | ||
| ); | ||
| console.log("field is ", JSON.stringify(field, null, 2)); | ||
| expect(field?.pathWithDbField).toBe("embeddedWithDbFields._sample_id"); | ||
| }); |
There was a problem hiding this comment.
@sashankaryal I agree with 🐇 if you do
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/packages/utilities/src/schema.test.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/packages/utilities/src/schema.test.ts
This PR fixes the bug where hover handlers were not being used to update
fos.hoveredSample. I also removed usage ofgetSanitizedGroupByExpressionthat had a special case for_sample_id. Instead, we construct the dynamic groups key value fromdbFields in the schema.Summary by CodeRabbit
New Features
pathWithDbFieldproperty to theFieldinterface to store concatenated paths ofdbFieldvalues for enhanced schema information.Bug Fixes
SampleWrappercomponent to enhance user interaction.Refactor
useHoveredSampleby destructuring an object forupdateandclearfunctions.Tests
pathWithDbFieldproperty in schema queries.Chores
getSanitizedGroupByExpressionfunction across multiple files to streamline codebase.