fix(task-input): preserve picked folder when adding to recents#2277
Open
richardsolomou wants to merge 2 commits into
Open
fix(task-input): preserve picked folder when adding to recents#2277richardsolomou wants to merge 2 commits into
richardsolomou wants to merge 2 commits into
Conversation
The useEffect that syncs selectedDirectory from view.folderId was re-running whenever the folders list changed. Picking a new folder via "Open folder..." triggers addFolder, which invalidates getFolders, which refetches folders, which fires this effect and reverts selectedDirectory back to the original view.folderId folder. Track the last applied folderId in a ref so the effect only syncs when view.folderId actually changes, not on every folders refetch. Generated-By: PostHog Code Task-Id: 0dcecc53-ea9a-4a39-8a6e-63973bea25bb
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/code/src/renderer/features/task-detail/components/TaskInput.tsx:404-416
**No regression test for the fixed behaviour**
The bug being fixed (folder selection reverted after `addFolder` triggers a `folders` refetch) is subtle enough that it could be reintroduced silently. There are currently no test files for `TaskInput`, so a dedicated unit/integration test — e.g. mounting the component with a `view.folderId`, simulating a `folders` list change after `addFolder`, and asserting that `selectedDirectory` retains the user-picked path — would pin the contract and prevent future regressions.
Reviews (1): Last reviewed commit: "fix(task-input): preserve picked folder ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
404
to
416
| const lastInitializedFolderIdRef = useRef<string | undefined>(undefined); | ||
| useEffect(() => { | ||
| if (view.folderId) { | ||
| const folder = folders.find((f) => f.id === view.folderId); | ||
| if (folder) { | ||
| setSelectedDirectory(folder.path); | ||
| } | ||
| if (!view.folderId) { | ||
| lastInitializedFolderIdRef.current = undefined; | ||
| return; | ||
| } | ||
| if (lastInitializedFolderIdRef.current === view.folderId) return; | ||
| const folder = folders.find((f) => f.id === view.folderId); | ||
| if (folder) { | ||
| setSelectedDirectory(folder.path); | ||
| lastInitializedFolderIdRef.current = view.folderId; | ||
| } | ||
| }, [view.folderId, folders]); |
Contributor
There was a problem hiding this comment.
The bug being fixed (folder selection reverted after addFolder triggers a folders refetch) is subtle enough that it could be reintroduced silently. There are currently no test files for TaskInput, so a dedicated unit/integration test — e.g. mounting the component with a view.folderId, simulating a folders list change after addFolder, and asserting that selectedDirectory retains the user-picked path — would pin the contract and prevent future regressions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/task-detail/components/TaskInput.tsx
Line: 404-416
Comment:
**No regression test for the fixed behaviour**
The bug being fixed (folder selection reverted after `addFolder` triggers a `folders` refetch) is subtle enough that it could be reintroduced silently. There are currently no test files for `TaskInput`, so a dedicated unit/integration test — e.g. mounting the component with a `view.folderId`, simulating a `folders` list change after `addFolder`, and asserting that `selectedDirectory` retains the user-picked path — would pin the contract and prevent future regressions.
How can I resolve this? If you propose a fix, please make it concise.Extract the folder-id-to-directory sync into useInitialDirectoryFromFolderId so the contract can be tested in isolation: the hook syncs once when the folder list resolves, ignores later folders refetches (the case that broke the picker), and re-syncs only when folderId changes. Generated-By: PostHog Code Task-Id: 0dcecc53-ea9a-4a39-8a6e-63973bea25bb
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.
Problem
When creating a new task and clicking Open folder... in local/worktree mode, the selected folder is added to the Recent list but isn't actually selected as the task's folder.
Changes
The
useEffectinTaskInput.tsxthat syncsselectedDirectoryfromview.folderIdhadfoldersin its dependency array and re-ran every timefolderschanged. Picking a new folder via the file dialog triggersaddFolder, which invalidatesgetFolders, which refetches and updatesfolders— causing the effect to fire and revertselectedDirectoryback to the originalview.folderIdfolder, overwriting the user's pick.Track the last applied
folderIdin a ref so the effect only syncs whenview.folderIdactually changes, not on every folders refetch. The dependency onfoldersis preserved so the initial sync still works when the folder list hasn't loaded yet on mount.How did you test this?
pnpm --filter code typecheck) passes.pnpm build) succeeds.pnpm --filter code test— all TaskInput-related tests pass; the 2 failing tests are pre-existing archive service integration timeouts unrelated to this change.Publish to changelog?
no
Created with PostHog Code