Repository navigation
Fix Unicode normalization in memory viewer search - #542
Conversation
|
@kaushalrog is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe PR improves memory search filtering by applying Unicode normalization (NFKC) and case-insensitive matching to both search queries and memory content. The ChangesUnicode-Normalized Memory Search
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/viewer/index.html (1)
2228-2235: ⚡ Quick winRedundant lowercasing of search term.
The search term is already lowercased on line 2228, then normalized and lowercased again on line 2235 (inside the filter loop, repeated for every item). This is inefficient and redundant.
Consider normalizing and lowercasing the search term once before the filter:
var search = state.memories.search; var normalizedSearch = search ? search.normalize("NFKC").toLowerCase() : '';Then remove the normalization/lowercasing inside the filter loop.
Also note: The file consistently uses
varthroughout, but the new code usesconst(lines 2233-2243). For consistency, consider usingvarto match the existing style.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/viewer/index.html` around lines 2228 - 2235, Move the normalization and lowercasing of the search term out of the items.filter loop: compute a single normalizedSearch once from state.memories.search (e.g., var normalizedSearch = state.memories.search ? state.memories.search.normalize("NFKC").toLowerCase() : '') before creating filtered, then inside the filter callback (the function used to build filtered from items) use that normalizedSearch instead of recomputing; also keep the file's var-style declarations (use var for normalizedSearch) to stay consistent with existing code and remove the redundant toLowerCase() on the already-lowercased state.memories.search.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/viewer/index.html`:
- Around line 2231-2254: The PR claims to fix issue `#514` but the diff in
items.filter (variables normalizedSearch, normalizedTitle, normalizedContent
used to implement Unicode-normalized multilingual search) is unrelated to
running agentmemory persistently; confirm the intended target and either change
the PR title/description and issue reference to the correct multilingual-search
issue (or create one) or update the code to actually address issue `#514`;
specifically, review the search-related changes inside the items.filter callback
(normalizedSearch/normalizedTitle/normalizedContent) and update the PR metadata
so the referenced issue matches these changes, or revert these search changes
and implement the persistent/scalable agentmemory fix required by `#514`.
- Around line 2245-2252: The search filter's condition incorrectly checks
normalizedContent.includes(normalizedContent) which always succeeds and prevents
content matches; update the condition inside the if in the search block so it
tests normalizedContent.includes(normalizedSearch) (i.e., compare
normalizedContent to normalizedSearch), keeping the existing variables (search,
normalizedTitle, normalizedContent, normalizedSearch) and the overall if/return
false logic intact so both title and content are properly searched.
---
Nitpick comments:
In `@src/viewer/index.html`:
- Around line 2228-2235: Move the normalization and lowercasing of the search
term out of the items.filter loop: compute a single normalizedSearch once from
state.memories.search (e.g., var normalizedSearch = state.memories.search ?
state.memories.search.normalize("NFKC").toLowerCase() : '') before creating
filtered, then inside the filter callback (the function used to build filtered
from items) use that normalizedSearch instead of recomputing; also keep the
file's var-style declarations (use var for normalizedSearch) to stay consistent
with existing code and remove the redundant toLowerCase() on the
already-lowercased state.memories.search.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
Thanks @kaushalrog — NFKC on both sides of the search comparison is the right call for full-width CJK input. Merged. |
…uard (#588) Three concerns surfaced when auditing PRs merged since v0.9.21: 1) Graph parser regex from #494 was correct for self-closing tags ONLY when they're the only entity in the document. With a self-closing entity followed by an explicit-close entity, the greedy `[^>]*` ate the trailing `/` of the self-close, the alternation fell through to the explicit-close branch, and `[\s\S]*?` ran ahead to the *next* `</entity>` — merging two entity declarations into one match and silently dropping a node. Switch to lazy `[^>]*?` so the self-closing alternation gets first chance. Test from #494 (which was failing on main but I didn't notice) now passes. 2) #386 shipped `${AGENTMEMORY_URL}` / `${AGENTMEMORY_SECRET}` placeholders in plugin/.mcp.json. Claude Code and Cursor expand those at MCP launch time; some hosts pass the literal string through. A truthy literal `"${AGENTMEMORY_URL}"` defeats the existing `|| DEFAULT_URL` fallback and would have the shim POST to `${AGENTMEMORY_URL}/agentmemory/...` (DNS failure). Add a defensive guard in src/mcp/rest-proxy.ts that strips any value of the form `${...}` so the fallback engages. Mirror in src/mcp/standalone.ts's mode-announce log line. 3) CHANGELOG entries for all PRs landed since v0.9.21 (#321, #325, #386, #454, #494, #526, #542, #560, #561, #562, #564, #567) plus the regex + env-guard hardening here. Validation: - npm test → 98 files, 1091 tests pass - npm pack → 142 files, fresh install clean, iii-sdk@0.11.2 pinned, plugin/ shipped with hooks/scripts/skills/opencode/ - New test file covers 8 placeholder cases (unset, empty, `${VAR}` literal, mismatched braces, real values with $, unclosed `${`, no-braces `$VAR`).
…uard (rohitg00#588) Three concerns surfaced when auditing PRs merged since v0.9.21: 1) Graph parser regex from rohitg00#494 was correct for self-closing tags ONLY when they're the only entity in the document. With a self-closing entity followed by an explicit-close entity, the greedy `[^>]*` ate the trailing `/` of the self-close, the alternation fell through to the explicit-close branch, and `[\s\S]*?` ran ahead to the *next* `</entity>` — merging two entity declarations into one match and silently dropping a node. Switch to lazy `[^>]*?` so the self-closing alternation gets first chance. Test from rohitg00#494 (which was failing on main but I didn't notice) now passes. 2) rohitg00#386 shipped `${AGENTMEMORY_URL}` / `${AGENTMEMORY_SECRET}` placeholders in plugin/.mcp.json. Claude Code and Cursor expand those at MCP launch time; some hosts pass the literal string through. A truthy literal `"${AGENTMEMORY_URL}"` defeats the existing `|| DEFAULT_URL` fallback and would have the shim POST to `${AGENTMEMORY_URL}/agentmemory/...` (DNS failure). Add a defensive guard in src/mcp/rest-proxy.ts that strips any value of the form `${...}` so the fallback engages. Mirror in src/mcp/standalone.ts's mode-announce log line. 3) CHANGELOG entries for all PRs landed since v0.9.21 (rohitg00#321, rohitg00#325, rohitg00#386, rohitg00#454, rohitg00#494, rohitg00#526, rohitg00#542, rohitg00#560, rohitg00#561, rohitg00#562, rohitg00#564, rohitg00#567) plus the regex + env-guard hardening here. Validation: - npm test → 98 files, 1091 tests pass - npm pack → 142 files, fresh install clean, iii-sdk@0.11.2 pinned, plugin/ shipped with hooks/scripts/skills/opencode/ - New test file covers 8 placeholder cases (unset, empty, `${VAR}` literal, mismatched braces, real values with $, unclosed `${`, no-braces `$VAR`).
This PR improves memory viewer search behavior for Chinese and Japanese text by normalizing Unicode strings before matching.
Changes:
Fixes #514
Summary by CodeRabbit
Release Notes