Skip to content

fix: get real value of select#13

Open
zzxming wants to merge 2 commits intomainfrom
feat-option
Open

fix: get real value of select#13
zzxming wants to merge 2 commits intomainfrom
feat-option

Conversation

@zzxming
Copy link
Collaborator

@zzxming zzxming commented Mar 11, 2026

Summary by CodeRabbit

  • New Features
    • Extended the tooltip callback to include access to the originating control element, enabling handlers to receive additional context when the tooltip appears.

@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

Walkthrough

The TooltipItem.onShow callback signature was extended to include a fourth parameter—toolControl of type HTMLButtonElement | HTMLSelectElement—enabling handlers to access the originating control element that triggered the tooltip.

Changes

Cohort / File(s) Summary
Public API - Tooltip Callback
src/index.ts
Extended onShow callback signature to accept toolControl parameter, allowing access to the originating control element.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A button's voice now echoes clear,
The tooltip knows which friend is here,
With toolControl passed along the way,
Handlers shine in fresh display! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title mentions 'get real value of select' but the actual changes extend the onShow callback signature to include a toolControl parameter for both buttons and selects, which is broader than just 'select' and represents a more significant API change. Consider a more accurate title like 'feat: add toolControl parameter to onShow callback' to reflect that this is a broader API enhancement, not just a select value fix.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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 feat-option

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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/index.ts (1)

69-76: The fix correctly handles the select value edge case.

The logic properly reads the value attribute directly from the selected option, avoiding the browser's fallback to text content. Consider using instanceof for cleaner type narrowing without requiring an explicit cast:

💡 Optional: Use instanceof for type narrowing
 let currentValue = toolControl.value || '';
-if (toolControl.tagName.toLowerCase() === 'select') {
-  // When an <option> has no `value` attribute, `select.value` falls back to the
-  // option's text content. To avoid this, read the `value` attribute directly
-  // from the selected option element, which returns null (not the text) when absent.
-  const selectTool = toolControl as HTMLSelectElement;
-  currentValue = selectTool.options[selectTool.selectedIndex]?.getAttribute?.('value') || '';
+if (toolControl instanceof HTMLSelectElement) {
+  // When an <option> has no `value` attribute, `select.value` falls back to the
+  // option's text content. To avoid this, read the `value` attribute directly
+  // from the selected option element, which returns null (not the text) when absent.
+  currentValue = toolControl.options[toolControl.selectedIndex]?.getAttribute('value') || '';
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/index.ts` around lines 69 - 76, The currentValue handling for selects
should use type-safe narrowing instead of an explicit cast: inside the block
that checks toolControl.tagName, replace the cast-based approach by using
"toolControl instanceof HTMLSelectElement" to narrow the type and then read the
selected option's value attribute (via
toolControl.options[toolControl.selectedIndex]?.getAttribute('value') || '').
Update references to selectTool to operate directly on toolControl and keep the
same fallback to '' when the attribute is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/index.ts`:
- Around line 69-76: The currentValue handling for selects should use type-safe
narrowing instead of an explicit cast: inside the block that checks
toolControl.tagName, replace the cast-based approach by using "toolControl
instanceof HTMLSelectElement" to narrow the type and then read the selected
option's value attribute (via
toolControl.options[toolControl.selectedIndex]?.getAttribute('value') || '').
Update references to selectTool to operate directly on toolControl and keep the
same fallback to '' when the attribute is missing.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7372a178-e905-40b0-822e-680a53c1a198

📥 Commits

Reviewing files that changed from the base of the PR and between 6b6402e and f238db4.

📒 Files selected for processing (1)
  • src/index.ts

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