Change order and selections of annotation tools new property, annotationToolsIds #532
Conversation
…so persisting the selection of defaultToolId
WalkthroughAdds a new config property Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant Tabs as Tabs Component
participant Action as selectTab Action
participant Store as Store
Note over Tabs: User selects tab (optional toolId for annotate)
U->>Tabs: select tab (tabId, toolId?)
Tabs->>Action: dispatch selectTab { tabId, toolId? }
Action->>Store: update state.toolId (payload.toolId or tab default)
Store-->>Tabs: state updated -> re-render UI
sequenceDiagram
autonumber
participant Init as public/init.js
participant FE as FilerobotImageEditor
participant UI as ToolsBar
Note right of Init: define selectedAnnotationTools (TOOLS.* list)
Init->>FE: init with config { annotationToolsIds: selectedAnnotationTools }
FE->>UI: render ToolsBar with config.annotationToolsIds
UI-->>FE: display configured annotation tools (subset or all if empty)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (1)
104-104: 💡 Verification agent🧩 Analysis chain
Verify toolId usage against currentToolId
On line 104, you're checking if
TABS_TOOLS[currentTabId]includescurrentToolId, but in the subsequent lines 105-107, you're usingtoolIdinstead ofcurrentToolId. This inconsistency could lead to bugs.
🏁 Script executed:
#!/bin/bash # Check for usage pattern of toolId vs currentToolId grep -n "toolId" --include="*.jsx" --include="*.js" packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsxLength of output: 471
Inconsistent tool identifier: Align usage of
currentToolIdandtoolIdThe current implementation checks whether
TABS_TOOLS[currentTabId]includescurrentToolIdon line 104, but then accesses tool properties viatoolIdin lines 105–107 (and elsewhere). This discrepancy risks bugs due to potential mismatches between the two variables. Please verify which identifier should be the single source of truth. For example:
- If
currentToolIdis intended to be the primary identifier, update lines 105–107 (and any related logic) to consistently usecurrentToolId.- If
toolId(e.g., as derived on line 28) is the correct identifier, then adjust the check on line 104 to usetoolIdinstead.Ensuring consistency here will help prevent unintended behavior in the tool selection logic.
🧹 Nitpick comments (9)
README.md (1)
438-450: Documentation forannotationToolsIdsis clear and follows project conventionsThe documentation follows the established format and clearly explains the purpose and behavior of the new property. It provides useful information about the default tools available.
However, the code block is missing a language specifier, which would improve syntax highlighting.
-``` +```javascript ['Text', 'Image', 'Rect', 'Ellipse', 'Polygon', 'Pen', 'Line', 'Arrow']<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 448-448: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>packages/react-filerobot-image-editor/src/actions/selectTab.js (1)</summary><blockquote> `18-18`: **Minor formatting issue** There's a linting error related to indentation on this line. The closing curly brace should be properly indented. ```diff -}; + };🧰 Tools
🪛 ESLint
[error] 18-18: Insert
······(prettier/prettier)
packages/react-filerobot-image-editor/src/components/Tabs/index.jsx (3)
11-11: Fix import orderThe import for
TABS_TOOLSshould be moved before the local imports to follow consistent import ordering./** Internal Dependencies */ import { useStore } from 'hooks'; import { SELECT_TAB } from 'actions'; +import { TABS_TOOLS } from 'components/tools/tools.constants'; import TabItem from './TabItem'; import { AVAILABLE_TABS } from './Tabs.constants'; -import { TABS_TOOLS } from 'components/tools/tools.constants';🧰 Tools
🪛 ESLint
[error] 11-11:
components/tools/tools.constantsimport should occur before import of./TabItem(import/order)
25-26: Implementation of tool persistence across tab selectionThis logic determines which tool should be selected when switching tabs, supporting the PR objective to persist the
defaultToolIdwhen switching between tabs.However, the code formatting should be improved:
- const annotateToolIdKey = defaultTabId === newTabId ? TABS_TOOLS.Annotate.indexOf(toolId) : 0; + const annotateToolIdKey = defaultTabId === newTabId + ? TABS_TOOLS.Annotate.indexOf(toolId) + : 0;🧰 Tools
🪛 ESLint
[error] 25-25: Insert
⏎·····(prettier/prettier)
31-31: Missing trailing commaAdd a trailing comma for consistency with the code style:
tabId: newTabId, - toolId: annotateToolIdKey + toolId: annotateToolIdKey, },🧰 Tools
🪛 ESLint
[error] 31-31: Insert
,(prettier/prettier)
packages/react-filerobot-image-editor/src/actions/selectTool.js (1)
12-12: Add missing semicolonThe function is missing a semicolon at the end:
-} +};🧰 Tools
🪛 ESLint
[error] 12-12: Insert
;(prettier/prettier)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (3)
44-46: Remove console.log statements from production codeDebug logging should not be committed to production code. It clutters the console and may expose implementation details.
const items = useMemo(() => { - console.log({TABS_IDS, TABS_TOOLS: TABS_TOOLS.Annotate, tabTools}); const shouldShowTool = (id) =>🧰 Tools
🪛 ESLint
[error] 45-45: Replace
TABS_IDS,·TABS_TOOLS:·TABS_TOOLS.Annotate,·tabToolswith·TABS_IDS,·TABS_TOOLS:·TABS_TOOLS.Annotate,·tabTools·(prettier/prettier)
49-61: Use optional chaining for better error handlingThe static analysis suggests using optional chaining on line 51 to make the code more robust against potential null/undefined values.
const renderItem = (id) => { const { Item, hideFn } = TOOLS_ITEMS[id]; - if (!Item || (hideFn && hideFn({ useCloudimage }))) return null; + if (!Item || (hideFn?.({ useCloudimage }))) return null; return ( <Item key={id} selectTool={selectTool} t={t} isSelected={currentToolId === id} /> ); };🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 ESLint
[error] 52-52: Delete
··(prettier/prettier)
82-83: Format dependency array correctlyPer the linting error, the dependency array needs proper formatting for readability.
- }, [currentTabId, tabTools, annotationToolsIds, currentToolId, useCloudimage]); + }, [ + currentTabId, + tabTools, + annotationToolsIds, + currentToolId, + useCloudimage, + ]);🧰 Tools
🪛 ESLint
[error] 83-83: Replace
currentTabId,·tabTools,·annotationToolsIds,·currentToolId,·useCloudimagewith⏎····currentTabId,⏎····tabTools,⏎····annotationToolsIds,⏎····currentToolId,⏎····useCloudimage,⏎··(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
README.md(1 hunks)packages/react-filerobot-image-editor/src/actions/selectTab.js(1 hunks)packages/react-filerobot-image-editor/src/actions/selectTool.js(1 hunks)packages/react-filerobot-image-editor/src/components/Tabs/index.jsx(1 hunks)packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx(3 hunks)packages/react-filerobot-image-editor/src/context/defaultConfig.js(1 hunks)packages/react-filerobot-image-editor/src/index.d.ts(1 hunks)public/demo-config.js(1 hunks)public/init.js(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
public/init.js (2)
public/demo-config.js (1)
FilerobotImageEditor(11-11)packages/react-filerobot-image-editor/src/index.d.ts (1)
TOOLS(366-366)
packages/react-filerobot-image-editor/src/components/Tabs/index.jsx (11)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (1)
useStore(17-25)packages/react-filerobot-image-editor/src/components/tools/Watermark/WatermarksGallery.jsx (1)
useStore(17-17)packages/react-filerobot-image-editor/src/components/App/index.jsx (1)
useStore(43-53)packages/react-filerobot-image-editor/src/components/MainCanvas/CanvasNode.jsx (1)
useStore(35-46)packages/react-filerobot-image-editor/src/components/Topbar/ResetButton.jsx (1)
useStore(12-12)packages/react-filerobot-image-editor/src/components/Topbar/UndoButton.jsx (1)
useStore(12-12)packages/react-filerobot-image-editor/src/components/Topbar/RedoButton.jsx (1)
useStore(12-12)packages/react-filerobot-image-editor/src/components/NodeControls/index.jsx (1)
useStore(16-16)packages/react-filerobot-image-editor/src/components/common/AnnotationOptions/index.jsx (1)
useStore(40-43)packages/react-filerobot-image-editor/src/actions/selectTab.js (1)
selectTab(6-18)packages/react-filerobot-image-editor/src/components/tools/tools.constants.js (2)
TABS_TOOLS(122-149)TABS_TOOLS(122-149)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (4)
packages/react-filerobot-image-editor/src/components/tools/tools.constants.js (2)
TABS_TOOLS(122-149)TABS_TOOLS(122-149)packages/react-filerobot-image-editor/src/actions/selectTool.js (1)
selectTool(3-12)packages/react-filerobot-image-editor/src/components/Tabs/index.jsx (1)
currentTabId(22-22)packages/react-filerobot-image-editor/src/utils/constants.js (2)
TABS_IDS(13-20)TABS_IDS(13-20)
🪛 ESLint
packages/react-filerobot-image-editor/src/actions/selectTab.js
[error] 18-18: Insert ······
(prettier/prettier)
packages/react-filerobot-image-editor/src/actions/selectTool.js
[error] 4-4: Replace payload,·state with ·payload,·state·
(prettier/prettier)
[error] 12-12: Insert ;
(prettier/prettier)
packages/react-filerobot-image-editor/src/components/Tabs/index.jsx
[error] 11-11: components/tools/tools.constants import should occur before import of ./TabItem
(import/order)
[error] 25-25: Insert ⏎·····
(prettier/prettier)
[error] 31-31: Insert ,
(prettier/prettier)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx
[error] 7-7: Replace ·TABS_TOOLS,·TOOLS_ITEMS,·AVAILABLE_ANNOTATIONS_TOOLS· with ⏎··TABS_TOOLS,⏎··TOOLS_ITEMS,⏎··AVAILABLE_ANNOTATIONS_TOOLS,⏎
(prettier/prettier)
[error] 7-7: AVAILABLE_ANNOTATIONS_TOOLS not found in 'components/tools/tools.constants'
(import/named)
[error] 45-45: Replace TABS_IDS,·TABS_TOOLS:·TABS_TOOLS.Annotate,·tabTools with ·TABS_IDS,·TABS_TOOLS:·TABS_TOOLS.Annotate,·tabTools·
(prettier/prettier)
[error] 48-48: Delete ··
(prettier/prettier)
[error] 52-52: Delete ··
(prettier/prettier)
[error] 65-65: Delete ··
(prettier/prettier)
[error] 72-72: Replace orderedToolIds with ·orderedToolIds·
(prettier/prettier)
[error] 76-76: Delete ··
(prettier/prettier)
[error] 81-81: Delete ··
(prettier/prettier)
[error] 83-83: Replace currentTabId,·tabTools,·annotationToolsIds,·currentToolId,·useCloudimage with ⏎····currentTabId,⏎····tabTools,⏎····annotationToolsIds,⏎····currentToolId,⏎····useCloudimage,⏎··
(prettier/prettier)
🪛 markdownlint-cli2 (0.17.2)
README.md
448-448: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 Biome (1.9.4)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
packages/react-filerobot-image-editor/src/context/defaultConfig.js (1)
95-95: Well-structured addition of the newannotationToolsIdspropertyThe new property is properly added and initialized as an empty array, consistent with the similar
tabsIdsproperty above it. This enables users to provide a custom list of annotation tools and arrange them in any desired order.packages/react-filerobot-image-editor/src/index.d.ts (1)
295-295: Type definition correctly added forannotationToolsIdsThe TypeScript type is appropriately defined as an optional property accepting either an array of available tools or an empty array. This matches the implementation in defaultConfig.js and provides proper type checking for this new feature.
packages/react-filerobot-image-editor/src/actions/selectTab.js (1)
12-12: Improved tab selection logic to maintain tool selectionThe change enhances the tab selection functionality by utilizing the tool ID from the payload rather than always defaulting to the first tool. This ensures that the
defaultToolIdis persisted when a user changes tabs and then returns to a previous tab, improving the overall user experience.public/demo-config.js (1)
354-354: Adds a new configuration property for customizing annotation tools orderThis new property
annotationToolsIdsaligns with the PR objective of allowing users to provide a custom list of annotation tools in their preferred order. It's commented out by default, which makes it a non-intrusive addition to the existing configuration.packages/react-filerobot-image-editor/src/components/Tabs/index.jsx (1)
17-17: Added toolId to handle persistent tool selectionAdding the
toolIdto the destructured properties from useStore enables tracking the currently selected tool, which is necessary for the new feature that persists tool selection across tab changes.public/init.js (3)
30-30: Added TOOLS import to support annotation tools customizationThis change imports the TOOLS constant needed for defining the custom list of annotation tools.
53-62: Definition of default annotation toolsThis new constant defines a complete set of annotation tools that will be used as the default configuration. This implementation correctly supports the PR objective of allowing custom ordering of annotation tools.
The array includes all the annotation tools that were previously shown by default, ensuring backward compatibility while adding configurability.
77-77: Applied annotation tools configurationThe new
annotationToolsIdsproperty is added to the plugin configuration, utilizing the predefined list of tools. This enables the feature described in the PR objectives, allowing users to provide a custom list of annotation tools and arrange them in their desired order.packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (1)
24-24: LGTM! Config properly updatedThe addition of
annotationToolsIdsto the config destructuring aligns with the PR objective to support custom ordering of annotation tools.
packages/react-filerobot-image-editor/src/actions/selectTool.js
Outdated
Show resolved
Hide resolved
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (2)
7-7: Adhere to Prettier formatting for import statement.
The static analysis hints mention inserting spacing around braces for consistency with Prettier rules.- import { TABS_TOOLS, TOOLS_ITEMS} from 'components/tools/tools.constants'; + import { TABS_TOOLS, TOOLS_ITEMS } from 'components/tools/tools.constants';🧰 Tools
🪛 ESLint
[error] 7-7: Insert
·(prettier/prettier)
63-72: Consider simplifying the tool reordering for clarity and performance.
The current mapping logic is somewhat complex and can be O(n²). It also silently discards unknown tool IDs via.filter(Boolean). If you want to maintain a custom order with minimal overhead, consider a more direct approach:- const annotateToolIds = annotationToolsIds.length - ? annotationToolsIds - .map((_, index) => - TABS_TOOLS.Annotate.find(id => annotationToolsIds.indexOf(id) === index), - ) - .filter(Boolean) - : TABS_TOOLS.Annotate; + // Prefer a simpler approach: + let annotateToolIds; + if (annotationToolsIds.length > 0) { + // Keep valid user-specified IDs in order + annotateToolIds = annotationToolsIds.filter(id => + TABS_TOOLS.Annotate.includes(id) + ); + // Append remaining defaults that were not included in annotationToolsIds + TABS_TOOLS.Annotate.forEach((id) => { + if (!annotateToolIds.includes(id)) { + annotateToolIds.push(id); + } + }); + } else { + annotateToolIds = [...TABS_TOOLS.Annotate]; + }🧰 Tools
🪛 ESLint
[error] 65-65: Replace
?·annotationToolsIds.map((_,·index)·=>·TABS_TOOLS.Annotate.find(id·=>·annotationToolsIds.indexOf(id)·===·index))with··?·annotationToolsIds⏎············.map((_,·index)·=>⏎··············TABS_TOOLS.Annotate.find(⏎················(id)·=>·annotationToolsIds.indexOf(id)·===·index,⏎··············),⏎············)⏎············(prettier/prettier)
[error] 66-66: Insert
··(prettier/prettier)
[error] 70-70: Delete
··(prettier/prettier)
[error] 72-72: Replace
currentTabId,·tabTools,·annotationToolsIds,·currentToolId,·useCloudimagewith⏎····currentTabId,⏎····tabTools,⏎····annotationToolsIds,⏎····currentToolId,⏎····useCloudimage,⏎··(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-filerobot-image-editor/src/actions/selectTool.js(1 hunks)packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/react-filerobot-image-editor/src/actions/selectTool.js (2)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (1)
selectTool(35-42)packages/react-filerobot-image-editor/src/actions/updateState.js (1)
payload(7-10)
🪛 ESLint
packages/react-filerobot-image-editor/src/actions/selectTool.js
[error] 4-10: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 5-5: Insert ··
(prettier/prettier)
[error] 6-6: Insert ··
(prettier/prettier)
[error] 7-7: Insert ··
(prettier/prettier)
[error] 8-8: Replace ······ with ········
(prettier/prettier)
[error] 9-9: Insert ··
(prettier/prettier)
[error] 10-10: Insert ··
(prettier/prettier)
[error] 11-11: Insert ;
(prettier/prettier)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx
[error] 7-7: Insert ·
(prettier/prettier)
[error] 49-49: Delete ··
(prettier/prettier)
[error] 52-52: Delete ··
(prettier/prettier)
[error] 65-65: Replace ?·annotationToolsIds.map((_,·index)·=>·TABS_TOOLS.Annotate.find(id·=>·annotationToolsIds.indexOf(id)·===·index)) with ··?·annotationToolsIds⏎············.map((_,·index)·=>⏎··············TABS_TOOLS.Annotate.find(⏎················(id)·=>·annotationToolsIds.indexOf(id)·===·index,⏎··············),⏎············)⏎············
(prettier/prettier)
[error] 66-66: Insert ··
(prettier/prettier)
[error] 70-70: Delete ··
(prettier/prettier)
[error] 72-72: Replace currentTabId,·tabTools,·annotationToolsIds,·currentToolId,·useCloudimage with ⏎····currentTabId,⏎····tabTools,⏎····annotationToolsIds,⏎····currentToolId,⏎····useCloudimage,⏎··
(prettier/prettier)
🔇 Additional comments (2)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (2)
24-24: NewannotationToolsIdsproperty looks good.
This new configuration prop aligns well with the feature requirements for customizing annotation tools.
45-48: Visibility check logic is clear.
TheisToolVisiblemethod is straightforward and effectively hides tools ifhideFnis set.
packages/react-filerobot-image-editor/src/actions/selectTool.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-filerobot-image-editor/src/actions/selectTool.js (1)
3-10: Logic is correct; consider fixing indentation for style compliance.The function is concise, correct, and follows reducer best practices. Only minor indentation issues remain, as flagged by static analysis tools. Fixing these will improve code consistency and satisfy Prettier/ESLint.
Apply this diff to fix indentation:
-const selectTool = (state, payload) => - state.toolId === payload.toolId - ? state - : { - ...state, - toolId: payload.toolId, - selectionsIds: payload.keepSelections ? state.selectionsIds : [], - }; +const selectTool = (state, payload) => + state.toolId === payload.toolId + ? state + : { + ...state, + toolId: payload.toolId, + selectionsIds: payload.keepSelections ? state.selectionsIds : [], + };🧰 Tools
🪛 ESLint
[error] 5-5: Insert
··(prettier/prettier)
[error] 6-6: Insert
··(prettier/prettier)
[error] 7-7: Replace
······with········(prettier/prettier)
[error] 8-8: Insert
··(prettier/prettier)
[error] 9-9: Insert
··(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-filerobot-image-editor/src/actions/selectTool.js(1 hunks)
🧰 Additional context used
🪛 ESLint
packages/react-filerobot-image-editor/src/actions/selectTool.js
[error] 5-5: Insert ··
(prettier/prettier)
[error] 6-6: Insert ··
(prettier/prettier)
[error] 7-7: Replace ······ with ········
(prettier/prettier)
[error] 8-8: Insert ··
(prettier/prettier)
[error] 9-9: Insert ··
(prettier/prettier)
[error] 10-11: Replace };⏎ with ··};
(prettier/prettier)
packages/react-filerobot-image-editor/src/actions/selectTool.js
Outdated
Show resolved
Hide resolved
| const currentTabId = tabId || defaultTabId; | ||
|
|
||
| const selectTab = useCallback((newTabId) => { | ||
| const annotateToolIdKey = defaultTabId === newTabId ? TABS_TOOLS.Annotate.indexOf(toolId) : 0; |
There was a problem hiding this comment.
Why do u need to pass the index and reuse the index from the tab_tools constant as long as the toolId is going to be exact the same as the toolId needed? it's more performant to use the id directly and drop searching for the id by the key every time and only make a verification that uses the constants.TABS_IDS and add a fallback for the first item of the array in the reducer.
There was a problem hiding this comment.
Changed to: const annotateToolIdKey = newTabId === TABS_IDS.ANNOTATE ? toolId : null;
| if (currentTabId === TABS_IDS.ANNOTATE) { | ||
| const annotateToolIds = annotationToolsIds.length | ||
| ? annotationToolsIds.map((_, index) => TABS_TOOLS.Annotate.find(id => annotationToolsIds.indexOf(id) === index)).filter(Boolean) | ||
| : TABS_TOOLS.Annotate; | ||
|
|
There was a problem hiding this comment.
I think this will need to be changed if u applied the last comment of using the toolId instead of the tool key/index
There was a problem hiding this comment.
Changed to:
const annotateToolIds = annotationToolsIds.length
? annotationToolsIds.filter((toolId) =>TABS_TOOLS.Annotate.includes(toolId))
: TABS_TOOLS.Annotate;
| }; | ||
| // TABS_IDS | ||
| tabsIds?: availableTabs[] | []; | ||
| annotationToolsIds?: availableTools[] | []; |
There was a problem hiding this comment.
since this will be only for annotation tab I think we need to limit this with the annotation tools as long as u are using TAB_TOOLS
There was a problem hiding this comment.
Created type:
declare const ANNOTATION_TOOLS = {
TEXT: 'Text',
IMAGE: 'Image',
RECT: 'Rect',
ELLIPSE: 'Ellipse',
POLYGON: 'Polygon',
PEN: 'Pen',
LINE: 'Line',
ARROW: 'Arrow',
} as const;
/ ANNOTATION_TOOLS_IDS
type availableAnnotationTools = typeof ANNOTATION_TOOLS[keyof typeof ANNOTATION_TOOLS];
| ...state, | ||
| tabId: payload.tabId, | ||
| toolId: TABS_TOOLS[payload.tabId][0], | ||
| toolId: TABS_TOOLS[payload.tabId][payload.toolId], |
There was a problem hiding this comment.
let's use the toolId directly instead of the tool index as this is possible to be changed anytime + have the fallback for the first item of the array in-case the toolId isn't found
There was a problem hiding this comment.
Changed to:
toolId: payload.toolId || TABS_TOOLS[payload.tabId][0],
|
@HimonRana FYI - this will be possible in the new major version of the editor as it will be more customized by providing the functionality through the context and you could reuse it with whatever UI (but this will take time to be released) |
…ing and availableAnnotationTools type added.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/react-filerobot-image-editor/src/components/Tabs/index.jsx (1)
24-36: Fix stale closure: selectTab should depend on toolId and props.Without deps, tool persistence can break after changing the annotation tool; the callback captures an old toolId.
- const selectTab = useCallback((newTabId) => { + const selectTab = useCallback((newTabId) => { const annotateToolIdKey = newTabId === TABS_IDS.ANNOTATE ? toolId : null; dispatch({ type: SELECT_TAB, payload: { tabId: newTabId, toolId: annotateToolIdKey, }, }); toggleMainMenu(false); - }, []); + }, [toolId, dispatch, toggleMainMenu]);packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (1)
81-99: Use currentToolId consistently when resolving ItemOptions/hideFn.Avoids transient mismatch when toolId isn’t set yet (first render).
- return ( - currentTabId && - currentToolId && - TABS_TOOLS[currentTabId].includes(currentToolId) && - (!TOOLS_ITEMS[toolId]?.hideFn || - !TOOLS_ITEMS[toolId]?.hideFn({ useCloudimage })) && - TOOLS_ITEMS[toolId]?.ItemOptions - ); + return ( + currentTabId && + currentToolId && + TABS_TOOLS[currentTabId].includes(currentToolId) && + (!TOOLS_ITEMS[currentToolId]?.hideFn || + !TOOLS_ITEMS[currentToolId]?.hideFn({ useCloudimage })) && + TOOLS_ITEMS[currentToolId]?.ItemOptions + );
🧹 Nitpick comments (11)
packages/react-filerobot-image-editor/src/actions/selectTool.js (1)
12-13: Prettier: remove the stray blank line before export.Keeps formatting consistent with the codebase.
- export default selectTool;packages/react-filerobot-image-editor/src/index.d.ts (2)
40-50: Avoid duplicated sources of truth for annotation tool IDs.You can derive the type directly from availableTools and drop ANNOTATION_TOOLS to prevent drift.
-declare const ANNOTATION_TOOLS = { - TEXT: 'Text', - IMAGE: 'Image', - RECT: 'Rect', - ELLIPSE: 'Ellipse', - POLYGON: 'Polygon', - PEN: 'Pen', - LINE: 'Line', - ARROW: 'Arrow', -} as const; - -// ANNOTATION_TOOLS_IDS -type availableAnnotationTools = typeof ANNOTATION_TOOLS[keyof typeof ANNOTATION_TOOLS]; +// ANNOTATION_TOOLS_IDS +type availableAnnotationTools = Extract< + availableTools, + 'Text' | 'Image' | 'Rect' | 'Ellipse' | 'Polygon' | 'Pen' | 'Line' | 'Arrow' +>;Also applies to: 57-59
309-309: Type simplification.The union with [] is redundant for arrays.
- annotationToolsIds?: availableAnnotationTools[] | []; + annotationToolsIds?: availableAnnotationTools[];packages/react-filerobot-image-editor/src/actions/selectTab.js (3)
2-2: Terminate import with semicolon (Prettier).-import { TABS_TOOLS } from 'components/tools/tools.constants' +import { TABS_TOOLS } from 'components/tools/tools.constants';
12-13: Validate toolId against the tab’s tools and guard fallback.Prevents invalid toolIds and avoids a crash if TABS_TOOLS[payload.tabId] is undefined.
- toolId: payload.toolId || TABS_TOOLS[payload.tabId][0], + toolId: + payload.toolId && + TABS_TOOLS[payload.tabId]?.includes(payload.toolId) + ? payload.toolId + : TABS_TOOLS[payload.tabId]?.[0],
18-18: Prettier: fix indentation for closing brace.-}; + };packages/react-filerobot-image-editor/src/components/Tabs/index.jsx (2)
11-11: Import order: place external/aliased imports before relative ones.Moves utils/constants import above './TabItem' to satisfy import/order.
-import TabItem from './TabItem'; -import { AVAILABLE_TABS } from './Tabs.constants'; -import { TABS_IDS } from 'utils/constants'; +import { TABS_IDS } from 'utils/constants'; +import TabItem from './TabItem'; +import { AVAILABLE_TABS } from './Tabs.constants';
38-54: Memo deps: include useCloudimage.chosenTabs uses useCloudimage in hideFn; add it to deps to refresh when it changes.
- }, [tabsIds]); + }, [tabsIds, useCloudimage]);packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (3)
1-5: Typo in header comments.“Depepdencneis” -> “Dependencies”.
-/** External Depepdencneis */ +/** External Dependencies */ ... -/** Internal Depepdencneis */ +/** Internal Dependencies */
76-100: Memo deps: include useCloudimage for ToolOptionsComponent.It is used in hideFn checks.
- }, [currentTabId, currentToolId, annotations, selectionsIds]); + }, [currentTabId, currentToolId, annotations, selectionsIds, useCloudimage]);
24-25: Optional: default to the first configured annotation tool.If annotationToolsIds is set, using its first entry as the initial tool aligns defaults with the custom order.
Outside the changed hunk (supporting change):
- const currentToolId = - toolId || defaultToolId || TABS_TOOLS[currentTabId]?.[0]; + const currentToolId = + toolId || + defaultToolId || + (currentTabId === TABS_IDS.ANNOTATE && annotationToolsIds?.length + ? annotationToolsIds[0] + : TABS_TOOLS[currentTabId]?.[0]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/react-filerobot-image-editor/src/actions/selectTab.js(2 hunks)packages/react-filerobot-image-editor/src/actions/selectTool.js(1 hunks)packages/react-filerobot-image-editor/src/components/Tabs/index.jsx(1 hunks)packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx(2 hunks)packages/react-filerobot-image-editor/src/index.d.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (3)
packages/react-filerobot-image-editor/src/components/tools/tools.constants.js (4)
TOOLS_ITEMS(23-120)TOOLS_ITEMS(23-120)TABS_TOOLS(122-149)TABS_TOOLS(122-149)packages/react-filerobot-image-editor/src/actions/selectTool.js (1)
selectTool(3-10)packages/react-filerobot-image-editor/src/utils/constants.js (2)
TABS_IDS(13-20)TABS_IDS(13-20)
packages/react-filerobot-image-editor/src/actions/selectTab.js (1)
packages/react-filerobot-image-editor/src/actions/updateState.js (1)
payload(7-10)
packages/react-filerobot-image-editor/src/components/Tabs/index.jsx (3)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (1)
useStore(17-25)packages/react-filerobot-image-editor/src/actions/selectTab.js (1)
selectTab(6-18)packages/react-filerobot-image-editor/src/utils/constants.js (2)
TABS_IDS(13-20)TABS_IDS(13-20)
🪛 ESLint
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx
[error] 49-49: Delete ··
(prettier/prettier)
[error] 52-52: Delete ··
(prettier/prettier)
[error] 65-65: 'toolId' is already declared in the upper scope on line 21 column 5.
(no-shadow)
[error] 72-72: Delete ··
(prettier/prettier)
[error] 74-74: Replace currentTabId,·tabTools,·annotationToolsIds,·currentToolId,·useCloudimage with ⏎····currentTabId,⏎····tabTools,⏎····annotationToolsIds,⏎····currentToolId,⏎····useCloudimage,⏎··
(prettier/prettier)
packages/react-filerobot-image-editor/src/actions/selectTab.js
[error] 2-2: Insert ;
(prettier/prettier)
[error] 18-18: Insert ······
(prettier/prettier)
packages/react-filerobot-image-editor/src/actions/selectTool.js
[error] 12-13: Delete ⏎
(prettier/prettier)
packages/react-filerobot-image-editor/src/components/Tabs/index.jsx
[error] 11-11: utils/constants import should occur before import of ./TabItem
(import/order)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (1)
90-98: UsecurrentToolId(nottoolId) when resolving options and visibilityOn first render (before
SELECT_TOOLfires), options won’t render becausetoolIdis undefined. UsecurrentToolIdconsistently and adduseCloudimageto deps since it affectshideFn.return ( currentTabId && currentToolId && TABS_TOOLS[currentTabId].includes(currentToolId) && - (!TOOLS_ITEMS[toolId]?.hideFn || - !TOOLS_ITEMS[toolId]?.hideFn({ useCloudimage })) && - TOOLS_ITEMS[toolId]?.ItemOptions + (!TOOLS_ITEMS[currentToolId]?.hideFn || + !TOOLS_ITEMS[currentToolId]?.hideFn({ useCloudimage })) && + TOOLS_ITEMS[currentToolId]?.ItemOptions ); -}, [currentTabId, currentToolId, annotations, selectionsIds]); +}, [currentTabId, currentToolId, annotations, selectionsIds, useCloudimage]);
♻️ Duplicate comments (1)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (1)
63-69: Fix shadowed variable and formatting in annotation tools filterAvoids no-shadow on
toolId, and satisfies Prettier spacing/comma rules.- const annotateToolIds = annotationToolsIds.length - ? annotationToolsIds.filter((toolId) =>TABS_TOOLS.Annotate.includes(toolId)) - : TABS_TOOLS.Annotate; + const annotateToolIds = annotationToolsIds.length + ? annotationToolsIds.filter((id) => TABS_TOOLS.Annotate.includes(id)) + : TABS_TOOLS.Annotate;
🧹 Nitpick comments (3)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (3)
44-72: Include all referenced deps initemsmemo and apply Prettier-friendly formatting
itemsusesselectTool,t, anduseCloudimage; add them to deps. Also switch to multiline deps to appease Prettier.- }, [currentTabId, tabTools, annotationToolsIds, currentToolId, useCloudimage]); + }, [ + currentTabId, + tabTools, + annotationToolsIds, + currentToolId, + useCloudimage, + selectTool, + t, + ]);
44-61: Optional: guard against duplicate/invalid IDs inannotationToolsIdsIf a consumer passes duplicates, they’ll render twice. Deduplicate while preserving order.
- const annotateToolIds = annotationToolsIds.length - ? annotationToolsIds.filter((id) => TABS_TOOLS.Annotate.includes(id)) + const annotateToolIds = annotationToolsIds.length + ? Array.from( + new Set( + annotationToolsIds.filter((id) => + TABS_TOOLS.Annotate.includes(id), + ), + ), + ) : TABS_TOOLS.Annotate;
1-6: Fix typos in headers (“Dependencies”)Minor readability nit.
-/** External Depepdencneis */ +/** External Dependencies */ ... -/** Internal Depepdencneis */ +/** Internal Dependencies */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (2)
packages/react-filerobot-image-editor/src/components/tools/tools.constants.js (4)
TOOLS_ITEMS(23-120)TOOLS_ITEMS(23-120)TABS_TOOLS(122-149)TABS_TOOLS(122-149)packages/react-filerobot-image-editor/src/utils/constants.js (2)
TABS_IDS(13-20)TABS_IDS(13-20)
🪛 ESLint
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx
[error] 49-49: Delete ··
(prettier/prettier)
[error] 52-52: Delete ··
(prettier/prettier)
[error] 65-65: 'toolId' is already declared in the upper scope on line 21 column 5.
(no-shadow)
[error] 65-65: Replace TABS_TOOLS.Annotate.includes(toolId) with ⏎············TABS_TOOLS.Annotate.includes(toolId),⏎··········
(prettier/prettier)
[error] 70-70: Delete ··
(prettier/prettier)
[error] 72-72: Replace currentTabId,·tabTools,·annotationToolsIds,·currentToolId,·useCloudimage with ⏎····currentTabId,⏎····tabTools,⏎····annotationToolsIds,⏎····currentToolId,⏎····useCloudimage,⏎··
(prettier/prettier)
🔇 Additional comments (1)
packages/react-filerobot-image-editor/src/components/ToolsBar/index.jsx (1)
100-107: Effect deps for default tool selection — verify intentIf the intent is to re-select a default tool when tabs change or config updates, consider adding deps; otherwise the current empty array runs only once.
-useEffect(() => { +useEffect(() => { if (!toolId && currentToolId) { dispatch({ type: SELECT_TOOL, payload: { toolId: currentToolId }, }); } -}, []); +}, [toolId, currentToolId, dispatch]);If this risks unwanted reselection, keep the empty deps but confirm that tab-change selection is handled elsewhere (e.g., in
selectTab).
2 participants
It is possible to provide own list of annotation tools and have in any order by using the property annotationToolsIds.
Extra fix is persisting the defaultToolId when selecting another tab and back persist the tool selection.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation