Fix sidebar state persistence #4745
Conversation
WalkthroughThe changes involve updates to several files within the application, focusing on modifications to GraphQL fragments, state management, and testing. Key alterations include replacing the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Sidebar
participant Modal
participant StateManagement
User->>Sidebar: Toggle "PRIMITIVES" group
Sidebar->>StateManagement: Update sidebar state
User->>Modal: Open first sample
User->>Modal: Close modal
Sidebar->>User: Display updated state (group hidden)
Warning Review ran into problemsProblems (1)
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 as PR comments)
Additionally, you can add 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: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (3)
app/packages/app/src/pages/datasets/__generated__/DatasetPageQuery.graphql.tsis excluded by!**/__generated__/**app/packages/relay/src/fragments/__generated__/sidebarGroupsFragment.graphql.tsis excluded by!**/__generated__/**app/packages/relay/src/queries/__generated__/datasetQuery.graphql.tsis excluded by!**/__generated__/**
Files selected for processing (6)
- app/packages/relay/src/fragments/sidebarGroupsFragment.ts (1 hunks)
- app/packages/relay/src/graphQLSyncFragmentAtomFamily.ts (2 hunks)
- app/packages/state/src/hooks/useSetModalState.ts (2 hunks)
- app/packages/state/src/recoil/sidebar.ts (2 hunks)
- app/packages/state/src/recoil/sidebarExpanded.ts (2 hunks)
- e2e-pw/src/oss/specs/smoke-tests/quickstart.spec.ts (1 hunks)
Additional context used
Path-based instructions (6)
app/packages/relay/src/fragments/sidebarGroupsFragment.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/sidebarExpanded.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/quickstart.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.app/packages/state/src/hooks/useSetModalState.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/relay/src/graphQLSyncFragmentAtomFamily.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.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 (33)
app/packages/relay/src/fragments/sidebarGroupsFragment.ts (1)
5-5: Verify the impact of changing the queried field.The field being queried has changed from
nametodatasetId. Ensure that all components using this fragment are updated to handledatasetIdinstead ofname.Run the following script to verify the usage of this fragment in the codebase:
#!/bin/bash # Description: Verify all components using `sidebarGroupsFragment` are updated to handle `datasetId`. # Test: Search for the fragment usage. Expect: Only occurrences of `datasetId`. rg --type graphql -A 5 $'sidebarGroupsFragment'app/packages/state/src/recoil/sidebarExpanded.ts (2)
8-8: LGTM!The change clarifies the purpose of the atom family and distinguishes it from other related entities.
The code changes are approved.
22-22: LGTM!The change aligns the key with the naming convention used in the atom family.
The code changes are approved.
e2e-pw/src/oss/specs/smoke-tests/quickstart.spec.ts (1)
89-94: LGTM!The new test case enhances the test coverage by ensuring that the sidebar behaves correctly when interacting with other components.
The code changes are approved.
app/packages/state/src/hooks/useSetModalState.ts (5)
13-13: LGTM!The import statement for
sidebarExpandedAtomsis correct and aligns with the file's purpose.
47-47: LGTM!The new entry is correctly added to the
dataarray, ensuring the expanded state of the sidebar is managed.
48-48: LGTM!The new entry is correctly added to the
dataarray, ensuring the expanded state of the sidebar is managed.
49-49: LGTM!The new entry is correctly added to the
dataarray, ensuring the expanded state of the sidebar is managed.
50-50: LGTM!The new entry is correctly added to the
dataarray, ensuring the expanded state of the sidebar is managed.app/packages/relay/src/graphQLSyncFragmentAtomFamily.ts (2)
59-60: LGTM!The comment reformatting improves readability and does not affect functionality.
97-97: LGTM!The use of optional chaining enhances the robustness of the code by preventing potential runtime errors.
app/packages/state/src/recoil/sidebar.ts (22)
217-217: LGTM!The change ensures that only groups explicitly marked as boolean values in the
expandedobject are considered for expansion.
319-319: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
322-322: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
323-323: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
324-324: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
325-325: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
326-326: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
327-327: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
328-328: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
329-329: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
330-330: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
331-331: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
332-332: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
333-333: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
334-334: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
335-335: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
336-336: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
337-337: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
338-338: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
339-339: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
340-340: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
341-341: LGTM!The simplification focuses the export on the GraphQL sync fragment atom family, improving code clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (3)
app/packages/app/src/pages/datasets/__generated__/DatasetPageQuery.graphql.tsis excluded by!**/__generated__/**app/packages/relay/src/fragments/__generated__/sidebarGroupsFragment.graphql.tsis excluded by!**/__generated__/**app/packages/relay/src/queries/__generated__/datasetQuery.graphql.tsis excluded by!**/__generated__/**
Files selected for processing (6)
- app/packages/relay/src/fragments/sidebarGroupsFragment.ts (1 hunks)
- app/packages/relay/src/graphQLSyncFragmentAtomFamily.ts (2 hunks)
- app/packages/state/src/hooks/useSetModalState.ts (2 hunks)
- app/packages/state/src/recoil/sidebar.ts (2 hunks)
- app/packages/state/src/recoil/sidebarExpanded.ts (2 hunks)
- e2e-pw/src/oss/specs/smoke-tests/quickstart.spec.ts (1 hunks)
Files skipped from review due to trivial changes (2)
- app/packages/relay/src/graphQLSyncFragmentAtomFamily.ts
- app/packages/state/src/recoil/sidebarExpanded.ts
Additional context used
Path-based instructions (4)
app/packages/relay/src/fragments/sidebarGroupsFragment.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/quickstart.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.app/packages/state/src/hooks/useSetModalState.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.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 (5)
app/packages/relay/src/fragments/sidebarGroupsFragment.ts (1)
5-5: LGTM!The change from
nametodatasetIdis appropriate and aligns with the PR objectives.The code changes are approved.
e2e-pw/src/oss/specs/smoke-tests/quickstart.spec.ts (1)
88-94: LGTM!The new test case
sidebar persistenceis well-structured and effectively verifies the sidebar state persistence.The code changes are approved.
app/packages/state/src/hooks/useSetModalState.ts (1)
13-13: LGTM!The addition of
sidebarExpandedAtomsand its integration into the modal state management logic is appropriate and aligns with the PR objectives.The code changes are approved.
Also applies to: 65-68
app/packages/state/src/recoil/sidebar.ts (2)
217-218: LGTM!The change to use
typeof expanded[group.name] === "boolean"ensures that only groups with a boolean value inexpandedare marked as expanded, improving the precision of the expansion logic.The code changes are approved.
319-366: LGTM!The simplification of the
sidebarGroupsDefinitionexport enhances clarity and maintainability by focusing solely on the synchronization of sidebar groups with GraphQL fragments.The code changes are approved.
What changes are proposed in this pull request?
Fixes sidebar ordering state persistance, as well as group and field filter expansion. Currently, Opening the modal resets the current ordering due to the page transition. See recording below to observe the unwanted behavior
Screen.Recording.2024-08-28.at.1.44.29.PM.mov
How is this patch tested? If it is not, please explain why.
Added test case to e2e quickstart smoke test
Release Notes
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Chores