Fix adding new tags in tagging action #5378
Conversation
WalkthroughThe pull request focuses on enhancing type safety and resolving potential state management issues in several React components within the Actions module. The changes primarily involve modifying type definitions to allow null values for certain properties, updating event handling logic, and introducing a new Changes
Assessment against linked issues
Possibly related PRs
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: 0
🧹 Nitpick comments (5)
app/packages/core/src/components/Actions/utils.tsx (1)
34-38: LGTM! Type annotations enhance type safety.The added TypeScript type annotations improve type safety and make the function signature more explicit. This change aligns with React/TypeScript best practices.
Consider documenting the function parameters.
While the types are now explicit, adding JSDoc comments would further improve maintainability by documenting the purpose of each parameter.
Add documentation like this:
/** * Hook to manage hover state and styling for interactive elements * @param disabled - Whether the hover effect is disabled * @param override - Optional override for the hover state * @param color - Optional custom color for the hover effect */ export const useHighlightHover = (app/packages/core/src/components/Actions/Checker.tsx (3)
97-97: Consider simplifying the count display logicThe current implementation can be more concise while maintaining the same functionality.
- {(count ?? 0) > 0 ? numeral(count).format("0,0") : "0"} + {numeral(count ?? 0).format("0,0")}
182-194: Consider simplifying the index calculation logicThe index calculation and boundary checking can be simplified using the modulo operator.
- let index: null | number = null; - if (e.key === "ArrowDown") { - index = active === null ? 0 : names.indexOf(active) + 1; - } else if (e.key === "ArrowUp") { - index = active === null ? names.length - 1 : names.indexOf(active) - 1; - } - - if (typeof index === "number") { - if (index < 0) { - index = names.length - 1; - } else if (index > names.length - 1) { - index = 0; - } + if (e.key === "ArrowDown" || e.key === "ArrowUp") { + const currentIndex = active === null ? -1 : names.indexOf(active); + const delta = e.key === "ArrowDown" ? 1 : -1; + const index = ((currentIndex + delta) + names.length) % names.length; setActive(names[index]); }
67-69: Consider extracting the click target check to improve readabilityThe click handler's condition could be more readable by extracting the target check logic.
+ const isCheckboxClicked = (target: EventTarget | null): boolean => + target === ref.current || ref.current?.contains(target as Node); + onClick={(e) => - !disabled && - !( - e.target === ref.current || ref.current?.contains(e.target as Node) - ) && - onCheck() + !disabled && !isCheckboxClicked(e.target) && onCheck() }app/packages/core/src/components/Actions/Tagger.tsx (1)
188-193: Consider adding input validation.The current implementation should validate the input value to prevent creating empty tags or tags with only whitespace.
onKeyDown={(e) => { if (e.key === "Enter" && hasCreate) { + const trimmedValue = value.trim(); + if (!trimmedValue) return; e.stopPropagation(); setValue(""); setActive(null); - setChanges({ ...changes, [value]: CheckState.ADD }); + setChanges({ ...changes, [trimmedValue]: CheckState.ADD }); } }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/packages/core/src/components/Actions/Checker.tsx(8 hunks)app/packages/core/src/components/Actions/Tagger.tsx(5 hunks)app/packages/core/src/components/Actions/utils.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/core/src/components/Actions/utils.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/core/src/components/Actions/Tagger.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/core/src/components/Actions/Checker.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.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: lint / eslint
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
🔇 Additional comments (6)
app/packages/core/src/components/Actions/Checker.tsx (2)
33-37: Type safety improvements look good!The explicit null handling in interface properties improves type safety and makes the component's behavior more predictable. The changes are consistent across both
CheckPropsandCheckerPropsinterfaces.Also applies to: 154-161
207-207: LGTM! Clear functionality properly integratedThe clear functionality is correctly integrated with the keyboard navigation handler, maintaining consistency with the PR objectives of clearing input after actions.
app/packages/core/src/components/Actions/Tagger.tsx (4)
102-102: Great type safety improvement!The explicit type definition for
activestate helps prevent potential runtime errors and improves code maintainability.
Line range hint
104-112: Good fix for hook dependencies!Adding
closeto theuseLayoutEffectdependencies list ensures proper cleanup when the tagging state changes.
189-191: Good fixes for event handling and state management!The changes correctly prevent event bubbling and clear the input state after tag creation, improving the user experience.
205-205: Good addition of the clear prop!The new
clearprop follows React's unidirectional data flow pattern for resetting the input state.
|
Was caught in meetings earlier. |
What changes are proposed in this pull request?
Closes #5328
Screen.Recording.2025-01-12.at.2.22.04.PM.mov
Release Notes
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Type Safety Improvements
CheckerandTaggercomponentsNew Features
clearfunctionality in componentsBug Fixes
Performance