-
-
Notifications
You must be signed in to change notification settings - Fork 803
feat(wrapper): allow nesting submenus #3520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughIntroduces a configurable depth parameter in ItemSubMenu to support multi-level nested context menu submenus. Stores depth in component state and passes it through to MenuSubMenuItem during render, replacing a previously hard-coded depth value. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant CM as ContextMenuV2
participant ISM as ItemSubMenu (React)
participant MSI as MenuSubMenuItem
Caller->>CM: new ItemSubMenu({ text, items, depth })
CM->>ISM: construct with depth (default 1)
note right of ISM: Store depth in state: _depth
ISM->>MSI: render({ text, items, depth: _depth })
MSI-->>Caller: submenu rendered with correct nesting level
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jsHelper/spicetifyWrapper.js (2)
1819-1823
: Bug: incorrect setter used forleadingIcon
(no reactive updates).
this._setIcon
is undefined in this class; should callthis._setLeadingIcon
.set leadingIcon(name) { this._leadingIcon = name; - this._setIcon?.(this._leadingIcon); + this._setLeadingIcon?.(this._leadingIcon); }
1748-1750
: Support Sets when rendering submenu items.
items
is manipulated with.add/.delete
elsewhere, implying aSet
..filter
on aSet
throws. Convert to array first.- static itemsToComponents = (items, props, trigger, target) => { - return items.filter((item) => (item.shouldAdd || (() => true))?.(props, trigger, target)).map((item) => item._element); - }; + static itemsToComponents = (items, props, trigger, target) => { + const list = Array.isArray(items) ? items : Array.from(items ?? []); + return list + .filter((item) => (item?.shouldAdd || (() => true))?.(props, trigger, target)) + .map((item) => item._element); + };
🧹 Nitpick comments (4)
jsHelper/spicetifyWrapper.js (4)
1752-1761
: Clamp and sanitize the newdepth
input.Guard against
0
, negatives, floats, or non-numbers to avoid undefined UI behavior.- constructor({ text, disabled = false, leadingIcon, divider, items, depth = 1, shouldAdd = () => true }) { + constructor({ text, disabled = false, leadingIcon, divider, items, depth = 1, shouldAdd = () => true }) { this.shouldAdd = shouldAdd; this._text = text; this._disabled = disabled; this._leadingIcon = leadingIcon; this._divider = divider; this._items = items; - this._depth = depth; + // Sanitize depth: integer >= 1 + const d = Number.isFinite(depth) ? Math.floor(depth) : 1; + this._depth = Math.max(1, d);
1767-1768
: Depth in React state: either expose a setter or keep it as a plain prop.If depth won’t change post‑construction, you can drop state and pass
this._depth
directly; otherwise, expose a setter to reactively update it. Pick one to reduce complexity.Proposed setter (outside changed hunk, add near other setters):
set depth(value) { const d = Number.isFinite(value) ? Math.floor(value) : 1; this._depth = Math.max(1, d); this._setDepth?.(this._depth); } get depth() { return this._depth; }Also applies to: 1775-1776, 1782-1783
1959-1969
: Exposedepth
via wrapper constructors for convenience.
ContextMenu.SubMenu
can’t set depth currently; consumers must drop toItemSubMenu
. Consider adding an optionaldepth
arg and forwarding it.- constructor(name, items, shouldAdd, disabled = false, icon = undefined) { + constructor(name, items, shouldAdd, disabled = false, icon = undefined, depth = 1) { super({ text: name, disabled, leadingIcon: icon, items, + depth, shouldAdd: (props) => { const parsedProps = Spicetify.ContextMenuV2.parseProps(props); return parsedProps && shouldAdd(...parsedProps); }, }); }
1895-1899
: Mirror thedepth
option inMenu.SubMenu
as well.Parity between wrappers avoids API surprises.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
jsHelper/spicetifyWrapper.js
(2 hunks)
🔇 Additional comments (1)
jsHelper/spicetifyWrapper.js (1)
1792-1793
: VerifyMenuSubMenuItem
actually consumesdepth
.Sanity‑check in-app that hover/focus behavior stays correct for depth >= 2 and that positioning (placement right-start) doesn’t regress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the depth be automatically calculated tho?
Used to be hardcoded to 1 causing the nested submenu to disappear when hovered.
Allows this to work
To use add the depth and then 1+ the previous
Summary by CodeRabbit