Skip to content

refactor(core): address audit findings (bugfixes, dedup, dead-code, comment hygiene)#383

Merged
BANANASJIM merged 1 commit into
mainfrom
fix/audit-core
Jun 7, 2026
Merged

refactor(core): address audit findings (bugfixes, dedup, dead-code, comment hygiene)#383
BANANASJIM merged 1 commit into
mainfrom
fix/audit-core

Conversation

@BANANASJIM

@BANANASJIM BANANASJIM commented Jun 7, 2026

Copy link
Copy Markdown
Owner

Addresses a batch of codebase-audit findings across src/core/.

Changes

  • gyro (correctness): selectTiltAxis returns null for .yaw instead of 0.0. Previously axis_y = "yaw" in tilt mode produced a constant-zero joy_y (non-null), which spuriously set suppress_right_stick_gyro and zeroed the physical stick Y axis. Now the axis is correctly absent. + regression test.
  • gyro (comment hygiene): removed the vader5 origin annotation; renumbered the process-step comments to a contiguous [1..5].
  • macro_player (correctness): emitPendingReleases now walks executed steps in reverse to compute net-held keys, removing the fixed 32-entry held[] cap that silently dropped releases for macros with >32 outstanding .down steps (stuck keys on cancel / layer-switch). + regression test (40 distinct held keys all released).
  • macro_player (comment hygiene): stripped resolved issue #99 references from doc comments.
  • mapper (dead-code): removed self.injected_buttons &= ~mask in the pending-tap-release flush — injected_buttons is unconditionally zeroed before output, so the masking had no effect; fixed the misleading comment.
  • mapper (perf): hoisted effectiveStickConfig(.left/.right) so each is computed once per apply() frame instead of twice.
  • mapper (comment hygiene): stripped resolved issue #99 reference from the axis-floor comment.
  • mapper (test-gap): added integration tests for gesture tap and gesture hold (gamepad bit persists across frames then clears on release) through apply(), and for pause_for_release macro drain on layer deactivation.

Skipped (verified, not changed)

  • timer_queue.cancel tombstone rewrite — micro-perf on a <32-entry heap; tombstone-by-token risks suppressing re-armed tokens. Current rebuild is correct and simpler.
  • gyro/stick mode []const u8 → enum — wide cross-file refactor (runtime structs, resolve fns, ~11 call sites) for poll-rate string compares the finding itself calls harmless.
  • layer.processLayerTriggers ResolvedLayerConfig — same perf-only tradeoff, requires a parallel resolved array kept in sync.
  • currentMappedGamepadFrame dedup into buildEmitState — ~60-line refactor of intentionally-divergent logic; high behavior-change risk for a simplification.

Test plan

  • scripts/padctl-docker test → EXIT=0 (full suite green).
  • Each new regression test proven falsifiable by reverting its production fix and confirming only that test fails for the right reason.

refs: codebase audit

Summary by CodeRabbit

Bug Fixes

  • Corrected tilt mode behavior where yaw axis was incorrectly converted to joystick output
  • Expanded macro support to handle more than 32 distinct keys held simultaneously, eliminating previous tracking limitations
  • Enhanced gesture tap/hold remapping and macro/layer deactivation to ensure proper key release handling across frames

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@coderabbitai

coderabbitai Bot commented Jun 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4420b732-4697-4bed-8d86-a15e766b89ca

📥 Commits

Reviewing files that changed from the base of the PR and between 8e1c4d6 and 78e3ddf.

📒 Files selected for processing (3)
  • src/core/gyro.zig
  • src/core/macro_player.zig
  • src/core/mapper.zig

📝 Walkthrough

Walkthrough

Three core input-processing improvements: tilt mode now skips yaw-to-stick conversion by returning null from selectTiltAxis; macro pending-release handling removes the 32-entry tracking limit via reverse-scan logic; mapper.apply optimizes tap-release clearing and reuses stick-config computations while expanding gesture and macro/layer test coverage.

Changes

Input Processing Improvements

Layer / File(s) Summary
Tilt axis null handling
src/core/gyro.zig
selectTiltAxis returns null for yaw instead of 0.0, preventing tilt-mode yaw-to-joystick conversion; processMotion step comments clarified; test added verifying joy_y == null when configured with axis_y = .yaw.
Macro pending release refactor
src/core/macro_player.zig
emitPendingReleases changed from forward-simulation with fixed 32-entry buffer to reverse-scan of executed steps, determining which .down targets remain net-held and emitting releases for uncovered targets; test added validating release of 40 distinct held keys.
Tap release and stick config optimization
src/core/mapper.zig
Mapper.apply now nulls pending_tap_release without explicit injected_buttons bit masking; effectiveStickConfig calls moved earlier to share left_cfg/right_cfg across gyro override and stick suppression logic, eliminating duplicate config computations.
Gesture remap and macro/layer interaction tests
src/core/mapper.zig
Added gesture tap/hold remap tests verifying tap key emission and gamepad bit persistence; added macro/layer interaction test confirming pause_for_release macros drain and emit releases correctly on layer deactivation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • BANANASJIM/padctl#313: Both PRs modify selectTiltAxis tilt-axis handling in src/core/gyro.zig and update related tilt joystick tests.
  • BANANASJIM/padctl#339: Main PR's MacroPlayer.emitPendingReleases refactor is directly related to the retrieved PR's use of the same method for clearing macro-held analog floors.
  • BANANASJIM/padctl#198: Both PRs modify Mapper.apply flow for tap emission and pending-release handling across frames.

Poem

🐰 A hop through input flows so clean,
Tilt axes null where yaw has been,
Macros release all forty keys,
Configs reused with utmost ease—
Padctl's precision, fresh and keen!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the PR's primary focus: addressing multiple audit findings across the core codebase with a mix of bugfixes, code deduplication, dead code removal, and comment improvements.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/audit-core

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…omment hygiene)

- gyro: selectTiltAxis returns null for .yaw (was 0.0), so axis_y="yaw" in
  tilt mode leaves joy_y absent instead of a constant-zero stick output that
  spuriously suppressed the physical Y axis; add regression test
- gyro: drop "vader5" origin annotation and renumber the process-step comments
  to a contiguous [1..5] sequence
- macro_player: emitPendingReleases walks executed steps in reverse instead of a
  fixed 32-entry held[] stack, so macros with >32 outstanding .down steps no
  longer leak stuck keys on cancel/layer-switch; add regression test
- macro_player: strip resolved issue #99 references from doc comments
- mapper: drop dead self.injected_buttons &= ~mask in the pending-tap-release
  flush (injected_buttons is unconditionally zeroed before output) and fix the
  misleading comment
- mapper: hoist effectiveStickConfig(.left/.right) so each is computed once per
  apply() frame instead of twice
- mapper: strip resolved issue #99 reference from the axis-floor comment
- mapper: add integration tests for gesture tap and gesture hold (gamepad bit
  persists then clears on release) through apply(), and for pause_for_release
  macro drain on layer deactivation

test plan: scripts/padctl-docker test (EXIT=0); each new regression test proven
falsifiable by reverting its production fix

refs: codebase audit
@BANANASJIM BANANASJIM merged commit 387ac36 into main Jun 7, 2026
36 checks passed
@BANANASJIM BANANASJIM deleted the fix/audit-core branch June 7, 2026 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant