Skip to content

Fix LovCombo dropping string selections under native RegExp.escape#7796

Open
labkey-martyp wants to merge 4 commits into
release26.3-SNAPSHOTfrom
26.3_fb_lovcombo_timeofday_1266
Open

Fix LovCombo dropping string selections under native RegExp.escape#7796
labkey-martyp wants to merge 4 commits into
release26.3-SNAPSHOTfrom
26.3_fb_lovcombo_timeofday_1266

Conversation

@labkey-martyp

@labkey-martyp labkey-martyp commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Rationale

Newer browsers (Chrome, Firefox 140+) ship a native RegExp.escape that throws a TypeError on non-String input. Ext.ux.form.LovCombo.setValue() passed JSON.stringify(value) to RegExp.escape to avoid that crash, but JSON.stringify() quotes string values, so they no longer matched the raw, unquoted output of getCheckedValue() and every selection in a string-valued combo (e.g. the WNPRC EHR Time of Day multi-select) was silently dropped. Using String() yields a valid String for RegExp.escape without quoting string values, fixing both the numeric-valueField crash and the string-valueField regression. See https://github.com/LabKey/internal-issues/issues/1266.

Related Pull Requests

Changes

  • core/webapp/Ext.ux.form.LovCombo.js: in setValue(), pass String(...) rather than JSON.stringify(...) to RegExp.escape.

Newer browsers (Chrome, Firefox 140+) ship a native RegExp.escape that throws a TypeError on non-String input. Ext.ux.form.LovCombo.setValue() previously passed JSON.stringify(value) to RegExp.escape to avoid that, but JSON.stringify() quotes string values, so they no longer matched the raw, unquoted output of getCheckedValue() and every selection in a string-valued combo (e.g. the WNPRC EHR Time of Day multi-select) was silently dropped. Use String() instead: it yields a valid String for RegExp.escape without quoting string values, fixing both the numeric-valueField crash and the string-valueField regression.

LabKey/internal-issues#1266
Comment thread core/webapp/Ext.ux.form.LovCombo.js Outdated
this.store.suspendEvents(true);
this.store.clearFilter();
this.store.each(function (r) {
// Issue 1266: String() (not JSON.stringify) so native RegExp.escape doesn't throw on a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs to be GH Issue 1266 if you want it to link correctly. Otherwise, it will link to lk.org 1266.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added it. I don't see a link or anything, is that just for our reference? Or is it supposed to actually link them somewhere

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IntelliJ links our comments as we provide expressions to say what it should link. For example, this pattern for Kanban specific issues was added last week. LabKey/server#1416

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Huh, weird doesn't seem to be working for me. It's showing up as just plain comment in intelliJ. I see the GH Issue rule in vcs.xml. Maybe I need to refresh caches or something, looks pretty new.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, you'll likely need to refresh your IntelliJ settings and/or initiate a new project as these are just templates. We've had linking to our issues for a very long time. This is just the most recent flavors.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh wrong year, not very new. I got it working running ijConfigure. I never run that, always afraid it's going to mess up my intelliJ config. I guess this will be the test.

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.

3 participants