-
Notifications
You must be signed in to change notification settings - Fork 1
#VFB-227 - Apply queries grouping in the term info to the new VFB #208
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
base: development
Are you sure you want to change the base?
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2372f2bb53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const queries = useSelector((state) => state.queries.queries); | ||
| const queryComponentOpened = useSelector( | ||
| (state) => state.globalInfo.queryComponentOpened | ||
| (state) => state.queries.queryComponentOpened | ||
| ); |
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.
Read queryComponentOpened from wrong state slice
TermInfo now selects queryComponentOpened from state.queries, but this flag is owned by the global reducer (and other components like Layout/queryBuilder still read state.globalInfo?.queryComponentOpened). That means this selector resolves to undefined, so the useEffect that syncs toggleReadMore never reflects global open/close changes; the term info panel can get out of sync when the query component is toggled elsewhere. Please keep the selector on the global slice or move the flag into the queries reducer so it updates here.
Useful? React with 👍 / 👎.
| const groupedQueries = queryGroups.map(group => ({ | ||
| ...group, | ||
| queries: queriesData?.filter(q => group.keys.some(key => q.label.startsWith(key))) || [] |
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.
Guard groupedQueries label matching against null labels
The new grouping filter calls q.label.startsWith(...) directly. If any query lacks a label (or it is not a string), this throws at render time and breaks the entire queries tree. Earlier code used optional chaining for label checks, so this is a regression for incomplete query metadata. Use q?.label?.startsWith(...) or normalize labels before grouping.
Useful? React with 👍 / 👎.
Update term info queries section to display queries grouped as in v2 version of vfb. Keeps displaying inner Table of preview of queries when expanding a query. For those queries who are not part of a group, they get place under Other Queries. This last part is a bit of a change from v2.virtualflybrain, as those leaf queries where groupless there, whereas in dev we have a new group call Other Queries where these leafless queries can be placed as in screenshot and this branch. @ddelpiano
This pull request refactors and enhances the way queries are grouped and displayed in the
TermInfocomponent. The main changes include introducing configurable query groupings, improving state management for queries data, and updating the UI to reflect these new groupings for a more organized and scalable presentation.Query grouping and UI improvements:
queryGroupsconfiguration to categorize queries by label prefixes, and created logic to group queries accordingly, making it easier to manage and extend groupings in the future.SimpleTreeViewto iterate over these groups, displaying each group and its queries in a structured way. The "Other queries" section now cleanly displays any queries not matching the predefined groups. [1] [2] [3] [4] [5]State management enhancements:
queriesDatastate to manage queries independently of the maintermInfoData, ensuring that updates to the data prop are reflected in both pieces of state. [1] [2]UI consistency and accessibility:
Minor UI tweaks:
SimpleTreeViewfor better clarity.