Vanilla state management for language chooser#113
Vanilla state management for language chooser#113gmjgeek wants to merge 68 commits intosillsdev:mainfrom
Conversation
imnasnainaec
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 19 files reviewed, all discussions resolved
components/language-chooser/common/language-chooser-controller/src/state-management/field.ts line 14 at r1 (raw file):
* Callback to update the UI when the field changes */ onUpdate: ((newValue: T) => void) | null = null;
What is onUpdate for? It's never set within this class (or even within this pr). Should it be explicitly public? Or made private and added to the constructor analogously to onUpdateRequested?
components/language-chooser/common/language-chooser-controller/src/selectable.ts line 10 at r1 (raw file):
for (let i = 0; i < items.length; i++) { items[i].isSelected.value = i === index; }
What about something like
items.forEach((item, i) => {
item.isSelected.value = i === index;
});
|
@imnasnainaec Thanks for the feedback. |
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 3 of 34 files at r2, all commit messages.
Reviewable status: 3 of 54 files reviewed, all discussions resolved
components/language-chooser/common/language-chooser-controller/src/view-models/language-card.ts line 17 at r3 (raw file):
if (onSelect) { onSelect(isSelected); }
This can be shortened to onSelect?.(isSelected);, though that's a stylistic choice.
components/language-chooser/common/language-chooser-controller/src/view-models/script-card.ts line 17 at r3 (raw file):
if (onSelect) { onSelect(isSelected); }
This can be shortened to onSelect?.(isSelected);, though that's a stylistic choice.
components/state-management/state-management-core/src/field.ts line 29 at r3 (raw file):
if (this._onUpdateRequested) { this._onUpdateRequested(value, oldValue); }
This can be shortened to this._onUpdateRequested?.(value, oldValue);, though that's a stylistic choice.
components/state-management/state-management-core/src/field.ts line 41 at r3 (raw file):
public set value(value: T) { try { if (this.onUpdate) this.onUpdate(value);
This can be shortened to this.onUpdate?.(value);, though that's a stylistic choice.
There was a problem hiding this comment.
Pull request overview
This PR introduces a framework-agnostic state management system for a language chooser component, including a core library, Svelte adapter, controller logic, and a demo application using DaisyUI.
Key Changes:
- New state management core library with a
Fieldclass for reactive data binding - Svelte adapter that transforms vanilla view models into Svelte-compatible reactive properties
- Language chooser controller with view models for language selection, script selection, and customization
- Complete Svelte+DaisyUI implementation with demo application
Reviewed changes
Copilot reviewed 63 out of 64 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added new workspace entries for state-management and svelte packages |
| nx.json | Updated project list to include new packages in alphabetical order |
| eslint.config.mjs | Added ignore patterns for vite/vitest timestamp files and fixed trailing comma |
| components/state-management/state-management-core/* | Core state management library with Field class, tests, and documentation |
| components/state-management/state-management-svelte/* | Svelte adapter for transforming view models with reactive fields |
| components/language-chooser/common/language-chooser-controller/* | Framework-agnostic view models and logic for language selection |
| components/language-chooser/svelte/language-chooser-svelte-daisyui/* | Svelte components and demo app using DaisyUI for styling |
| .vscode/extensions.json | Added Prettier extension recommendation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
components/state-management/state-management-svelte/vite.config.ts
Outdated
Show resolved
Hide resolved
...ents/language-chooser/common/language-chooser-controller/src/view-models/language-chooser.ts
Outdated
Show resolved
Hide resolved
components/language-chooser/common/language-chooser-controller/test/language-chooser.spec.ts
Outdated
Show resolved
Hide resolved
components/state-management/state-management-core/vite.config.ts
Outdated
Show resolved
Hide resolved
components/language-chooser/common/language-chooser-controller/test/language-card.spec.ts
Outdated
Show resolved
Hide resolved
...ents/language-chooser/common/language-chooser-controller/src/view-models/language-chooser.ts
Outdated
Show resolved
Hide resolved
components/language-chooser/common/language-chooser-controller/package.json
Show resolved
Hide resolved
components/language-chooser/svelte/language-chooser-svelte-daisyui/package.json
Show resolved
Hide resolved
|
devinreview says Field.value setter calls updateUI before setting _value In field.ts:40-45, the |
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk partially reviewed 77 files and all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @gmjgeek).
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk partially reviewed 24 files and all commit messages, made 2 comments, resolved 13 discussions, and dismissed @copilot[bot] from 12 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gmjgeek).
components/language-chooser/common/language-chooser-controller/src/view-models/language-chooser.ts line 154 at r7 (raw file):
customizations.value.customDisplayName = displayName.value; _updateIsReadyToSubmit(); }
devinreview says the below. Do we need a comment at least? Or is this pointing out a problem?
components/language-chooser/common/language-chooser-controller/src/view-models/language-chooser.ts:R150-154
_onDisplayNameChanged mutates customizations object without triggering _onCustomizationsChanged
At line 151, customizations.value ??= {} uses the nullish coalescing assignment with the .value setter, which triggers updateUI but NOT _onUpdateRequested (i.e., _onCustomizationsChanged). Then line 152 mutates the object in-place: customizations.value.customDisplayName = displayName.value. This means changing the display name does NOT trigger _onCustomizationsChanged which would set selectedLanguage.value ??= UNLISTED_LANGUAGE. This appears intentional — the display name change should not force an unlisted language selection. However, this pattern of mutating the object in-place means the updateUI callback won't fire for the mutation on line 152, so the Svelte binding for customizations won't be notified of the customDisplayName change. In practice this may not matter since displayName is separately tracked, but it's a subtle reactivity gap worth being aware of.
components/language-chooser/common/language-chooser-controller/src/view-models/language-chooser.ts line 267 at r7 (raw file):
customDisplayName: customizations.value?.customDisplayName, }); selectedScript.requestUpdate(script);
Sorry; devinreview picked up something else:
submitCustomizeLanguageModal updates script after tag preview and display name are recalculated, leaving them stale
When submitCustomizeLanguageModal is called, the tag preview and display name are computed with the old script value because selectedScript is updated after customizations.requestUpdate(...) triggers the recalculation.
In submitCustomizeLanguageModal (lines 262-268), the order of operations is:
customizations.requestUpdate({...})— this triggers_onCustomizationsChanged→_onOrthographyChanged→_updateTagPreview()and_updateDisplayName(), both of which readselectedScript.value(still the old value at this point).selectedScript.requestUpdate(script)— this updatesselectedScript.valueto the new script, but sinceselectedScriptwas created without anonUpdateRequestedcallback (new Field<IScript | undefined>(undefined)at line 41), no recalculation of tag preview or display name is triggered.
As a result, tagPreview and displayName reflect the old script, not the newly submitted one. The fix is to set selectedScript.value = script before calling customizations.requestUpdate(...), so that _onOrthographyChanged computes with the correct script.
Impact: After submitting the customize language modal with a different script, the language tag preview and display name shown to the user will be incorrect until some other action triggers a recalculation.
Suggested fix
selectedScript.value = script;
customizations.requestUpdate({
region,
dialect,
customDisplayName: customizations.value?.customDisplayName,
});
}
| <button | ||
| class="btn btn-ghost" | ||
| onclick={() => languageChooser.promptForCustomTag()} | ||
| >Enter Custom Tag</button | ||
| > |
There was a problem hiding this comment.
🚩 CustomizationModal 'Enter Custom Tag' button bypasses onCustomizeButtonClicked routing
In CustomizationModal.svelte:94, the 'Enter Custom Tag' button calls languageChooser.promptForCustomTag() with no arguments. This bypasses the onCustomizeButtonClicked method in the controller, which passes the current customLanguageTag.value as a default: promptForCustomTag.value(customLanguageTag.value) (language-chooser.ts:224). The result is that when the user clicks 'Enter Custom Tag' from within the customization dialog, the prompt won't pre-populate with the existing custom tag. This may be intentional (the dialog is for creating a new custom tag, not editing), but differs from the controller's own onCustomizeButtonClicked behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function _cancelSearch() { | ||
| _currentSearchId++; | ||
| } | ||
|
|
||
| // Public methods | ||
| async function search(query: string) { | ||
| listedLanguages.value = []; | ||
| _cancelSearch(); | ||
| if (query.length > 1) { | ||
| const searchId = _currentSearchId; | ||
| await asyncSearchForLanguage(query, (results) => { | ||
| if (searchId !== _currentSearchId) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🚩 Public search() method bypasses _onSearchStringUpdated side effects
The search() function is exposed as a public method and also called internally from _onSearchStringUpdated. When called directly (e.g., viewModel.search("en")), it clears listedLanguages and performs the search, but does NOT clear selectedLanguage, selectedScript, customLanguageTag, or customizations, nor does it update searchString.value. This is a different code path from the user typing in the search bar (which goes through searchString.requestUpdate → _onSearchStringUpdated).
The unit tests use search() directly for convenience, but in a real app, calling search() while a language is selected would leave stale selection state. This appears intentional (as a lower-level API), but the asymmetry could surprise consumers.
Was this helpful? React with 👍 or 👎 to provide feedback.
imnasnainaec
left a comment
There was a problem hiding this comment.
@andrew-polk asked if I wanted to take another look at this pr. Most of my comments are ⛏️nitpicks and ❓questions that can be ignored. And none of them are substantial enough that they should hold up this pr. Though I think 2 are worth addressing in another pr if not addressed here:
npm run lintfailures- property inheritance in
transform-view-model.ts
@imnasnainaec partially reviewed 12 files and all commit messages, and made 10 comments.
Reviewable status: 65 of 81 files reviewed, 19 unresolved discussions (waiting on andrew-polk and gmjgeek).
components/state-management/state-management-svelte/src/transform-view-model.ts line 0 at r10 (raw file):
GH Copilot warns me of a mismatch between the property checking of this file and its tests. Here you use in svelteFields and in viewModel, both of which check for own properties and inherited properties. However, the test file only has tests for direct properties, not inherited. It might be good to add a test case with inherited properties in the test file so that the intended behavior is explicit. Or in the agent's words:
They do not test inherited-property behavior, so they neither require nor reject
in-prototype traversal.
If you want to lock this behavior down, add one test with a prototype property and assert it is not visible through the proxy.
And if you don't want to capture inherited properties, then you need something like the following changes:
in svelteFields->Object.prototype.hasOwnProperty.call(svelteFields, prop)in viewModel->of Object.keys(viewModel)
Also, in either case, it's probably worthwhile adding docstrings in this file.
components/language-chooser/common/find-language/scripts/langtagProcessingHelpers.ts line 0 at r10 (raw file):
⛏️ there's another "macrolangauge" in components/language-chooser/common/find-language/README.md
components/language-chooser/common/language-chooser-controller/src/view-models/language-chooser.ts line 308 at r10 (raw file):
} function hasValidDisplayName(selection: IOrthography) {
⛏️❓ hasValidDisplayName uses .trim() in just one place on customDisplayName. Have you considered if you should trim elsewhere in hasValidDisplayName or canSubmitOrthography (e.g., the other customDisplayName or on language, dialect, or region?.name)?
components/state-management/state-management-svelte/README.md line 0 at r10 (raw file):
⛏️ This README has variable line-lengths (as do others), so they could probably use Prettier attention.
components/state-management/state-management-svelte/tsconfig.json line 3 at r10 (raw file):
{ "compilerOptions": { "jsx": "react-jsx",
⛏️❓ Why does a svelte-folder tsconfig need "jsx": "react-jsx",?
components/language-chooser/svelte/language-chooser-svelte-daisyui/package.json line 0 at r10 (raw file):
🌱❓ Are there plans to create Storybook and/or Playwright tests for this new Svelte component?
components/language-chooser/svelte/language-chooser-svelte-daisyui/package.json line 13 at r10 (raw file):
"prebuild": "npm run typecheck", "build": "nx vite:build", "lint": "eslint .",
When I run npm run lint in this folder, it throws lots of errors (apparently a Prettier complaint due to line-endings).
This is not the only folder with that issue. I reckon they're overlooked because the root directory's package.json's "scripts": lacks "lint": "eslint .".
components/language-chooser/svelte/language-chooser-svelte-daisyui/vite.config.ts line 14 at r10 (raw file):
plugins: [ nxViteTsPaths(),
⛏️ I don't know why, but my IDE complains on nxViteTsPaths(), that Type 'Plugin<any>' is not assignable to type 'PluginOption'. (...and likewise on the dts({... below). And it only does that here and in none of the other vite.config.ts files. 🤷
components/language-chooser/svelte/language-chooser-svelte-daisyui/src/lib/LanguageChooser.svelte line 100 at r10 (raw file):
onSelect={onLanguageSelected} /> {#if lang.isSelected && viewModel.listedScripts.length > 0}
I'm not sure how close you intend to match the behavior of this to the React component.
For example, this shows script selector when viewModel.listedScripts.length > 0, but language-chooser-react-mui/src/LanguageChooser.tsx has language.scripts.length > 1.
You can ask an AI agent to compare the two files if you want to align them more.
A framework-agnostic view model for a language chooser.
The goal of this project is to make it easy to create a language chooser in any reactive framework (React, Svelte, Vue, etc).
TODO:
This change is