fix(InputGroup): only set tag ID when input has none#39
Conversation
Unconditionally calling setTagID() overwrites any ID the input already has (e.g. set by selectize() for its JS binding), breaking widgets like selectize that capture the element ID at construction time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe ChangesInputGroup tag ID preservation
Poem
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Ease/TWB5/InputGroup.php (2)
34-36: 🏗️ Heavy liftAdd test coverage for the conditional ID assignment.
The fix preserves pre-existing input IDs, which is critical for JavaScript widgets like selectize. Consider adding unit tests to verify:
- Inputs with pre-existing IDs are not overwritten
- Inputs without IDs receive auto-generated IDs
- Label
for=attribute correctly references the input ID in both casesThis would prevent regression and document the intended behavior.
Would you like me to help draft test cases for this fix?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Ease/TWB5/InputGroup.php` around lines 34 - 36, Add unit tests covering the conditional ID assignment in InputGroup: create three tests that (1) construct or mock an input with a pre-set ID and assert that after InputGroup rendering the input ID remains unchanged (use the input object's getTagID/setTagID methods to locate behavior), (2) construct an input without an ID and assert that after rendering an auto-generated ID is assigned (getTagID returns a non-empty, unique value), and (3) for both cases assert that the generated label's for attribute matches the input's getTagID value (exercise the InputGroup rendering/path that emits the <label for="..."> output). Ensure tests target the InputGroup code path that calls $input->getTagID() and $input->setTagID() so pre-existing IDs are preserved and missing IDs are generated, and add assertions to prevent regressions.
34-36: 💤 Low valueConsider a more explicit ID check to handle edge cases.
Using
empty()will returntruefor an ID of"0"(a valid but unusual HTML id), which would cause the guard to incorrectly callsetTagID()and overwrite it. While this edge case is unlikely in practice, a more defensive check would be:if (!$input->getTagID()) { $input->setTagID(); }or explicitly:
if ($input->getTagID() === null || $input->getTagID() === '') { $input->setTagID(); }Note: FormGroup.php uses the same
empty()pattern (see context snippet), so the current code is consistent with the existing codebase convention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Ease/TWB5/InputGroup.php` around lines 34 - 36, The guard uses empty($input->getTagID()) which treats the string "0" as missing and can overwrite a valid id; replace that check in InputGroup.php with a stricter test such as if ($input->getTagID() === null || $input->getTagID() === '') { $input->setTagID(); } (or alternatively if (!$input->getTagID()) if you prefer the slightly different semantics), keeping references to the input->getTagID() / input->setTagID() calls so the ID is only generated when truly absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Ease/TWB5/InputGroup.php`:
- Around line 34-36: Add unit tests covering the conditional ID assignment in
InputGroup: create three tests that (1) construct or mock an input with a
pre-set ID and assert that after InputGroup rendering the input ID remains
unchanged (use the input object's getTagID/setTagID methods to locate behavior),
(2) construct an input without an ID and assert that after rendering an
auto-generated ID is assigned (getTagID returns a non-empty, unique value), and
(3) for both cases assert that the generated label's for attribute matches the
input's getTagID value (exercise the InputGroup rendering/path that emits the
<label for="..."> output). Ensure tests target the InputGroup code path that
calls $input->getTagID() and $input->setTagID() so pre-existing IDs are
preserved and missing IDs are generated, and add assertions to prevent
regressions.
- Around line 34-36: The guard uses empty($input->getTagID()) which treats the
string "0" as missing and can overwrite a valid id; replace that check in
InputGroup.php with a stricter test such as if ($input->getTagID() === null ||
$input->getTagID() === '') { $input->setTagID(); } (or alternatively if
(!$input->getTagID()) if you prefer the slightly different semantics), keeping
references to the input->getTagID() / input->setTagID() calls so the ID is only
generated when truly absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fda122b-34c5-483e-8f66-4f3c901e3723
📒 Files selected for processing (1)
src/Ease/TWB5/InputGroup.php
1 participant
Summary
InputGroup::__constructwas calling$input->setTagID()unconditionally, overwriting any ID the input already had$('#id').selectize({...})binding targeted the old ID, but the rendered HTML had the new one)setTagID()when the input does not already have an IDTest plan
for=attribute still correctly references the input ID🤖 Generated with Claude Code
Summary by CodeRabbit