Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些整体性的反馈：
- 在
OptionListRenderer中使用内联 IIFE 并手动维护visualIndex会增加复杂度；可以考虑预先计算一个派生的groupsWithRowIndex数组，或者使用reduce，让 JSX 只需要一个没有副作用的.map。 - 在
OptionEditor中，isLane相关的样式逻辑在多个分支里被重复使用；你可以抽取一个共享的辅助方法（例如getOptionContainerClassName({ depth, isLane, isOptionIncompatible })）或者一个小组件，从而避免重复相同的clsx片段。 - 对于使用了
role="switch"的开关行容器，可能值得通过aria-labelledby将它与可见标签连接（或者添加一个aria-label），这样屏幕阅读器才能正确地把标签文本和开关控件关联起来。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- The inline IIFE with a manually tracked `visualIndex` inside `OptionListRenderer` adds complexity; consider precomputing a derived `groupsWithRowIndex` array or using `reduce` so the JSX is a simple `.map` without side effects.
- The `isLane` styling logic is duplicated across multiple branches in `OptionEditor`; you could extract a shared helper (e.g. `getOptionContainerClassName({ depth, isLane, isOptionIncompatible })`) or a small component to avoid repeating the same `clsx` fragments.
- For the switch row container using `role="switch"`, it may be worth wiring it to the visible label via `aria-labelledby` (or adding an `aria-label`) so screen readers correctly associate the label text with the switch control.
## Individual Comments
### Comment 1
<location path="src/components/OptionEditor.tsx" line_range="447-448" />
<code_context>
+ basePath={basePath}
+ incompatibleReason={incompatibleReason}
+ />
+ <div className="pointer-events-none">
+ <SwitchButton value={isChecked} onChange={handleToggleSwitch} disabled={effectiveDisabled} />
+ </div>
+ </div>
</code_context>
<issue_to_address>
**issue (bug_risk):** The combination of `pointer-events-none` and the outer `onKeyDown` handler can cause confusing keyboard behavior for the nested `SwitchButton`.
Because `SwitchButton` may still be focusable and receive keyboard events, a space/enter press could both toggle its `onChange` and bubble to the outer `onKeyDown`, causing `handleToggleSwitch` to fire twice and effectively cancel the state change. It also leaves you with two focusable “switches” (outer `role="switch"` and inner control), which is problematic for accessibility. I’d recommend either making the inner `SwitchButton` presentational only (e.g., `tabIndex={-1}`, no `onChange`), or moving all input handling to `SwitchButton` and dropping the outer `role="switch"`/keyboard handler.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进之后的审查。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The inline IIFE with a manually tracked
visualIndexinsideOptionListRendereradds complexity; consider precomputing a derivedgroupsWithRowIndexarray or usingreduceso the JSX is a simple.mapwithout side effects. - The
isLanestyling logic is duplicated across multiple branches inOptionEditor; you could extract a shared helper (e.g.getOptionContainerClassName({ depth, isLane, isOptionIncompatible })) or a small component to avoid repeating the sameclsxfragments. - For the switch row container using
role="switch", it may be worth wiring it to the visible label viaaria-labelledby(or adding anaria-label) so screen readers correctly associate the label text with the switch control.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The inline IIFE with a manually tracked `visualIndex` inside `OptionListRenderer` adds complexity; consider precomputing a derived `groupsWithRowIndex` array or using `reduce` so the JSX is a simple `.map` without side effects.
- The `isLane` styling logic is duplicated across multiple branches in `OptionEditor`; you could extract a shared helper (e.g. `getOptionContainerClassName({ depth, isLane, isOptionIncompatible })`) or a small component to avoid repeating the same `clsx` fragments.
- For the switch row container using `role="switch"`, it may be worth wiring it to the visible label via `aria-labelledby` (or adding an `aria-label`) so screen readers correctly associate the label text with the switch control.
## Individual Comments
### Comment 1
<location path="src/components/OptionEditor.tsx" line_range="447-448" />
<code_context>
+ basePath={basePath}
+ incompatibleReason={incompatibleReason}
+ />
+ <div className="pointer-events-none">
+ <SwitchButton value={isChecked} onChange={handleToggleSwitch} disabled={effectiveDisabled} />
+ </div>
+ </div>
</code_context>
<issue_to_address>
**issue (bug_risk):** The combination of `pointer-events-none` and the outer `onKeyDown` handler can cause confusing keyboard behavior for the nested `SwitchButton`.
Because `SwitchButton` may still be focusable and receive keyboard events, a space/enter press could both toggle its `onChange` and bubble to the outer `onKeyDown`, causing `handleToggleSwitch` to fire twice and effectively cancel the state change. It also leaves you with two focusable “switches” (outer `role="switch"` and inner control), which is problematic for accessibility. I’d recommend either making the inner `SwitchButton` presentational only (e.g., `tabIndex={-1}`, no `onChange`), or moving all input handling to `SwitchButton` and dropping the outer `role="switch"`/keyboard handler.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the visual clarity and usability of top-level switch options in the task option list by adding alternating “zebra” backgrounds and making switch rows easier to interact with.
Changes:
- Add a
rowIndexconcept soOptionEditorcan render alternating background lanes for top-level rows. - Update option-group rendering to compute a “visual” index that skips
switchGridgroups (so stripes apply to visible rows consistently). - Make
switchoption rows clickable/focusable withrole="switch"and keyboard toggling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/components/TaskItem.tsx | Computes a visual row index (skipping switchGrid) and passes rowIndex into OptionEditor for striping. |
| src/components/OptionEditor.tsx | Adds rowIndex prop and applies zebra styling; refactors switch rendering to be row-clickable and keyboard-accessible. |
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我给出了一些整体上的反馈：
- 对于开关行，目前外层容器
div增加了role="switch"和键盘处理逻辑，而内部的SwitchButton仍然会被渲染（但带有pointer-events-none）；建议要么把焦点/ARIA 行为委托给SwitchButton，要么将其标记为aria-hidden/“仅用于展示”，以避免对辅助技术产生重复或不直观的语义。 - 在
OptionEditor中为输入框和选择框硬编码使用了w-[30%] flex-shrink-0的宽度，这在非常窄或非常宽的布局下可能表现不佳；建议使用响应式或最大/最小宽度工具类（例如max-w、min-w或基于断点的类）来替代固定百分比，从而让布局更加健壮。 InputField中错误信息容器现在使用了justify-end，但其宽度没有和输入列绑定，这会在大屏幕上把错误信息推得很靠右；建议将其限制在与输入框相同的 flex 区域内，或者在局部进行对齐，以保持其与对应输入的视觉关联。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- For the switch rows, the container `div` now has `role="switch"` and keyboard handling while the underlying `SwitchButton` is still rendered (but with `pointer-events-none`); consider either delegating focus/ARIA to `SwitchButton` or marking it `aria-hidden`/presentation-only to avoid duplicate/unintuitive semantics for assistive technologies.
- The hard-coded `w-[30%] flex-shrink-0` widths for inputs and selects in `OptionEditor` may not behave well on very narrow or very wide layouts; consider using responsive or max/min width utilities (e.g. `max-w`, `min-w`, or breakpoint-based classes) instead of a fixed percentage to keep the layout robust.
- The error message container in `InputField` now uses `justify-end` without tying its width to the input column, which can push the message to the far right on wide screens; consider constraining it to the same flex area as the input or aligning it more locally to maintain visual association.帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've left some high level feedback:
- For the switch rows, the container
divnow hasrole="switch"and keyboard handling while the underlyingSwitchButtonis still rendered (but withpointer-events-none); consider either delegating focus/ARIA toSwitchButtonor marking itaria-hidden/presentation-only to avoid duplicate/unintuitive semantics for assistive technologies. - The hard-coded
w-[30%] flex-shrink-0widths for inputs and selects inOptionEditormay not behave well on very narrow or very wide layouts; consider using responsive or max/min width utilities (e.g.max-w,min-w, or breakpoint-based classes) instead of a fixed percentage to keep the layout robust. - The error message container in
InputFieldnow usesjustify-endwithout tying its width to the input column, which can push the message to the far right on wide screens; consider constraining it to the same flex area as the input or aligning it more locally to maintain visual association.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For the switch rows, the container `div` now has `role="switch"` and keyboard handling while the underlying `SwitchButton` is still rendered (but with `pointer-events-none`); consider either delegating focus/ARIA to `SwitchButton` or marking it `aria-hidden`/presentation-only to avoid duplicate/unintuitive semantics for assistive technologies.
- The hard-coded `w-[30%] flex-shrink-0` widths for inputs and selects in `OptionEditor` may not behave well on very narrow or very wide layouts; consider using responsive or max/min width utilities (e.g. `max-w`, `min-w`, or breakpoint-based classes) instead of a fixed percentage to keep the layout robust.
- The error message container in `InputField` now uses `justify-end` without tying its width to the input column, which can push the message to the far right on wide screens; consider constraining it to the same flex area as the input or aligning it more locally to maintain visual association.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些整体性的反馈：
- 在 switch 行的 keydown 处理函数中，建议当事件来自嵌套的可交互元素（例如链接、按钮、输入框）时忽略
Enter/Space，可以通过检查if (event.target !== event.currentTarget) return;来实现，以避免对键盘交互进行双重处理。 - 由于 switch 控件被包在一个带有
pointer-events-none和aria-hidden的容器中，请再次确认SwitchButton本身不可获得焦点/不可 Tab 导航（例如内部没有处于 tab 顺序中的button），否则键盘用户可能会遇到一个不可交互或重复的开关，而此时该行已经拥有 ARIAswitch语义。 - 新增的固定宽度，例如输入框/选择框使用
w-[30%]，标签/描述使用max-w-[60%]，在非常窄或非常宽的容器中可能表现不佳；可以考虑使用更灵活的响应式尺寸（例如 min/max 宽度或断点）来代替固定百分比。
给 AI Agent 的提示词
Please address the comments from this code review:
## Overall Comments
- In the switch row keydown handler, consider ignoring `Enter`/`Space` when the event originates from nested interactive elements (e.g., links, buttons, inputs) by checking `if (event.target !== event.currentTarget) return;` to avoid double-handling keyboard interactions.
- With the switch control wrapped in a `pointer-events-none` + `aria-hidden` container, double‑check that `SwitchButton` itself is not focusable/tabbable (e.g., no internal `button` in the tab order), otherwise keyboard users may encounter a non-interactive or duplicated switch while the row already owns the ARIA `switch` semantics.
- The new fixed widths like `w-[30%]` for inputs/selects and `max-w-[60%]` for labels/descriptions may behave poorly on very narrow or very wide containers; consider using more flexible responsive sizing (e.g., min/max widths or breakpoints) instead of hard percentages.
## Individual Comments
### Comment 1
<location path="src/components/OptionEditor.tsx" line_range="450-423" />
<code_context>
+ translations={translations}
+ />
+ </div>
+ <div className="pointer-events-none flex-shrink-0" aria-hidden="true">
+ <SwitchButton
+ value={isChecked}
+ onChange={handleToggleSwitch}
+ disabled={effectiveDisabled}
+ />
+ </div>
</code_context>
<issue_to_address>
**issue (bug_risk):** Combining `pointer-events-none` with `aria-hidden` on the SwitchButton wrapper can lead to subtle accessibility and keyboard-focus inconsistencies.
Because the wrapper `div` is `aria-hidden` while the actual focusable control lives inside `SwitchButton`, assistive tech will ignore this subtree but the inner control can still receive keyboard focus (since `pointer-events` doesn’t affect focus). That can leave a focusable element that’s never announced, and events may not map cleanly to the outer row’s `role="switch"`. If you want the outer row to be the only interactive control, make the inner `SwitchButton` non-focusable (e.g. a visual-only variant, or `tabIndex={-1}` on the real control) instead of using `pointer-events-none` + `aria-hidden` on the wrapper.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In the switch row keydown handler, consider ignoring
Enter/Spacewhen the event originates from nested interactive elements (e.g., links, buttons, inputs) by checkingif (event.target !== event.currentTarget) return;to avoid double-handling keyboard interactions. - With the switch control wrapped in a
pointer-events-none+aria-hiddencontainer, double‑check thatSwitchButtonitself is not focusable/tabbable (e.g., no internalbuttonin the tab order), otherwise keyboard users may encounter a non-interactive or duplicated switch while the row already owns the ARIAswitchsemantics. - The new fixed widths like
w-[30%]for inputs/selects andmax-w-[60%]for labels/descriptions may behave poorly on very narrow or very wide containers; consider using more flexible responsive sizing (e.g., min/max widths or breakpoints) instead of hard percentages.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the switch row keydown handler, consider ignoring `Enter`/`Space` when the event originates from nested interactive elements (e.g., links, buttons, inputs) by checking `if (event.target !== event.currentTarget) return;` to avoid double-handling keyboard interactions.
- With the switch control wrapped in a `pointer-events-none` + `aria-hidden` container, double‑check that `SwitchButton` itself is not focusable/tabbable (e.g., no internal `button` in the tab order), otherwise keyboard users may encounter a non-interactive or duplicated switch while the row already owns the ARIA `switch` semantics.
- The new fixed widths like `w-[30%]` for inputs/selects and `max-w-[60%]` for labels/descriptions may behave poorly on very narrow or very wide containers; consider using more flexible responsive sizing (e.g., min/max widths or breakpoints) instead of hard percentages.
## Individual Comments
### Comment 1
<location path="src/components/OptionEditor.tsx" line_range="450-423" />
<code_context>
+ translations={translations}
+ />
+ </div>
+ <div className="pointer-events-none flex-shrink-0" aria-hidden="true">
+ <SwitchButton
+ value={isChecked}
+ onChange={handleToggleSwitch}
+ disabled={effectiveDisabled}
+ />
+ </div>
</code_context>
<issue_to_address>
**issue (bug_risk):** Combining `pointer-events-none` with `aria-hidden` on the SwitchButton wrapper can lead to subtle accessibility and keyboard-focus inconsistencies.
Because the wrapper `div` is `aria-hidden` while the actual focusable control lives inside `SwitchButton`, assistive tech will ignore this subtree but the inner control can still receive keyboard focus (since `pointer-events` doesn’t affect focus). That can leave a focusable element that’s never announced, and events may not map cleanly to the outer row’s `role="switch"`. If you want the outer row to be the only interactive control, make the inner `SwitchButton` non-focusable (e.g. a visual-only variant, or `tabIndex={-1}` on the real control) instead of using `pointer-events-none` + `aria-hidden` on the wrapper.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+412
to
+416
| const handleSwitchRowKeyDown = (event: KeyboardEvent<HTMLDivElement>) => { | ||
| if (event.key !== 'Enter' && event.key !== ' ') return; | ||
| event.preventDefault(); | ||
| handleToggleSwitch(); | ||
| }; |
There was a problem hiding this comment.
handleSwitchRowKeyDown toggles on keydown for the Space key. Because keydown repeats while the key is held, this can flip the switch multiple times unexpectedly. Consider either ignoring repeated events (event.repeat) or handling Space on keyup (keeping preventDefault to avoid page scroll).
| isOptionIncompatible && 'opacity-60', | ||
| )} | ||
| onClick={(e) => { | ||
| if ((e.target as HTMLElement).closest('a')) return; |
There was a problem hiding this comment.
The row-level onClick only exempts anchor clicks. Clicking the incompatible-warning tooltip icon (and any other non-<a> interactive child) will bubble and toggle the switch, which makes it hard to inspect the tooltip without changing the value. Consider stopping propagation on the tooltip trigger, or broadening the guard to skip clicks originating from tooltip triggers/other controls.
Suggested change
| if ((e.target as HTMLElement).closest('a')) return; | |
| const target = e.target as HTMLElement | null; | |
| if ( | |
| target && | |
| target.closest( | |
| 'a,button,input,select,textarea,[role="button"]', | |
| ) | |
| ) { | |
| return; | |
| } |
|
|
||
| return ( | ||
| <div className="space-y-3"> | ||
| <div className="space-y-4"> |
There was a problem hiding this comment.
The PR description mentions adding zebra-stripe “lane styling” for alternating top-level switch rows and passing a visible row index into OptionEditor, but the changes in this PR only adjust spacing/margins here. Either implement the described lane styling/index passing, or update the PR description to match what’s actually included.
Owner
Author
|
@sourcery-ai summary |
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
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.
Summary by Sourcery
改进选项编辑器布局以及开关选项的可访问性，使选项行更清晰、更一致。
改进内容：
Original summary in English
Summary by Sourcery
Improve option editor layout and switch option accessibility for clearer, more consistent option rows.
Enhancements:
改进点：
Original summary in English
Summary by Sourcery
改进选项编辑器布局以及开关选项的可访问性，使选项行更清晰、更一致。
改进内容：
Original summary in English
Summary by Sourcery
Improve option editor layout and switch option accessibility for clearer, more consistent option rows.
Enhancements:
改进内容：
Original summary in English
Summary by Sourcery
改进选项编辑器布局以及开关选项的可访问性，使选项行更清晰、更一致。
改进内容：
Original summary in English
Summary by Sourcery
Improve option editor layout and switch option accessibility for clearer, more consistent option rows.
Enhancements:
改进点：
Original summary in English
Summary by Sourcery
改进选项编辑器布局以及开关选项的可访问性，使选项行更清晰、更一致。
改进内容：
Original summary in English
Summary by Sourcery
Improve option editor layout and switch option accessibility for clearer, more consistent option rows.
Enhancements:
增强内容：
Original summary in English
Summary by Sourcery
改进选项编辑器布局以及开关选项的可访问性，使选项行更清晰、更一致。
改进内容：
Original summary in English
Summary by Sourcery
Improve option editor layout and switch option accessibility for clearer, more consistent option rows.
Enhancements:
改进点：
Original summary in English
Summary by Sourcery
改进选项编辑器布局以及开关选项的可访问性，使选项行更清晰、更一致。
改进内容：
Original summary in English
Summary by Sourcery
Improve option editor layout and switch option accessibility for clearer, more consistent option rows.
Enhancements:
改进内容：
Original summary in English
Summary by Sourcery
改进选项编辑器布局以及开关选项的可访问性，使选项行更清晰、更一致。
改进内容：
Original summary in English
Summary by Sourcery
Improve option editor layout and switch option accessibility for clearer, more consistent option rows.
Enhancements:
改进点：
Original summary in English
Summary by Sourcery
改进选项编辑器布局以及开关选项的可访问性，使选项行更清晰、更一致。
改进内容：
Original summary in English
Summary by Sourcery
Improve option editor layout and switch option accessibility for clearer, more consistent option rows.
Enhancements: