Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR extracts multiple UI setting components from a centralized file into modular, individually-located component files under the left_border_tabs directory. A 728-line file containing reusable settings UI building blocks is decomposed into eight separate module files. Import statements across the codebase are updated to reference new component locations and export shapes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…it-setting-components
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
frontend/javascripts/viewer/view/left_border_tabs/components/switch_setting.tsx (1)
56-57: UseSwitchas controlled-only here.Line 56 already controls the component via
checked; Line 57’sdefaultCheckedis unnecessary and can cause controlled/uncontrolled ambiguity.♻️ Suggested fix
<Switch onChange={onChange} checked={value} - defaultChecked={value} disabled={disabled} loading={loading} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/javascripts/viewer/view/left_border_tabs/components/switch_setting.tsx` around lines 56 - 57, The Switch component is being used as both controlled and uncontrolled by passing both checked and defaultChecked; remove the defaultChecked prop (defaultChecked={value}) from the Switch in switch_setting.tsx so it is purely controlled via checked={value} and ensure the onChange handler (if present) updates the same state used for value (referencing the Switch component and the value variable) to avoid controlled/uncontrolled ambiguity.frontend/javascripts/viewer/view/left_border_tabs/components/user_boundingbox_input.tsx (3)
128-135: Consider properly typing the event instead of usingts-expect-error.The event handler uses
React.SyntheticEventbut then accessesevt.target.valuewith a ts-expect-error. Using the correct event type would provide better type safety.♻️ Use proper event typing
- const handleNameChanged = (evt: React.SyntheticEvent) => { - // `@ts-expect-error` ts-migrate(2339) FIXME: Property 'value' does not exist on type 'EventTarg... Remove this comment to see the full error message - const currentEnteredName = evt.target.value; + const handleNameChanged = (evt: React.FocusEvent<HTMLInputElement> | React.KeyboardEvent<HTMLInputElement>) => { + const currentEnteredName = (evt.target as HTMLInputElement).value; if (currentEnteredName !== propName) { onNameChange(currentEnteredName); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/javascripts/viewer/view/left_border_tabs/components/user_boundingbox_input.tsx` around lines 128 - 135, The handler handleNameChanged is typed as React.SyntheticEvent but then reads evt.target.value with a ts-expect-error; change the event param to the proper input change type (e.g., React.ChangeEvent<HTMLInputElement>) or use evt.currentTarget.value so TypeScript recognizes .value, remove the ts-expect-error, and keep the existing logic that compares currentEnteredName against propName and calls onNameChange(currentEnteredName) when different.
107-121: Variable shadowing:isValidshadows the state variable.The local
isValidconstant on line 113 shadows the component's state variable from line 72. While this works due to closure timing, it reduces readability and can lead to confusion.♻️ Suggested rename to avoid shadowing
const handleChange = (evt: React.ChangeEvent<HTMLInputElement>) => { const newText = evt.target.value; // only numbers, commas and whitespace is allowed const isValidInput = /^[\d\s,]*$/g.test(newText); const value = stringToNumberArray(newText); const isValidLength = value.length === 6; - const isValid = isValidInput && isValidLength; + const isInputValid = isValidInput && isValidLength; - if (isValid) { + if (isInputValid) { onBoundingChange(numberArrayToVector6(value)); } setText(newText); - setIsValid(isValid); + setIsValid(isInputValid); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/javascripts/viewer/view/left_border_tabs/components/user_boundingbox_input.tsx` around lines 107 - 121, The local constant isValid inside handleChange shadows the component state variable isValid (and its setter setIsValid); rename the local to something like computedIsValid or validNow, update the subsequent checks/uses (the conditional that calls onBoundingChange and the setIsValid(...) call) to use the new name, and ensure no other identifiers are renamed so handleChange, stringToNumberArray, numberArrayToVector6 and setText remain unchanged.
234-246: Using bothdefaultValueandvalueon a controlled Input is redundant.When
valueis provided, the Input becomes controlled anddefaultValueis ignored after the initial render. Since you're managing state withvalue={name}, thedefaultValue={name}prop has no effect.♻️ Remove redundant defaultValue
<Input - defaultValue={name} placeholder="Bounding Box Name" size="small" value={name}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/javascripts/viewer/view/left_border_tabs/components/user_boundingbox_input.tsx` around lines 234 - 246, Remove the redundant defaultValue prop from the Input in user_boundingbox_input.tsx (component: the Input inside the user bounding box component using state variable name); the Input is already controlled via value={name} and onChange/setName, so delete defaultValue={name} and keep value, onChange, onBlur/onPressEnter handlers as-is to ensure controlled behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/javascripts/viewer/view/left_border_tabs/components/dropdown_setting.tsx`:
- Around line 10-13: The DropdownSetting props currently claim onChange: (value:
number) => void and value: number | string while the Select receives string
option values, causing type mismatch and masked .toString() hacks; update the
DropdownSetting prop types so value: string | number and onChange: (value:
string | number) => void, adjust options to be typed with value: string | number
(e.g., Array<{ label: React.ReactNode; value: string | number }>) and remove the
suppressed .toString() conversions in the component (referencing the
DropdownSetting component, its onChange prop and options usage) so the Select
receives and forwards values with correct types.
In
`@frontend/javascripts/viewer/view/left_border_tabs/components/number_input_popover_setting.tsx`:
- Around line 49-63: The popover trigger is currently an unfocusable <span> in
number_input_popover_setting.tsx which prevents keyboard users from opening the
popover; make the trigger keyboard-accessible by replacing or updating that
<span> used as the Popover trigger so it is focusable (add tabIndex=0 or use a
<button>), add role="button" and aria-label (or aria-haspopup) and implement an
onKeyDown handler to open the popover on Enter/Space (mirror the click
behavior), and ensure the EditOutlined icon and label remain visually unchanged;
update the element that wraps {label} {value != null ? value : "-"} and the
EditOutlined so keyboard users can focus and activate the popover.
---
Nitpick comments:
In
`@frontend/javascripts/viewer/view/left_border_tabs/components/switch_setting.tsx`:
- Around line 56-57: The Switch component is being used as both controlled and
uncontrolled by passing both checked and defaultChecked; remove the
defaultChecked prop (defaultChecked={value}) from the Switch in
switch_setting.tsx so it is purely controlled via checked={value} and ensure the
onChange handler (if present) updates the same state used for value (referencing
the Switch component and the value variable) to avoid controlled/uncontrolled
ambiguity.
In
`@frontend/javascripts/viewer/view/left_border_tabs/components/user_boundingbox_input.tsx`:
- Around line 128-135: The handler handleNameChanged is typed as
React.SyntheticEvent but then reads evt.target.value with a ts-expect-error;
change the event param to the proper input change type (e.g.,
React.ChangeEvent<HTMLInputElement>) or use evt.currentTarget.value so
TypeScript recognizes .value, remove the ts-expect-error, and keep the existing
logic that compares currentEnteredName against propName and calls
onNameChange(currentEnteredName) when different.
- Around line 107-121: The local constant isValid inside handleChange shadows
the component state variable isValid (and its setter setIsValid); rename the
local to something like computedIsValid or validNow, update the subsequent
checks/uses (the conditional that calls onBoundingChange and the setIsValid(...)
call) to use the new name, and ensure no other identifiers are renamed so
handleChange, stringToNumberArray, numberArrayToVector6 and setText remain
unchanged.
- Around line 234-246: Remove the redundant defaultValue prop from the Input in
user_boundingbox_input.tsx (component: the Input inside the user bounding box
component using state variable name); the Input is already controlled via
value={name} and onChange/setName, so delete defaultValue={name} and keep value,
onChange, onBlur/onPressEnter handlers as-is to ensure controlled behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a05b88d-c60d-479d-b8ef-7eebafeab1eb
📒 Files selected for processing (24)
frontend/javascripts/components/pricing_enforcers.tsxfrontend/javascripts/viewer/view/action_bar/dataset_rotation_popover_view.tsxfrontend/javascripts/viewer/view/action_bar/quick_select_settings.tsxfrontend/javascripts/viewer/view/action_bar/tools/brush_presets.tsxfrontend/javascripts/viewer/view/action_bar_view.tsxfrontend/javascripts/viewer/view/components/setting_input_views.tsxfrontend/javascripts/viewer/view/left_border_tabs/components/color_layer_settings.tsxfrontend/javascripts/viewer/view/left_border_tabs/components/color_setting.tsxfrontend/javascripts/viewer/view/left_border_tabs/components/dropdown_setting.tsxfrontend/javascripts/viewer/view/left_border_tabs/components/log_slider_setting.tsxfrontend/javascripts/viewer/view/left_border_tabs/components/number_input_popover_setting.tsxfrontend/javascripts/viewer/view/left_border_tabs/components/number_slider_setting.tsxfrontend/javascripts/viewer/view/left_border_tabs/components/segmentation_layer_settings.tsxfrontend/javascripts/viewer/view/left_border_tabs/components/setting_input_helper.tsxfrontend/javascripts/viewer/view/left_border_tabs/components/skeleton_layer_settings.tsxfrontend/javascripts/viewer/view/left_border_tabs/components/switch_setting.tsxfrontend/javascripts/viewer/view/left_border_tabs/components/user_boundingbox_input.tsxfrontend/javascripts/viewer/view/left_border_tabs/controls_and_rendering_settings_tab.tsxfrontend/javascripts/viewer/view/left_border_tabs/hide_unregistered_segments_switch.tsxfrontend/javascripts/viewer/view/left_border_tabs/layer_settings_tab.tsxfrontend/javascripts/viewer/view/left_border_tabs/mapping_settings_view.tsxfrontend/javascripts/viewer/view/right_border_tabs/bounding_box_tab.tsxfrontend/javascripts/viewer/view/right_border_tabs/connectome_tab/connectome_settings.tsxfrontend/javascripts/viewer/view/statusbar.tsx
💤 Files with no reviewable changes (1)
- frontend/javascripts/viewer/view/components/setting_input_views.tsx
Comment on lines
+10
to
+13
| onChange: (value: number) => void; | ||
| label: React.ReactNode | string; | ||
| value: number | string; | ||
| options: Array<Record<string, any>>; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "dropdown_setting.tsx" --exec wc -l {} \;Repository: scalableminds/webknossos
Length of output: 154
🏁 Script executed:
fd -t f "dropdown_setting.tsx" -exec cat -n {} \;Repository: scalableminds/webknossos
Length of output: 298
🏁 Script executed:
cat -n ./frontend/javascripts/viewer/view/left_border_tabs/components/dropdown_setting.tsxRepository: scalableminds/webknossos
Length of output: 1899
🏁 Script executed:
rg "DropdownSetting" --type ts --type tsx -B 2 -A 2Repository: scalableminds/webknossos
Length of output: 95
🏁 Script executed:
rg "DropdownSetting" -B 2 -A 2Repository: scalableminds/webknossos
Length of output: 3844
🏁 Script executed:
rg "DropdownSetting" -A 15 frontend/javascripts/viewer/view/left_border_tabs/controls_and_rendering_settings_tab.tsx | head -60Repository: scalableminds/webknossos
Length of output: 2483
🏁 Script executed:
rg "onChangeGpuFactor|getGpuFactorsWithLabels" -B 2 -A 5 frontend/javascripts/viewer/view/left_border_tabs/controls_and_rendering_settings_tab.tsxRepository: scalableminds/webknossos
Length of output: 1365
🏁 Script executed:
rg "onChangeUser|onChangeDataset" -A 3 frontend/javascripts/viewer/view/left_border_tabs/controls_and_rendering_settings_tab.tsx | head -40Repository: scalableminds/webknossos
Length of output: 2241
🏁 Script executed:
rg "onChange.*ChangeDataset\." -B 1 -A 1 frontend/javascripts/viewer/view/left_border_tabs/controls_and_rendering_settings_tab.tsxRepository: scalableminds/webknossos
Length of output: 657
🏁 Script executed:
rg "gpuMemoryFactor" -B 2 -A 2 frontend/javascripts/viewer/store.ts | head -20Repository: scalableminds/webknossos
Length of output: 316
Fix the DropdownSetting value contract (number vs string).
Line 10 declares numeric onChange, but the Select component receives option values as strings (either directly from options like "BEST_QUALITY_FIRST" or from stringified factors). This causes Select to pass string values to onChange callbacks despite the type declaring number, and the .toString() on lines 30 and 32 with suppressed errors masks this real typing drift.
Suggested fix
type DropdownSettingProps = {
- onChange: (value: number) => void;
+ onChange: (value: number | string) => void;
label: React.ReactNode | string;
value: number | string;
- options: Array<Record<string, any>>;
+ options: Array<{ label: React.ReactNode; value: number | string }>;
disabled?: boolean;
disabledReason?: string | null;
};
@@
- <Select
+ <Select<number | string>
onChange={onChange}
- // `@ts-expect-error` ts-migrate(2322) FIXME: Type 'string' is not assignable to type 'number | ... Remove this comment to see the full error message
- value={value.toString()}
- // `@ts-expect-error` ts-migrate(2322) FIXME: Type 'string' is not assignable to type 'number | ... Remove this comment to see the full error message
- defaultValue={value.toString()}
+ value={value}
size="small"
popupMatchSelectWidth={false}
options={options}
disabled={disabled}
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/javascripts/viewer/view/left_border_tabs/components/dropdown_setting.tsx`
around lines 10 - 13, The DropdownSetting props currently claim onChange:
(value: number) => void and value: number | string while the Select receives
string option values, causing type mismatch and masked .toString() hacks; update
the DropdownSetting prop types so value: string | number and onChange: (value:
string | number) => void, adjust options to be typed with value: string | number
(e.g., Array<{ label: React.ReactNode; value: string | number }>) and remove the
suppressed .toString() conversions in the component (referencing the
DropdownSetting component, its onChange prop and options usage) so the Select
receives and forwards values with correct types.
Comment on lines
+56
to
+60
| const calculateValue = (sliderValue: number) => { | ||
| const a = 200 / (Math.log(max) - Math.log(min)); | ||
| const b = (100 * (Math.log(min) + Math.log(max))) / (Math.log(min) - Math.log(max)); | ||
| return Math.exp((sliderValue - b) / a); | ||
| }; |
Contributor
There was a problem hiding this comment.
Guard log-scale math against invalid ranges.
Lines 57-58 and 73-74 assume a valid log domain. Without checks (min > 0, max > 0, max > min), the slider/input can become NaN and unusable.
🛡️ Suggested guard
export function LogSliderSetting(props: LogSliderSettingProps) {
@@
} = props;
+
+ const hasValidLogRange = min > 0 && max > 0 && max > min;
+ if (!hasValidLogRange) {
+ return null;
+ }Also applies to: 72-77
Comment on lines
+49
to
+63
| <span | ||
| style={{ | ||
| cursor: "pointer", | ||
| }} | ||
| > | ||
| {label} {value != null ? value : "-"} | ||
| <EditOutlined | ||
| style={{ | ||
| fontSize: 11, | ||
| opacity: 0.7, | ||
| margin: "0 0px 5px 3px", | ||
| }} | ||
| /> | ||
| </span> | ||
| </Popover> |
Contributor
There was a problem hiding this comment.
Make the popover trigger keyboard-accessible.
Line 49 uses a clickable <span> as the popover trigger. This is not keyboard-focusable and blocks keyboard-only users from editing the value.
♿ Suggested fix
- <span
+ <button
+ type="button"
style={{
cursor: "pointer",
+ background: "transparent",
+ border: 0,
+ padding: 0,
}}
+ aria-label={typeof label === "string" ? `Edit ${label}` : "Edit value"}
>
{label} {value != null ? value : "-"}
<EditOutlined
style={{
fontSize: 11,
opacity: 0.7,
margin: "0 0px 5px 3px",
}}
/>
- </span>
+ </button>📝 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.
Suggested change
| <span | |
| style={{ | |
| cursor: "pointer", | |
| }} | |
| > | |
| {label} {value != null ? value : "-"} | |
| <EditOutlined | |
| style={{ | |
| fontSize: 11, | |
| opacity: 0.7, | |
| margin: "0 0px 5px 3px", | |
| }} | |
| /> | |
| </span> | |
| </Popover> | |
| <button | |
| type="button" | |
| style={{ | |
| cursor: "pointer", | |
| background: "transparent", | |
| border: 0, | |
| padding: 0, | |
| }} | |
| aria-label={typeof label === "string" ? `Edit ${label}` : "Edit value"} | |
| > | |
| {label} {value != null ? value : "-"} | |
| <EditOutlined | |
| style={{ | |
| fontSize: 11, | |
| opacity: 0.7, | |
| margin: "0 0px 5px 3px", | |
| }} | |
| /> | |
| </button> | |
| </Popover> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/javascripts/viewer/view/left_border_tabs/components/number_input_popover_setting.tsx`
around lines 49 - 63, The popover trigger is currently an unfocusable <span> in
number_input_popover_setting.tsx which prevents keyboard users from opening the
popover; make the trigger keyboard-accessible by replacing or updating that
<span> used as the Popover trigger so it is focusable (add tabIndex=0 or use a
<button>), add role="button" and aria-label (or aria-haspopup) and implement an
onKeyDown handler to open the popover on Enter/Space (mirror the click
behavior), and ensure the EditOutlined icon and label remain visually unchanged;
update the element that wraps {label} {value != null ? value : "-"} and the
EditOutlined so keyboard users can focus and activate the popover.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR splits the long collection of components from
setting_input_views.tsxfile into 8 individual files/components. All code was moved manually (no AI) without any changes (exceptions below). These components are reused all through the codebase. Not only is a 800LOC file hard to read, using one large file with many imports is highly likely to cause cyclic import chains.Exceptions:
user_boundingbox_input.tsx: replacemapStateToProps()withuseWkSelector()for accessingvisibleSegmentationLayerSETTING_LEFT_SPAN, SETTING_MIDDLE_SPAN, SETTING_VALUE_SPAN) into a new filesetting_input_helperURL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)