Skip to content

Fix review findings: safer type check, ARIA role, static import, deterministic sort#43

Merged
bd73-com merged 1 commit intomainfrom
claude/recreate-user-email-feature-MFpyw
Feb 20, 2026
Merged

Fix review findings: safer type check, ARIA role, static import, deterministic sort#43
bd73-com merged 1 commit intomainfrom
claude/recreate-user-email-feature-MFpyw

Conversation

@bd73-com
Copy link
Owner

@bd73-com bd73-com commented Feb 20, 2026

Summary

  • Safer selectorWarning type check: Use typeof check instead of bare as cast in use-monitors.ts to prevent runtime type mismatch
  • Accessibility: Add role="alert" to error detail div in MonitorDetails.tsx for screen readers
  • Static import: Replace dynamic await import() with static import of extractValueFromHtml in routes.ts to avoid circular dependency risk
  • Deterministic sort: Add localeCompare tiebreaker to auto-heal selector sort in scraper.ts
  • Fix auto-heal value truncation: Re-extract full value from page HTML instead of using sampleText (truncated to 80 chars), which could cause false change notifications for monitors with long values
  • Additional tests: Add tests for discoverSelectors and validateCssSelector edge cases

Supersedes PR #42 (which had merge conflicts with main after PR #40 landed).

Test plan

  • All 455 tests pass across 11 test files
  • Verify monitor creation shows correct selectorWarning type
  • Verify screen reader announces error details on MonitorDetails page
  • Verify auto-heal produces full (non-truncated) values for long content

https://claude.ai/code/session_017srEmUs7Xo11UjBtrszmvn

Summary by CodeRabbit

  • Bug Fixes

    • Improved CSS selector auto-healing with more deterministic results and accurate value re-extraction.
    • Enhanced type safety for monitor error handling.
  • Accessibility

    • Added proper ARIA role to alert notifications for better screen reader support.
  • Tests

    • Added comprehensive test suite for CSS selector validation and discovery functionality.

…rministic sort

Cherry-picks incremental fixes from PR #42 review:

- use-monitors.ts: Use typeof check for selectorWarning instead of bare
  `as` cast to prevent runtime type mismatch
- MonitorDetails.tsx: Add role="alert" to error detail div for
  accessibility (screen readers)
- routes.ts: Use static import of extractValueFromHtml instead of dynamic
  await import() to avoid circular dependency risk
- scraper.ts: Add localeCompare tiebreaker to auto-heal sort for
  deterministic selector selection
- scraper.ts: Fix auto-heal value truncation — re-extract full value from
  page HTML instead of using sampleText (truncated to 80 chars), which
  could cause false change notifications for long values
- scraper.test.ts: Add tests for discoverSelectors and validateCssSelector
  edge cases

https://claude.ai/code/session_017srEmUs7Xo11UjBtrszmvn
@coderabbitai
Copy link

coderabbitai bot commented Feb 20, 2026

No actionable comments were generated in the recent review. 🎉


📝 Walkthrough

Walkthrough

This PR refines selector discovery and auto-healing logic in the scraper service, adds type-safety guardrails in the client hook, improves accessibility in the monitor details page, optimizes imports, and adds comprehensive test coverage for CSS selector validation and browserless token handling.

Changes

Cohort / File(s) Summary
Type Safety & Imports
client/src/hooks/use-monitors.ts, server/routes.ts
Type-guard added to explicitly validate selectorWarning as string before use; static import of extractValueFromHtml replaces dynamic import in pre-check logic.
Accessibility
client/src/pages/MonitorDetails.tsx
Added role="alert" attribute to error alert div for improved semantic accessibility.
Selector Discovery & Healing Logic
server/services/scraper.ts
Auto-heal selector replacement now uses lexicographic tie-breaking (localeCompare) for deterministic ordering and re-extracts values from full HTML instead of truncated sample text to ensure accuracy of healed content.
Test Coverage
server/services/scraper.test.ts
Added comprehensive unit tests for discoverSelectors and validateCssSelector, including SSRF validation, Playwright/CDP mocking, browserless token handling, auto-prefixing of class selectors, edge cases (child combinators, nth-child, data attributes, boundary lengths), and error scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes all four main changes: type-safe checking, ARIA accessibility, static import, and deterministic sorting.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch claude/recreate-user-email-feature-MFpyw

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

@bd73-com bd73-com merged commit 363461c into main Feb 20, 2026
1 check passed
@bd73-com bd73-com deleted the claude/recreate-user-email-feature-MFpyw branch February 20, 2026 07:39
@bd73-com bd73-com added the fix label Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants