-
Notifications
You must be signed in to change notification settings - Fork 52
fix: secret editor performance #689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…or performance Signed-off-by: rohan <[email protected]>
…rmance Signed-off-by: rohan <[email protected]>
…ement Signed-off-by: rohan <[email protected]>
Signed-off-by: rohan <[email protected]>
Signed-off-by: rohan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR aims to improve performance of the secret editor UI by implementing React optimization patterns including memo, useMemo, and useCallback to reduce unnecessary re-renders and recomputations. The changes also include code organization improvements by extracting components into separate files.
Key Changes
- Wrapped components in
React.memowith custom comparison functions to prevent unnecessary re-renders - Memoized expensive computations (filtering, sorting, map creation) using
useMemo - Wrapped callbacks in
useCallbackto maintain referential stability - Extracted inline components (
AppFolderRow,EnvFolder) into separate files for better code organization - Refactored state update logic to use functional setState patterns for better performance
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| SecretRow.tsx | Added memo wrapper with comparison function; imported memo from React |
| SecretFolderRow.tsx | Wrapped component in memo with field-based comparison logic |
| page.tsx (environment path) | Added useMemo/useCallback for performance; refactored unsavedChanges calculation and state handlers |
| AppSecrets.tsx | Memoized callbacks and filters; refactored complex state update logic; removed inline component definitions |
| AppSecretRow.tsx | Added memo wrapper; changed staged delete logic to use per-secret flags instead of prop list |
| EnvFolder.tsx | New file extracting EnvFolder component with memoization |
| AppFolderRow.tsx | New file extracting AppFolderRow component with memoization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frontend/app/[team]/apps/[app]/environments/[environment]/[[...path]]/page.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Signed-off-by: rohan <[email protected]>
Signed-off-by: rohan <[email protected]>
Signed-off-by: rohan <[email protected]>
|
@cursor review |
| if ((p?.stagedForDelete ?? false) !== (n?.stagedForDelete ?? false)) return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Memo comparison missing callback function checks
The areAppSecretRowEqual comparison function doesn't verify that callback props like updateKey, updateValue, addEnvValue, deleteEnvValue, and deleteKey remain unchanged. Since addEnvValue, deleteEnvValue, and handleStageClientSecretForDelete aren't wrapped in useCallback in AppSecrets.tsx, they get new references on every render. The memo will prevent re-renders even when these callbacks change, causing the component to use stale function closures that may reference outdated state, leading to incorrect behavior when users interact with secrets.
🔍 Overview
The secret editor UI has various performance issues that cause slow downs in user interaction including input field latency. This PR aims to significantly improve performance and latency when interacting with various elements on the single and cross-env editors
💡 Proposed Changes
useMemoto prevent re-renders and re-computes when not necssaryuseCallback🖼️ Screenshots or Demo
Include before and after screenshots or animated GIFs/demo links to illustrate the changes visually. This is especially useful for UI/UX improvements.
🎯 Reviewer Focus
Verify that functionality of the single and cross-env editor screens are intact, including:
Verify that performance is improved and does not noticable interfere with user inputs
💚 Did You...
- [ ] Update dependencies and lockfiles (if required)- [ ] Update migrations (if required)- [ ] Regenerate graphql schema and types (if required)Note
Optimize single- and cross-environment secret editors via memoization, component extraction, and streamlined delete/update handling; includes version bump.
AppSecrets,AppSecretRow,EnvSecret):useMemo/useCallback); add stable env sorting.memowith custom comparators) to cut re-renders.stagedForDeleteflags, key-level toggling, and updatedunsavedChangeschecks.AppFolderRowandEnvFolder(memoized); passpathnamefor links.page.tsx):canonicalSecretaccessor.useCallback; simplifystageSecretForDelete.noSecrets; rename prop tocanonicalSecret.SecretRow: memoized; usecanonicalSecretfor permissions and diffing; minor UI tweaks.SecretFolderRow: memoized to avoid unnecessary re-renders.v2.54.2.Written by Cursor Bugbot for commit c765190. This will update automatically on new commits. Configure here.