[WIP] Handle sidebar groups config update #5191
Conversation
WalkthroughThe changes primarily involve updates to the sidebar module in the Recoil state management system, focusing on import path modifications and the introduction of a new function, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (7)
app/packages/state/src/recoil/sidebar/sidebar-utils.test.ts (1)
1-6: Consider adding type imports for better type safetyWhile the imports are well-organized, adding type information would improve type safety and documentation.
import { describe, expect, it, vi } from "vitest"; +import type { Group } from "./types"; // Assuming there's a types file vi.mock("recoil"); vi.mock("recoil-relay"); -import { merge, mergeGroups } from "./sidebar-utils"; +import type { merge, mergeGroups } from "./sidebar-utils";app/packages/state/src/recoil/sidebar/sidebar-utils.ts (3)
3-9: Add input validation for edge casesThe function should validate its inputs to handle edge cases gracefully:
- Check if arrays are defined and non-null
- Validate if the key exists in the source array before accessing indices
const hasNeighbor = (sink: string[], source: string[], key: string) => { + if (!sink?.length || !source?.length || !key) return false; const index = source.indexOf(key); + if (index === -1) return false; const before = source[index - 1]; const after = source[index + 1]; return sink.includes(before) || sink.includes(after); };
58-89: Consider breaking down complex logic into smaller functionsThe function handles multiple responsibilities: mapping creation, key merging, and path merging. Breaking these into separate functions would improve maintainability and testability.
Consider refactoring into smaller, focused functions:
const createGroupMappings = ( sink: State.SidebarGroup[], source: State.SidebarGroup[] ) => { return { sinkMapping: Object.fromEntries(sink.map((g) => [g.name, { ...g }])), sourceMapping: Object.fromEntries(source.map((g) => [g.name, { ...g }])), }; }; const mergePaths = ( mapping: Record<string, State.SidebarGroup>, source: State.SidebarGroup[], sourceKeys: string[] ) => { // Path merging logic here }; export const mergeGroups = ( sink: State.SidebarGroup[], source: State.SidebarGroup[] ): State.SidebarGroup[] => { const { sinkMapping, sourceMapping } = createGroupMappings(sink, source); // ... rest of the logic };
1-89: Consider performance implications for large datasetsThe merge operations perform multiple array iterations and could be expensive for large datasets. Consider:
- Adding memoization for repeated merge operations
- Using Map instead of Object.fromEntries for better lookup performance
- Documenting performance characteristics and limitations
Would you like assistance in implementing these performance optimizations?
app/packages/state/src/recoil/sidebar/index.ts (2)
Line range hint
214-293: Consider breaking down resolveGroups for better maintainability.The function handles multiple responsibilities including:
- Group resolution
- Expansion state management
- Special case handling for video/image datasets
- Path management
Consider breaking it down into smaller, more focused functions for better maintainability and testability.
Example refactor:
-export const resolveGroups = ( +const resolveExpansionState = ( + groups: State.SidebarGroup[], + configGroups: State.SidebarGroup[] +): State.SidebarGroup[] => { + const expanded = configGroups.reduce((map, { name, expanded }) => { + map[name] = expanded; + return map; + }, {}); + + return groups.map((group) => { + return typeof expanded[group.name] === "boolean" + ? { ...group, expanded: expanded[group.name] } + : { ...group }; + }); +}; + +const ensureRequiredGroups = ( + groups: State.SidebarGroup[] +): State.SidebarGroup[] => { + // Handle metadata and tags group logic + // Return updated groups +}; + +export const resolveGroups = ( sampleFields: StrictField[], frameFields: StrictField[], currentGroups: State.SidebarGroup[], configGroups: State.SidebarGroup[] ): State.SidebarGroup[] => { let groups = currentGroups.length ? currentGroups : configGroups.length ? configGroups : frameFields.length ? DEFAULT_VIDEO_GROUPS : DEFAULT_IMAGE_GROUPS; if (currentGroups.length && configGroups.length) { groups = mergeGroups(groups, configGroups); } - const expanded = configGroups.reduce((map, { name, expanded }) => { - map[name] = expanded; - return map; - }, {}); - - groups = groups.map((group) => { - return typeof expanded[group.name] === "boolean" - ? { ...group, expanded: expanded[group.name] } - : { ...group }; - }); + groups = resolveExpansionState(groups, configGroups); + groups = ensureRequiredGroups(groups);
Line range hint
214-293: Add error handling for critical state operations.Critical operations like group merging and resolution lack proper error handling. Consider:
- Validating input data structures
- Handling edge cases (empty arrays, null values)
- Adding error boundaries for state updates
This will improve the robustness of the sidebar state management.
Example implementation:
export const resolveGroups = ( sampleFields: StrictField[], frameFields: StrictField[], currentGroups: State.SidebarGroup[], configGroups: State.SidebarGroup[] ): State.SidebarGroup[] => { + if (!Array.isArray(sampleFields) || !Array.isArray(frameFields)) { + throw new Error('Invalid field arrays provided to resolveGroups'); + } + + if (!Array.isArray(currentGroups) || !Array.isArray(configGroups)) { + throw new Error('Invalid groups arrays provided to resolveGroups'); + } let groups = currentGroups.length ? currentGroups : configGroups.length ? configGroups : frameFields.length ? DEFAULT_VIDEO_GROUPS : DEFAULT_IMAGE_GROUPS;Also applies to: 456-481
fiftyone/server/query.py (1)
Line range hint
588-596: Specify exception types inexceptclauseUsing a bare
except:clause can catch unexpected exceptions, including system exit events and keyboard interrupts, which may hinder debugging. It's best practice to catch specific exceptions to ensure proper error handling.Apply this diff to catch specific exceptions:
try: doc = dataset._get_saved_view_doc(saved_view_slug, slug=True) view = dataset.load_saved_view(doc.name) view_name = view.name if serialized_view: for stage in serialized_view: view = view.add_stage(fosg.ViewStage._from_dict(stage)) -except: +except Exception: view = fov.DatasetView._build(dataset, serialized_view or [])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
app/packages/state/src/recoil/sidebar/index.ts(3 hunks)app/packages/state/src/recoil/sidebar/sidebar-utils.test.ts(1 hunks)app/packages/state/src/recoil/sidebar/sidebar-utils.ts(1 hunks)fiftyone/core/singletons.py(2 hunks)fiftyone/core/state.py(1 hunks)fiftyone/server/query.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/state/src/recoil/sidebar/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.
app/packages/state/src/recoil/sidebar/sidebar-utils.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.
app/packages/state/src/recoil/sidebar/sidebar-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.
🔇 Additional comments (5)
fiftyone/core/state.py (1)
100-102: Consider documenting the use of private parameters
The use of private parameters _create=False and _force_load=True suggests this is an internal implementation detail. While the change appears to optimize dataset loading, it would be beneficial to document why these specific flags are necessary.
Let's verify the dataset loading behavior across the codebase:
Consider adding a comment explaining:
- Why
_create=Falseis necessary - The implications of
_force_load=True - Any performance considerations
✅ Verification successful
Let me gather more context about these parameters by checking their usage in the test files and the Dataset class implementation.
Let me check one more aspect to understand the relationship between _force_load and dataset loading behavior.
Private parameters are used consistently and serve a clear purpose
The usage of _create=False and _force_load=True is consistent with the codebase's internal patterns. These parameters are used in both state.py and query.py for similar purposes, with _force_load being handled by the singleton pattern to update the last loaded timestamp. The parameters are well-tested across multiple test files and serve specific internal implementation needs:
_create=Falseprevents unnecessary dataset creation_force_load=Trueensures the dataset's timestamp is updated through the singleton pattern
No documentation is needed as these are internal implementation details used consistently throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar dataset loading patterns to ensure consistency
ast-grep --pattern 'fo.Dataset($_, _create=False, _force_load=True)'
# Look for any potential issues with these flags in the test files
rg -l "(_create|_force_load)" "tests/"
Length of output: 788
Script:
#!/bin/bash
# Check Dataset class implementation for these parameters
ast-grep --pattern 'class Dataset:
$$$
def __init__($$$):
$$$'
# Check test cases using these parameters
rg "_create.*_force_load" "tests/"
# Look for documentation or comments about these parameters
rg -B2 -A2 "(_create|_force_load)" "fiftyone/core/dataset.py"
Length of output: 3470
Script:
#!/bin/bash
# Search for _force_load parameter usage and its implications
rg -B3 -A3 "_force_load" "fiftyone/core/"
# Look for any documentation about force loading
rg -l "force.*load" "docs/"
Length of output: 1086
fiftyone/core/singletons.py (1)
41-43: LGTM! Verify consistent usage of _force_load parameter
The addition of the force parameter to _update_last_loaded_at is a clean and backward-compatible change.
Let's verify the consistent usage of _force_load parameter across the codebase:
✅ Verification successful
Let's check the implementation of _update_last_loaded_at to understand how the force parameter is used:
LGTM! The _force_load parameter is used correctly and consistently
The implementation and usage of _force_load parameter is correct and consistent across the codebase:
- The
_update_last_loaded_atmethod in Dataset class properly handles theforceparameter to bypass server-related checks when needed - The
_force_load=Trueis used in appropriate server-related contexts:- In server query handling to ensure dataset is loaded
- In state management to force dataset loading
- In singleton management with proper default fallback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for all Dataset instantiations to ensure consistent usage of _force_load
# Expected: All Dataset loads that need forced update should use _force_load=True
# Search for Dataset instantiations
rg "Dataset\(.*_force_load" --type py
# Search for _update_last_loaded_at calls
rg "_update_last_loaded_at\(.*force" --type py
Length of output: 265
Script:
#!/bin/bash
# Let's check the implementation of _update_last_loaded_at to understand the force parameter usage
ast-grep --pattern $'def _update_last_loaded_at(self, force=False):
$$$'
# Let's also check where Dataset is being used to ensure _force_load is used appropriately
rg "Dataset\(" -A 2 --type py
Length of output: 113588
Script:
#!/bin/bash
# Let's examine the implementation of _update_last_loaded_at in Dataset class
ast-grep --pattern $'def _update_last_loaded_at(self, force=False):
$$$'
# Also check where _force_load is used in the codebase
rg "_force_load" -B 2 -A 2 --type py
Length of output: 1375
app/packages/state/src/recoil/sidebar/index.ts (2)
Line range hint 37-73: LGTM! Import organization improves maintainability.
The reorganization of imports by relative path depth enhances code maintainability and readability.
214-216: Consider adding validation for merged groups.
While the merging logic is well-structured, consider adding validation to ensure the merged result maintains data integrity and expected structure.
Let's verify the implementation of mergeGroups:
fiftyone/server/query.py (1)
588-588: LGTM!
The updated dataset loading approach using fo.Dataset(dataset_name, _create=False, _force_load=True) enhances consistency with the application's dataset handling methods.
| it("merges current and config groups", () => { | ||
| expect( | ||
| mergeGroups( | ||
| [ | ||
| { name: "one", paths: ["one.one", "one.three"] }, | ||
| { name: "three", paths: [] }, | ||
| ], | ||
|
|
||
| [ | ||
| { name: "zero", paths: [] }, | ||
| { | ||
| name: "one", | ||
| paths: ["one.zero", "one.one", "one.two"], | ||
| }, | ||
| { name: "two", paths: [] }, | ||
| ] | ||
| ) | ||
| ).toStrictEqual([ | ||
| { name: "zero", paths: [] }, | ||
| { name: "one", paths: ["one.zero", "one.one", "one.two", "one.three"] }, | ||
| { name: "two", paths: [] }, | ||
| { name: "three", paths: [] }, | ||
| ]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for edge cases and error handling
While the current test case covers the happy path, consider adding tests for:
- Groups with identical paths
- Empty or invalid group names
- Malformed group objects
- Error handling scenarios
Also, consider breaking down this large test into smaller, more focused test cases for better maintainability.
it("merges current and config groups", () => {
expect(
mergeGroups(
[
{ name: "one", paths: ["one.one", "one.three"] },
{ name: "three", paths: [] },
],
[
{ name: "zero", paths: [] },
{
name: "one",
paths: ["one.zero", "one.one", "one.two"],
},
{ name: "two", paths: [] },
]
)
).toStrictEqual([/*...*/]);
});
+
+ it("handles groups with identical paths", () => {
+ expect(
+ mergeGroups(
+ [{ name: "one", paths: ["path1"] }],
+ [{ name: "one", paths: ["path1"] }]
+ )
+ ).toStrictEqual([
+ { name: "one", paths: ["path1"] }
+ ]);
+ });
+
+ it("validates group names", () => {
+ expect(() =>
+ mergeGroups(
+ [{ name: "", paths: [] }],
+ []
+ )
+ ).toThrow();
+ });Committable suggestion skipped: line range outside the PR's diff.
| describe("test sidebar groups resolution", () => { | ||
| it("test list merge", () => { | ||
| expect(merge([], ["one", "two"])).toStrictEqual(["one", "two"]); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases
The current test only covers merging an empty array with a non-empty array. Consider adding test cases for:
- Two non-empty arrays
- Arrays with duplicate elements
- Arrays with different data types (if applicable)
- Edge cases like null/undefined inputs
describe("test sidebar groups resolution", () => {
it("test list merge", () => {
expect(merge([], ["one", "two"])).toStrictEqual(["one", "two"]);
+ expect(merge(["one"], ["two"])).toStrictEqual(["one", "two"]);
+ expect(merge(["one"], ["one", "two"])).toStrictEqual(["one", "two"]);
+ expect(merge(null, ["one"])).toStrictEqual(["one"]);
});Committable suggestion skipped: line range outside the PR's diff.
| const insertFromNeighbor = (sink: string[], source: string[], key: string) => { | ||
| if (sink.includes(key)) { | ||
| return; | ||
| } | ||
|
|
||
| const sourceIndex = source.indexOf(key); | ||
| const before = source[sourceIndex - 1]; | ||
| const beforeIndex = sink.indexOf(before); | ||
|
|
||
| if (beforeIndex >= 0) { | ||
| sink.splice(beforeIndex + 1, 0, key); | ||
| return; | ||
| } | ||
|
|
||
| const after = source[sourceIndex + 1]; | ||
| const afterIndex = sink.indexOf(after); | ||
|
|
||
| if (afterIndex >= 0) { | ||
| sink.splice(afterIndex, 0, key); | ||
| return; | ||
| } | ||
|
|
||
| sink.push(key); | ||
| return; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document the insertion strategy and consider immutability
The function has complex insertion logic that would benefit from documentation. Also, mutating the input array directly could lead to unexpected side effects.
Consider:
- Adding JSDoc comments explaining the insertion strategy
- Making the function pure by returning a new array instead of mutating the input
+/**
+ * Inserts a key into the sink array based on its neighbors' positions in the source array.
+ * The insertion strategy prioritizes:
+ * 1. After an existing "before" neighbor
+ * 2. Before an existing "after" neighbor
+ * 3. Appends to the end if no neighbors exist
+ * @returns A new array with the inserted key
+ */
-const insertFromNeighbor = (sink: string[], source: string[], key: string) => {
+const insertFromNeighbor = (sink: string[], source: string[], key: string): string[] => {
+ const result = [...sink];
if (sink.includes(key)) {
- return;
+ return result;
}
// ... rest of the logic, operating on result instead of sink
- return;
+ return result;
};Committable suggestion skipped: line range outside the PR's diff.
| export const merge = (sink: string[], source: string[]) => { | ||
| const missing = new Set(source.filter((key) => !sink.includes(key))); | ||
|
|
||
| while (missing.size) { | ||
| const force = [...missing].every( | ||
| (name) => !hasNeighbor(sink, source, name) | ||
| ); | ||
| for (const name of missing) { | ||
| if (!force && !hasNeighbor(sink, source, name)) { | ||
| continue; | ||
| } | ||
|
|
||
| insertFromNeighbor(sink, source, name); | ||
|
|
||
| missing.delete(name); | ||
| } | ||
| } | ||
|
|
||
| return sink; | ||
| }; |
There was a problem hiding this comment.
Add safety guard and improve type annotations
The while loop could potentially run indefinitely if the conditions aren't met. Also, the function would benefit from explicit return type annotation.
-export const merge = (sink: string[], source: string[]) => {
+export const merge = (sink: string[], source: string[]): string[] => {
const missing = new Set(source.filter((key) => !sink.includes(key)));
+ let iterations = 0;
+ const MAX_ITERATIONS = 1000; // Adjust based on expected maximum size
- while (missing.size) {
+ while (missing.size && iterations++ < MAX_ITERATIONS) {
const force = [...missing].every(
(name) => !hasNeighbor(sink, source, name)
);
// ... rest of the logic
}
+ if (iterations >= MAX_ITERATIONS) {
+ console.warn('Maximum iterations reached in merge function');
+ }
return sink;
};📝 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.
| export const merge = (sink: string[], source: string[]) => { | |
| const missing = new Set(source.filter((key) => !sink.includes(key))); | |
| while (missing.size) { | |
| const force = [...missing].every( | |
| (name) => !hasNeighbor(sink, source, name) | |
| ); | |
| for (const name of missing) { | |
| if (!force && !hasNeighbor(sink, source, name)) { | |
| continue; | |
| } | |
| insertFromNeighbor(sink, source, name); | |
| missing.delete(name); | |
| } | |
| } | |
| return sink; | |
| }; | |
| export const merge = (sink: string[], source: string[]): string[] => { | |
| const missing = new Set(source.filter((key) => !sink.includes(key))); | |
| let iterations = 0; | |
| const MAX_ITERATIONS = 1000; // Adjust based on expected maximum size | |
| while (missing.size && iterations++ < MAX_ITERATIONS) { | |
| const force = [...missing].every( | |
| (name) => !hasNeighbor(sink, source, name) | |
| ); | |
| for (const name of missing) { | |
| if (!force && !hasNeighbor(sink, source, name)) { | |
| continue; | |
| } | |
| insertFromNeighbor(sink, source, name); | |
| missing.delete(name); | |
| } | |
| } | |
| if (iterations >= MAX_ITERATIONS) { | |
| console.warn('Maximum iterations reached in merge function'); | |
| } | |
| return sink; | |
| }; |
brimoor
changed the title
What changes are proposed in this pull request?
Screen.Recording.2024-11-25.at.2.54.49.PM.mov
How is this patch tested? If it is not, please explain why.
Unit tests
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
mergeGroupsfor improved sidebar group management.Bug Fixes
Tests
Documentation