Fix four review issues from PR #59 folder-context-menu feature - #65
Draft
GOODBOY008 with Copilot wants to merge 5 commits into
Draft
Fix four review issues from PR #59 folder-context-menu feature#65GOODBOY008 with Copilot wants to merge 5 commits into
GOODBOY008 with Copilot wants to merge 5 commits into
Conversation
…ion-dialog, and test file
Copilot
AI
changed the title
[WIP] Fix code based on review comments
Fix four review issues from PR #59 folder-context-menu feature
Jul 26, 2026
PR #61: - Add aria-label and aria-expanded to folder chevron button for accessibility - Restore activeConnectionIds memoization to prevent unnecessary re-renders - Fix expand/collapse state preservation using flat map for deep tree support PR #62: - Replace parseInt with Number() + integer floor for number inputs - Reset to protocol-specific default port when field is cleared - Use getDefaultPort(protocol) for port fallback when editing connections
Owner
|
@copilot resolve the merge conflicts in this pull request |
…icts Resolved conflicts in: - src/App.tsx: kept typeof guard for folderPath parameter - src/__tests__/connection-dialog-folder.test.tsx: kept proper ComponentProps import - src/components/connection-dialog.tsx: kept main's displayValues/handleNumberInput pattern from PR #62, re-applied dedicated initialFolder effect fix - src/components/connection-manager.tsx: kept main's mergeExpanded from PR #61, re-applied conditional onNewConnection rendering and aria-label fixes
Contributor
Author
Resolved the merge conflicts with
All 508 tests pass. |
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
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.
PR #59 added a "New Connection" folder context menu entry that pre-selects the clicked folder in the connection dialog. The code review identified four correctness issues that this PR addresses.
Fixes
connection-manager.tsx— "New Connection" menu item was always rendered even whenonNewConnectionis absent, producing a silent no-op click. Now conditionally rendered only when the prop is provided. Also updated prop type to(folderPath?: string) => void.App.tsx—handleNewTab(folderPath?)was passed directly as a Radix/DOMonClickhandler in some callsites, meaning it would receive aMouseEventand store it as a folder path. Added atypeof folderPath === 'string'guard:connection-dialog.tsx— Folder pre-selection was inlined inside the mainopen/editingConnectioneffect andinitialFolderwas added to its dep array, causing the full reset/reload to re-run on everyinitialFolderchange (wiping in-progress edits). Moved pre-selection into a dedicated effect with its own dep array:connection-dialog-folder.test.tsx— Test usedReact.ComponentPropswithout importing theReactnamespace. Switched toimport { type ComponentProps } from 'react'.