feat: configurable mobile top buttons! #361
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughA new mobile button configuration system was introduced. The configuration file now supports defining an array of mobile button actions, and corresponding TypeScript types were added to describe these buttons. The React component for mobile top buttons was refactored to dynamically render and handle actions based on the configuration, replacing previously hardcoded buttons. Additionally, F3 key handling was enhanced with debug HUD toggling and long press modal selection, and the debug overlay was updated to use global reactive state for visibility. Changes
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/appConfig.ts (2)
8-17: ✅ Types look solid, but add JSDoc for easier discoveryThe new
MobileButtonTypeunion and theMobileButtonConfiginterface read well and give consumers immediate type-safety.
Adding a short JSDoc block (or inline comments) for every property—especially the optional ones—would help future contributors understand which combinations are valid (e.g.command+type: 'command',modal+type: 'modal', etc.).No functional blockers here.
39-40: Consider markingmobileButtonsasreadonlyto avoid runtime mutationsBecause the configuration is loaded once at boot time, treating it as immutable prevents accidental mutations during runtime:
- mobileButtons?: MobileButtonConfig[] + readonly mobileButtons?: readonly MobileButtonConfig[]This preserves type-safety while signalling intent.
config.json (1)
72-78: Redundantmodalfield forchat/pauseentries
MobileTopButtons.tsxignoresbutton.modalfor"chat"and"pause"types and hard-codes the target modal instead.
If the field is meant for future use, add a comment inconfig.json; otherwise consider removing it to prevent confusion.src/react/MobileTopButtons.tsx (1)
190-194: Usepointerupfor key releases that should be heldFor actions like sneak (
ShiftLeft) you currently press & release after 100 ms regardless of finger duration.
If the intention is “hold while touching,” add aonPointerUplistener that triggers thekeyup, and remove the artificial timeout.This yields more natural control over continuous actions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
config.json(1 hunks)src/appConfig.ts(2 hunks)src/react/MobileTopButtons.tsx(3 hunks)
🔇 Additional comments (1)
config.json (1)
40-45: VerifyF3→KeyboardEvent.codemappingThe component dispatches:
new KeyboardEvent('keydown', { code: 'F3' })While browsers accept
'F3'forcode, it’s worth confirming on mobile Safari / Chrome to avoid silent no-ops.
If problems appear, switch to'F3'’s canonical code ('F3'is usually correct, but testing is advised).
src/react/MobileTopButtons.tsx
Outdated
| // Displaying buttons from the configuration | ||
| const renderConfigButtons = () => { | ||
| return mobileButtonsConfig?.map((button, index) => { | ||
| let className = styles['debug-btn'] | ||
| let label = button.label || button.icon || '?' | ||
|
|
||
| if (button.type === 'chat') { | ||
| className = styles['chat-btn'] | ||
| label = '' | ||
| } else if (button.type === 'pause') { | ||
| className = styles['pause-btn'] | ||
| label = '' | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Long-press scoreboard lost for custom “chat” buttons
The default chat button included chatLongPressEvent to open the player list (Tab).
renderConfigButtons skips that handler, so users lose a useful gesture when they switch to config-driven buttons.
Consider re-using chatLongPressEvent when button.type === 'chat':
- className = styles['chat-btn']
- label = ''
+ className = styles['chat-btn']
+ label = ''
+ extraProps = chatLongPressEventand spread extraProps on the <div>.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/react/MobileTopButtons.tsx around lines 172 to 184, the
renderConfigButtons function does not assign the chatLongPressEvent handler to
custom chat buttons, causing loss of the long-press gesture functionality. To
fix this, define an extraProps object that includes chatLongPressEvent when
button.type is 'chat', and spread extraProps onto the button's <div> element so
the long-press event is preserved for custom chat buttons.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/react/MobileTopButtons.tsx (1)
40-50: 💡 Verification agent🧩 Analysis chain
Potentially unused functions after refactor.
The
onChatLongPressandonChatClickfunctions may be unused if the component now relies entirely on the config-driven system. Consider removing them if they're no longer needed, or ensure they're properly integrated with the new system.
🏁 Script executed:
#!/bin/bash # Search for usage of onChatLongPress and onChatClick in the codebase echo "Searching for onChatLongPress usage:" rg "onChatLongPress" --type tsx --type ts echo "" echo "Searching for onChatClick usage:" rg "onChatClick" --type tsx --type tsLength of output: 304
🏁 Script executed:
#!/bin/bash # Retry searching for onChatLongPress and onChatClick without file-type filters echo "Searching for onChatLongPress usage:" rg "onChatLongPress" -n echo "" echo "Searching for onChatClick usage:" rg "onChatClick" -nLength of output: 402
Remove unused
onChatLongPressThe
onChatLongPressfunction is declared at src/react/MobileTopButtons.tsx:40 but never referenced elsewhere. You can safely remove it (and any related props/config) unless you plan to wire up a long‐press handler.The
onChatClickfunction, however, is used at line 58 and should remain as is.• src/react/MobileTopButtons.tsx:40 – delete unused
onChatLongPressdeclaration
• src/react/MobileTopButtons.tsx:58 – (keep)onChatClick()invocation
♻️ Duplicate comments (1)
src/react/MobileTopButtons.tsx (1)
93-163:⚠️ Potential issueMissing chat long-press functionality from previous implementation.
The
renderConfigButtonsfunction doesn't integrate the existingonChatLongPressfunctionality for chat buttons, causing a regression from the previous hardcoded implementation where chat buttons had Tab key long-press support.The previous review comment about this issue wasn't fully addressed. When
button.action === 'chat', the component should preserve the long-press gesture for opening the player list (Tab key).Consider adding long-press support for chat buttons:
if (button.action === 'chat') { className = styles['chat-btn'] label = '' + // Add long press support for chat buttons + const chatLongPressEvent = { + onTouchStart: (e) => { + const timerId = setTimeout(() => { + onChatLongPress() + }, 500) + e.currentTarget.dataset.chatLongPressTimer = String(timerId) + }, + onTouchEnd: (e) => { + const timerId = e.currentTarget.dataset.chatLongPressTimer + if (timerId) { + clearTimeout(parseInt(timerId, 10)) + delete e.currentTarget.dataset.chatLongPressTimer + } + } + } }Then spread these events onto the button's
<div>element.
🧹 Nitpick comments (1)
src/react/MobileTopButtons.tsx (1)
132-132: Replace Russian comment with English.The comment should be in English for consistency with the codebase.
- // Очищаем таймер long press если он есть + // Clear long press timer if it exists
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
config.json(1 hunks)src/appConfig.ts(2 hunks)src/controls.ts(4 hunks)src/globalState.ts(1 hunks)src/react/DebugOverlay.tsx(4 hunks)src/react/MobileTopButtons.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/globalState.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- config.json
- src/appConfig.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/react/DebugOverlay.tsx (1)
src/globalState.ts (1)
miscUiState(113-134)
src/controls.ts (2)
src/react/KeybindingsScreen.tsx (1)
contro(27-268)src/globalState.ts (1)
miscUiState(113-134)
🔇 Additional comments (5)
src/react/DebugOverlay.tsx (1)
2-5: LGTM! Clean migration to global state management.The transition from local component state to global reactive state is well-executed. Using
useSnapshot(miscUiState)for reactive updates andmiscUiState.showDebugHudfor state management provides consistent debug HUD control across the application.Also applies to: 33-33, 60-60, 179-179
src/controls.ts (1)
68-68: Excellent F3 command implementation with comprehensive keybind support.The F3 handling logic is well-designed:
- Properly tracks key state in
hardcodedPressedKeys- Supports F3+key combinations for specific actions via
f3Keybinds- Falls back to debug HUD toggle for standalone F3 press
- Correctly handles both press and release events
The integration with the existing keybind system is seamless.
Also applies to: 403-418, 514-514
src/react/MobileTopButtons.tsx (3)
2-6: LGTM! Clean integration with global state and type system.The new imports properly integrate the component with the global state management (
useSnapshot,miscUiState) and the new mobile button configuration types (MobileButtonConfig,ActionHoldConfig).
10-12: Well-designed interface extension for long press actions.The
ExtendedActionHoldConfiginterface cleanly extends the base configuration to support special long press behaviors like the F3 menu.
52-91: Robust unified command handling with proper event management.The
handleCommandfunction effectively centralizes command processing for both string commands andActionHoldConfigobjects. The logic correctly handles:
- Special cases (chat, pause, inventory)
- General control commands with proper emit/release cycles
- Command validation and routing
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-4u73ykub6-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/react/MobileTopButtons.tsx (1)
54-57: Custom chat buttons lose long-press functionalityThe default chat button's long-press gesture to open the player list is not preserved when using configuration-driven buttons.
Consider preserving the long-press event for chat buttons by checking the action type and applying the appropriate event handlers.
🧹 Nitpick comments (1)
config.json (1)
41-82: Consider using consistent types foractionHoldpropertyThe
actionHoldproperty is inconsistently typed across buttons - it's an empty string for most buttons but an object for the F3 button (lines 60-63). This could lead to type safety issues and runtime errors.Consider using
nullor omitting the property entirely when no hold action is needed:{ "action": "general.zoom", - "actionHold": "", + "actionHold": null, "label": "Zoom", "icon": "" },Or ensure all buttons use the object format:
{ "action": "general.zoom", - "actionHold": "", + "actionHold": { + "command": "", + "longPressAction": "" + }, "label": "Zoom", "icon": "" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config.json(1 hunks)src/controls.ts(6 hunks)src/react/MobileTopButtons.tsx(2 hunks)
🔇 Additional comments (1)
src/controls.ts (1)
406-421:✅ Verification successful
Verify F3 key handling doesn't create race conditions
The code adds 'F3' to
hardcodedPressedKeysand then checkscontro.pressedKeysfor other keys, assuming 'F3' won't be incontro.pressedKeys. This assumption might not always hold true.Consider adding explicit filtering to ensure correct behavior:
case 'general.debugOverlay': if (pressed) { hardcodedPressedKeys.add('F3') - const pressedKey = [...contro.pressedKeys].find(key => key !== 'F3') + // Explicitly filter out F3 to avoid race conditions + const pressedKey = [...contro.pressedKeys].filter(key => key !== 'F3')[0] if (pressedKey) { const keybind = f3Keybinds.find((v) => v.key === pressedKey) if (keybind && (keybind.enabled?.() ?? true)) { void keybind.action() } } else { miscUiState.showDebugHud = !miscUiState.showDebugHud } } else { hardcodedPressedKeys.delete('F3') } breakAdditionally, verify that the F3 keydown event handler at line 758 properly prevents F3 from being added to
contro.pressedKeys:
🏁 Script executed:
#!/bin/bash # Check if there's any preventDefault or stopPropagation for F3 key handling rg -A 5 -B 5 "keydown.*F3|F3.*keydown"Length of output: 1548
To inspect how
handleF3is implemented (and whether it callspreventDefault), let’s pull in its definition:
🏁 Script executed:
#!/bin/bash # Show handleF3 implementation to verify if it prevents default F3 handling rg -n "handleF3" -A5 -B5 src/react/DebugOverlay.tsxLength of output: 1351
No changes needed for the debugOverlay handler
The
handleF3listener in DebugOverlay.tsx already callse.preventDefault(), and thehardcodedPressedKeysmechanism in controls.ts ensures that'F3'is never present incontro.pressedKeys. Furthermore, using[...contro.pressedKeys].find(key => key !== 'F3')already filters out the F3 key correctly, so there’s no race condition and no additional filtering is required.
src/react/MobileTopButtons.tsx
Outdated
| const handleCommand = (command: string | ActionHoldConfig, isDown: boolean) => { | ||
| const commandString = typeof command === 'string' ? command : command.command | ||
|
|
||
| const defaultOptions = { | ||
| shouldPreventDefault: true, | ||
| delay: 500, | ||
| if (!stringStartsWith(commandString, 'custom')) { | ||
| if (isDown) { | ||
| contro.emit('trigger', { command: commandString } as any) | ||
| } else { | ||
| contro.emit('release', { command: commandString } as any) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for command existence before emitting events
The handleCommand function emits commands without verifying they exist in the contro schema. This could lead to runtime errors if invalid commands are configured.
Consider validating the command exists before emitting:
const handleCommand = (command: string | ActionHoldConfig, isDown: boolean) => {
const commandString = typeof command === 'string' ? command : command.command
+
+ // Validate command exists in schema
+ const [schema, action] = commandString.split('.')
+ if (!contro.inputSchema?.commands?.[schema]?.[action]) {
+ console.warn(`Invalid command: ${commandString}`)
+ return
+ }
if (!stringStartsWith(commandString, 'custom')) {
if (isDown) {
contro.emit('trigger', { command: commandString } as any)
} else {
contro.emit('release', { command: commandString } as any)
}
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/react/MobileTopButtons.tsx between lines 31 and 41, the handleCommand
function emits events based on command strings without validating whether the
command exists in the contro schema. Add a validation step to check if the
command is valid before emitting events, and only emit if the command is
recognized to prevent potential runtime errors.
| export const onF3LongPress = async () => { | ||
| const select = await showOptionsModal('', f3Keybinds.filter(f3Keybind => { | ||
| return f3Keybind.mobileTitle && (f3Keybind.enabled?.() ?? true) | ||
| }).map(f3Keybind => { | ||
| return `${f3Keybind.mobileTitle}${f3Keybind.key ? ` (F3+${f3Keybind.key})` : ''}` | ||
| })) | ||
| if (!select) return | ||
| const f3Keybind = f3Keybinds.find(f3Keybind => f3Keybind.mobileTitle === select) | ||
| if (f3Keybind) void f3Keybind.action() | ||
| } |
There was a problem hiding this comment.
Fix keybind selection logic in onF3LongPress
The modal displays formatted strings with key information (e.g., "Reload chunks (F3+KeyA)") but the selection logic searches for an exact match with mobileTitle, which will fail.
Fix the selection logic:
export const onF3LongPress = async () => {
+ const enabledKeybinds = f3Keybinds.filter(f3Keybind => {
+ return f3Keybind.mobileTitle && (f3Keybind.enabled?.() ?? true)
+ })
const select = await showOptionsModal('', enabledKeybinds.map(f3Keybind => {
return `${f3Keybind.mobileTitle}${f3Keybind.key ? ` (F3+${f3Keybind.key})` : ''}`
}))
if (!select) return
- const f3Keybind = f3Keybinds.find(f3Keybind => f3Keybind.mobileTitle === select)
+ // Find by index since we modified the display string
+ const selectedIndex = enabledKeybinds.findIndex(f3Keybind =>
+ `${f3Keybind.mobileTitle}${f3Keybind.key ? ` (F3+${f3Keybind.key})` : ''}` === select
+ )
+ const f3Keybind = enabledKeybinds[selectedIndex]
if (f3Keybind) void f3Keybind.action()
}Alternatively, pass the indices or original values to the modal for cleaner selection.
📝 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.
| export const onF3LongPress = async () => { | |
| const select = await showOptionsModal('', f3Keybinds.filter(f3Keybind => { | |
| return f3Keybind.mobileTitle && (f3Keybind.enabled?.() ?? true) | |
| }).map(f3Keybind => { | |
| return `${f3Keybind.mobileTitle}${f3Keybind.key ? ` (F3+${f3Keybind.key})` : ''}` | |
| })) | |
| if (!select) return | |
| const f3Keybind = f3Keybinds.find(f3Keybind => f3Keybind.mobileTitle === select) | |
| if (f3Keybind) void f3Keybind.action() | |
| } | |
| export const onF3LongPress = async () => { | |
| // Pre-filter the enabled keybinds once | |
| const enabledKeybinds = f3Keybinds.filter(f3Keybind => { | |
| return f3Keybind.mobileTitle && (f3Keybind.enabled?.() ?? true) | |
| }) | |
| // Show the modal based on the already-filtered list | |
| const select = await showOptionsModal( | |
| '', | |
| enabledKeybinds.map(f3Keybind => { | |
| return `${f3Keybind.mobileTitle}${f3Keybind.key ? ` (F3+${f3Keybind.key})` : ''}` | |
| }) | |
| ) | |
| if (!select) return | |
| // Find the index of the selected display string | |
| const selectedIndex = enabledKeybinds.findIndex(f3Keybind => | |
| `${f3Keybind.mobileTitle}${f3Keybind.key ? ` (F3+${f3Keybind.key})` : ''}` === select | |
| ) | |
| const f3Keybind = enabledKeybinds[selectedIndex] | |
| if (f3Keybind) void f3Keybind.action() | |
| } |
🤖 Prompt for AI Agents
In src/controls.ts around lines 938 to 947, the onF3LongPress function displays
options with formatted strings that include key information, but the selection
logic searches for an exact match with mobileTitle, which will not work because
the displayed string includes additional key info. To fix this, modify the
selection logic to match the selected string against the formatted options, or
pass indices or original objects to the modal for accurate identification.
Update the code to ensure the selected option correctly maps back to the
corresponding f3Keybind object.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/controls.ts (1)
939-948: Fix keybind selection logic in onF3LongPressThe modal displays formatted strings with key information but the selection logic searches for an exact match with
mobileTitle, which will fail.
🧹 Nitpick comments (1)
src/controls.ts (1)
957-978: Consider simplifying the command type handling logicThe type narrowing in
handleMobileButtonActionCommandis complex and could be clearer. The current implementation works but could benefit from better variable naming and structure.Consider this refactoring for better clarity:
-export const handleMobileButtonActionCommand = (command: ActionType | ActionHoldConfig, isDown: boolean) => { - const commandValue = typeof command === 'string' ? command : 'command' in command ? command.command : command +export const handleMobileButtonActionCommand = (action: ActionType | ActionHoldConfig, isDown: boolean) => { + // Extract the actual command/action from the input + let commandOrAction: string | CustomAction + if (typeof action === 'string') { + commandOrAction = action + } else if ('command' in action) { + commandOrAction = action.command + } else { + commandOrAction = action as CustomAction + } - if (typeof commandValue === 'string' && !stringStartsWith(commandValue, 'custom')) { + if (typeof commandOrAction === 'string' && !stringStartsWith(commandOrAction, 'custom')) { const event: CommandEventArgument<typeof contro['_commandsRaw']> = { - command: commandValue as Command, + command: commandOrAction as Command, schema: { keys: [], gamepad: [] } } if (isDown) { contro.emit('trigger', event) } else { contro.emit('release', event) } - } else if (typeof commandValue === 'object') { + } else if (typeof commandOrAction === 'object') { if (isDown) { - handleMobileButtonCustomAction(commandValue) + handleMobileButtonCustomAction(commandOrAction) } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/appConfig.ts(2 hunks)src/controls.ts(7 hunks)src/react/MobileTopButtons.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/appConfig.ts
- src/react/MobileTopButtons.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (4)
src/controls.ts (4)
31-31: LGTM!The import statement correctly brings in the types needed for mobile button configuration support.
59-59: Key bindings look good!The new key bindings follow standard Minecraft conventions:
Tabfor player listF3for debug overlayHfor select itemdebugOverlayHelpMenuis appropriately set to null for mobile-only accessAlso applies to: 67-69
407-427: Debug overlay implementation is well-structured!The F3 key handling correctly:
- Tracks F3 state using
hardcodedPressedKeys- Detects F3 + key combinations for keybinds
- Toggles debug HUD when F3 is pressed alone
- Provides mobile access via
debugOverlayHelpMenu
438-458: Good refactoring of UI command handling!The consolidation of
ui.backandui.pauseMenulogic eliminates duplication while maintaining the expected behavior. The switch statement improves readability.
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-1xtby9370-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/react/MobileTopButtons.tsx (1)
30-46: Simplify the complex action extraction logic.The action extraction on line 31 is difficult to read and maintain. Consider extracting this into a helper function for better clarity.
+ const getActionForStyling = (button: MobileButtonConfig): string | undefined => { + if (button.action) return button.action + if (button.actionHold && typeof button.actionHold === 'object' && 'command' in button.actionHold) { + return button.actionHold.command + } + return undefined + } const getButtonClassName = (button: MobileButtonConfig): string => { - const actionForStyle = button.action || (button.actionHold && typeof button.actionHold === 'object' && 'command' in button.actionHold ? button.actionHold.command : undefined) + const actionForStyle = getActionForStyling(button)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config.json(1 hunks)src/react/MobileTopButtons.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (2)
src/react/MobileTopButtons.tsx (2)
1-28: LGTM! Clean setup with proper reactive state management.The imports are well-organized and the component setup correctly uses valtio's
useSnapshotfor reactive state management. The visibility control logic is straightforward and appropriate.
156-162: LGTM! Clean conditional rendering.The component return logic correctly handles both configured and fallback scenarios with proper ref assignment.
| onChatLongPress, | ||
| onChatClick, | ||
| { | ||
| shouldPreventDefault: true, | ||
| delay: 300, | ||
| } | ||
| ) | ||
| if (longPressTimerIdRef.current) { | ||
| clearTimeout(longPressTimerIdRef.current) | ||
| longPressTimerIdRef.current = null | ||
| } | ||
| actionToShortPressRef.current = null | ||
|
|
||
| const { actionHold, action } = button | ||
|
|
||
| if (actionHold) { | ||
| if (typeof actionHold === 'object' && 'command' in actionHold) { | ||
| const config = actionHold | ||
| if (config.longPressAction) { | ||
| actionToShortPressRef.current = config.command | ||
| longPressTimerIdRef.current = window.setTimeout(() => { | ||
| handleMobileButtonLongPress(config) | ||
| actionToShortPressRef.current = null | ||
| longPressTimerIdRef.current = null | ||
| }, config.duration || 500) | ||
| } else { | ||
| handleMobileButtonActionCommand(config.command, true) | ||
| } | ||
| } else if (action) { | ||
| actionToShortPressRef.current = action | ||
| longPressTimerIdRef.current = window.setTimeout(() => { | ||
| handleMobileButtonActionCommand(actionHold, true) | ||
| actionToShortPressRef.current = null | ||
| longPressTimerIdRef.current = null | ||
| }, 500) | ||
| } else { | ||
| handleMobileButtonActionCommand(actionHold, true) | ||
| } | ||
| } else if (action) { | ||
| handleMobileButtonActionCommand(action, true) | ||
| } | ||
| } | ||
|
|
||
| const onPointerUp = (e: React.PointerEvent) => { | ||
| const elem = e.currentTarget as HTMLElement | ||
| elem.releasePointerCapture(e.pointerId) | ||
|
|
||
| const { actionHold, action } = button | ||
| let wasShortPressHandled = false | ||
|
|
||
| if (longPressTimerIdRef.current) { | ||
| clearTimeout(longPressTimerIdRef.current) | ||
| longPressTimerIdRef.current = null | ||
| if (actionToShortPressRef.current) { | ||
| handleMobileButtonActionCommand(actionToShortPressRef.current, true) | ||
| handleMobileButtonActionCommand(actionToShortPressRef.current, false) | ||
| wasShortPressHandled = true | ||
| } | ||
| } | ||
|
|
||
| if (!wasShortPressHandled) { | ||
| if (actionHold) { | ||
| if (typeof actionHold === 'object' && 'command' in actionHold) { | ||
| const config = actionHold | ||
| if (config.longPressAction) { | ||
| if (actionToShortPressRef.current === null) { | ||
| if (typeof config.longPressAction === 'string') { | ||
| handleMobileButtonActionCommand(config.longPressAction, false) | ||
| } | ||
| } | ||
| } else { | ||
| handleMobileButtonActionCommand(config.command, false) | ||
| } | ||
| } else if (action) { | ||
| if (actionToShortPressRef.current === null) { | ||
| handleMobileButtonActionCommand(actionHold, false) | ||
| } | ||
| } else { | ||
| handleMobileButtonActionCommand(actionHold, false) | ||
| } | ||
| } else if (action) { | ||
| handleMobileButtonActionCommand(action, false) | ||
| } | ||
| } | ||
| actionToShortPressRef.current = null | ||
| } | ||
|
|
||
| return ( | ||
| <div | ||
| key={index} | ||
| className={className} | ||
| onPointerDown={onPointerDown} | ||
| onPointerUp={onPointerUp} | ||
| onLostPointerCapture={onPointerUp} | ||
| > | ||
| {label} | ||
| </div> | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor complex pointer event logic for better maintainability.
The pointer event handling logic is quite complex and difficult to follow. Consider breaking it down into smaller, focused functions for better readability and maintainability.
+ const handlePointerDown = (button: MobileButtonConfig, e: React.PointerEvent) => {
+ const elem = e.currentTarget as HTMLElement
+ elem.setPointerCapture(e.pointerId)
+
+ // Clear any existing timer
+ if (longPressTimerIdRef.current) {
+ clearTimeout(longPressTimerIdRef.current)
+ longPressTimerIdRef.current = null
+ }
+ actionToShortPressRef.current = null
+
+ // Handle the specific button action
+ // ... extract the logic here
+ }
+
+ const handlePointerUp = (button: MobileButtonConfig, e: React.PointerEvent) => {
+ // ... extract the logic here
+ }
const renderConfigButtons = () => {
return mobileButtonsConfig?.map((button, index) => {
// ...
- const onPointerDown = (e: React.PointerEvent) => {
- // ... complex logic
- }
- const onPointerUp = (e: React.PointerEvent) => {
- // ... complex logic
- }
+ const onPointerDown = (e: React.PointerEvent) => handlePointerDown(button, e)
+ const onPointerUp = (e: React.PointerEvent) => handlePointerUp(button, e)Also consider adding cleanup on component unmount:
+ useEffect(() => {
+ return () => {
+ if (longPressTimerIdRef.current) {
+ clearTimeout(longPressTimerIdRef.current)
+ }
+ }
+ }, [])Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/react/MobileTopButtons.tsx around lines 58 to 154, the onPointerDown and
onPointerUp handlers contain complex nested logic that reduces readability and
maintainability. Refactor by extracting smaller helper functions to handle
distinct parts of the logic, such as processing long press setup, executing
short press actions, and handling long press completions. Additionally, add a
cleanup effect using useEffect to clear any active longPressTimerIdRef timeout
when the component unmounts to prevent potential memory leaks or unexpected
behavior.
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-5zjalt3kz-zaro.vercel.app |
|
/deploy |
zardoy
changed the title
|
Deployed to Vercel Preview: https://prismarine-abh4qbr4e-zaro.vercel.app |
User description
Implements mobileButtons configuration in appConfig.ts and MobileTopButtons component.
Supports various button types (command, keypress, chat, pause) and integrates with
existing control system via contro.inputSchema.
PR Type
Enhancement
Description
Introduces configurable mobile buttons via
mobileButtonsin app configAdds support for multiple button types (command, keypress, chat, pause, modal)
Implements dynamic rendering and action handling for mobile buttons
Updates configuration and typings to support new button system
Changes walkthrough 📝
appConfig.ts
Add types and config entry for mobile button customizationsrc/appConfig.ts
MobileButtonTypeandMobileButtonConfigtypes for mobilebuttons
mobileButtonsarray toAppConfigtypeconfig.json
Add mobileButtons configuration with multiple button typesconfig.json
mobileButtonsarray with various button definitionscontrols
MobileTopButtons.tsx
Implement dynamic mobile button rendering and actionssrc/react/MobileTopButtons.tsx
modal buttons
Summary by CodeRabbit