Refactor configurator architecture and expand tests - #4
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (28)
📝 WalkthroughWalkthroughThe PR modularizes the configurator and profile workflow, adds mapping and profile validation, introduces coverage commands and tests, updates shared profile geometry, improves CLI/HID handling, and translates tooling messages to English. ChangesConfigurator and profile workflow
Profile generation and shared mapping logic
CLI, HID, and thread-status tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant ConfiguratorWorkspace
participant MappingDialog
participant ProfileExportPanel
participant profile-workflow
App->>ConfiguratorWorkspace: render mapping workspace
ConfiguratorWorkspace->>MappingDialog: open mapping or export dialog
MappingDialog->>ProfileExportPanel: load export panel
ProfileExportPanel->>profile-workflow: prepare and review profile
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12f1b49bed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
tests/input-layer.test.mjs (1)
247-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the control lookups so fixture drift fails with a clear message.
Lines 249, 253 and 254 assign to the result of
.find(...)without a check. If a control id inprofiles/claude-shortcuts/mapping.jsonis renamed or removed, the test throwsTypeError: Cannot set properties of undefinedinstead of reporting the missing control. Line 247 has the same coupling through positional access.♻️ Proposed refactor
+ mutate({ manifest, mapping }) { + const control = (id) => { + const found = mapping.controls.find((candidate) => candidate.id === id); + assert.ok(found, `fixture must define control ${id}`); + return found; + }; mapping.formatVersion = "2.0.0"; mapping.presetId = `${manifest.id}-other`; mapping.layer.rgb.hex = "orange"; mapping.layer.name = "Other"; mapping.controls[1].id = mapping.controls[0].id; mapping.controls = mapping.controls.filter((control) => control.id !== "command-row-right"); - mapping.controls.find((control) => control.id === "command-row-center-right").action = { - type: "shortcut", - keys: ["Meta", "X"], - }; - mapping.controls.find((control) => control.id === "encoder-rotate").action = {}; - mapping.controls.find((control) => control.id === "joystick").action = {}; + control("command-row-center-right").action = { type: "shortcut", keys: ["Meta", "X"] }; + control("encoder-rotate").action = {}; + control("joystick").action = {}; mapping.unusedControls = []; },🤖 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 `@tests/input-layer.test.mjs` around lines 247 - 254, Update the fixture setup around the positional control access and the .find calls to validate that each expected control exists before reading or assigning its properties. Add clear assertions or guard failures naming the missing control IDs, including the control currently accessed through mapping.controls[1] and the controls used by command-row-center-right, encoder-rotate, and joystick, while preserving the existing mutations.tests/hid-device.test.mjs (1)
94-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: derive the sleep from the exported spacing constant.
The 65 ms value duplicates the
CALL_SPACING_MSvalue inscripts/lib/hid-device.mjs. Export that constant and use it here, so the intent stays clear if the firmware spacing changes.🤖 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 `@tests/hid-device.test.mjs` around lines 94 - 95, Export the CALL_SPACING_MS constant from the HID device module and update the test’s delay before the handle.writes assertion to derive from that exported constant instead of the duplicated 65 ms literal. Preserve the existing timing behavior and assertion.prototype/src/components/ConfiguratorWorkspace.jsx (1)
146-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
type="button"to the remaining buttons.These buttons omit
type, so the default issubmit. No form wraps this markup today, so behavior is correct.Topbaralready setstype="button"at line 47. Set it here too for consistency and to prevent a submit regression if a form is added later.Also applies to: 205-205, 212-212, 222-223
🤖 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 `@prototype/src/components/ConfiguratorWorkspace.jsx` around lines 146 - 147, Update the button elements in ConfiguratorWorkspace, including the one keyed by control.id and the additional buttons at the referenced locations, to explicitly set type="button"; match the existing Topbar convention and preserve their current click behavior.prototype/src/profile-workflow.js (1)
33-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComplete the optional chain on
subtle.
crypto.subtleis undefined in an insecure context.crypto?.subtle.digestthen throws a TypeError that the catch block converts to"". The outcome is the same empty hash, but the intent is clearer withcrypto?.subtle?.digest. The empty string also reaches the review UI as a valid value; consider returningnullso callers can distinguish "no hash available" from a computed digest.♻️ Proposed refactor
- const digest = await crypto?.subtle.digest( + const digest = await crypto?.subtle?.digest( "SHA-256", new TextEncoder().encode(text), );🤖 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 `@prototype/src/profile-workflow.js` around lines 33 - 46, Update sha256Hex to use optional chaining on subtle before invoking digest, preserving the existing empty-string fallback. Do not change the return contract to null unless the surrounding callers and review UI are explicitly updated to handle that distinct unavailable-hash value.prototype/src/configurator-presenter.js (1)
13-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse own-property lookup for catalog entries.
ACTIONS[entry]resolves inherited names such astoStringandconstructor. The??fallbacks currently absorb the result, so behavior is correct today. An own-property check makes the intent explicit and keeps the fallback path predictable if the shape changes.♻️ Proposed refactor
+function actionFor(entry) { + return typeof entry === "string" && Object.hasOwn(ACTIONS, entry) ? ACTIONS[entry] : undefined; +} + export function entryExportLabel(entry) { if (isCustomJoystick(entry)) return `${entry.directions} DIR`; if (isCustom(entry)) return formatCustomKeys(entry.keys); - return ACTIONS[entry]?.exportLabel ?? ACTIONS.none.exportLabel; + return actionFor(entry)?.exportLabel ?? ACTIONS.none.exportLabel; }Apply the same helper in
entryIconandentryShortcut.🤖 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 `@prototype/src/configurator-presenter.js` around lines 13 - 37, Add a shared own-property lookup helper for ACTIONS catalog entries, then use it in entryExportLabel, entryIcon, and entryShortcut instead of direct ACTIONS[entry] access. Preserve each function’s existing fallback behavior while ensuring inherited keys such as toString and constructor use the fallback path.prototype/src/hooks/use-theme.js (1)
16-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport an injectable
saveThemeand drop the unreachable fallback.
detectThemeaccepts an injectedstorage, but line 34 writes towindow.localStoragedirectly. The write path is therefore not testable, unlikesaveLocaleinprototype/src/i18n/index.jsandsaveStoredStateinprototype/src/configurator-state.js. At line 18 the?? THEME_ORDER[0]branch is unreachable becauseindexOfreturns -1 for unknown themes and(-1 + 1) % 3is 0.♻️ Proposed refactor
export function nextTheme(theme) { const index = THEME_ORDER.indexOf(theme); - return THEME_ORDER[(index + 1) % THEME_ORDER.length] ?? THEME_ORDER[0]; + return THEME_ORDER[(index + 1) % THEME_ORDER.length]; } + +export function saveTheme(theme, storage) { + try { + const target = storage ?? globalThis.window?.localStorage; + target?.setItem(THEME_STORAGE_KEY, theme); + } catch { + // Private browsing: the preference just will not persist. + } +} @@ apply(); - try { - window.localStorage.setItem(THEME_STORAGE_KEY, theme); - } catch { - // Private browsing: the preference just will not persist. - } + saveTheme(theme);🤖 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 `@prototype/src/hooks/use-theme.js` around lines 16 - 42, Update nextTheme to return the indexed theme directly without the unreachable nullish fallback, and export an injectable saveTheme function that accepts a theme and storage dependency and persists using that storage. Replace the direct window.localStorage write in useTheme with saveTheme, passing window.localStorage while preserving the existing private-browsing error handling.prototype/src/styles.css (1)
412-472: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTranslate the remaining French comments for consistency.
This diff rewrites almost every other comment in the file to English (for example the comments at lines 63-65, 241-255, and 526-534). Two comment blocks added in this same change stay in French: the
.keycap-action-assetcomments at line 414 and lines 418-421, and the icon-sizing comments at lines 458-461. Translate them so the file uses one documentation language consistently.🤖 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 `@prototype/src/styles.css` around lines 412 - 472, Translate the remaining French comments in the .keycap-action-asset and icon-sizing sections to English, including the comments describing hotspot centering, disc coverage, and Claude icon sizing; leave the CSS rules and behavior unchanged.prototype/tests/app-render.test.mjs (1)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport React, react-dom/server, and Vite via public entry points.
These imports reach into internal package files instead of the documented public API. This package serves as the entry point to the DOM and server renderers for React.
react-dom/serveris the intended import path, notreact-dom/server.node.js. The same applies tovite, whose public entry point is thevitepackage itself, notvite/dist/node/index.js.Internal paths are not part of the packages' stable contract and can move or disappear on minor/patch upgrades. A relative path into
node_modulesalso assumes dependencies are not hoisted elsewhere (e.g., a workspace root), unlike a bare specifier.♻️ Proposed fix
-import React from "../node_modules/react/index.js"; -import { renderToString } from "../node_modules/react-dom/server.node.js"; -import { createServer } from "../node_modules/vite/dist/node/index.js"; +import React from "react"; +import { renderToString } from "react-dom/server"; +import { createServer } from "vite";🤖 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 `@prototype/tests/app-render.test.mjs` around lines 3 - 5, Update the imports in the test to use the public bare package entry points: import React from react, renderToString from react-dom/server, and createServer from vite. Remove the relative node_modules paths and internal react-dom/server.node.js and vite/dist/node/index.js references.
🤖 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 `@prototype/src/App.jsx`:
- Around line 48-53: Update loadProfileWorkflow to clear profileWorkflowPromise
when the dynamic import rejects, allowing later calls to retry the fetch. Also
update the prefetch call sites using loadProfileWorkflow so their returned
promise has an attached rejection handler, preventing unhandled rejections.
In `@prototype/src/components/JoystickDial.jsx`:
- Around line 76-90: Move the closeTitle element from the SVG root into the
.joystick-dial-close path element so the tooltip applies only to the close zone.
Keep a single title instance and preserve the existing path geometry and glyph
rendering.
In `@prototype/src/components/KeyAssignmentEditor.jsx`:
- Around line 37-40: Update duplicateControlFor so duplicate detection proceeds
when either selectedControl.type is "key" or editingJoystickSlot is true;
continue returning null for the "none" entry and preserve the existing
duplicateKeyControls lookup.
In `@prototype/src/configurator-state.js`:
- Around line 213-223: Update parseOptionalNonNegativeInteger to validate that
the trimmed input uses only decimal digit characters before converting it,
rejecting hexadecimal, exponent, binary, and other non-decimal numeric forms
with the existing INVALID_APPSENSE_ID error. Preserve the optional empty-string
behavior and acceptance of non-negative decimal integers.
In `@prototype/src/profile-panel-loader.js`:
- Around line 1-6: Update prototype/src/profile-panel-loader.js lines 1-6 to
clear profilePanelPromise when the dynamic import used by loadProfileExportPanel
rejects, allowing subsequent calls to retry; update
prototype/src/components/MappingDialog.jsx lines 106-119 to wrap the
Suspense/ProfileExportPanel rendering in an error boundary that displays a retry
or fallback message instead of crashing the dialog.
In `@scripts/configure.mjs`:
- Line 24: Translate the remaining French messages to English: update the
startup log in scripts/configure.mjs at lines 24-24, translate the assertion
message in tests/thread-slots.test.mjs at lines 113-113, and rename the
enclosing test title in tests/hid-lighting.test.mjs at lines 92-93 to describe
that slotsToThreadEntries requires exactly six rows.
In `@tests/cli.test.mjs`:
- Around line 51-64: Update the repository validation test around the script
list in the “repository validation commands run successfully from the checkout”
test so it no longer unconditionally executes scripts/prepare-gui.mjs, which may
invoke ensureGuiDependencies and npm ci. Keep assertions focused on validation
scripts that do not install dependencies, or explicitly cover both existing and
missing prototype/node_modules outcomes without requiring network access.
---
Nitpick comments:
In `@prototype/src/components/ConfiguratorWorkspace.jsx`:
- Around line 146-147: Update the button elements in ConfiguratorWorkspace,
including the one keyed by control.id and the additional buttons at the
referenced locations, to explicitly set type="button"; match the existing Topbar
convention and preserve their current click behavior.
In `@prototype/src/configurator-presenter.js`:
- Around line 13-37: Add a shared own-property lookup helper for ACTIONS catalog
entries, then use it in entryExportLabel, entryIcon, and entryShortcut instead
of direct ACTIONS[entry] access. Preserve each function’s existing fallback
behavior while ensuring inherited keys such as toString and constructor use the
fallback path.
In `@prototype/src/hooks/use-theme.js`:
- Around line 16-42: Update nextTheme to return the indexed theme directly
without the unreachable nullish fallback, and export an injectable saveTheme
function that accepts a theme and storage dependency and persists using that
storage. Replace the direct window.localStorage write in useTheme with
saveTheme, passing window.localStorage while preserving the existing
private-browsing error handling.
In `@prototype/src/profile-workflow.js`:
- Around line 33-46: Update sha256Hex to use optional chaining on subtle before
invoking digest, preserving the existing empty-string fallback. Do not change
the return contract to null unless the surrounding callers and review UI are
explicitly updated to handle that distinct unavailable-hash value.
In `@prototype/src/styles.css`:
- Around line 412-472: Translate the remaining French comments in the
.keycap-action-asset and icon-sizing sections to English, including the comments
describing hotspot centering, disc coverage, and Claude icon sizing; leave the
CSS rules and behavior unchanged.
In `@prototype/tests/app-render.test.mjs`:
- Around line 3-5: Update the imports in the test to use the public bare package
entry points: import React from react, renderToString from react-dom/server, and
createServer from vite. Remove the relative node_modules paths and internal
react-dom/server.node.js and vite/dist/node/index.js references.
In `@tests/hid-device.test.mjs`:
- Around line 94-95: Export the CALL_SPACING_MS constant from the HID device
module and update the test’s delay before the handle.writes assertion to derive
from that exported constant instead of the duplicated 65 ms literal. Preserve
the existing timing behavior and assertion.
In `@tests/input-layer.test.mjs`:
- Around line 247-254: Update the fixture setup around the positional control
access and the .find calls to validate that each expected control exists before
reading or assigning its properties. Add clear assertions or guard failures
naming the missing control IDs, including the control currently accessed through
mapping.controls[1] and the controls used by command-row-center-right,
encoder-rotate, and joystick, while preserving the existing mutations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b06e0540-c3d2-4da4-8832-6dda48dc1517
📒 Files selected for processing (54)
.gitignorepackage.jsonprototype/package.jsonprototype/src/App.jsxprototype/src/components/ConfiguratorWorkspace.jsxprototype/src/components/JoystickDial.jsxprototype/src/components/KeyAssignmentEditor.jsxprototype/src/components/MappingDialog.jsxprototype/src/components/ProfileExportPanel.jsxprototype/src/components/ProfileLoader.jsxprototype/src/components/ProfileReview.jsxprototype/src/configurator-catalog.jsxprototype/src/configurator-presenter.jsprototype/src/configurator-state.jsprototype/src/hooks/use-theme.jsprototype/src/i18n/de.jsprototype/src/i18n/en.jsprototype/src/i18n/es.jsprototype/src/i18n/fr.jsprototype/src/i18n/index.jsprototype/src/profile-panel-loader.jsprototype/src/profile-session.jsprototype/src/profile-workflow.jsprototype/src/styles.cssprototype/tests/app-render.test.mjsprototype/tests/architecture.test.mjsprototype/tests/configurator-state.test.mjsprototype/tests/i18n.test.mjsprototype/tests/presentation.test.mjsprototype/tests/profile-session.test.mjsscripts/build-input-profile.mjsscripts/configure.mjsscripts/enable-agent-keys.mjsscripts/lib/gui-dependencies.mjsscripts/lib/hid-device.mjsscripts/lib/hid-frame.mjsscripts/lib/hid-lighting.mjsscripts/lib/thread-slots.mjsscripts/lighting-probe.mjsscripts/lighting.mjsscripts/prepare-gui.mjsscripts/thread-status.mjsshared/input-profile.mjsshared/thread-status-palette.mjstests/cli.test.mjstests/gui-dependencies.test.mjstests/helpers/input-profile-fixture.mjstests/hid-device.test.mjstests/hid-frame.test.mjstests/hid-lighting.test.mjstests/input-layer.test.mjstests/input-profile.test.mjstests/thread-slots.test.mjsthread-status/bin/emit.mjs
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Review fixes are now in @codex review |
|
✅ Action performedReview finished.
|
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41e2dded14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
The remaining joystick-sector duplicate warning is fixed in @codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Why
The previous configurator concentrated too many responsibilities in one large component and several hardware-facing paths were difficult to exercise safely. This refactor makes behavior easier to understand, test, and evolve while reducing repeated computation and keeping hardware/profile mutations behind explicit boundaries.
Validation
npm run checkSummary by CodeRabbit
New Features
Bug Fixes