fix(books): address CodeRabbit findings on the series autocomplete (#179)#188
Conversation
) CodeRabbit's inline review (5 Major findings) landed after #187 was merged and the docs follow-up commit was rate-limited, so they went unaddressed. This applies them. - Security/convention: the four series autocomplete fields used HtmlHelper::e() in the view; views must use htmlspecialchars(..., ENT_QUOTES, 'UTF-8') (12 occurrences across option value/text + hidden input). - Correctness (#74 class): the series _onEnterKey only committed the typed value when NOTHING was highlighted. With a highlighted suggestion that differs from the typed text, Enter fell through to Choices and committed the wrong value. Apply the same highlighted-vs-typed guard the publisher field already uses; add a dedicated regression test. - Tests: the issue-179 skip guard now also requires E2E_ADMIN_PASS (not just the email), so a missing password skips instead of failing at login. - Tests: setSeriesAutocomplete (series-cycles) and the collana fill (full-test) now drive the real Choices widget (open -> type -> Enter) instead of poking the hidden input, so they catch widget regressions. The helper stays context-aware: pages without the widget (series-detail admin form) fall back to a plain fill. Validated: PHPStan L5, php -l, issue-179 6/6, series-cycles 15/15, full-test 135/135.
|
Warning Review limit reached
More reviews will be available in 36 minutes and 46 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughFix del monkey-patch ModificheFix autocomplete serie e aggiornamento test
Diagrammi di sequenzasequenceDiagram
actor Utente
participant ChoicesWidget as Choices.js Widget
participant MonkeyPatch as _onEnterKey patch
participant HiddenInput as `#serie_padre`
Utente->>ChoicesWidget: digita "Seed: Fairy"
ChoicesWidget-->>Utente: mostra suggerimento evidenziato "Seed: Fairy Tail Universe"
Utente->>MonkeyPatch: preme Enter
MonkeyPatch->>MonkeyPatch: typed="Seed: Fairy", hl="Seed: Fairy Tail Universe"
MonkeyPatch->>MonkeyPatch: typed != hl (case-insensitive) → preventDefault()
MonkeyPatch->>HiddenInput: commitTyped("Seed: Fairy")
HiddenInput-->>Utente: valore = "Seed: Fairy"
Stima dello sforzo di revisione🎯 3 (Moderate) | ⏱️ ~20 minuti PR correlate
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@tests/full-test.spec.js`:
- Around line 690-700: The test for the collana (series) Choices.js field is
using an incorrect pattern for E2E stability. Replace the current approach where
collanaSearch.press('Enter') commits the value directly. Instead, after filling
the collanaSearch input with seriesValue, add a waitForTimeout to allow the
Choices dropdown suggestions to render, then click on the matching suggestion
element that appears. This follows the required fill + waitForTimeout + click
suggestion pattern for Choices.js tests to ensure stable E2E behavior and avoid
flakiness.
In `@tests/issue-179-series-autocomplete.spec.js`:
- Line 15: The test.skip() condition on the test declaration only checks for
ADMIN_EMAIL and ADMIN_PASS environment variables, but should also include checks
for the E2E database credentials (E2E_DB_*) to be consistent with the rest of
the E2E suite. Expand the condition to skip the test if either the admin
credentials or the database credentials are missing, preventing avoidable test
failures when database credentials are not configured and ensuring the skip
behavior is aligned across all E2E tests.
In `@tests/series-cycles.spec.js`:
- Around line 154-187: The setSeriesAutocomplete function currently uses
press('Enter') immediately after filling the search input, which doesn't follow
the established E2E contract for Choices.js interactions. Replace the
search.press('Enter') call with the proper sequence: after search.fill(v), add a
waitForTimeout to allow the dropdown suggestions to render, then locate and
click on the suggestion matching the value instead of relying on the Enter
keypress. This will make the test more deterministic and consistent with the
repository's E2E testing conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16285ab3-a8c6-4788-b37b-da874147922e
📒 Files selected for processing (4)
app/Views/libri/partials/book_form.phptests/full-test.spec.jstests/issue-179-series-autocomplete.spec.jstests/series-cycles.spec.js
…ries fields (#179) CodeRabbit (#188): the setSeriesAutocomplete helper and the full-test collana fill committed via Enter; the repo convention (.coderabbit.yaml) is fill + waitForTimeout + click suggestion. Switch both to it. The earlier click attempt failed only because the dropdown locator was page-scoped and matched a hidden dropdown of one of the other Choices widgets on the page — scope it to the field's own .choices wrapper. The typed value is offered as a create-option so it works for new names too. Validated: series-cycles 15/15, full-test 135/135.
1 participant
Applies CodeRabbit's 5 inline Major findings from #187 (autocomplete serie/universo/ciclo). They were posted after #187 had already been merged, and the docs follow-up commit hit CodeRabbit's review rate-limit, so they were never addressed — this PR closes that gap.
Findings addressed
book_form.phpHtmlHelper::e(); views must usehtmlspecialchars(..., ENT_QUOTES, 'UTF-8')(12 occurrences)book_form.php_onEnterKeyonly committed the typed value when nothing was highlighted. With a highlighted suggestion ≠ typed text, Enter committed the wrong value. Apply the same highlighted-vs-typed guard the publisher field already uses + new regression testfull-test.spec.jsissue-179 specE2E_ADMIN_PASS, not just the emailseries-cycles.spec.jssetSeriesAutocompletedrives the real widget; stays context-aware — pages without the widget (series-detail form) fall back to a plain fillValidation
php -l✅issue-179-series-autocomplete6/6 (incl. new adding new author #74 regression)series-cycles15/15full-test135/135 (0 failed)Summary by CodeRabbit
Release Notes
Bug Fixes
Tests