feat(desktop): persist v2 diff-pane expand state, auto-expand clicked file#3885
Merged
feat(desktop): persist v2 diff-pane expand state, auto-expand clicked file#3885
Conversation
…lick Lift `showFullDiff` out of `DiffFileEntry` local state into a persisted `expandedFiles` array on `DiffPaneData`, synced through the existing paneLayout collection. Clicking a file from the v2 changes tab now adds its path to `expandedFiles` (and clears it from `collapsedFiles`), so a large file auto-renders its diff instead of showing the deferred placeholder, and the decision survives navigation/reload.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLifted per-file "expanded" state from individual DiffFileEntry components into the DiffPane container. DiffPane now stores and persists Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Openers as useWorkspacePaneOpeners
participant Pane as DiffPane
participant Entry as DiffFileEntry
User->>Openers: openDiffPane(filePath)
Openers->>Pane: createOrReusePane({... expandedFiles: [filePath] ...})
Pane->>Pane: updateData(expandedFiles)
Pane->>Entry: render(entryProps { expanded, onSetExpanded })
User->>Entry: click "Show diff"
Entry->>Pane: onSetExpanded(filePath, true)
Pane->>Pane: updateData(expandedFiles) / persist
Pane->>Entry: re-render with expanded=true
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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. Review rate limit: 4/8 reviews remaining, refill in 24 minutes and 18 seconds.Comment |
Greptile SummaryThis PR lifts Confidence Score: 5/5Safe to merge; both findings are P2 and do not block the primary user path. No P0/P1 bugs. The unbounded-growth issue is a maintenance concern and the null-guard gap requires very old persisted state to trigger. Core logic correctly mirrors the existing collapsed-files pattern. useWorkspacePaneOpeners.ts (missing null-guard on collapsedFiles); DiffPane.tsx (expandedFiles never pruned).
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.ts | Adds optional expandedFiles?: string[] to DiffPaneData; clean backward-compatible addition. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx | Introduces expandedSet + setExpanded callback mirroring the existing collapsed-files pattern; expandedFiles grows unboundedly as no removal path exists. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileEntry/DiffFileEntry.tsx | Replaces local showFullDiff state with the expanded prop; logic for deferred rendering and shouldMount is unchanged and correct. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspacePaneOpeners/useWorkspacePaneOpeners.ts | Auto-adds clicked file to expandedFiles and removes it from collapsedFiles in all three pane-open paths; prev.collapsedFiles.filter(...) lacks a null-guard for old persisted data. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User clicks file in v2 changes tab] --> B[openDiffPane called]
B --> C{Pane already exists?}
C -- Yes --> D[setPaneData: add filePath to expandedFiles\nremove from collapsedFiles]
C -- No / New Tab --> E[Create pane with expandedFiles: filePath\ncollapsedFiles: empty]
D --> F[DiffPane re-renders]
E --> F
F --> G[expandedSet = new Set of expandedFiles]
G --> H[DiffFileEntry receives expanded=true]
H --> I{deferReason?}
I -- large/deleted + expanded=true --> J[Render WorkspaceDiff immediately]
I -- null --> K[Render via viewport proximity as before]
L[User clicks Show diff button] --> M[handleShowFullDiff → onSetExpanded path true]
M --> N[setExpanded adds path to expandedFiles]
N --> F
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx
Line: 87-98
Comment:
**`expandedFiles` grows without bound**
`setExpanded` is only ever called with `value = true` (from `handleShowFullDiff`), so paths are appended to `expandedFiles` but never removed. Over a long session with many file navigations, the persisted array accumulates every file the user has ever clicked or expanded. Unlike `collapsedFiles`, which shrinks when a file is uncollapsed, `expandedFiles` has no equivalent trim path. The `Array.includes` check in `setExpanded` is also O(n) on each call.
Consider pruning stale entries when the changeset changes (e.g., keep only paths that are still in the current `files` list), or simply mirror the `collapsedFiles` pattern and allow callers to set `value = false` to remove entries.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspacePaneOpeners/useWorkspacePaneOpeners.ts
Line: 52
Comment:
**Potential null-dereference on `collapsedFiles`**
`prev.collapsedFiles.filter(...)` will throw if an existing persisted pane loaded from an older snapshot omits the `collapsedFiles` field. The `DiffPane` update helpers already guard against this with `current.collapsedFiles ?? []`, and the same defensive pattern should be applied here.
```suggestion
collapsedFiles: (prev.collapsedFiles ?? []).filter((p) => p !== filePath),
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(desktop): persist large-diff expand..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+87
to
+98
| const setExpanded = useCallback( | ||
| (path: string, value: boolean) => { | ||
| const current = dataRef.current; | ||
| const expanded = current.expandedFiles ?? []; | ||
| const has = expanded.includes(path); | ||
| if (value === has) return; | ||
| const next = value | ||
| ? [...expanded, path] | ||
| : expanded.filter((p) => p !== path); | ||
| updateData({ ...current, expandedFiles: next } as PaneViewerData); | ||
| }, | ||
| [updateData], |
There was a problem hiding this comment.
expandedFiles grows without bound
setExpanded is only ever called with value = true (from handleShowFullDiff), so paths are appended to expandedFiles but never removed. Over a long session with many file navigations, the persisted array accumulates every file the user has ever clicked or expanded. Unlike collapsedFiles, which shrinks when a file is uncollapsed, expandedFiles has no equivalent trim path. The Array.includes check in setExpanded is also O(n) on each call.
Consider pruning stale entries when the changeset changes (e.g., keep only paths that are still in the current files list), or simply mirror the collapsedFiles pattern and allow callers to set value = false to remove entries.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx
Line: 87-98
Comment:
**`expandedFiles` grows without bound**
`setExpanded` is only ever called with `value = true` (from `handleShowFullDiff`), so paths are appended to `expandedFiles` but never removed. Over a long session with many file navigations, the persisted array accumulates every file the user has ever clicked or expanded. Unlike `collapsedFiles`, which shrinks when a file is uncollapsed, `expandedFiles` has no equivalent trim path. The `Array.includes` check in `setExpanded` is also O(n) on each call.
Consider pruning stale entries when the changeset changes (e.g., keep only paths that are still in the current `files` list), or simply mirror the `collapsedFiles` pattern and allow callers to set `value = false` to remove entries.
How can I resolve this? If you propose a fix, please make it concise.| data: { | ||
| ...prev, | ||
| path: filePath, | ||
| collapsedFiles: prev.collapsedFiles.filter((p) => p !== filePath), |
There was a problem hiding this comment.
collapsedFiles
prev.collapsedFiles.filter(...) will throw if an existing persisted pane loaded from an older snapshot omits the collapsedFiles field. The DiffPane update helpers already guard against this with current.collapsedFiles ?? [], and the same defensive pattern should be applied here.
Suggested change
| collapsedFiles: prev.collapsedFiles.filter((p) => p !== filePath), | |
| collapsedFiles: (prev.collapsedFiles ?? []).filter((p) => p !== filePath), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspacePaneOpeners/useWorkspacePaneOpeners.ts
Line: 52
Comment:
**Potential null-dereference on `collapsedFiles`**
`prev.collapsedFiles.filter(...)` will throw if an existing persisted pane loaded from an older snapshot omits the `collapsedFiles` field. The `DiffPane` update helpers already guard against this with `current.collapsedFiles ?? []`, and the same defensive pattern should be applied here.
```suggestion
collapsedFiles: (prev.collapsedFiles ?? []).filter((p) => p !== filePath),
```
How can I resolve this? If you propose a fix, please make it concise.Older persisted DiffPane snapshots may not have collapsedFiles set; the existing setCollapsed helper already uses `?? []` for the same reason. Match the defensive read here so reusing an existing diff pane doesn't TypeError on undefined.filter.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
showFullDiffout ofDiffFileEntrylocal state into a persistedexpandedFilesarray onDiffPaneData, synced through the existingpaneLayoutcollection — "Show diff" decisions on large files now survive navigation and reload.expandedFiles(and clears it fromcollapsedFiles), so a large file auto-renders its diff in the changes pane instead of showing the deferred placeholder.Test plan
Summary by cubic
Persisted expanded state for large diffs in the v2 diff pane and auto-expand the clicked file from the changes tab. Large files now render immediately, and your expand choice survives navigation and reload; also fixes a crash with legacy panes.
New Features
DiffPaneData.expandedFiles(persisted viapaneLayout).expandedFilesand removes it fromcollapsedFiles; new tabs open with that file expanded.Bug Fixes
collapsedFilesreads in the diff pane opener to handle legacy panes without this field and avoid undefined.filter errors.Written for commit 7b7e194. Summary will update on new commits. Review in cubic
Summary by CodeRabbit