fix: Storing view on server creates view key with hashed view name instead of UUID #2995
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
|
Warning Rate limit exceeded@mtrezza has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPreserves and propagates persistent view IDs: EditViewDialog stores Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Views as Views.react
participant VPM as ViewPreferencesManager
participant Storage as Storage/API
User->>Views: Create view (confirm)
Views->>VPM: generateViewId()
VPM-->>Views: id (UUID)
Views->>Views: newView = { id, ...userFields }
Views->>Storage: Save newView
Storage-->>Views: Ack / Error
Views-->>User: Result
note right of Views: ID assigned before save
sequenceDiagram
autonumber
actor User
participant EditDlg as EditViewDialog
participant Parent as Parent (caller)
User->>EditDlg: Open with view { id, ... }
EditDlg->>EditDlg: state.id = view.id
User->>EditDlg: Confirm edits
EditDlg-->>Parent: onConfirm({ id, name, className, query/cloudFunction, ... })
note right of EditDlg: id propagated in confirm payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/ViewPreferencesManager.js (1)
186-198: Critical: Inconsistent ID generation for views without IDs.When a view lacks an
id, this code generates a UUID on line 186 (for deletion comparison) and again on line 198 (for saving). These will be different UUIDs for the same view object, causing:
- Incorrect deletion detection: The deletion logic (line 187) won't recognize the view because
newViewIds[i]differs from the ID actually saved on line 215.- Orphaned server configs: Old view entries won't be cleaned up, accumulating stale data.
- Broken identity: Violates the UUID-based identity system introduced by this PR.
Root cause: Views should arrive with IDs already assigned. The
|| this._generateViewId()fallback masks upstream bugs where views lack IDs.Apply this diff to fail fast when views lack IDs, forcing upstream code to assign them:
async _saveViewsToServer(appId, views) { try { // First, get existing views from server to know which ones to delete const existingViewConfigs = await this.serverStorage.getConfigsByPrefix('views.view.', appId); const existingViewIds = Object.keys(existingViewConfigs).map(key => key.replace('views.view.', '') ); // Delete views that are no longer in the new views array - const newViewIds = views.map(view => view.id || this._generateViewId()); + const newViewIds = views.map(view => { + if (!view.id) { + throw new Error('View is missing required id field. All views must have an id before saving.'); + } + return view.id; + }); const viewsToDelete = existingViewIds.filter(id => !newViewIds.includes(id)); await Promise.all( viewsToDelete.map(id => this.serverStorage.deleteConfig(`views.view.${id}`, appId) ) ); // Save or update current views await Promise.all( views.map(view => { - const viewId = view.id || this._generateViewId(); + if (!view.id) { + throw new Error('View is missing required id field. All views must have an id before saving.'); + } + const viewId = view.id; const viewConfig = { ...view }; delete viewConfig.id; // Don't store ID in the config itself
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/dashboard/Data/Views/EditViewDialog.react.js(2 hunks)src/dashboard/Data/Views/Views.react.js(1 hunks)src/lib/ViewPreferencesManager.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/ViewPreferencesManager.js (1)
Parse-Dashboard/CLI/mfa.js (1)
crypto(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (4)
src/lib/ViewPreferencesManager.js (1)
265-271: LGTM! Public API for ID generation.The new
generateViewId()method provides a clean public API for generating view IDs, properly delegating to the internal implementation.src/dashboard/Data/Views/Views.react.js (1)
722-728: LGTM! Correct ID assignment for new views.The code correctly generates a unique ID for new views before saving them to state, using the
ViewPreferencesManager.generateViewId()when available with a fallback tocrypto.randomUUID(). This ensures views have IDs before being persisted.Note: The
crypto.randomUUID()fallback has the same browser compatibility concern mentioned in ViewPreferencesManager.js (requires Chrome 92+, Firefox 95+, Safari 15.4+).src/dashboard/Data/Views/EditViewDialog.react.js (2)
33-33: LGTM! Preserves view identity in state.Correctly initializes
state.idfromview.idto preserve the view's identity during editing.
77-77: LGTM! Propagates view ID in confirmation payload.Including
idin theonConfirmpayload ensures the view's identity is preserved when the edit is saved, maintaining consistency with the UUID-based ID system.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/dashboard/Data/Views/Views.react.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Node 18
- GitHub Check: Docker linux/amd64
🔇 Additional comments (1)
src/dashboard/Data/Views/Views.react.js (1)
722-728: LGTM! UUID generation for new views implemented correctly.The implementation properly wraps the user-provided view with a UUID-generated
idbefore storing it in state, aligning with the PR objective to prevent view key collisions from hashed names.
# [7.6.0-alpha.6](7.6.0-alpha.5...7.6.0-alpha.6) (2025-10-05) ### Bug Fixes * Storing view on server creates view key with hashed view name instead of UUID ([#2995](#2995)) ([7cb65f3](7cb65f3))
|
🎉 This change has been released in version 7.6.0-alpha.6 |
New Pull Request Checklist
Issue Description
Storing view on server creates view key with hashed view name instead of UUID. This can create view key collisions.
Approach
Use UUID for view key instead of hashed view name. JS console scripts are already using such a key syntax.
Summary by CodeRabbit