Skip to content

Code quality: address review findings to reach 9.5/10 #345

@thymikee

Description

@thymikee

Context

A bulk code review of all changes since v0.11.3 (15 commits, 102 files, +6,324/-660 lines) rated the codebase 7.5/10. This issue tracks the concrete fixes needed to reach 9.5/10.

Findings & Execution Plan

1. Magic Numbers & Undocumented Thresholds (recurring across files)

Scattered heuristic constants with no rationale. Makes maintenance and tuning harder.

Files & constants to document:

  • src/daemon/android-snapshot-freshness.tsANDROID_FRESHNESS_WINDOW_MS = 2_500
  • src/daemon/handlers/snapshot-capture.ts — retry delays, empty interactive thresholds
  • src/daemon/handlers/snapshot.ts — 2,000ms stale detection window
  • src/daemon/handlers/find.ts — 750ms snapshot cache window
  • src/daemon/handlers/interaction-scroll.ts — stall count threshold (2)
  • src/daemon/handlers/interaction-targeting.ts — 0.5px rect tolerance, 85-95% viewport overly-broad thresholds
  • src/daemon/scroll-planner.ts — 25% safe-band zones, 10%/8px lane padding
  • src/utils/output.ts — 20-node minimum / 8+ duplicates for nav subtree detection
  • src/utils/scroll-indicator.ts — 1%/99% boundary thresholds

2. Operator Precedence Bug in scrollable.ts

Line 11: role || '' + subrole || ''+ binds tighter than ||, producing incorrect concatenation.

3. Long Functions Need Decomposition

  • src/core/dispatch.tsdispatchCommand() ~786 lines
  • src/daemon/request-router.ts — 560+ lines, heavy RequestRouterDeps
  • src/daemon/handlers/session-observability.ts — three major commands in one function

4. Test Gaps: Missing Edge Cases

  • src/utils/__tests__/mobile-snapshot-semantics.test.ts — zero-width viewports, exact boundary nodes, negative coordinates
  • src/daemon/handlers/__tests__/snapshot-handler.test.ts — malformed snapshot data
  • src/utils/__tests__/output.test.ts — boundary conditions for nav subtree detection

5. Minor Issues

  • src/platforms/ios/devices.ts:339whichCmd('xcrun') result unused
  • src/platforms/android/app-lifecycle.ts:495 — global cachedBundletoolInvocation not safe for concurrent multi-device
  • src/platforms/android/ui-hierarchy.ts:322 — BFS uses shift() on array (O(n) per pop)

Scope

Items 1, 2, 4, and 5 are in scope for this issue. Item 3 (function decomposition) is intentionally deferred — it's a larger refactor that should be a separate PR.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions