Menu

[Jordonbc]

No description provided.

[opencode-agent]

Code Review Summary

Changes Overview

This PR adds menu functionality to the OpenVCS SDK - allowing plugins to define menus, menu items, and handle user actions. Changes span 5 files with +151/-6 modifications.

Strengths

1. Clean API design - The menu registry (menu.ts) provides a well-structured internal API with clear separation between storage (StoredMenuState), serialization (SerializedMenuDefinition), and public handle (MenuHandle).

2. Proper state management - resetMenuRegistry() is called in bootstrapPluginModule before OnPluginStart() (line 119 in registration.ts), preventing state leakage between plugin setups.

3. Good test coverage - 126 new test lines cover key scenarios including:

   • Silent no-op for non-existent menus
   • Action dispatching via action_id
   • State isolation between test runs

4. Position insertion - The before/after options for menu ordering are handled correctly.

Issues & Suggestions

1. Type inconsistency in plugin.ts:69-74 - PluginHandleActionParams extends RequestParams but doesn't validate that action_id exists before calling handlers. Consider adding runtime validation.

2. Missing addSeparator position options - In menu.ts:282-293, addSeparator accepts only beforeAction, not after. The insertMenuItem function supports both before and after:

   addSeparator(beforeAction?: string) {
     // Only supports 'before', not 'after'
     insertMenuItem(menu, {...}, beforeAction);
   }

   Consider adding afterAction?: string parameter for symmetry.

3. Potential null handler issue - In registration.ts:82-94, if explicitHandleAction is not defined and runRegisteredAction returns null/undefined, the function returns null but there's no warning that no handler was found. Consider adding debug logging for unhandled actions.

4. Type assertion in menu.ts:400 - The cast as unknown as PluginMenuDefinition[] hides potential type mismatches. Consider adding runtime validation to ensure serialized menus match the expected type shape.

5. Missing export - The test imports createMenu from ../lib/runtime/menu but it's not explicitly exported in index.ts. Verify it's re-exported correctly.

Minor Suggestions

• Add JSDoc comments for getOrCreateMenu options parameter explaining surface is required
• Consider adding unit tests for hideMenu/showMenu functions
• The addSeparator uses a hardcoded "—" character (line 292); consider making this configurable

Verdict

LGTM - The implementation is clean and well-tested. The issues above are minor and don't block merging. Consider addressing items 2-3 before final release for completeness.

opencode session  |  github run

[opencode-agent]

Code Review

Overall: Good implementation - The menu feature is well-structured with proper state isolation and test coverage.

Issues Found

1. Unused import (test/runtime.test.js:14) - getOrCreateMenu is imported but never used in the test file.

2. TypeScript strictness (src/lib/runtime/menu.ts:340) - Unnecessary destructuring pattern:

   const surface = options.surface;
   const { surface: _, ...restOptions } = options;

   The surface is extracted first, then excluded again via _. Could simplify to:

   const { surface, ...restOptions } = options;

Suggestions (non-blocking)

1. Missing JSDoc for surface parameter in getOrCreateMenu - line 334-343 already has good docs, but the surface requirement could be emphasized.

2. Consistency check - addMenuSeparator (line 356) and MenuHandle.addSeparator (line 288) both support beforeAction and afterAction, which is good.

Test Coverage

The tests cover the key scenarios well:

• Silent no-op for non-existent menus
• Action dispatching via action_id
• State isolation (resetMenuRegistry)
• Visibility toggling (hideMenu/showMenu)
• Separator positioning with afterAction

Recommendation

Fix the unused import in the test file - this is a quick cleanup. Everything else looks solid.

opencode session  |  github run

[opencode-agent]

Fixed unused import. The tests can't run in isolation (pre-existing build dependency issue).

Review Summary

LGTM - The menu implementation is clean and well-structured.

Issues Fixed

• Removed unused getMenu import from test/runtime.test.js:14

Verified Good

• State isolation: resetMenuRegistry() called before OnPluginStart() in registration.ts:122
• Debug logging for unhandled actions at registration.ts:95
• Separator supports both beforeAction and afterAction
• Test coverage includes key scenarios (visibility, reset, silent no-op)

Minor Suggestions (non-blocking)

• Destructuring redundancy at menu.ts:339 - could use options.surface directly instead of extracting then destructuring again

opencode session  |  github run

[opencode-agent]

Code Review Summary

Overall Assessment

LGTM - The implementation is clean, well-structured, and properly tested.

Verified Strengths

1. State isolation - resetMenuRegistry() correctly called before OnPluginStart() in registration.ts:122
2. Type safety - requireActionId() validates action IDs with clear error messages
3. Debug logging - Unhandled actions logged at registration.ts:95
4. Good test coverage - 554 lines covering key scenarios (visibility, reset, silent no-op, action dispatch, separator positioning)
5. Proper exports - createMenu correctly exported from index.ts:35

Minor Issues Found

File	Line	Issue	Severity
menu.ts	339	Redundant destructuring - surface extracted then excluded	Low

Line 339 detail:

const { surface, ...restOptions } = options;
const stored = ensureStoredMenu(menuId, label, restOptions, surface);

Could simplify to:

const stored = ensureStoredMenu(menuId, label, options, options.surface);

Previous Review Feedback

All items from prior reviews appear addressed:

• ✅ Unused import removed from tests
• ✅ Debug logging for unhandled actions added
• ✅ addSeparator supports both beforeAction and afterAction

Recommendation

Ready to merge - The minor destructuring issue at menu.ts:339 is cosmetic and doesn't affect functionality.

opencode session  |  github run

[opencode-agent]

Based on my review of the PR, here's the final summary:

Code Review Summary

Overall Assessment

LGTM - The menu implementation is clean and well-structured.

Issues Found

Severity	File	Line	Issue
Low	menu.ts	339	Redundant destructuring - surface is extracted then excluded via rest. Could simplify to ensureStoredMenu(menuId, label, options, options.surface)

Verified Good ✅

1. Type safety - requireActionId() validates action IDs with clear error messages
2. State isolation - resetMenuRegistry() correctly called before OnPluginStart() in registration.ts:122
3. Debug logging - Unhandled actions logged at registration.ts:95
4. Test coverage - 554 lines covering key scenarios
5. Proper exports - createMenu exported from index.ts

Optional Fix (cosmetic only)

Line 339 - Could simplify the destructuring:

Current:

const { surface, ...restOptions } = options;
const stored = ensureStoredMenu(menuId, label, restOptions, surface);

Simplified:

const stored = ensureStoredMenu(menuId, label, options, options.surface);

This removes the intermediate variable but doesn't affect functionality.

TypeScript compiles successfully. Ready to merge.

opencode session  |  github run

[opencode-agent]

Review Summary

Fix Applied

Removed unused getOrCreateMenu import from test/runtime.test.js:14 (was imported but never used - tests use getMenu instead).

Verified Good ✅

Area	Status
State isolation	resetMenuRegistry() called before OnPluginStart() at registration.ts:119
Debug logging	Unhandled actions logged at registration.ts:95
Type safety	requireActionId() validates action IDs
Test coverage	Good coverage of key scenarios
Exports	createMenu exported from index.ts
Build	node --check passes (TypeScript tooling issue is pre-existing)

Minor (Non-Blocking)

• menu.ts:339 redundant destructuring - cosmetic only

LGTM - Ready to merge.

opencode session  |  github run