feat: persist search history by locale#3231
Open
lemagnetic wants to merge 3 commits intofeat/multilanguagefrom
Open
feat: persist search history by locale#3231lemagnetic wants to merge 3 commits intofeat/multilanguagefrom
lemagnetic wants to merge 3 commits intofeat/multilanguagefrom
Conversation
Store and read search history keyed by session locale;
lemagnetic
requested review from
eduardoformiga and
lariciamota
and removed request for
a team
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
actor Session
participant Hook as useSearchHistory
participant Config
participant Store as searchHistoryStore
Note over Session,Config: Hook initializes
Session->>Hook: provide session.locale
Config->>Hook: provide defaultLocale & localization.enabled
Hook->>Store: read value
alt Store is legacy array
Hook->>Hook: migrateSearchHistory(array) -> Record
Hook->>Store: write migrated Record
end
Session->>Hook: addToSearchHistory(term)
Hook->>Store: read current Record
Hook->>Hook: update Record[currentLocale] (dedupe, limit)
Hook->>Store: write updated Record
Session->>Hook: clearSearchHistory()
Hook->>Store: read current Record
Hook->>Hook: remove currentLocale entry
Hook->>Store: write updated Record (or empty object)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
lemagnetic
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/test/sdk/search/useSearchHistory.test.ts (1)
212-214: PreferwaitForover microtask flushing for migration assertion.Lines 212-214 rely on
Promise.resolve()timing. UsingwaitForon store shape is less brittle to scheduling changes.🔧 Suggested patch
-import { act, renderHook } from '@testing-library/react' +import { act, renderHook, waitFor } from '@testing-library/react' @@ - await act(async () => { - await Promise.resolve() - }) + await waitFor(() => { + expect(Array.isArray(searchHistoryStore.read())).toBe(false) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/sdk/search/useSearchHistory.test.ts` around lines 212 - 214, The test currently flushes microtasks via act(async () => { await Promise.resolve() }) which is brittle; replace that with testing-library's waitFor to assert the expected store shape/state instead. In the useSearchHistory test, remove the Promise.resolve() flush and wrap the assertion that inspects the store (or the hook's returned state) inside await waitFor(() => { /* assert store shape or expected values from useSearchHistory */ }); this will wait for the state to settle reliably and avoid timing flakiness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/sdk/search/useSearchHistory.ts`:
- Around line 53-55: The current construction of newHistoryArray in
useSearchHistory adds newHistory then truncates with slice(0, maxHistorySize)
before deduplication, which can drop newer unique items; change the operation
order so you first build the full array ([newHistory, ...current]), run the
dedupe filter using the existing Set (set) to remove duplicates (.filter((item)
=> !set.has(item.term) && set.add(item.term))), and only then apply .slice(0,
maxHistorySize) to truncate; update the code around newHistoryArray in
useSearchHistory to perform filter then slice so maxHistorySize reflects unique
entries.
- Around line 24-30: migrateSearchHistory currently casts any plain object to
SearchHistoryByLocale without checking structure; update it to validate runtime
shape: in the object branch (migrateSearchHistory) iterate Object.entries(value)
and only accept keys whose values are arrays where every element is a valid
History (e.g., typeof item === 'object' && item !== null and has required
properties like 'query' and any other expected fields), building a new
SearchHistoryByLocale from only those valid entries; in the Array branch
validate each element before returning { [DEFAULT_LOCALE]: validatedArray } and
if validation fails return an empty object or { [DEFAULT_LOCALE]: [] } as a safe
fallback; stop using a blind type assertion and return the constructed validated
object instead.
---
Nitpick comments:
In `@packages/core/test/sdk/search/useSearchHistory.test.ts`:
- Around line 212-214: The test currently flushes microtasks via act(async () =>
{ await Promise.resolve() }) which is brittle; replace that with
testing-library's waitFor to assert the expected store shape/state instead. In
the useSearchHistory test, remove the Promise.resolve() flush and wrap the
assertion that inspects the store (or the hook's returned state) inside await
waitFor(() => { /* assert store shape or expected values from useSearchHistory
*/ }); this will wait for the state to settle reliably and avoid timing
flakiness.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/sdk/search/useSearchHistory.tspackages/core/test/sdk/search/useSearchHistory.test.ts
Comment on lines
+24
to
+30
| function migrateSearchHistory(value: unknown): SearchHistoryByLocale { | ||
| if (value && typeof value === 'object' && !Array.isArray(value)) { | ||
| return value as SearchHistoryByLocale | ||
| } | ||
| if (Array.isArray(value) && value.length > 0) { | ||
| return { [DEFAULT_LOCALE]: value as History[] } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "useSearchHistory.ts" -type fRepository: vtex/faststore
Length of output: 108
🏁 Script executed:
wc -l packages/core/src/sdk/search/useSearchHistory.tsRepository: vtex/faststore
Length of output: 109
🏁 Script executed:
cat -n packages/core/src/sdk/search/useSearchHistory.tsRepository: vtex/faststore
Length of output: 2548
Add validation to migration function for runtime safety.
Line 26 casts any object directly to SearchHistoryByLocale without checking structure. If IndexedDB persists malformed data (e.g., { "en-US": "not-an-array" }), code relying on locale values as arrays could fail. Validate each history entry before returning from migration.
🔧 Suggested patch
+function isHistory(value: unknown): value is History {
+ return (
+ typeof value === 'object' &&
+ value !== null &&
+ typeof (value as { term?: unknown }).term === 'string' &&
+ typeof (value as { path?: unknown }).path === 'string'
+ )
+}
+
+function normalizeHistory(value: unknown): History[] {
+ return Array.isArray(value) ? value.filter(isHistory) : []
+}
+
function migrateSearchHistory(value: unknown): SearchHistoryByLocale {
- if (value && typeof value === 'object' && !Array.isArray(value)) {
- return value as SearchHistoryByLocale
- }
- if (Array.isArray(value) && value.length > 0) {
- return { [DEFAULT_LOCALE]: value as History[] }
+ if (Array.isArray(value)) {
+ const normalized = normalizeHistory(value)
+ return normalized.length > 0 ? { [DEFAULT_LOCALE]: normalized } : {}
}
- return {}
+ if (!value || typeof value !== 'object') return {}
+
+ return Object.entries(value as Record<string, unknown>).reduce(
+ (acc, [locale, history]) => {
+ const normalized = normalizeHistory(history)
+ if (normalized.length > 0) acc[locale] = normalized
+ return acc
+ },
+ {} as SearchHistoryByLocale
+ )
}Per coding guidelines: "Ensure type safety and avoid type assertions when possible."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/sdk/search/useSearchHistory.ts` around lines 24 - 30,
migrateSearchHistory currently casts any plain object to SearchHistoryByLocale
without checking structure; update it to validate runtime shape: in the object
branch (migrateSearchHistory) iterate Object.entries(value) and only accept keys
whose values are arrays where every element is a valid History (e.g., typeof
item === 'object' && item !== null and has required properties like 'query' and
any other expected fields), building a new SearchHistoryByLocale from only those
valid entries; in the Array branch validate each element before returning {
[DEFAULT_LOCALE]: validatedArray } and if validation fails return an empty
object or { [DEFAULT_LOCALE]: [] } as a safe fallback; stop using a blind type
assertion and return the constructed validated object instead.
Contributor
Author
There was a problem hiding this comment.
We're not adding runtime validation here on purpose.
- We control all writes: The only code that persists to
fs::searchHistoryis in this hook (addToSearchHistory,clearSearchHistory, and the migration itself). Every write uses the typedHistory/SearchHistoryByLocaleshape, and TypeScript enforces that at compile time. - Malformed data would only occur if someone changed the value manually (e.g. in DevTools → Application → IndexedDB) or due to external corruption. We don't consider that a supported scenario worth extra runtime checks for this internal store.
So we're leaving the migration as-is.
There was a problem hiding this comment.
@lemagnetic, understood. That's a reasonable tradeoff—since all persistence paths go through typed functions in this hook, TypeScript guards the shape at compile time. Leaving it as-is makes sense for an internal store where manual corruption isn't a supported scenario.
✏️ Learnings added
Learnt from: lemagnetic
Repo: vtex/faststore PR: 3231
File: packages/core/src/sdk/search/useSearchHistory.ts:24-30
Timestamp: 2026-02-26T14:03:22.212Z
Learning: In FastStore's SDK hooks (e.g., useSearchHistory), runtime validation of persisted store data is intentionally omitted when all writes are controlled by the same typed hook. TypeScript enforces shape at compile time, and manual corruption via DevTools/IndexedDB is not considered a supported scenario worth extra runtime checks.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
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
1 participant
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.
What's the purpose of this pull request?
Stores with multiple locales shared one search history. This PR stores history per locale so each language has its own list.
How it works?
fs::searchHistory, but value is nowRecord<locale, History[]>(e.g.{ "en-US": [...], "pt-BR": [...] }).useSearchHistoryusesuseSession().localeto read/write only the current locale. Public API unchanged.{ defaultLocale: [...] }).How to test it?
pnpm dev).fs::searchHistory→ object with one key per locale.Starters Deploy Preview
https://brandless-cma5xay4001f6dn4xjwato8b4-mw8252v1z-vtex-fs.vercel.app/it-IT
https://brandless-cma5xay4001f6dn4xjwato8b4-mw8252v1z-vtex-fs.vercel.app/pt-BR
References
Summary by CodeRabbit
New Features
Behavior Improvements
Tests