fix: resolve React Error #31 when expanding memories#1889
fix: resolve React Error #31 when expanding memories#1889
Conversation
…#1879) Memory items in what_worked, what_failed, and recommendations arrays could be objects (e.g. {category, recommendation}) instead of strings when stored by Graphiti or AI agents that deviate from the expected string format. Add toSafeString() helper that safely converts any value to a renderable string, with specific handling for the {category, recommendation} object shape that caused the error. Update ParsedSessionInsight types to reflect that these arrays can contain mixed string/object items. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
area/frontend
📝 WalkthroughWalkthroughExpanded MemoryCard to permit mixed string/object entries in session insight fields and discoveries, added a toSafeString helper to stringify items safely, and updated list rendering to use the helper to avoid rendering errors. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Summary of ChangesHello @AndyMik90, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical React rendering error that previously caused the application to crash when users attempted to expand memory details in the Context -> Memories tab. The fix involves enhancing data type handling within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request effectively addresses React Error #31 by introducing a toSafeString helper function that safely converts potential object outputs from AI agents into renderable strings. This prevents React from attempting to render objects directly as children. The implementation covers the primary fields identified as problematic and provides a robust fallback mechanism.
| }; | ||
| } | ||
|
|
||
| function toSafeString(item: string | Record<string, unknown> | unknown): string { |
There was a problem hiding this comment.
The type signature string | Record<string, unknown> | unknown is redundant because unknown already encompasses all types. Since this function is intended to handle items from the parsed session insights, you can simplify the type to unknown or more specifically string | Record<string, unknown> to match the interface definitions.
| function toSafeString(item: string | Record<string, unknown> | unknown): string { | |
| function toSafeString(item: unknown): string { |
| function toSafeString(item: string | Record<string, unknown> | unknown): string { | ||
| if (typeof item === 'string') return item; | ||
| if (item === null || item === undefined) return ''; | ||
| if (typeof item === 'object') { | ||
| const obj = item as Record<string, unknown>; | ||
| // Handle {category, recommendation} object - the known cause of React Error #31 | ||
| if (typeof obj.category === 'string' && typeof obj.recommendation === 'string') { | ||
| return `[${obj.category}] ${obj.recommendation}`; | ||
| } | ||
| // Try other common single-field patterns | ||
| if (typeof obj.recommendation === 'string') return obj.recommendation; | ||
| if (typeof obj.text === 'string') return obj.text; | ||
| if (typeof obj.description === 'string') return obj.description; | ||
| if (typeof obj.message === 'string') return obj.message; | ||
| // Fallback: combine all string values from the object | ||
| const parts = Object.entries(obj) | ||
| .filter(([, v]) => typeof v === 'string') | ||
| .map(([k, v]) => `${k}: ${v}`); | ||
| return parts.length > 0 ? parts.join(', ') : JSON.stringify(item); | ||
| } | ||
| return String(item); | ||
| } |
There was a problem hiding this comment.
While defining toSafeString within this file works for the current fix, this utility function is likely to be useful in other components (such as PRReviewCard.tsx) that might encounter similar AI-generated object structures. Consider moving this function to a shared utility file like apps/frontend/src/renderer/components/context/utils.ts to improve maintainability and allow for reuse across the codebase.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/frontend/src/renderer/components/context/MemoryCard.tsx`:
- Around line 47-68: Update toSafeString to accept a single unknown parameter
(replace the redundant union signature) and add an early guard for arrays using
Array.isArray(item); when an array is detected, return the joined representation
by mapping each element through toSafeString and joining with ", ". Keep the
existing object handling for plain objects (only after ruling out arrays), but
ensure the fallback that combines string values still calls toSafeString for
nested values if needed; preserve current special-case checks for
category/recommendation and common fields. This targets the toSafeString
function so arrays no longer produce index-keyed output and the type signature
is simplified to unknown.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
PR #1889 reviewed: 12 findings (1 structural issues). Verdict: approve.
Findings (4 selected of 12 total)
🔵 [SEC-1] [LOW] Potential XSS via unsanitized AI-generated content rendered in JSX
📁 apps/frontend/src/renderer/components/context/MemoryCard.tsx:49
The toSafeString() function converts arbitrary AI-generated object data into strings that are rendered as React children. While React's JSX escapes string content by default (preventing direct XSS), the function blindly concatenates all string values from unknown objects via Object.entries(obj).filter(...).map(...) and also falls back to JSON.stringify(item). If this rendered content is ever used in a dangerouslySetInnerHTML context or passed to a component that does so, it could become exploitable. Currently the risk is mitigated by React's default escaping, but the pattern of trusting arbitrary object shapes from AI output is worth noting.
Suggested fix:
Ensure no downstream component uses dangerouslySetInnerHTML with these values. Consider adding explicit allowlisting of known safe keys rather than iterating all object entries.
🔵 [SEC-2] [LOW] Unsafe JSON.parse of memory content without validation
📁 apps/frontend/src/renderer/components/context/MemoryCard.tsx:55
The parseMemoryContent function parses arbitrary JSON content and casts it to ParsedSessionInsight without any schema validation. The toSafeString function then handles the resulting unvalidated data. While the type definitions suggest expected shapes, the actual runtime data is fully attacker/AI-controlled. There is no validation that the parsed object conforms to the expected interface before being used throughout the component. This is a defense-in-depth concern — prototype pollution or unexpected property types could cause unexpected behavior.
Suggested fix:
Consider using a runtime schema validation library (e.g., zod) to validate the parsed JSON conforms to ParsedSessionInsight before using it.
🔵 [QLT-5] [LOW] Function defined at module scope but only used internally — consider co-location or export for testing
📁 apps/frontend/src/renderer/components/context/MemoryCard.tsx:43
The toSafeString helper is a pure function with non-trivial branching logic that would benefit from unit tests, but it's a private module-level function that can't be imported in tests. Given it handles the core fix for this bug, it should either be exported or extracted to a utility module with dedicated tests.
Suggested fix:
Extract `toSafeString` to a shared utility file (e.g., `utils/rendering.ts`) and export it, then add unit tests covering string input, `{category, recommendation}` objects, objects with no string values, null/undefined, and primitive numbers.
🔵 [QLT-6] [LOW] Array items keyed by index — potential rendering issues on reorder
📁 apps/frontend/src/renderer/components/context/MemoryCard.tsx:233
All list items use array index as React key (key={idx}, key={\rec-${idx}`}`). While this is pre-existing and not introduced by this PR, it's worth noting since the data comes from AI-generated content that could potentially be reordered or deduplicated.
Suggested fix:
Consider using a content-derived key (e.g., a hash or the string itself) instead of array index for more stable React reconciliation, especially if the list could be re-sorted.
This review was generated by Auto Claude.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
PR #1889 reviewed: 12 findings (1 structural issues). Verdict: approve.
Findings (8 selected of 12 total)
🟡 [QLT-1] [MEDIUM] toSafeString uses overly broad unknown parameter type with type assertion
📁 apps/frontend/src/renderer/components/context/MemoryCard.tsx:43
The function signature toSafeString(item: string | Record<string, unknown> | unknown) is effectively just unknown since unknown absorbs all other types in a union. The subsequent as Record<string, unknown> cast bypasses type safety. The parameter type should match the array element types from the interface, and the cast should be guarded more carefully.
Suggested fix:
Change signature to `toSafeString(item: string | Record<string, unknown>): string` to match the actual array element types defined in the interface. This removes the need for the `unknown` catch-all and makes the type assertion unnecessary since the `typeof item === 'object'` check already narrows to `Record<string, unknown>`.
🟡 [QLT-2] [MEDIUM] Speculative generality in toSafeString — handling fields that may never occur
📁 apps/frontend/src/renderer/components/context/MemoryCard.tsx:54
The function handles obj.text, obj.description, and obj.message field patterns without any evidence these shapes exist in the data. This adds unnecessary complexity and makes the function's behavior harder to reason about. If a future object happens to have a text field alongside other important fields, only text would be shown, potentially hiding information silently.
Suggested fix:
Remove the speculative field checks for `text`, `description`, and `message`. Keep only the documented `{category, recommendation}` pattern and the generic fallback. Add a comment documenting why the known pattern exists (AI agent output format).
🔵 [QLT-3] [LOW] JSON.stringify fallback can produce unreadable output for complex nested objects
📁 apps/frontend/src/renderer/components/context/MemoryCard.tsx:59
When the object has no string values at all, JSON.stringify(item) is used as the ultimate fallback. This could render raw JSON in the UI, which is a poor user experience. Consider logging a warning in this case so developers can identify and add proper handling for new object shapes.
Suggested fix:
Add a `console.warn('MemoryCard: unexpected item shape', item)` before the JSON.stringify fallback so new unhandled shapes are surfaced during development.
🟡 [QLT-4] [MEDIUM] toSafeString not applied to all list-rendering sites in the component
📁 apps/frontend/src/renderer/components/context/MemoryCard.tsx:230
The fix only wraps what_worked, what_failed, recommendations_for_next_session, and discoveries.recommendations with toSafeString. However, other arrays in ParsedSessionInsight (e.g., subtasks_completed, patterns_discovered, changed_files) are also rendered in this component and could potentially contain unexpected object shapes from AI output. The same defensive approach should be applied consistently to all dynamically-rendered list items.
Suggested fix:
Audit all `.map((item, idx) => ...)` render sites in MemoryCard and wrap each item render with `toSafeString()` for consistency, or at minimum the `patterns_discovered` array which already has a union type `Array<{...} | string>` in the interface.
🟡 [DEEP-1] [MEDIUM] Incomplete application of toSafeString — other dynamic fields not protected
📁 apps/frontend/src/renderer/components/context/MemoryCard.tsx:26
The fix applies toSafeString() to what_worked, what_failed, recommendations_for_next_session, and discoveries.recommendations, but the same component renders other AI-generated array fields like subtasks_completed, discoveries.patterns_discovered, and discoveries.file_insights which could also contain unexpected object shapes. If the AI agent can produce objects for the four fixed fields, it can do the same for these others, leading to the same React Error #31 in those sections.
Suggested fix:
Apply `toSafeString()` (or equivalent safe rendering) to all dynamically-rendered list items from parsed AI content, including `subtasks_completed`, `patterns_discovered`, and any other fields rendered via `{item}` in JSX.
🔵 [DEEP-2] [LOW] toSafeString fallback to JSON.stringify can produce unreadable or very long output
📁 apps/frontend/src/renderer/components/context/MemoryCard.tsx:66
When an object has no string-valued fields, toSafeString falls back to JSON.stringify(item). If the AI produces deeply nested or large objects, this will render raw JSON in the UI, which is a poor user experience and could also cause layout issues with very long unbroken strings.
Suggested fix:
Consider truncating the JSON.stringify output (e.g., limit to 200 characters with an ellipsis) or rendering a more user-friendly fallback like '[Complex data]' with a tooltip showing the full content.
🟡 [DEEP-3] [MEDIUM] Type widening without upstream validation creates a silent data contract issue
📁 apps/frontend/src/renderer/components/context/MemoryCard.tsx:73
The interface ParsedSessionInsight was widened from string[] to Array<string | Record<string, unknown>> for several fields, and toSafeString handles the rendering. However, there is no validation at parse time (in parseMemoryContent) that rejects or normalizes unexpected shapes. This means the component silently accepts any structure the AI produces. If new object shapes appear (e.g., arrays of arrays, numbers), toSafeString will handle them via the String() fallback, but the data inconsistency is never logged or reported, making debugging harder.
Suggested fix:
Add a normalization step in `parseMemoryContent` that coerces array items to strings using `toSafeString` at parse time, so the rest of the component can assume `string[]`. Optionally log a warning when unexpected object shapes are encountered to aid debugging.
🔵 [DEEP-4] [LOW] toSafeString does not handle array-typed values
📁 apps/frontend/src/renderer/components/context/MemoryCard.tsx:49
The toSafeString function checks for string, null, undefined, and object (plain objects), but arrays are also typeof === 'object'. If an array is passed, it will be treated as a plain object — Object.entries on an array produces index-based entries like 0: value, which yields output like 0: foo, 1: bar. This is technically not wrong but produces confusing UI text. Since the AI output is unpredictable, an array nested inside one of these fields is plausible.
Suggested fix:
Add an explicit `Array.isArray(item)` check before the object branch, e.g., `if (Array.isArray(item)) return item.map(toSafeString).join(', ');`
This review was generated by Auto Claude.
…enders - Simplify toSafeString signature from redundant union to plain `unknown` - Add explicit Array.isArray guard before the object branch so arrays are joined with ', ' instead of producing '0: val, 1: val' via Object.entries - Verified all existing .map() render sites: what_worked, what_failed, recommendations, and discoveries.recommendations already use toSafeString; subtasks_completed and changed_files are typed as string[] so no change needed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/frontend/src/renderer/components/context/MemoryCard.tsx`:
- Around line 47-69: toSafeString currently maps arrays with an explicit lambda
and returns JSON.stringify(item) for objects with no string fields; change the
array mapping to the point-free form by using item.map(toSafeString) in the
Array.isArray branch, and replace the UX fallback in the object branch (where
parts.length === 0) to return a friendlier sentinel like "[complex value]"
instead of raw JSON.stringify(item) so users don't see raw JSON; the function to
update is toSafeString and adjust the Array.isArray branch and the final object
fallback accordingly.
| function toSafeString(item: unknown): string { | ||
| if (typeof item === 'string') return item; | ||
| if (item === null || item === undefined) return ''; | ||
| if (Array.isArray(item)) return item.map(i => toSafeString(i)).join(', '); | ||
| if (typeof item === 'object') { | ||
| const obj = item as Record<string, unknown>; | ||
| // Handle {category, recommendation} object - the known cause of React Error #31 | ||
| if (typeof obj.category === 'string' && typeof obj.recommendation === 'string') { | ||
| return `[${obj.category}] ${obj.recommendation}`; | ||
| } | ||
| // Try other common single-field patterns | ||
| if (typeof obj.recommendation === 'string') return obj.recommendation; | ||
| if (typeof obj.text === 'string') return obj.text; | ||
| if (typeof obj.description === 'string') return obj.description; | ||
| if (typeof obj.message === 'string') return obj.message; | ||
| // Fallback: combine all string values from the object | ||
| const parts = Object.entries(obj) | ||
| .filter(([, v]) => typeof v === 'string') | ||
| .map(([k, v]) => `${k}: ${v}`); | ||
| return parts.length > 0 ? parts.join(', ') : JSON.stringify(item); | ||
| } | ||
| return String(item); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
toSafeString correctly addresses the root cause; two minor follow-ups.
Both issues raised in the previous review cycle (unknown type signature and Array.isArray guard) are properly addressed. Two remaining points:
- Line 50 — nitpick:
item.map(i => toSafeString(i))is a wrapper that can be replaced with the point-free form.
♻️ Proposed simplification
- if (Array.isArray(item)) return item.map(i => toSafeString(i)).join(', ');
+ if (Array.isArray(item)) return item.map(toSafeString).join(', ');- Line 66 — UX fallback: For objects where all field values are non-string (e.g.,
{ count: 5, nested: {} }),partsis empty andJSON.stringify(item)is returned directly into the UI. This is safe for React, but users may see raw JSON. Consider a friendlier sentinel like'[complex value]'if raw JSON would be confusing in this context.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function toSafeString(item: unknown): string { | |
| if (typeof item === 'string') return item; | |
| if (item === null || item === undefined) return ''; | |
| if (Array.isArray(item)) return item.map(i => toSafeString(i)).join(', '); | |
| if (typeof item === 'object') { | |
| const obj = item as Record<string, unknown>; | |
| // Handle {category, recommendation} object - the known cause of React Error #31 | |
| if (typeof obj.category === 'string' && typeof obj.recommendation === 'string') { | |
| return `[${obj.category}] ${obj.recommendation}`; | |
| } | |
| // Try other common single-field patterns | |
| if (typeof obj.recommendation === 'string') return obj.recommendation; | |
| if (typeof obj.text === 'string') return obj.text; | |
| if (typeof obj.description === 'string') return obj.description; | |
| if (typeof obj.message === 'string') return obj.message; | |
| // Fallback: combine all string values from the object | |
| const parts = Object.entries(obj) | |
| .filter(([, v]) => typeof v === 'string') | |
| .map(([k, v]) => `${k}: ${v}`); | |
| return parts.length > 0 ? parts.join(', ') : JSON.stringify(item); | |
| } | |
| return String(item); | |
| } | |
| function toSafeString(item: unknown): string { | |
| if (typeof item === 'string') return item; | |
| if (item === null || item === undefined) return ''; | |
| if (Array.isArray(item)) return item.map(toSafeString).join(', '); | |
| if (typeof item === 'object') { | |
| const obj = item as Record<string, unknown>; | |
| // Handle {category, recommendation} object - the known cause of React Error `#31` | |
| if (typeof obj.category === 'string' && typeof obj.recommendation === 'string') { | |
| return `[${obj.category}] ${obj.recommendation}`; | |
| } | |
| // Try other common single-field patterns | |
| if (typeof obj.recommendation === 'string') return obj.recommendation; | |
| if (typeof obj.text === 'string') return obj.text; | |
| if (typeof obj.description === 'string') return obj.description; | |
| if (typeof obj.message === 'string') return obj.message; | |
| // Fallback: combine all string values from the object | |
| const parts = Object.entries(obj) | |
| .filter(([, v]) => typeof v === 'string') | |
| .map(([k, v]) => `${k}: ${v}`); | |
| return parts.length > 0 ? parts.join(', ') : JSON.stringify(item); | |
| } | |
| return String(item); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/frontend/src/renderer/components/context/MemoryCard.tsx` around lines 47
- 69, toSafeString currently maps arrays with an explicit lambda and returns
JSON.stringify(item) for objects with no string fields; change the array mapping
to the point-free form by using item.map(toSafeString) in the Array.isArray
branch, and replace the UX fallback in the object branch (where parts.length ===
0) to return a friendlier sentinel like "[complex value]" instead of raw
JSON.stringify(item) so users don't see raw JSON; the function to update is
toSafeString and adjust the Array.isArray branch and the final object fallback
accordingly.
1 participant
Summary
recommendations_for_next_session,what_worked, andwhat_failedas arrays of objects ({category, recommendation}) instead of plain strings.MemoryCard.tsxrendered these directly as{item}in JSX, which React rejects for objectstoSafeString()helper inMemoryCard.tsxthat safely converts any value to a renderable string, with specific handling for{category, recommendation}shaped objects. Applied to all list item rendersFixes #1879
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit