fix: enhance connection storage handling and improve UI accessibility#1616
fix: enhance connection storage handling and improve UI accessibility#1616shahar-biron merged 2 commits intostagingfrom
Conversation
Completed Working on "Code Review"✅ Review publishing complete: comments posted from all chunks and final review submitted (COMMENT). ✅ Workflow completed successfully. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the updates. Consolidated review summary based on posted findings:
- Total by importance: 4 MAJOR, 1 MINOR (0 BLOCKER, 0 CRITICAL)
- Affected files:
app/providers.tsx,app/graph/Chat.tsx,app/graph/graphInfo.tsx,package.json
Key themes
- Connection-scoped persistence lifecycle risks: initialization/teardown sequencing can skip valid data load or leave scoped data behind across sessions.
- State/input hardening gaps: numeric settings restoration from storage lacks robust validation for some fields.
- Behavior/performance safety: repeated in-render sorting and dependency changes (ECharts downgrade) need guardrails to avoid regressions.
Recommended next steps
- Ensure chat history loading re-runs when connection prefix becomes ready (not only on graph-name changes).
- Harden all restored numeric UI settings with finite/range validation + defaults.
- Expand logout cleanup to remove all relevant connection-scoped keys before clearing prefix.
- Memoize or precompute property key sorting to avoid repeated render cost on large datasets.
- Add CI/runtime chart smoke coverage (or equivalent verification) for the ECharts 6→5 downgrade and document constraints.
|
|
||
| // Load messages and cypher only preference for current graph on mount | ||
| useEffect(() => { | ||
| if (!getConnectionPrefix()) return; |
There was a problem hiding this comment.
[major]: The new early return (if (!getConnectionPrefix()) return;) avoids unscoped reads, but it can also permanently skip chat-history initialization when this effect runs before the connection prefix is set. Since the effect appears to run only on graph-name changes, users may miss valid saved history for the active graph after login/session hydration.
Suggested fix: Trigger history loading when prefix readiness changes (e.g., include a provider prefixReady signal in effect dependencies) or defer loading until prefix is initialized instead of returning without a re-run path.
| setRowHeight(parseInt(localStorage.getItem("rowHeight") || "40", 10)); | ||
| setRowHeightExpandMultiple(parseInt(localStorage.getItem("rowHeightExpandMultiple") || "3", 10)); | ||
| setMaxItemsForSearch(parseInt(localStorage.getItem("maxItemsForSearch") || "20", 10)); | ||
| const parsedMaxItems = parseInt(localStorage.getItem("maxItemsForSearch") || "20", 10); |
There was a problem hiding this comment.
[major]: UI settings loaded from localStorage are parsed with parseInt and assigned directly for column/row sizing, so invalid persisted values (e.g. non-numeric strings) produce NaN and can break initial slider/value rendering before user interaction. This regresses initial UX stability for users with corrupted or legacy storage.
Suggested fix: Apply the same finite-and-range guard used for maxItemsForSearch to columnWidth, rowHeight, and rowHeightExpandMultiple (fallback to defaults when parse fails).
| } else { | ||
| // Clean up scoped data before clearing the prefix, | ||
| // so removeConnectionItem can still resolve the scoped key. | ||
| removeConnectionItem("savedContent"); |
There was a problem hiding this comment.
[major]: On sign-out, only scoped savedContent is removed before clearing the prefix. Connection-scoped graph keys like chat-* and cypherOnly-* are not cleared, so old per-connection data can persist across sessions on shared browsers.
Suggested fix: During logout, iterate localStorage keys with the active connection prefix and remove all scoped app keys (or at least include query history, chat-*, and cypherOnly-*) before calling `clearConnectionPrefix().
| <ul className="flex flex-wrap gap-2 p-2 overflow-auto"> | ||
| { | ||
| PropertyKeys && PropertyKeys.filter(key => key.toLowerCase().includes(propertyKeysSearch.toLowerCase())).map((key) => ( | ||
| PropertyKeys && PropertyKeys.filter(key => key.toLowerCase().includes(propertyKeysSearch.toLowerCase())).sort((a, b) => a.localeCompare(b)).map((key) => ( |
There was a problem hiding this comment.
[minor]: Sorting PropertyKeys inside render adds O(n log n) work on every re-render, which can noticeably slow the panel for large metadata sets even when the underlying keys are unchanged.
Suggested fix: Memoize the filtered+sorted list with useMemo keyed by PropertyKeys and propertyKeysSearch, or pre-sort once when PropertyKeys changes.
| "date-fns": "^4.1.0", | ||
| "depcheck": "^1.4.7", | ||
| "echarts": "^6.0.0", | ||
| "echarts": "^5.6.0", |
There was a problem hiding this comment.
[major]: The dependency was downgraded from ECharts 6 to 5 while echarts-for-react and chart code may rely on v6 APIs/behaviors. This can cause runtime chart failures after deployment if any v6-only options or defaults are in use.
Suggested fix: Add/extend chart smoke tests (or a quick runtime verification in CI) that render representative charts against ECharts 5, and document the downgrade rationale/constraints in the PR or changelog.
WalkthroughThis PR introduces SSR safety guards to connection storage operations, extends migration logic to handle legacy graph-specific keys, updates default values for layout and search settings, gates persistence behind connection readiness checks, and improves form accessibility. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/connection-storage.ts (1)
86-105: Migration loop correctly handles index shifts, but consider null-safe access.The
i--afterremoveItemproperly compensates for array shrinkage. However, line 97 uses a non-null assertion that could theoretically fail if the key is removed between thestartsWithcheck andgetItem.🛡️ Suggested safer alternative
if (key.startsWith(p)) { const scopedKey = prefixed(key); if (localStorage.getItem(scopedKey) === null) { - localStorage.setItem(scopedKey, localStorage.getItem(key)!); + const value = localStorage.getItem(key); + if (value !== null) { + localStorage.setItem(scopedKey, value); + } } localStorage.removeItem(key);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/connection-storage.ts` around lines 86 - 105, The migration loop in connection-storage.ts uses a non-null assertion when calling localStorage.getItem(key) inside the SCOPED_KEY_PREFIXES handling which could fail if the entry disappears between checks; update the logic in that block (the loop referencing localStorage, SCOPED_KEY_PREFIXES and the prefixed(...) helper) to read the value into a local variable first (const value = localStorage.getItem(key)), check value !== null before calling localStorage.setItem(scopedKey, value), and then call localStorage.removeItem(key) and decrement i; this avoids the non-null assertion and prevents double reads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/connection-storage.ts`:
- Around line 86-105: The migration loop in connection-storage.ts uses a
non-null assertion when calling localStorage.getItem(key) inside the
SCOPED_KEY_PREFIXES handling which could fail if the entry disappears between
checks; update the logic in that block (the loop referencing localStorage,
SCOPED_KEY_PREFIXES and the prefixed(...) helper) to read the value into a local
variable first (const value = localStorage.getItem(key)), check value !== null
before calling localStorage.setItem(scopedKey, value), and then call
localStorage.removeItem(key) and decrement i; this avoids the non-null assertion
and prevents double reads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9fb19488-5ee2-4cd9-ba65-3841a65d9686
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
app/graph/Chat.tsxapp/graph/graphInfo.tsxapp/loginVerification.tsxapp/providers.tsxapp/settings/browserSettings.tsxlib/connection-storage.tspackage.json
🔒 Trivy Security Scan Results |
2 participants
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Improvements
Chores