feat: radial menu#4223
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces a complete radial menu system. A new 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
🧹 Nitpick comments (1)
js/app/packages/ui/components/RadialMenu/RadialMenu.tsx (1)
135-136: ⚡ Quick winReplace nested ternaries with
if/elsehelpers.This block uses nested ternaries in two places, which violates the TS/TSX project rule and makes branching harder to scan.
As per coding guidelines: `**/*.{js,jsx,ts,tsx}`: “Avoid nested ternary operators - prefer switch statements or if/else chains for multiple conditions”.Guideline-compliant rewrite
const transformFor = (align: LabelAlign): string => { - const x = align === 'center' ? '-50%' : align === 'west' ? '-100%' : '0'; + let x = '0'; + if (align === 'center') x = '-50%'; + else if (align === 'west') x = '-100%'; return `translate(${x}, -50%)`; }; + + const itemStateClass = (disabled: boolean, active: boolean): string => { + if (disabled) return 'border-edge-muted text-ink-disabled'; + if (active) return 'border-accent text-accent ring-1 ring-accent'; + return 'border-edge text-ink'; + }; @@ - cell.item.disabled - ? 'border-edge-muted text-ink-disabled' - : isActive() - ? 'border-accent text-accent ring-1 ring-accent' - : 'border-edge text-ink' + itemStateClass(cell.item.disabled ?? false, isActive())Also applies to: 258-263
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app/packages/ui/components/RadialMenu/RadialMenu.tsx` around lines 135 - 136, Replace the nested ternary operators used for calculating the x translation value with an if/else chain or switch statement. In the section where x is assigned based on the align parameter (checking if align equals 'center', 'west', or defaults to '0'), extract this logic into a helper function or use an if/else block to determine the correct x value before returning the translate string. Apply the same refactoring pattern to the similar nested ternary condition found in the second location (around lines 258-263) to maintain consistency and comply with the project's coding guidelines against nested ternary operators.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/app/packages/app/component/useRadialMenu.ts`:
- Line 51: In the `onCleanup` callback of `useRadialMenu.ts`, before calling
`removeScope(commandScopeId)`, check if the currently active scope (obtained
from `activeScope()`) matches the `commandScopeId` being removed. If it does,
restore the active scope to a DOM scope (typically by activating the parent
scope or a default scope) before removing the command scope to prevent leaving
hotkeys in a stale state after cleanup.
In `@js/app/packages/ui/components/RadialMenu/RadialMenu.tsx`:
- Around line 205-210: The click event listener for the RadialMenu is registered
in the bubble phase (default), which allows clicks to reach the underlying UI
before the menu can intercept them. This causes clicks to potentially trigger
actions in the UI behind the menu. Fix this by registering the click event
listener in the capture phase instead of the bubble phase, so that the menu can
intercept and prevent the click from reaching underlying elements. Change the
addEventListener call for 'click' on window (line 210) to use the capture phase
by passing true as the third argument to addEventListener or using an options
object with capture: true.
In `@js/app/packages/ui/components/RadialMenu/types.ts`:
- Around line 23-27: The slots property in the RadialMenu types file is
currently typed as Direction[], which allows an empty array that cannot be
selected since downstream code uses some(...) to check slots. Change the slots
property type from Direction[] to a non-empty array type (such as [Direction,
...Direction[]]) to ensure at least one direction is always specified and
prevent invalid menu items that cannot be aimed or selected.
- Around line 65-70: The JSDoc comments documenting default values for
deadZoneRadius, ringThickness, and ringGap properties in the type definitions do
not match the actual default values implemented in the RadialMenu component.
Update the documented defaults in the comments to align with the actual
implementation values: change deadZoneRadius default from 32 to 40,
ringThickness default from 56 to 96, and ringGap default from 6 to 8 to ensure
the public contract accurately reflects how the component behaves.
---
Nitpick comments:
In `@js/app/packages/ui/components/RadialMenu/RadialMenu.tsx`:
- Around line 135-136: Replace the nested ternary operators used for calculating
the x translation value with an if/else chain or switch statement. In the
section where x is assigned based on the align parameter (checking if align
equals 'center', 'west', or defaults to '0'), extract this logic into a helper
function or use an if/else block to determine the correct x value before
returning the translate string. Apply the same refactoring pattern to the
similar nested ternary condition found in the second location (around lines
258-263) to maintain consistency and comply with the project's coding guidelines
against nested ternary operators.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b7dbe16d-024f-4884-8fab-63995fe4207e
📒 Files selected for processing (10)
js/app/packages/app/component/RadialMenuDemo.tsxjs/app/packages/app/component/split-layout/componentRegistry.tsxjs/app/packages/app/component/useRadialMenu.tsjs/app/packages/ui/components/RadialMenu/RadialMenu.tsxjs/app/packages/ui/components/RadialMenu/geometry.test.tsjs/app/packages/ui/components/RadialMenu/geometry.tsjs/app/packages/ui/components/RadialMenu/index.tsjs/app/packages/ui/components/RadialMenu/types.tsjs/app/packages/ui/index.tsjs/app/vitest.config.ts
| type: 'command', | ||
| activationKeys: config.triggerHotkey ? [config.triggerHotkey] : [], | ||
| }); | ||
| onCleanup(() => removeScope(commandScopeId)); |
There was a problem hiding this comment.
Restore active scope before removing the command scope.
If unmount happens while activeScope() is this command scope, removeScope(commandScopeId) deletes it without reactivating a DOM scope (command-scope removal does not do that in @core/hotkey/utils). That can leave hotkeys in a broken/stale active-scope state after teardown.
Suggested fix
- onCleanup(() => removeScope(commandScopeId));
+ onCleanup(() => {
+ if (activeScope() === commandScopeId) {
+ activateClosestDOMScope();
+ }
+ removeScope(commandScopeId);
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/app/packages/app/component/useRadialMenu.ts` at line 51, In the
`onCleanup` callback of `useRadialMenu.ts`, before calling
`removeScope(commandScopeId)`, check if the currently active scope (obtained
from `activeScope()`) matches the `commandScopeId` being removed. If it does,
restore the active scope to a DOM scope (typically by activating the parent
scope or a default scope) before removing the command scope to prevent leaving
hotkeys in a stale state after cleanup.
| window.addEventListener('pointermove', onMove); | ||
| window.addEventListener('resize', onResize); | ||
| window.addEventListener('keydown', onKeyDown); | ||
| window.addEventListener('keyup', onRelease); | ||
| window.addEventListener('pointerup', onRelease); | ||
| window.addEventListener('click', onClick); |
There was a problem hiding this comment.
Prevent click-through to underlying UI while the menu is open.
Line 230 makes the overlay non-interactive (pointer-events-none), and Line 210 handles click on window in bubble phase. The underlying target processes the click first, so committing a radial selection can also trigger whatever is behind it.
Proposed interception in capture phase
- const onClick = () => {
- if (effectiveMode() !== 'hold' && armed) commitAim();
+ const onClick = (e: MouseEvent) => {
+ if (!armed) return;
+ e.preventDefault();
+ e.stopPropagation();
+ if (effectiveMode() !== 'hold') commitAim();
};
@@
- window.addEventListener('click', onClick);
+ window.addEventListener('click', onClick, true);
@@
- window.removeEventListener('click', onClick);
+ window.removeEventListener('click', onClick, true);Also applies to: 228-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/app/packages/ui/components/RadialMenu/RadialMenu.tsx` around lines 205 -
210, The click event listener for the RadialMenu is registered in the bubble
phase (default), which allows clicks to reach the underlying UI before the menu
can intercept them. This causes clicks to potentially trigger actions in the UI
behind the menu. Fix this by registering the click event listener in the capture
phase instead of the bubble phase, so that the menu can intercept and prevent
the click from reaching underlying elements. Change the addEventListener call
for 'click' on window (line 210) to use the capture phase by passing true as the
third argument to addEventListener or using an options object with capture:
true.
| * Contiguous slots this item occupies. A single direction (`['N']`) or an arc | ||
| * built with `span('N', 'W')`. Slots may be listed in any rotational order. | ||
| */ | ||
| slots: Direction[]; | ||
| /** Which ring the item lives on. Defaults to `'inner'`. */ |
There was a problem hiding this comment.
Make slots non-empty at the type level.
Line 26 currently allows [], which produces an item that can render but can never be aimed/selected because downstream lookup uses some(...) over slots.
Proposed type tightening
- slots: Direction[];
+ slots: [Direction, ...Direction[]];📝 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.
| * Contiguous slots this item occupies. A single direction (`['N']`) or an arc | |
| * built with `span('N', 'W')`. Slots may be listed in any rotational order. | |
| */ | |
| slots: Direction[]; | |
| /** Which ring the item lives on. Defaults to `'inner'`. */ | |
| * Contiguous slots this item occupies. A single direction (`['N']`) or an arc | |
| * built with `span('N', 'W')`. Slots may be listed in any rotational order. | |
| */ | |
| slots: [Direction, ...Direction[]]; | |
| /** Which ring the item lives on. Defaults to `'inner'`. */ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/app/packages/ui/components/RadialMenu/types.ts` around lines 23 - 27, The
slots property in the RadialMenu types file is currently typed as Direction[],
which allows an empty array that cannot be selected since downstream code uses
some(...) to check slots. Change the slots property type from Direction[] to a
non-empty array type (such as [Direction, ...Direction[]]) to ensure at least
one direction is always specified and prevent invalid menu items that cannot be
aimed or selected.
| /** Dead-zone radius in px. Defaults to 32. */ | ||
| deadZoneRadius?: number; | ||
| /** Radial thickness of each ring band in px. Defaults to 56. */ | ||
| ringThickness?: number; | ||
| /** Gap between the dead zone and rings in px. Defaults to 6. */ | ||
| ringGap?: number; |
There was a problem hiding this comment.
Align documented defaults with actual component defaults.
Line 65-70 documents defaults 32 / 56 / 6, but js/app/packages/ui/components/RadialMenu/RadialMenu.tsx (Line 25-27) uses 40 / 96 / 8. This public contract mismatch can mislead consumers configuring layout behavior.
[suggested fix: make docs and implementation match in one direction before merge.]
Proposed doc-side alignment (if runtime defaults are intentional)
- /** Dead-zone radius in px. Defaults to 32. */
+ /** Dead-zone radius in px. Defaults to 40. */
deadZoneRadius?: number;
- /** Radial thickness of each ring band in px. Defaults to 56. */
+ /** Radial thickness of each ring band in px. Defaults to 96. */
ringThickness?: number;
- /** Gap between the dead zone and rings in px. Defaults to 6. */
+ /** Gap between the dead zone and rings in px. Defaults to 8. */
ringGap?: number;📝 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.
| /** Dead-zone radius in px. Defaults to 32. */ | |
| deadZoneRadius?: number; | |
| /** Radial thickness of each ring band in px. Defaults to 56. */ | |
| ringThickness?: number; | |
| /** Gap between the dead zone and rings in px. Defaults to 6. */ | |
| ringGap?: number; | |
| /** Dead-zone radius in px. Defaults to 40. */ | |
| deadZoneRadius?: number; | |
| /** Radial thickness of each ring band in px. Defaults to 96. */ | |
| ringThickness?: number; | |
| /** Gap between the dead zone and rings in px. Defaults to 8. */ | |
| ringGap?: number; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/app/packages/ui/components/RadialMenu/types.ts` around lines 65 - 70, The
JSDoc comments documenting default values for deadZoneRadius, ringThickness, and
ringGap properties in the type definitions do not match the actual default
values implemented in the RadialMenu component. Update the documented defaults
in the comments to align with the actual implementation values: change
deadZoneRadius default from 32 to 40, ringThickness default from 56 to 96, and
ringGap default from 6 to 8 to ensure the public contract accurately reflects
how the component behaves.
1 participant
No description provided.