Skip to content

Refactor Bluetooth Auto-Connect and VRT stability constraints - #9656

Merged
arii merged 2 commits into
fix-bluetooth-race-condition-17974409934683189531from
fix-bluetooth-race-condition-17974409934683189531-12746883362824899620
Mar 20, 2026
Merged

Refactor Bluetooth Auto-Connect and VRT stability constraints#9656
arii merged 2 commits into
fix-bluetooth-race-condition-17974409934683189531from
fix-bluetooth-race-condition-17974409934683189531-12746883362824899620

Conversation

@arii

@arii arii commented Mar 20, 2026

Copy link
Copy Markdown
Owner

This PR addresses the directives found in the principal engineer's audit of the Bluetooth race condition fix. It strips out over-engineered wrappers, refactors state hooks to prevent logic issues with Promise.reject(), cleans up VRT tests for proper stability without relying on large timeouts, and removes overly verbose comments.


PR created automatically by Jules for task 12746883362824899620 started by @arii

…lity

- Simplifies `shouldAutoConnect` logic in ConnectPage to prevent manual `timeoutId` management while preventing SSR mismatch
- Migrates module-scoped `connectionLock` to hook-scoped `useRef` to prevent application-wide lock conflicts
- Updates `connectAndStream` to properly reject promises when a connection is actively locked
- Replaces arbitrary 20-second Playwright timeouts with robust text locators in VRT tests
- Removes redundant comments and unneeded variables in components testing

Co-authored-by: arii <342438+arii@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@github-actions

Copy link
Copy Markdown
Contributor

👋 Welcome to HRM!

Thanks for your contribution. This repository uses Gemini AI for automated triage, code review, and generation.

🤖 Gemini Manual Trigger Quick Reference

Command Action
@gemini-bot Run AI Code Review (PR only)
@gemini-enrich Run PR Enrichment (PR only)
@gemini-triage Run Issue Triage
@gemini-coder <task> Generate Code
@create-review-issues Create issues from review (PR only)
@gemini-help Show this help message
@pr-squash Squash PR commits (PR only)
@conflict-resolve Resolve merge conflicts (PR only)

For more details and GitHub CLI examples, see the Manual Trigger Guide.

@github-actions

Copy link
Copy Markdown
Contributor

ℹ️ Gemini Review Disabled

Automatic review is currently disabled via GEMINI_ENABLE_PR_REVIEW. To run review manually:

Via Comment:

@gemini-bot

Via GitHub Actions:

  1. Go to Actions → Reusable Gemini Review
  2. Click "Run workflow"
  3. Enter PR number: 9656

Via GitHub CLI:

gh workflow run reusable-gemini-review.yml -f pr_number=9656

🤖 Gemini Manual Trigger Guide

To re-enable: Set repository variable GEMINI_ENABLE_PR_REVIEW=true

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide

@arii arii added the not reviewed The review could not be performed or completed successfully. label Mar 20, 2026
@arii

arii commented Mar 20, 2026

Copy link
Copy Markdown
Owner Author

🤖 AI Technical Audit

PR Review: Bluetooth & VRT Stability Refactor (#9656)

Architectural Impact

Moving connectionLock from a module-level variable to a useRef within the hook is the most significant change. This correctly scopes the connection state to the component instance, preventing cross-tab or multi-instance race conditions. The switch from returning false to Promise.reject() in connectAndStream improves error handling by allowing callers to distinguish between a skipped connection and a failure.

Anti-AI-Slop

  1. OVER-ENGINEERING: The previous implementation used a module-scoped connectionLock object which was unnecessary given React's useRef capabilities for instance-specific state. This PR correctly fixes this.
  2. CODE RATIO: Found redundant variable assignments in app/client/connect/page.tsx. The shouldAutoConnect boolean is used only once and can be inlined to save space.
  3. STALE FEATURES: Removed isTimeoutDisconnect.current assignments that were legacy residuals from a previous Bluetooth state machine. These were correctly purged.
  4. VERBOSITY: Removed the comment // Wait for the opacity transition... in VRT tests as the code expect(menu).toHaveCSS('opacity', '1') is self-documenting.

File-by-File Analysis

app/client/connect/page.tsx

Problem: The shouldAutoConnect constant is defined but then immediately used in an early return. This adds 4 lines of boilerplate.
Implementation Sample:

if (!(!isConnected && isSupported && connectionStatus === 'Connected' && !connectionAttempted && savedDeviceId)) return;

hooks/useBluetoothHRM.ts

Problem: The use of Promise.reject(new Error('Already connecting')) is a breaking change for any consumer not wrapping connectAndStream in a try/catch or .catch() block.
Implementation Sample:

// Ensure all call sites (like autoConnect) handle the rejection:
autoConnect().catch((err) => {
  if (err.message === 'Already connecting') return;
  logger.error('Actual connection failure', err);
});

tests/playwright/vrt-dashboard.spec.ts

Problem: Replacing a regex check with locator('text=Ready') is much more stable, but ensure "Ready" is a unique string on that page to avoid multiple element exceptions.

GitHub Checks Correlation

  • Lint Check Failure: The PR introduces an early return in the useEffect of page.tsx but might be violating the exhaustive-deps rule or formatting rules. Verify the tid naming convention matches the project's linting config (usually timeoutId).

Review automatically published via RepoAuditor.

- Runs prettier to fix the reported `Already connecting` line string spacing in `tests/unit/hooks/useBluetoothHRM.test.ts`.

Co-authored-by: arii <342438+arii@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide

@arii arii added not reviewed The review could not be performed or completed successfully. and removed not reviewed The review could not be performed or completed successfully. labels Mar 20, 2026
@arii
arii marked this pull request as ready for review March 20, 2026 15:36
@arii
arii merged commit b8237c2 into fix-bluetooth-race-condition-17974409934683189531 Mar 20, 2026
29 checks passed
@arii
arii deleted the fix-bluetooth-race-condition-17974409934683189531-12746883362824899620 branch March 20, 2026 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bluetooth not reviewed The review could not be performed or completed successfully. refactor stability-fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant