Fix for Chromium v140 select issue #4597
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe sortSelect jQuery plugin in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Ports Tab UI
participant Old as sortSelect (Before)
participant New as sortSelect (Now)
rect rgba(200,230,255,0.3)
UI->>Old: Invoke $(select).sortSelect()
Old->>Old: Collect subset of option elements\nSort subset by text (match-first)
Old->>Old: Reinsert sorted subset into DOM (partial mutation)
Old-->>UI: Return jQuery collection
end
rect rgba(200,255,200,0.3)
UI->>New: Invoke $(select).sortSelect()
New->>New: For each select: build optionData[] with\n(value, text, selected, disabled)
New->>New: Sort optionData (match-first, localeCompare)
New->>New: Clear all options and recreate from optionData\n(preserve selected/disabled)
New-->>UI: Return original jQuery collection
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (2 passed, 3 warnings)❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
haslinghuis
force-pushed
the
fix-chromium140
branch
from
cf69192 to
95a72db
Compare
|
works, just needs @coderabbitai full review |
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/js/utils/common.js (1)
96-101: Tighten the comment (grammar + reference)Prefer a precise note and reference the issue. If you keep the comment, tweak grammar and add context.
- Chrome 140 does not work when we use this function on select element directly - - Temporary disable sorting for now + Chrome/Edge (Chromium 140+) misbehaves when sorting <option> nodes directly on a <select>. + Temporarily disable sorting. See #4595.
🧹 Nitpick comments (1)
src/js/utils/common.js (1)
94-120: Gate the workaround to Chromium ≥140 and avoid block-commented code; also keep behavior docs accurateBlanket-disabling sorting affects all browsers. Scope it to affected Chromium builds and keep the original path active elsewhere. Also replace the block comment with code + a crisp inline note. Diff below keeps chainability and the “pin text first” behavior.
$.fn.sortSelect = function (text = "") { - - /* - - Chrome 140 does not work when we use this function on select element directly - - Temporary disable sorting for now - - const op = this.children("option"); - - op.sort((a, b) => { - if (a.text === text) { - return -1; - } - if (b.text === text) { - return 1; - } - return a.text.localeCompare(b.text, window.navigator.language, { ignorePunctuation: true }); - }); - - return this.empty().append(op); - - */ - - return this; + // Workaround for Chromium 140+ (#4595): disable sorting only on affected browsers. + const isAffectedChromium = (() => { + const m = navigator.userAgent.match(/\b(?:Chrom(?:e|ium)|Edg)\/(\d+)/); + return !!m && parseInt(m[1], 10) >= 140; + })(); + if (isAffectedChromium) return this; // preserve chainability when disabled + + const op = this.children("option").get(); + op.sort((a, b) => { + if (a.text === text) return -1; + if (b.text === text) return 1; + return a.text.localeCompare(b.text, window.navigator.language, { ignorePunctuation: true }); + }); + this.empty().append(op); + return this; };Outside this hunk, please align the JSDoc above to reflect the temporary Chromium 140+ disable:
/** * Returns a jQuery collection of options sorted alphabetically, with an optional `text` pinned on top. * Note: Sorting is temporarily disabled on Chromium 140+ due to a browser issue (see #4595). */Optionally, consider a feature-detection fallback instead of UA sniffing (try sorting options in a detached and verify the resulting order) if you want to future-proof the guard. 📜 Review details Configuration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📥 Commits Reviewing files that changed from the base of the PR and between 4a5b646 and 95a72db. 📒 Files selected for processing (1) src/js/utils/common.js (2 hunks) 🔇 Additional comments (1) src/js/utils/common.js (1) 118-119: Good: chainability preserved Returning this avoids breaking jQuery chains when the behavior is disabled.
haslinghuis
force-pushed
the
fix-chromium140
branch
from
95a72db to
b609e93
Compare
haslinghuis
force-pushed
the
fix-chromium140
branch
from
b609e93 to
b07ef40
Compare
|
|
Preview URL: https://b07ef401.betaflight-configurator.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/js/utils/common.js (2)
105-111: If we keep sorting later: pin by value as well as text.Many call sites pass a value, not the rendered label. Matching only by text risks pinning the wrong option in localized UIs.
- optionData.sort((a, b) => { - if (a.text === text) { return -1; } - if (b.text === text) { return 1; } + optionData.sort((a, b) => { + if (a.text === text || a.value === text) { return -1; } + if (b.text === text || b.value === text) { return 1; } return a.text.localeCompare(b.text, window.navigator.language, { ignorePunctuation: true }); });
112-123: Avoid O(n²) removal and reduce reflow if/when sorting returns.The while-remove(0) loop is quadratic and triggers repeated layout. Use a DocumentFragment plus replaceChildren for linear-time rebuild.
- // Remove all options - while (select.options.length) { select.remove(0); } - - // Add sorted options - optionData.forEach(opt => { - const option = document.createElement("option"); - option.value = opt.value; - option.text = opt.text; - option.selected = opt.selected; - option.disabled = opt.disabled; - select.add(option); - }); + // Rebuild efficiently + const frag = document.createDocumentFragment(); + optionData.forEach(opt => { + const option = document.createElement("option"); + option.value = opt.value; + option.text = opt.text; + option.selected = opt.selected; + option.disabled = opt.disabled; + frag.appendChild(option); + }); + select.replaceChildren(frag);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/utils/common.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/utils/common.js (2)
src/js/tabs/osd.js (2)
text(2513-2513)option(3462-3466)src/tabs/presets/presets.js (1)
text(238-238)
haslinghuis
changed the title
4 participants
Summary by CodeRabbit