Skip to content

Fix TypeError in LanguageDropdown when used in multiple mode#5823

Merged
bjester merged 1 commit intolearningequality:hotfixesfrom
akolson:fix-native_name-conundrum
Apr 14, 2026
Merged

Fix TypeError in LanguageDropdown when used in multiple mode#5823
bjester merged 1 commit intolearningequality:hotfixesfrom
akolson:fix-native_name-conundrum

Conversation

@akolson
Copy link
Copy Markdown
Member

@akolson akolson commented Apr 13, 2026

Summary

LanguageDropdown is used with multiple prop in SearchFilters.vue. In that mode, v-model holds an Array of selected language IDs. VAutocomplete's updateAutocomplete passes this.internalValue (the Array) to getValue, which eagerly evaluates getText(internalValue) as a JS argument before calling getPropertyFromItem — even though that fallback is never actually needed. getText in turn calls languageText(array). Arrays are objects in JS, so the primitive short-circuit guard in getPropertyFromItem does not apply, and languageText is invoked with the array directly. Since arrays have no native_name, item.native_name.split throws a TypeError.

Fix: guard languageText against item being null/undefined or lacking a native_name property (covers arrays, partial objects, and any future edge cases) by returning an empty string early.

Verification: ran the existing languageDropdown test suite and confirmed all tests pass, including two new cases covering the missing-native_name and array-input scenarios.

References

Fixes #5740
Recurrence of the issue addressed by #5465 (the null check was subsequently reverted without a documented reason).

Reviewer guidance

  • LanguageDropdown.vue:120 — the new guard uses == null (not !item.native_name) deliberately, to avoid catching empty-string native_name values that the existing test exercises.
  • SearchFilters.vue:68-71 — this is where multiple is used; reproduce the crash by opening Import from Channels, selecting a language filter, then blurring the dropdown.
  • No risky areas: change is purely defensive, touches only languageText in the dropdown component.

AI usage

I used Claude Code to trace through the Vuetify VAutocomplete and VSelect source to identify why languageText was receiving an Array argument in multiple mode. After understanding the root cause (eager argument evaluation in getValue), I wrote the guard and two new test cases. I reviewed the generated code and confirmed the condition == null is correct for the intended behaviour.

@akolson akolson requested a review from rtibblesbot April 13, 2026 12:26
Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

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

Tight, well-reasoned hotfix for the Sentry crash — guard and tests are exactly right.

CI passing. Phase 3: no dev server available; the template is entirely unchanged (only the languageText method was modified), so there is nothing visual to verify — the fix's correctness is established by the new unit tests and CI.

  • praise: see inline (×2)

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Reviewed the pull request diff checking for:

  • Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
  • Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
  • Architecture: duplicated concerns, minimal interfaces, composition over inheritance
  • Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
  • Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
  • Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
  • Checked CI status and linked issue acceptance criteria
  • For UI changes: inspected screenshots for layout, visual completeness, and consistency

},
methods: {
languageText(item) {
// VAutocomplete eagerly evaluates getText(internalValue) as a fallback arg to
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.

praise: The comment explaining Vuetify's eager getText(internalValue) argument evaluation is exactly the right thing to document here — this is non-obvious framework internals and would otherwise pull future maintainers into a VAutocomplete/VSelect source dive to understand why the guard exists.

// getValue, even when that fallback isn't needed. In multiple mode, internalValue
// is an Array, and arrays have no native_name. Guard against both that case and
// any other scenario where item or native_name is missing.
if (!item || item.native_name == null) {
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.

praise: == null (rather than !item.native_name) is the precisely correct check. It catches null, undefined, and array inputs (where .native_name is undefined) while deliberately allowing native_name: '' through so existing behaviour for empty-string entries is preserved. The companion test at line 82 of the spec explicitly pins this distinction — good defensive coding.

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.

You're right — I missed that the root cause is specifically an array being passed, not a null/undefined value. Checking item.native_name == null catches the array case only incidentally (because arrays have no native_name), but it also silently suppresses any case where a legitimate language object is missing native_name, which could hide real bugs. A direct Array.isArray(item) check is more precise and aligns with the documented root cause. Withdrawing my praise of this check.

@akolson akolson requested a review from bjester April 14, 2026 08:30
Copy link
Copy Markdown
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

Good investigation to find the reproduce scenario and root cause. I've verified that and agree with the assessment. Although, the defensive check doesn't really align with that. I can find no case where languageText is called with a falsy value. The item.native_name == null check is also indirect-- the analysis found it's called with an array and that's the cause of the issue. The native_name should always exist otherwise, so a more direct check would be to check that item is an array. That would align with the comment. Since we always expect native_name to exist otherwise, this would cause silent suppression of a potentially real issue.

VAutocomplete eagerly evaluates getText(internalValue) as the fallback
argument to getValue, even when the fallback is never used. In multiple
mode (used in SearchFilters), internalValue is an Array of selected ids.
Arrays are objects in JS, so getPropertyFromItem does not short-circuit
on the primitive guard and calls languageText with the array directly.
Since arrays have no native_name property, this throws a TypeError.

Guard languageText against item being null/undefined or lacking a
native_name (covers arrays, partial objects, and any future edge cases).

Fixes learningequality#5740.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
@akolson akolson force-pushed the fix-native_name-conundrum branch from 9fe7d4f to 4fa95bb Compare April 14, 2026 16:00
@akolson
Copy link
Copy Markdown
Member Author

akolson commented Apr 14, 2026

Fixed — changed the guard to Array.isArray(item) (matching the documented root cause) and dropped the test for missing native_name which was only there to cover the old broader check.

@akolson akolson requested a review from bjester April 14, 2026 16:01
@rtibblesbot
Copy link
Copy Markdown
Contributor

Thanks — is the right call, and dropping the -missing test keeps the suite honest. Looks good.

Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

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

Prior findings

1 prior finding unchanged (standing praise for the Vuetify root-cause comment at line 116).

Newly resolved:

  • Precision of the item guard (praise withdrawn in prior review, Array.isArray suggested) — code now uses Array.isArray(item) rather than == null

1/2 prior findings resolved. 0 re-raised.


CI passing. No UI files changed — Phase 3 skipped.

  • praise: Array.isArray(item) is the right fix — see inline.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

  • Retrieved prior bot reviews deterministically via the GitHub API
  • Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
  • Only raised NEW findings for newly introduced code
  • Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
  • Checked CI status and linked issue acceptance criteria

// getValue, even when that fallback isn't needed. In multiple mode, internalValue
// is an Array, so languageText receives the array directly. Return early to avoid
// calling .split() on undefined.
if (Array.isArray(item)) {
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.

praise: Array.isArray(item) is precisely right here. It targets the documented failure mode (VAutocomplete's eager getText(internalValue) in multiple mode) without silently swallowing errors from legitimate language objects that might be missing native_name. The guard is as narrow as the root cause requires — no more.

Copy link
Copy Markdown
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

Verified no errors occurred. Targeted fix looks good, and the reasoning was confirmed looking at the Vuetify source.

@bjester bjester merged commit 1af90af into learningequality:hotfixes Apr 14, 2026
18 checks passed
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