Trackpad page: grid layout refactor #49
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors the Trackpad UI into modular key components (ArrowKeys, FnKeys, MediaKeys), replaces ControlBar with ControlKeys (adds keyboard toggle), removes ExtraKeys, adds style/className props to TouchArea, updates route to a 6-column grid with showKeyboard state, and removes sensitivity scaling from useTrackpadGesture. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Keys as Key Component
participant Trackpad as TrackpadRoute
participant Sender as Key Sender
participant Input as HiddenInput
User->>Keys: pointerDown(key)
Keys->>Trackpad: sendKey(key)
Trackpad->>Sender: normalize & forward(key)
Sender->>Input: dispatch native/key action
Input-->>Sender: acknowledge
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/trackpad.tsx (1)
131-132:⚠️ Potential issue | 🟡 MinorAggressive
onBlurre-focus could trap focus.The hidden input immediately re-focuses itself on blur via
setTimeout. This creates a focus trap that prevents users from interacting with other focusable elements (e.g., browser address bar, navigation). On mobile this is likely acceptable since the input powers the on-screen keyboard, but on desktop it can be problematic.
🤖 Fix all issues with AI agents
In `@src/components/Trackpad/ControlKeys.tsx`:
- Around line 27-50: The four buttons in ControlKeys.tsx (the elements invoking
handleInteraction with onToggleScroll, onLeftClick, onRightClick, and
onToggleKeyboard) are missing explicit type attributes and default to submit;
update each <button> in this component to include type="button" so they match
sibling components (ArrowKeys, FnKeys, MediaKeys) and satisfy the useButtonType
rule.
In `@src/routes/trackpad.tsx`:
- Around line 85-94: The container div that uses onClick={handleContainerClick}
lacks an ARIA role and keyboard handling; add role="application" to that div and
wire an onKeyDown prop (e.g., onKeyDown={handleContainerKeyDown}) that at
minimum prevents default/no-ops or delegates to the same focus logic so keyboard
users are covered; implement a small handler function named
handleContainerKeyDown in the component (matching handleContainerClick behavior
or simply a no-op that calls event.preventDefault()/event.stopPropagation() as
appropriate) to satisfy Biome's accessibility flag while preserving existing
focus/refocus behavior tied to showKeyboard and handleContainerClick.
🧹 Nitpick comments (3)
src/components/Trackpad/MediaKeys.tsx (2)
1-75: Component looks good overall; minor naming concern.The component is well-structured and consistent with the other key group components. One observation:
MediaKeysrenders bothUTILITY_KEYS(Esc, Tab, Ctrl, Alt, Shift, Meta, Home, End, PgUp, PgDn, Ins, Del) and actual media keys. The nameMediaKeysdoesn't fully reflect its scope. Consider a more descriptive name likeExtraKeysorKeyboardExtrasif you want accuracy, but this is a nit.
38-42:isPlayingstate can drift from actual remote playback state.The local
isPlayingtoggle has no feedback from the remote system. If the remote player's state changes independently (e.g., track ends, another client pauses), this button label will be out of sync. This is likely acceptable for a simple remote, but worth noting.src/routes/trackpad.tsx (1)
95-102: Ensure the TouchArea wrapper stretches to fill its1frrow.The wrapper
divat Line 95 relies on CSS Grid's defaultalign-items: stretchto fill the1frrow. This works, but if any ancestor overrides alignment, the touch area could collapse. Consider addingh-fullormin-h-0on this wrapper for explicitness.♻️ Defensive fix
- <div style={{ gridColumn: '1 / -1' }}> + <div style={{ gridColumn: '1 / -1' }} className="min-h-0 h-full">
|
@imxade i have completed the Trackpad UI changes. 
Pls review these changes. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/routes/trackpad.tsx`:
- Around line 127-131: The MediaKeys is being passed a hardcoded isPlaying=false
so the UI never reflects real playback; update the Trackpad route to provide the
actual playback state (e.g., the server-reported play state or local player
state) to MediaKeys instead of the literal false — locate the JSX where
MediaKeys is rendered (showKeyboard, sendKey, MediaKeys) and wire isPlaying to
the appropriate boolean (for example playbackState.isPlaying or a derived local
state updated from the server), or if no state is available change the UX to a
single toggle by removing the boolean prop and ensuring sendKey sends a toggle
command.
🧹 Nitpick comments (2)
src/routes/trackpad.tsx (1)
139-141: Aggressive focus trapping on blur may hurt accessibility.The
onBlurhandler unconditionally re-focuses the hidden input after 10ms, which can trap keyboard focus and prevent users from tabbing to browser controls or other UI elements. Consider gating the re-focus on whether the newdocument.activeElementis still within the trackpad container.Proposed improvement
- onBlur={() => { - setTimeout(() => hiddenInputRef.current?.focus(), 10); - }} + onBlur={() => { + setTimeout(() => { + const container = hiddenInputRef.current?.closest('[role="application"]'); + if (container?.contains(document.activeElement) || document.activeElement === document.body) { + hiddenInputRef.current?.focus(); + } + }, 10); + }}src/components/Trackpad/MediaKeys.tsx (1)
8-21: Modifier keys (Ctrl, Alt, Shift, Meta) sent as single taps — limited usefulness for key combos.These modifier keys are dispatched via
sendKeyas one-shot{ type: 'key' }events, which means there's no press-and-hold semantic for combinations like Ctrl+C. This is likely out of scope for this layout PR, but worth tracking as a follow-up to support modifier + key chords (e.g., viapress/releaseevents).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/trackpad.tsx (1)
139-141:⚠️ Potential issue | 🟡 MinorAggressive auto-refocus on blur may trap focus and frustrate screen-reader / keyboard users.
The
onBlurhandler immediately re-focuses the hidden input after 10 ms, making it nearly impossible for a user to intentionally move focus elsewhere (e.g., to navigate away via keyboard). Consider gating the re-focus onshowKeyboardor a user-intent signal so that focus isn't permanently captured.
🧹 Nitpick comments (4)
src/components/Trackpad/MediaKeys.tsx (2)
36-39:handlePlayPauseis redundant — it's identical tohandleInteract(e, "audioplay").You can remove this handler and wire the Play/Pause button the same way as every other button, reducing duplication.
♻️ Suggested simplification
- const handlePlayPause = (e: React.PointerEvent) => { - e.preventDefault(); - sendKey("audioplay"); - };And on the button:
- onPointerDown={handlePlayPause} + onPointerDown={(e) => handleInteract(e, "audioplay")}
7-20: Component nameMediaKeysis misleading — it bundles utility/modifier keys with media keys.
UTILITY_KEYS(Esc, Tab, Ctrl, Alt, Shift, Meta, Home, End, PgUp, PgDn, Ins, Del) are navigation and modifier keys, not media keys. Consider renaming to something likeExtraKeysorKeyboardKeys, or splitting into separate components for clarity — especially since the PR's goal is to decompose the old monolithicExtraKeysinto focused, modular components.src/routes/trackpad.tsx (2)
103-110: Grid item wrapper<div>s are unnecessary — the child components can be grid items directly.Each section is wrapped in a plain
<div>solely to applygridColumn. You could apply these styles directly to the child components (or wrap them with a styled fragment) to reduce DOM nesting. Not blocking, just a cleanliness note.♻️ Example for TouchArea
- <div style={{ gridColumn: '1 / -1' }}> - <TouchArea - isTracking={isTracking} - scrollMode={scrollMode} - handlers={handlers} - status={status} - /> - </div> + <TouchArea + isTracking={isTracking} + scrollMode={scrollMode} + handlers={handlers} + status={status} + style={{ gridColumn: '1 / -1' }} + />This would require the child components to forward the
styleprop (or accept aclassName). If that's too invasive, the wrappers are fine as-is.Also applies to: 111-120, 121-131
92-98: Consider using Tailwind grid utilities instead of inlinestylefor the template.The column template is static (
repeat(6, 1fr)) and can be expressed asgrid-cols-6. The row template is dynamic based onshowKeyboard, which justifies inline style, but you could also use a conditional className. Minor consistency point with the rest of the Tailwind-based styling.♻️ Partial migration
- className="grid h-full overflow-hidden" - style={{ - gridTemplateColumns: 'repeat(6, 1fr)', - gridTemplateRows: showKeyboard - ? '1fr auto auto auto' - : '1fr auto auto', - }} + className={`grid grid-cols-6 h-full overflow-hidden ${ + showKeyboard + ? '[grid-template-rows:1fr_auto_auto_auto]' + : '[grid-template-rows:1fr_auto_auto]' + }`}
|
the layout doesn't match with the mockup
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/trackpad.tsx (1)
131-133:⚠️ Potential issue | 🟡 MinorAggressive auto-refocus on blur may trap focus.
The
onBlurhandler immediately re-focuses the hidden input after 10ms. This could prevent users from interacting with other page elements (e.g., browser controls, navigation) since focus is always pulled back. Consider gating the re-focus on whether the new active element is still within the trackpad container.🛡️ Proposed guard
onBlur={() => { - setTimeout(() => hiddenInputRef.current?.focus(), 10); + setTimeout(() => { + // Only re-focus if focus didn't move to another interactive element + if (!document.activeElement || document.activeElement === document.body) { + hiddenInputRef.current?.focus(); + } + }, 10); }}
🧹 Nitpick comments (3)
src/components/Trackpad/ArrowKeys.tsx (1)
9-22: Component name vs. contents mismatch is minor but worth noting.
ArrowKeyscontains Esc, Tab, Ctrl, Alt, Shift, Meta, Del, and PrtSc alongside the actual arrow keys. A name likeUtilityKeysorNavKeyswould better describe the contents. Not blocking since this follows the reviewer's guidance to merge utility and arrow keys into one component.src/components/Trackpad/MediaKeys.tsx (1)
28-36: Consider addingaria-labelfor icon-only buttons.
titleprovides a tooltip but screen readers generally rely onaria-labelfor button purpose. Since these buttons have no visible text, addingaria-label={label}improves accessibility.♻️ Proposed fix
<button key={key} type="button" className="btn btn-sm min-w-0" onPointerDown={(e) => handleInteract(e, key)} title={label} + aria-label={label} >src/routes/trackpad.tsx (1)
92-96: Parentgrid-cols-6is unused — every child spans all 6 columns.Since all children use
gridColumn: '1 / -1', the 6-column definition at the parent level has no effect. The sub-components define their own internal 6-column grids. You could simplify the parent to justgrid(single column) with the row template, which would make the intent clearer and remove the need forgridColumn: '1 / -1'on every child.♻️ Simplified parent grid
- className={`grid grid-cols-6 h-full overflow-hidden ${ + className={`grid h-full overflow-hidden ${ showKeyboard - ? '[grid-template-rows:1fr_auto_auto_auto_auto]' - : '[grid-template-rows:1fr_auto]' + ? '[grid-template-rows:1fr_auto_auto_auto_auto]' + : '[grid-template-rows:1fr_auto]' }`}Then remove
style={{ gridColumn: '1 / -1' }}from each child component.
|
Hey @imxade i have done the suggested changes, pls review the UI changes. 
thanks for your suggestion and time |
|
this looks better rename the components back i.e. controlkeys -> controlbar and arrowkeys -> extrakeys so that I can compare changes in the files I dont think the controlbar component needed any change also use the "btn-primary" for media keys, if it looks good |
|
@imxade is it ok now? |
|
In my case clicking on keyboard-toggle toggles extrakeys instead of keyboard. I though of it as extrakeys will have a fixed position, and theyll get covered when keyboard pops. |
|
@imxade now try it out, i fixed the issue. |
|
the position of extrakeys will be fixed, i.e. when the keyboard is toggled on they'll be behind the keyboard instead sticking above only controlkeys wont be fixed, i.e. it'll stick above the keyboard when its on and above extrakeys when keyboard is off note than when keyboard is toggled on clicking on anything else than the toggle should not turn it off and when its off clicking anywhere else shouldn't turn it on |
|
once done leave a message on discord |
Nakshatra480
force-pushed
the
feature/trackpad-grid-layout
branch
4 times, most recently
from
d8b2db0 to
ea83d0d
Compare
|
@imxade I implemented the 14-row grid layout, modularized the keys into separate components, and removed the gesture sensitivity scaling. |
Nakshatra480
force-pushed
the
feature/trackpad-grid-layout
branch
from
ea83d0d to
b6294bb
Compare
2 participants
Closes #48
Broke the trackpad page into a 6-col CSS grid and split the old monolith components into smaller ones.
What changed
ControlBar→ ControlKeys (now has a keyboard toggle)ExtraKeys→ split into ArrowKeys, FnKeys, MediaKeysh-fullinstead offlex-1to fill its grid cellmin-w-0+overflow-hiddenso nothing clips on narrow screensLayout
Tested on 390x844 (iPhone 14) and desktop. Build + existing tests pass.
Summary by CodeRabbit
New Features
Removed
Refactor