feat: add repository URL copying modal with interactive selection#24
Conversation
- Add interactive CopyUrlModal component with SSH/HTTPS selection - Implement keyboard navigation with up/down arrows and Enter to copy - Add visual selection indicators and dynamic hints - Support multiple copy methods (Enter, S for SSH, H for HTTPS) - Add cross-platform clipboard functionality with fallbacks - Include comprehensive test coverage (15 test cases) - Update README with new feature documentation Users can now press 'C' to open an interactive modal that displays both SSH and HTTPS clone URLs, with keyboard navigation to select and copy the desired URL format to clipboard. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@wiiiimm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 11 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)
WalkthroughAdds an interactive "Copy URL" feature: a new Ink modal component, RepoList keyboard shortcut/handlers and toast feedback, a clipboard utility with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant RL as RepoList
participant M as CopyUrlModal
participant UT as copyToClipboard
participant CB as clipboard (clipboardy / OS)
U->>RL: Press C
RL->>M: Open modal (repo, handlers)
rect rgba(240,248,255,0.9)
note right of M: Default = SSH\nNavigate with ↑/↓, ←/→, or press S/H\nEnter/Y to copy
U->>M: ↑/↓ / ←/→ / S / H / Enter / Y
alt copy action (Enter / S / H / Y)
M->>UT: onCopy(url, type)
UT->>CB: attempt copy (clipboardy → OS fallbacks)
CB-->>UT: success / failure
alt success
UT-->>M: resolved
M->>RL: onClose()
RL->>U: show success toast
else failure
UT-->>M: throw error
M->>M: display copyError
end
else close (Esc / Q / C)
U->>M: Esc/Q/C
M->>RL: onClose()
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Fix undefined input bug in CopyUrlModal keyboard handler - Fix command injection vulnerability in copyToClipboard utility - Replace shell command execution with secure spawn() calls - Guard input parameter before toLowerCase() operations Addresses PR feedback from @chatgpt-codex-connector and @cursor regarding critical P1 security and stability issues. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
package.json (1)
49-49: Dependency addition looks good; engines match clipboardy’s Node 18+ requirement.No issues adding
clipboardy@^4. One note: because you package binaries withpkg, the dynamic import may be excluded at build time and always hit your fallback. If that’s intentional, all good; otherwise add it to pkg’s assets or switch to a static import.README.md (1)
262-266: Keybinding consistency: consider supporting Left/Right as per modal UX guideline.Guidelines specify Left/Right to move focus; this modal uses Up/Down (documented here). Either add Left/Right as aliases or call out the deviation in the guidelines for this modal.
src/utils.ts (1)
62-66: Preserve error cause and give actionable guidance.Surface the underlying OS error for debugging, and suggest a utility to install (xclip/xsel/wl-clipboard).
- } catch (osError) { - throw new Error(`Failed to copy to clipboard. Please install a clipboard utility for your system.`); + } catch (osError) { + const hint = process.platform === 'linux' + ? 'Install xclip, xsel, or wl-clipboard.' + : process.platform === 'darwin' + ? 'pbcopy should be available on macOS.' + : 'Ensure clip.exe is available on PATH.'; + throw new Error(`Failed to copy to clipboard. ${hint}`, { cause: osError as unknown }); }src/ui/components/modals/CopyUrlModal.tsx (1)
74-116: Follow UI styling guideline: prefer chalk-coloured strings over Inkcolorprops.Pre-colour strings to avoid nested Text issues. Also consider adding
minHeighton the outer Box for consistent terminal spacing.- <Box + <Box flexDirection="column" borderStyle="round" borderColor="blue" paddingX={3} paddingY={2} - width={Math.min(terminalWidth - 8, 80)} + width={Math.min(terminalWidth - 8, 80)} + minHeight={12} > - <Text bold color="blue">Copy Repository URL</Text> + <Text>{chalk.blue.bold('Copy Repository URL')}</Text> @@ - <Text color="gray">SSH URL:</Text> + <Text>{chalk.gray('SSH URL:')}</Text> @@ - <Text color={selectedType === 'SSH' ? 'blue' : undefined}> - {selectedType === 'SSH' ? '▶ ' : ' '}{sshUrl} - </Text> + <Text> + {selectedType === 'SSH' ? chalk.blue(`▶ ${sshUrl}`) : ` ${sshUrl}`} + </Text> @@ - <Text color="gray">HTTPS URL:</Text> + <Text>{chalk.gray('HTTPS URL:')}</Text> @@ - <Text color={selectedType === 'HTTPS' ? 'blue' : undefined}> - {selectedType === 'HTTPS' ? '▶ ' : ' '}{httpsUrl} - </Text> + <Text> + {selectedType === 'HTTPS' ? chalk.blue(`▶ ${httpsUrl}`) : ` ${httpsUrl}`} + </Text> @@ - <Text color="gray"> - ↑↓ Select • Enter to copy {selectedType} • S copy SSH • H copy HTTPS • Esc/Q/C to close - </Text> + <Text> + {chalk.gray(`↑↓/←→ Select • Enter to copy ${selectedType} • S copy SSH • H copy HTTPS • Esc/Q/C to close`)} + </Text>src/ui/RepoList.tsx (2)
2135-2142: Prefer chalk-coloured strings over Ink colour props for the toastMatches local guideline to avoid nested Text colour issues.
- <Box borderStyle="round" borderColor={copyToast.includes('Failed') ? 'red' : 'green'} paddingX={2} paddingY={0}> - <Text color={copyToast.includes('Failed') ? 'red' : 'green'}>{copyToast}</Text> - </Box> + <Box borderStyle="round" borderColor={copyToast.includes('Failed') ? 'red' : 'green'} paddingX={2} paddingY={0}> + <Text>{copyToast.includes('Failed') ? chalk.red(copyToast) : chalk.green(copyToast)}</Text> + </Box>
1030-1034: Align modal navigation with documented Left/Right UXGuideline for src/ui/**/*.tsx modals specifies Left/Right to move focus, Enter to activate, Y to confirm, C/Esc to cancel. CopyUrlModal currently uses Up/Down for selection. Consider adding Left/Right aliases in the modal and updating its footer hint to include ←→.
I can draft the change in CopyUrlModal and extend tests for ←/→ if you’d like.
Also applies to: 1231-1238
tests/ui/CopyUrlModal.test.tsx (1)
81-93: Instruction footer assertions OK; consider future-proofingIf you add ←/→ support per guidelines, extend assertions to include those hints and “cancel” instead of “close”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
README.md(3 hunks)package.json(1 hunks)src/ui/RepoList.tsx(9 hunks)src/ui/components/modals/CopyUrlModal.tsx(1 hunks)src/ui/components/modals/index.ts(1 hunks)src/utils.ts(1 hunks)tests/ui/CopyUrlModal.test.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
package.json
📄 CodeRabbit inference engine (AGENTS.md)
Ensure package.json defines bin: { "gh-manager-cli": "dist/index.js" }
Files:
package.json
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Follow TypeScript strictly; avoid any types without strong justification
Files:
src/utils.tssrc/ui/RepoList.tsxsrc/ui/components/modals/CopyUrlModal.tsxsrc/ui/components/modals/index.ts
src/{index.tsx,ui/**/*.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/{index.tsx,ui/**/*.tsx}: Use React functional components with hooks for Ink UI
Use chalk for colours; pre-colour strings instead of Ink color props to avoid nested Text issues
Use British English for all user-facing text (e.g., colour, organisation, authorisation)
Files:
src/ui/RepoList.tsxsrc/ui/components/modals/CopyUrlModal.tsx
src/ui/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
src/ui/**/*.tsx: Use Box with minHeight for consistent spacing across terminals
Implement documented keybindings (navigation, filtering, actions) in UI components
Modal UX: Left/Right to move focus, Enter to activate, Y to confirm, C/Esc to cancel
Files:
src/ui/RepoList.tsxsrc/ui/components/modals/CopyUrlModal.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-02T19:40:09.867Z
Learnt from: CR
PR: wiiiimm/gh-manager-cli#0
File: AGENTS.md:0-0
Timestamp: 2025-09-02T19:40:09.867Z
Learning: Applies to src/ui/**/*.tsx : Modal UX: Left/Right to move focus, Enter to activate, Y to confirm, C/Esc to cancel
Applied to files:
src/ui/RepoList.tsx
🧬 Code graph analysis (3)
tests/ui/CopyUrlModal.test.tsx (3)
src/types.ts (1)
RepoNode(8-42)src/ui/components/modals/CopyUrlModal.tsx (1)
CopyUrlModal(15-126)tests/ui/RenameModal.test.tsx (7)
mockUseInput(50-351)mockUseInput(336-350)mockUseInput(77-93)mockUseInput(95-106)mockUseInput(256-273)mockUseInput(275-287)mockUseInput(108-122)
src/ui/RepoList.tsx (4)
src/types.ts (1)
RepoNode(8-42)src/utils.ts (1)
copyToClipboard(29-66)src/ui/components/modals/CopyUrlModal.tsx (1)
CopyUrlModal(15-126)src/ui/RepoList.main.tsx (1)
input(569-993)
src/ui/components/modals/CopyUrlModal.tsx (2)
src/types.ts (1)
RepoNode(8-42)src/ui/components/modals/InfoModal.tsx (2)
InfoModal(13-57)InfoModalProps(7-11)
🪛 Biome (2.1.2)
src/ui/components/modals/CopyUrlModal.tsx
[error] 27-27: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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: Cursor Bugbot
🔇 Additional comments (26)
README.md (2)
63-64: Docs align with the new ‘C Copy URL’ action.The feature description and action list look consistent with the implementation.
438-439: Roadmap entry accurately reflects the feature.Recently implemented section reads well and matches behaviour.
src/ui/components/modals/index.ts (1)
10-10: Barrel export added correctly.Re-exporting
CopyUrlModalkeeps import sites tidy and consistent with other modals.src/ui/RepoList.tsx (10)
12-12: Import surface looks goodRe-export usage via components/modals aligns with existing pattern.
15-15: Utility import OKcopyToClipboard integration is appropriate here.
151-155: Solid, minimal state for modal + toastNames are clear and scoped.
217-221: Clean modal open handlerInitialises target and resets toast—good.
223-227: Close handler resets all related stateGood hygiene.
1030-1034: Input trapping for modal is correctPrevents list handlers from consuming keys while the modal is open.
1231-1238: Keybinding to open modal fits the scheme‘C’ is consistent with footer/help.
1443-1443: Include copyUrlMode in modalOpenEnsures global dimming/UX consistency.
1982-1990: Modal render wiring is soundProps passed match the modal’s contract.
2116-2116: Help footer updated appropriatelyNice addition; keeps discoverability high.
tests/ui/CopyUrlModal.test.tsx (13)
7-14: Mocking Ink’s useInput correctlyThis isolates stdin and keeps tests deterministic.
47-53: Good fixture reset patternDynamic import ensures the mocked hook reference is current.
54-67: Render assertions cover the essentialsTitle, repo, and URLs verified.
69-79: Default selection checks are clearArrow marker and prompt verified.
95-110: Enter-to-copy path coveredOnCopy args validated.
112-127: Shortcut S for SSH worksGood coverage.
129-144: Shortcut H for HTTPS worksGood coverage.
146-178: Arrow navigation tests read wellState transitions asserted via prompt text.
180-220: Close keys (Esc/Q/C) coveredCovers the modal lifecycle.
222-229: Null repo path handledError message rendered as expected.
231-240: URL generation validatedBoth protocols asserted.
242-264: Selection switching integration test is usefulCovers up/down interaction end-to-end.
266-283: Enter uses current selectionCovers the main interaction flow comprehensively.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/ui/components/modals/CopyUrlModal.tsx (2)
19-24: Fix hooks order; add Left/Right navigation and Y to confirm; guard input safely.useInput must be unconditional; also align keys with modal UX (←/→ to move focus, Y to confirm). Compute URLs lazily inside the handler when repo exists.
Apply:
- if (!repo) { - return <Text color="red">No repository selected.</Text>; - } - - const sshUrl = `git@github.com:${repo.nameWithOwner}.git`; - const httpsUrl = `https://github.com/${repo.nameWithOwner}.git`; - // Handle keyboard input useInput((input, key) => { - if (key.escape || (input && (input.toLowerCase() === 'c' || input.toLowerCase() === 'q'))) { + const lower = typeof input === 'string' ? input.toLowerCase() : ''; + if (key.escape || lower === 'c' || lower === 'q') { onClose(); return; } - - // Up/Down arrow navigation - if (key.upArrow) { + + if (!repo) return; // no-op when there is no repository + + // Navigation (↑/↓ and ←/→) + if (key.upArrow || key.leftArrow) { setSelectedType('SSH'); return; } - - if (key.downArrow) { + + if (key.downArrow || key.rightArrow) { setSelectedType('HTTPS'); return; } - // Enter key copies selected option - if (key.return) { - const urlToCopy = selectedType === 'SSH' ? sshUrl : httpsUrl; + // Enter/Y confirm: copy selected + if (key.return || lower === 'y') { + const urlToCopy = + selectedType === 'SSH' + ? `git@github.com:${repo.nameWithOwner}.git` + : `https://github.com/${repo.nameWithOwner}.git`; handleCopy(urlToCopy, selectedType); return; } // S/H shortcuts still work - if (input && input.toLowerCase() === 's') { - handleCopy(sshUrl, 'SSH'); + if (lower === 's') { + handleCopy(`git@github.com:${repo.nameWithOwner}.git`, 'SSH'); return; } - if (input && input.toLowerCase() === 'h') { - handleCopy(httpsUrl, 'HTTPS'); + if (lower === 'h') { + handleCopy(`https://github.com/${repo.nameWithOwner}.git`, 'HTTPS'); return; } }); + + if (!repo) { + return <Text color="red">No repository selected.</Text>; + } + + const sshUrl = `git@github.com:${repo.nameWithOwner}.git`; + const httpsUrl = `https://github.com/${repo.nameWithOwner}.git`;Verify tests cover ←/→ and Y:
#!/bin/bash # Find tests for new keybindings rg -nC2 -g 'tests/**' -e 'leftArrow|rightArrow' -e "['\"]y['\"]"Also applies to: 26-61
68-70: Avoid any in catch; use unknown with safe narrowing.Keeps TS strict across src/**/*.
- } catch (error: any) { - setCopyError(`Failed to copy ${type} URL: ${error.message | | 'Unknown error'}`); + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error ?? 'Unknown error'); + setCopyError(`Failed to copy ${type} URL: ${message}`); }
🧹 Nitpick comments (8)
src/utils.ts (3)
40-57: Capture stderr and add a safety timeout for better diagnostics/robustness.Expose underlying failures (e.g., missing binary) and avoid hanging processes.
Apply:
- const spawnCommand = (command: string, args: string[] = []): Promise<void> => { - return new Promise((resolve, reject) => { - const child = spawn(command, args, { stdio: ['pipe', 'pipe', 'pipe'] }); + const spawnCommand = (command: string, args: string[] = [], timeoutMs = 3000): Promise<void> => { + return new Promise((resolve, reject) => { + const child = spawn(command, args, { stdio: ['pipe', 'pipe', 'pipe'] }); + let stderr = ''; + const timer = setTimeout(() => { + child.kill(); + reject(new Error(`${command} timed out after ${timeoutMs}ms`)); + }, timeoutMs); child.stdin.write(text); child.stdin.end(); + child.stderr.on('data', (d) => (stderr += d.toString())); child.on('close', (code) => { + clearTimeout(timer); if (code === 0) { resolve(); } else { - reject(new Error(`Command failed with code ${code}`)); + reject(new Error(`${command} exited with code ${code}${stderr ? `: ${stderr.trim()}` : ''}`)); } }); child.on('error', reject); }); };
65-68: Add Windows PowerShell fallback (Set-Clipboard).clip.exe is usually present, but having PowerShell as a secondary path hardens cross-environment behaviour.
- } else if (platform === 'win32') { - // Windows - use clip - await spawnCommand('clip'); + } else if (platform === 'win32') { + // Windows - try clip, then PowerShell + try { + await spawnCommand('clip'); + } catch { + await spawnCommand('powershell', ['-NoProfile', '-Command', 'Set-Clipboard']); + }
80-82: Preserve underlying error details in thrown message.Makes failures actionable without guessing which utility is missing.
- } catch (osError) { - throw new Error(`Failed to copy to clipboard. Please install a clipboard utility for your system.`); + } catch (osError) { + const msg = osError instanceof Error ? osError.message : String(osError ?? 'Unknown error'); + throw new Error(`Failed to copy to clipboard: ${msg}. If you are on Linux, install xclip, xsel, or wl-clipboard.`); }src/ui/components/modals/CopyUrlModal.tsx (5)
74-81: Add minHeight for layout consistency across terminals.Matches the UI spacing guideline.
return ( <Box flexDirection="column" borderStyle="round" borderColor="blue" paddingX={3} paddingY={2} - width={Math.min(terminalWidth - 8, 80)} + width={Math.min(terminalWidth - 8, 80)} + minHeight={12} >
82-82: Prefer chalk-coloured strings over Ink colour props for text.Avoid nested Text colour issues per guidelines.
- <Text bold color="blue">Copy Repository URL</Text> + <Text>{chalk.blue.bold('Copy Repository URL')}</Text>
88-98: Colour the SSH line with chalk instead of Text colour prop.Keeps styling consistent and avoids nested colour props.
- <Text color="gray">SSH URL:</Text> + <Text>{chalk.gray('SSH URL:')}</Text> @@ - <Text color={selectedType === 'SSH' ? 'blue' : undefined}> - {selectedType === 'SSH' ? '▶ ' : ' '}{sshUrl} - </Text> + <Text> + {selectedType === 'SSH' + ? chalk.blue(`▶ ${sshUrl}`) + : ` ${sshUrl}`} + </Text>
101-111: Colour the HTTPS line with chalk for consistency.- <Text color="gray">HTTPS URL:</Text> + <Text>{chalk.gray('HTTPS URL:')}</Text> @@ - <Text color={selectedType === 'HTTPS' ? 'blue' : undefined}> - {selectedType === 'HTTPS' ? '▶ ' : ' '}{httpsUrl} - </Text> + <Text> + {selectedType === 'HTTPS' + ? chalk.blue(`▶ ${httpsUrl}`) + : ` ${httpsUrl}`} + </Text>
114-116: Update hint text to reflect documented keybindings.Include ←/→ and Y; drop Q per guidelines.
- <Text color="gray"> - ↑↓ Select • Enter to copy {selectedType} • S copy SSH • H copy HTTPS • Esc/Q/C to close - </Text> + <Text color="gray"> + ↑↓/←→ Select • Enter/Y to copy {selectedType} • S copy SSH • H copy HTTPS • Esc/C to cancel + </Text>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/ui/components/modals/CopyUrlModal.tsx(1 hunks)src/utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Follow TypeScript strictly; avoid any types without strong justification
Files:
src/utils.tssrc/ui/components/modals/CopyUrlModal.tsx
src/{index.tsx,ui/**/*.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/{index.tsx,ui/**/*.tsx}: Use React functional components with hooks for Ink UI
Use chalk for colours; pre-colour strings instead of Ink color props to avoid nested Text issues
Use British English for all user-facing text (e.g., colour, organisation, authorisation)
Files:
src/ui/components/modals/CopyUrlModal.tsx
src/ui/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
src/ui/**/*.tsx: Use Box with minHeight for consistent spacing across terminals
Implement documented keybindings (navigation, filtering, actions) in UI components
Modal UX: Left/Right to move focus, Enter to activate, Y to confirm, C/Esc to cancel
Files:
src/ui/components/modals/CopyUrlModal.tsx
🧬 Code graph analysis (1)
src/ui/components/modals/CopyUrlModal.tsx (2)
src/types.ts (1)
RepoNode(8-42)src/ui/components/modals/InfoModal.tsx (2)
InfoModal(13-57)InfoModalProps(7-11)
🪛 Biome (2.1.2)
src/ui/components/modals/CopyUrlModal.tsx
[error] 27-27: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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: Cursor Bugbot
🔇 Additional comments (1)
src/utils.ts (1)
36-57: Resolved: command injection risk mitigated with spawn + stdin.Switching from shell-quoted exec to spawn with stdin removes injection vectors. Good security hardening.
- Change catch error type from 'any' to 'unknown' - Add proper type narrowing with instanceof Error check - Use safe String(error) fallback for non-Error objects - Ensure TypeScript strict rules are satisfied - Maintain existing error message functionality 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add timer ref management to prevent toast overlaps and memory leaks - Fix CopyUrlModal onCopy prop type to return Promise<void> - Improve error handling with proper unknown type narrowing - Clean up toast timers on component unmount - Fix test mock RepoNode to match strict interface requirements - Remove duplicate error messages between toast and modal - Prevent race conditions in rapid copying scenarios Addresses all PR feedback regarding toast notifications, TypeScript strict compliance, and memory management. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ui/RepoList.tsx (1)
228-239: Fix copy toast lifecycle: don’t clear on modal close; avoid duplicate error toastsCurrently, closeCopyUrlModal clears the toast, so a success toast disappears as soon as the modal closes. Also, failures are shown both inside the modal and as a global toast. Keep success toast after close, and show errors only inside the modal.
Apply:
function closeCopyUrlModal() { setCopyUrlMode(false); setCopyUrlTarget(null); - setCopyToast(null); } async function handleCopyUrl(url: string, type: 'SSH' | 'HTTPS'): Promise<void> { try { // Clear any existing timer before setting a new one if (copyToastTimerRef.current) { clearTimeout(copyToastTimerRef.current); copyToastTimerRef.current = null; } await copyToClipboard(url); setCopyToast(`Copied ${type} URL to clipboard`); // Set new timer for success toast copyToastTimerRef.current = setTimeout(() => { setCopyToast(null); copyToastTimerRef.current = null; }, 3000); } catch (error: unknown) { // Clear any existing timer before setting a new one if (copyToastTimerRef.current) { clearTimeout(copyToastTimerRef.current); copyToastTimerRef.current = null; } - - const message = error instanceof Error ? error.message : String(error) || 'Unknown error'; - setCopyToast(`Failed to copy ${type} URL: ${message}`); - - // Set new timer for error toast - copyToastTimerRef.current = setTimeout(() => { - setCopyToast(null); - copyToastTimerRef.current = null; - }, 5000); throw error; // Re-throw so modal can handle it } }Also applies to: 256-271
🧹 Nitpick comments (3)
src/ui/RepoList.tsx (1)
2170-2177: Use chalk to colour the toast text to match UI guidelinesPrefer pre-coloured strings over Ink colour props.
- <Box borderStyle="round" borderColor={copyToast.includes('Failed') ? 'red' : 'green'} paddingX={2} paddingY={0}> - <Text color={copyToast.includes('Failed') ? 'red' : 'green'}>{copyToast}</Text> + <Box borderStyle="round" borderColor={copyToast.includes('Failed') ? 'red' : 'green'} paddingX={2} paddingY={0}> + <Text>{copyToast.includes('Failed') ? chalk.red(copyToast) : chalk.green(copyToast)}</Text> </Box>src/ui/components/modals/CopyUrlModal.tsx (2)
115-117: Update instructions to reflect Left/Right and ‘Y’Make hints match implemented keys.
- ↑↓ Select • Enter to copy {selectedType} • S copy SSH • H copy HTTPS • Esc/Q/C to close + ↑↓/←→ Select • Enter/Y to copy {selectedType} • S copy SSH • H copy HTTPS • Esc/Q/C to close
120-124: Optional: pre-colour strings with chalk instead of Ink colour propsTo avoid nested Text colour issues and match guidelines, consider:
- <Text color="red">{copyError}</Text> + <Text>{chalk.red(copyError)}</Text>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/ui/RepoList.tsx(9 hunks)src/ui/components/modals/CopyUrlModal.tsx(1 hunks)tests/ui/CopyUrlModal.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/ui/CopyUrlModal.test.tsx
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Follow TypeScript strictly; avoid any types without strong justification
Files:
src/ui/components/modals/CopyUrlModal.tsxsrc/ui/RepoList.tsx
src/{index.tsx,ui/**/*.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/{index.tsx,ui/**/*.tsx}: Use React functional components with hooks for Ink UI
Use chalk for colours; pre-colour strings instead of Ink color props to avoid nested Text issues
Use British English for all user-facing text (e.g., colour, organisation, authorisation)
Files:
src/ui/components/modals/CopyUrlModal.tsxsrc/ui/RepoList.tsx
src/ui/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
src/ui/**/*.tsx: Use Box with minHeight for consistent spacing across terminals
Implement documented keybindings (navigation, filtering, actions) in UI components
Modal UX: Left/Right to move focus, Enter to activate, Y to confirm, C/Esc to cancel
Files:
src/ui/components/modals/CopyUrlModal.tsxsrc/ui/RepoList.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-02T19:40:09.867Z
Learnt from: CR
PR: wiiiimm/gh-manager-cli#0
File: AGENTS.md:0-0
Timestamp: 2025-09-02T19:40:09.867Z
Learning: Applies to src/ui/**/*.tsx : Modal UX: Left/Right to move focus, Enter to activate, Y to confirm, C/Esc to cancel
Applied to files:
src/ui/RepoList.tsx
🧬 Code graph analysis (2)
src/ui/components/modals/CopyUrlModal.tsx (2)
src/types.ts (1)
RepoNode(8-42)src/ui/components/modals/InfoModal.tsx (2)
InfoModalProps(7-11)InfoModal(13-57)
src/ui/RepoList.tsx (3)
src/types.ts (1)
RepoNode(8-42)src/utils.ts (1)
copyToClipboard(29-84)src/ui/components/modals/CopyUrlModal.tsx (1)
CopyUrlModal(15-127)
🪛 Biome (2.1.2)
src/ui/components/modals/CopyUrlModal.tsx
[error] 27-27: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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: Cursor Bugbot
🔇 Additional comments (9)
src/ui/RepoList.tsx (7)
12-12: LGTM: correct imports and utility usageImporting CopyUrlModal and copyToClipboard is correct and aligns with the new workflow.
Also applies to: 15-15
151-166: Toast timer lifecycle guard looks goodNice use of a ref and cleanup effect to avoid dangling timers.
1065-1068: LGTM: input trapping while copy modal is openGood isolation—parent ignores input when CopyUrlModal is active.
1266-1273: LGTM: keyboard shortcut to open modal‘C’ to open aligns with the README and the footer help.
2017-2025: LGTM: modal integrationProps and async onCopy wiring are correct; width constraint matches other modals.
1478-1479: LGTM: modalOpen includes copy modalEnsures proper dimming/interaction block across the UI.
2151-2152: LGTM: footer help updatedHelp text correctly advertises “C Copy URL”.
src/ui/components/modals/CopyUrlModal.tsx (2)
63-71: Good: strict error typing and safe narrowingUsing unknown with proper narrowing is compliant with TS strict rules.
74-126: UI layout is clear and consistentBorders, selection indicators, and dynamic hinting are well executed.
- Move useInput hook before early return to comply with Rules of Hooks - Add Left/Right arrow navigation with wrap-around functionality - Support both lowercase and uppercase 'Y' key for copy action - Update footer instructions to show all available navigation options - Add 5 comprehensive tests for new navigation features - Ensure proper state management and prevent default behavior Fixes critical React hooks violation while enhancing user experience with more intuitive navigation patterns. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Keep left/right arrow functionality as a hidden convenience feature while only showing the primary ↑↓ navigation in the footer instructions. This provides a cleaner UI while still supporting intuitive horizontal navigation for power users. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/ui/components/modals/CopyUrlModal.tsx (1)
22-29: Normalise input handling via a single lowercased char variableReduces repetition and keeps arrow/printable-key checks tidy. This repeats a prior suggestion.
Apply:
- useInput((input, key) => { + useInput((input, key) => { + const ch = input?.toLowerCase(); @@ - if (key.escape || (input && (input.toLowerCase() === 'c' || input.toLowerCase() === 'q'))) { + if (key.escape || ch === 'c' || ch === 'q') { onClose(); return; } @@ - if (key.return || (input && input.toLowerCase() === 'y')) { + if (key.return || ch === 'y') { @@ - if (input && input.toLowerCase() === 's') { + if (ch === 's') { @@ - if (input && input.toLowerCase() === 'h') { + if (ch === 'h') {Also applies to: 57-77
🧹 Nitpick comments (7)
src/ui/components/modals/CopyUrlModal.tsx (7)
100-107: Clamp width and add a minimum height for consistent layoutAvoid negative widths on narrow terminals and align with the “use Box with minHeight” guideline.
Apply:
- return ( - <Box + const modalWidth = Math.max(40, Math.min(terminalWidth - 8, 80)); + return ( + <Box flexDirection="column" borderStyle="round" borderColor="blue" paddingX={3} paddingY={2} - width={Math.min(terminalWidth - 8, 80)} + width={modalWidth} + minHeight={12} >
108-149: Pre‑colour strings with chalk instead of Ink Text colour propsOur UI guideline prefers chalk-coloured strings to avoid nested Text quirks. Replace Text colour props with chalk, keeping border colours as-is.
Apply:
- <Text bold color="blue">Copy Repository URL</Text> + <Text>{chalk.blue.bold('Copy Repository URL')}</Text> @@ - <Text color="gray">SSH URL:</Text> + <Text>{chalk.gray('SSH URL:')}</Text> @@ - > - <Text color={selectedType === 'SSH' ? 'blue' : undefined}> - {selectedType === 'SSH' ? '▶ ' : ' '}{sshUrl} - </Text> + > + <Text> + {selectedType === 'SSH' + ? chalk.blue(`▶ ${sshUrl}`) + : ` ${sshUrl}`} + </Text> </Box> @@ - <Text color="gray">HTTPS URL:</Text> + <Text>{chalk.gray('HTTPS URL:')}</Text> @@ - > - <Text color={selectedType === 'HTTPS' ? 'blue' : undefined}> - {selectedType === 'HTTPS' ? '▶ ' : ' '}{httpsUrl} - </Text> + > + <Text> + {selectedType === 'HTTPS' + ? chalk.blue(`▶ ${httpsUrl}`) + : ` ${httpsUrl}`} + </Text> </Box> @@ - <Text color="gray"> - ↑↓←→ Select • Enter/Y to copy {selectedType} • S copy SSH • H copy HTTPS • Esc/Q/C to close - </Text> + <Text> + {chalk.gray(`↑↓←→ Select • Enter/Y to copy ${selectedType} • S copy SSH • H copy HTTPS • Esc/C to cancel`)} + </Text> @@ - {copyError && ( + {copyError && ( <> <Box height={1}><Text> </Text></Box> - <Text color="red">{copyError}</Text> + <Text>{chalk.red(copyError)}</Text> </> )}
81-83: Use chalk for the error message to match the guidelineApply:
- if (!repo) { - return <Text color="red">No repository selected.</Text>; - } + if (!repo) { + return <Text>{chalk.red('No repository selected.')}</Text>; + }
58-77: Deduplicate URL construction inside the input handlerReuse the render-scoped sshUrl/httpsUrl instead of recomputing them in each branch.
Apply:
- if (key.return || (input && input.toLowerCase() === 'y')) { - const sshUrl = `git@github.com:${repo.nameWithOwner}.git`; - const httpsUrl = `https://github.com/${repo.nameWithOwner}.git`; - const urlToCopy = selectedType === 'SSH' ? sshUrl : httpsUrl; + if (key.return || (input && input.toLowerCase() === 'y')) { + const urlToCopy = selectedType === 'SSH' ? sshUrl : httpsUrl; handleCopy(urlToCopy, selectedType); return; } @@ - if (input && input.toLowerCase() === 's') { - const sshUrl = `git@github.com:${repo.nameWithOwner}.git`; + if (input && input.toLowerCase() === 's') { handleCopy(sshUrl, 'SSH'); return; } @@ - if (input && input.toLowerCase() === 'h') { - const httpsUrl = `https://github.com/${repo.nameWithOwner}.git`; + if (input && input.toLowerCase() === 'h') { handleCopy(httpsUrl, 'HTTPS'); return; }Note: If your linter flags “use-before-define” for sshUrl/httpsUrl in the handler, move their declarations above useInput and guard with repo checks, or keep current recomputation.
Also applies to: 85-87
13-20: Make urlTypes readonly and derive UrlType from itKeeps the union in sync with the options array automatically.
Apply:
-type UrlType = 'SSH' | 'HTTPS'; +const urlTypes = ['SSH', 'HTTPS'] as const; +type UrlType = typeof urlTypes[number]; @@ - const urlTypes: UrlType[] = ['SSH', 'HTTPS']; + // urlTypes defined above as readonly tuple
94-96: Guarantee a meaningful fallback error messageString(error) is almost always truthy; ensure we fall back to 'Unknown error' when needed.
Apply:
- const message = error instanceof Error ? error.message : String(error) || 'Unknown error'; + const message = + error instanceof Error + ? (error.message || 'Unknown error') + : (error != null && String(error).trim() !== '' ? String(error) : 'Unknown error');
140-142: Wording: use ‘cancel’ (British English) and align with documented keysGuideline says: Left/Right to move focus, Enter to activate, Y to confirm, C/Esc to cancel. If you keep ‘Q’ as an extra, either include it in docs or drop it from the handler.
Apply (text-only change; keep handler as-is or update docs):
- {chalk.gray(`↑↓←→ Select • Enter/Y to copy ${selectedType} • S copy SSH • H copy HTTPS • Esc/Q/C to close`)} + {chalk.gray(`↑↓←→ Select • Enter/Y to copy ${selectedType} • S copy SSH • H copy HTTPS • Esc/C to cancel`)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/ui/components/modals/CopyUrlModal.tsx(1 hunks)tests/ui/CopyUrlModal.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/ui/CopyUrlModal.test.tsx
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Follow TypeScript strictly; avoid any types without strong justification
Files:
src/ui/components/modals/CopyUrlModal.tsx
src/{index.tsx,ui/**/*.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/{index.tsx,ui/**/*.tsx}: Use React functional components with hooks for Ink UI
Use chalk for colours; pre-colour strings instead of Ink color props to avoid nested Text issues
Use British English for all user-facing text (e.g., colour, organisation, authorisation)
Files:
src/ui/components/modals/CopyUrlModal.tsx
src/ui/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
src/ui/**/*.tsx: Use Box with minHeight for consistent spacing across terminals
Implement documented keybindings (navigation, filtering, actions) in UI components
Modal UX: Left/Right to move focus, Enter to activate, Y to confirm, C/Esc to cancel
Files:
src/ui/components/modals/CopyUrlModal.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-02T19:40:09.867Z
Learnt from: CR
PR: wiiiimm/gh-manager-cli#0
File: AGENTS.md:0-0
Timestamp: 2025-09-02T19:40:09.867Z
Learning: Applies to src/ui/**/*.tsx : Modal UX: Left/Right to move focus, Enter to activate, Y to confirm, C/Esc to cancel
Applied to files:
src/ui/components/modals/CopyUrlModal.tsx
🧬 Code graph analysis (1)
src/ui/components/modals/CopyUrlModal.tsx (2)
src/types.ts (1)
RepoNode(8-42)src/ui/components/modals/InfoModal.tsx (2)
InfoModalProps(7-11)InfoModal(13-57)
⏰ 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: Cursor Bugbot
🔇 Additional comments (1)
src/ui/components/modals/CopyUrlModal.tsx (1)
21-44: Hooks order and keyboard UX look solid
- useInput is unconditional and guarded for repo presence.
- Left/Right focus, Up/Down shortcuts, Enter and Y to confirm, and C/Esc to cancel are implemented. Nice wrap-around logic.
Also applies to: 46-78
- Create single 'ch' variable for normalized character input - Remove repetitive input?.toLowerCase() calls - Clean up conditional checks for character-based inputs - Maintain exact same functionality with cleaner code - Reduces repetition and improves readability 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/ui/CopyUrlModal.test.tsx (4)
79-91: Remove duplicate “footer instructions” test.The test at Lines 364–376 duplicates the one at Lines 79–91. Keep one to avoid redundant assertions.
- it('shows updated footer instructions with new navigation options', () => { - mockUseInput.mockImplementation(() => {}); - - const { lastFrame, unmount } = render(<CopyUrlModal {...defaultProps} />); - - const output = lastFrame() || ''; - expect(output).toContain('↑↓ Select'); - expect(output).toContain('Enter/Y to copy SSH'); - expect(output).toContain('S copy SSH'); - expect(output).toContain('H copy HTTPS'); - expect(output).toContain('Esc/Q/C to close'); - unmount(); - });Also applies to: 364-376
45-50: Reset all spies between tests to prevent cross-test leakage.Add a global clear to reset any vi.fn() call counts (e.g. defaultProps spies) between tests.
beforeEach(async () => { // Reset and get the mocked useInput using dynamic import const ink = await import('ink'); mockUseInput = (ink as any).useInput; mockUseInput.mockReset(); + vi.clearAllMocks(); });
93-108: Also assert the modal triggers onClose after a successful copy.This verifies the full success path, not just onCopy invocation.
- it('handles Enter key to copy selected option (SSH by default)', () => { - const mockOnCopy = vi.fn().mockResolvedValue(undefined); + it('handles Enter key to copy selected option (SSH by default) and closes on success', async () => { + const mockOnCopy = vi.fn().mockResolvedValue(undefined); + const mockOnClose = vi.fn(); let inputCallback: any; mockUseInput.mockImplementation((callback: any) => { inputCallback = callback; }); - const { unmount } = render(<CopyUrlModal {...defaultProps} onCopy={mockOnCopy} />); + const { unmount } = render(<CopyUrlModal {...defaultProps} onCopy={mockOnCopy} onClose={mockOnClose} />); // Simulate Enter key (SSH should be selected by default) inputCallback('', { return: true }); expect(mockOnCopy).toHaveBeenCalledWith('git@github.com:owner/test-repo.git', 'SSH'); + // allow async handler to resolve and close + await Promise.resolve(); + expect(mockOnClose).toHaveBeenCalled(); unmount(); });
220-227: Add a guard test: ignore input when repo is null.Ensures the early-return in the input handler is honoured.
it('shows "No repository selected" when repo is null', () => { mockUseInput.mockImplementation(() => {}); const { lastFrame, unmount } = render(<CopyUrlModal {...defaultProps} repo={null} />); expect(lastFrame()).toContain('No repository selected.'); unmount(); }); + + it('ignores inputs when repo is null', () => { + const mockOnClose = vi.fn(); + let cb: any; + mockUseInput.mockImplementation((fn: any) => { cb = fn; }); + const { unmount } = render(<CopyUrlModal {...defaultProps} repo={null} onClose={mockOnClose} />); + cb('q', {}); // would normally close + cb('', { escape: true }); + expect(mockOnClose).not.toHaveBeenCalled(); + unmount(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/ui/components/modals/CopyUrlModal.tsx(1 hunks)tests/ui/CopyUrlModal.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/components/modals/CopyUrlModal.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ui/CopyUrlModal.test.tsx (2)
src/types.ts (1)
RepoNode(8-42)src/ui/components/modals/CopyUrlModal.tsx (1)
CopyUrlModal(15-152)
🔇 Additional comments (2)
tests/ui/CopyUrlModal.test.tsx (2)
1-377: Solid, comprehensive UI test coverage.Good use of ink-testing-library, targeted keyboard-event simulation, and cleanup with unmount(). Overall structure reads well.
19-36: RepoNode mock matches the source type.The mock now aligns with src/types.RepoNode (no extraneous fields). Nice fix.
- Test onCopy rejection scenario - Verify error message displays in UI - Ensure modal stays open on error - Add comprehensive error path coverage 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
# [1.21.0](v1.20.0...v1.21.0) (2025-09-03) ### Features * add repository URL copying modal with interactive selection ([#24](#24)) ([45998e9](45998e9))
|
🎉 This PR is included in version 1.21.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Summary
• Add interactive modal for copying repository clone URLs (SSH/HTTPS)
• Implement keyboard navigation with visual selection indicators
• Support multiple interaction methods for improved UX
• Include comprehensive test coverage with 15 test cases
Features Added
Technical Details
copyToClipboard()utility function with platform detectionTest Coverage
✅ 15 test cases covering:
User Experience
Users can now press 'C' on any repository to:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores