Fix #2346: apps/memos-local-plugin: viewer i18n locale default is untestable under Node (de - #2347
Open
Memtensor-AI wants to merge 2 commits into
Open
Conversation
Before this change, detectDefault() was a module-private function that read navigator.language and localStorage directly. Under Node (Vitest / SSR / CI without jsdom) navigator is undefined and localStorage throws (swallowed by try/catch), so the exported locale signal always initialised to "en" and the zh-* -> "zh" branch was unreachable from unit tests. Any test needing zh strings had to mutate locale.value. Export detectDefault with an options bag (navLanguage, storage, storageKey). All fields are optional; omitted fields fall back to the existing ambient globals, so browser behaviour is byte-for-byte identical. Tests inject a stub storage / navLanguage instead of reaching into module internals. Closes MemTensor#2346 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Memtensor-AI
added
ai:generated
Collaborator
Author
🤖 Open Code ReviewTarget: PR #2347 ✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s). Generated by cloud-assistant via Open Code Review. |
Collaborator
Author
🔧 Open Code Review requested Agent fixOpen Code Review found 3 issue(s). I have resumed the development Agent to fix them.
The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed. |
Three findings from the Open Code Review on MemTensor#2347: 1. Flatten the nested ternary that resolved the ambient/injected storage into a plain if/else chain. Same three cases (opts.storage provided -> use it; opts.storage undefined and globalThis has a localStorage -> use ambient; otherwise -> null), just readable at a glance. Behaviour is unchanged. 2. Document the asymmetric null vs undefined semantics on DetectDefaultOptions.storage. Omitting the field falls back to ambient localStorage; passing null is the explicit "disable storage" sentinel. Callers wanting a no-op stub (rather than disabling the lookup) should pass an object whose getItem returns null, not null itself. 3. Tag detectDefault as @internal - it is exported strictly for unit tests, not as a stable public API. Also note in the JSDoc that the opts.storage seam only covers the read path; setLocale still writes to ambient localStorage, so a Map-backed stub in tests will not observe writes. No behavioural change. All 13 tests in tests/unit/viewer/i18n-detect-default.test.ts still pass, and the wider viewer test suite (69 tests) is green. tsc --noEmit is clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Memtensor-AI
added
status:ready
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2346 — apps/memos-local-plugin viewer i18n
detectDefaultis now injectable so thezh-*→"zh"branch is testable under Node/Vitest/SSR.Root cause.
viewer/src/stores/i18n.tsdefineddetectDefault()as a module-private function that readnavigator.languageandlocalStoragedirectly. Under Node these globals are absent (localStorage access throws, swallowed by try/catch), so the module-levellocalesignal always initialised to"en"and thezh-*branch was unreachable from unit tests. Any test needing zh strings had to reach into the store and mutatelocale.value = "zh"(seetests/unit/viewer/restart-copy.test.tsfor the current workaround).Fix. Exported
detectDefault(opts?: { navLanguage?: string; storage?: Pick<Storage, "getItem"> | null; storageKey?: string })and aDetectDefaultOptionsinterface. All fields are optional; omitted fields fall back to the ambient browser globals (globalThis.localStorage,navigator.language) exactly as before, soexport const locale = signal<Locale>(detectDefault())at the module top level behaves byte-for-byte identically in the browser. Tests and SSR hosts can now inject a Map-backed storage stub or an explicitnavLanguageand drive both branches without touching global state.Verification.
tests/unit/viewer/i18n-detect-default.test.tscovers 13 cases: injected zh/en saved values, ambient-global fallbacks, custom storageKey, throwing-storage fallback, case-insensitive zh-* matching, and precedence of injected options over ambient globals. All 13 pass (0 skipped).restart-copy.test.tsworkaround still works unchanged.npm run lint(==tsc --noEmit) passes cleanly.tests/unit/memory/,tests/unit/storage/, etc. exist ondev-v2.0.30before this change and are outside the scope of the plugin viewer fix — this commit adds 13 passing tests and introduces zero new failures.Related Issue (Required): Fixes #2346
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Automated tests are pending.
Checklist
@whipser030, @hijzy please review this PR.
Reviewer Checklist