Refactor copy paths UI into unified component with gesture support#87
Merged
Refactor copy paths UI into unified component with gesture support#87
Conversation
…ck menu Both the file-header copy button and the bulk sidebar/status-strip buttons now share one <x-copy-paths-button> component. Left-click copies relative path(s); right-click or 400 ms long-press opens the existing 3-option menu (file names / relative paths / full paths). Tooltip surfaces the primary action plus a "Right-click for options" hint. The capture-phase wrapper sits inside <flux:dropdown> around just the trigger, so menu-item clicks bypass it. Long-press / right-click open the menu via the overlay's _popoverable.setState — <ui-dropdown> doesn't expose its own open(); finding the overlay via lastElementChild avoids matching <flux:tooltip.content> on its popover attribute.
- Drop orphaned copyVisibleFilePaths() from review-page (callers all moved to the new factory's copyAs()). - Trim duplicate Flux-internals doc block in copy-paths-button.js; the WHY now lives only on openMenu() where it's non-obvious. - Restore real timers between describe blocks in the JS test. - Use toEqualCanonicalizing in the browser tests so exact membership and count assert in one expression.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a unified Alpine.js factory ChangesCopy-Paths Button & Integration
Sequence DiagramsequenceDiagram
participant User
participant Component as copyPathsButton
participant Clipboard as Clipboard/Toast
User->>Component: Left-click
Component->>Component: onClick() -> copyAs('relative')
Component->>Clipboard: dispatch copy-to-clipboard with relative paths + toast
Clipboard-->>User: show toast / clipboard updated
User->>Component: Press & hold (> LONG_PRESS_MS)
Component->>Component: onMouseDown() -> start long-press timer
Component->>Component: openMenu() (timer fires)
User->>Component: Select option (e.g., 'full')
Component->>Component: copyAs('full')
Component->>Clipboard: dispatch copy-to-clipboard with full paths + toast
Clipboard-->>User: show toast / clipboard updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
public/js/copy-paths-button.js (1)
104-107: ⚡ Quick winAdd a guard flag to prevent
autoInstall()from re-registering during SPA navigation.The
autoInstall()function currently lacks a sentinel to prevent re-execution. On Livewire SPA navigation, this script can run again, stacking listeners and re-registering the Alpine component.Suggested fix
function autoInstall(root) { + if (root.__copyPathsButtonAttached) { + return; + } + root.__copyPathsButtonAttached = true; + const init = () => root.Alpine.data('copyPathsButton', copyPathsButton); root.Alpine ? init() : root.document.addEventListener('alpine:init', init); }Per guideline: "Guard against SPA-navigation re-execution in Livewire by using a guard flag pattern:
if (window.__fooAttached) return; window.__fooAttached = true;"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/js/copy-paths-button.js` around lines 104 - 107, The autoInstall function can run multiple times during Livewire SPA navigation and should use a guard flag to prevent re-registration: at the top of autoInstall add a global sentinel check (e.g. window.__copyPathsButtonAttached) that returns early if set, and set it to true before registering the Alpine component; update the body that calls root.Alpine.data('copyPathsButton', copyPathsButton) (and the document event listener path) only when the sentinel was not previously set so copyPathsButton and its listeners are registered exactly once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@public/js/copy-paths-button.js`:
- Around line 104-107: The autoInstall function can run multiple times during
Livewire SPA navigation and should use a guard flag to prevent re-registration:
at the top of autoInstall add a global sentinel check (e.g.
window.__copyPathsButtonAttached) that returns early if set, and set it to true
before registering the Alpine component; update the body that calls
root.Alpine.data('copyPathsButton', copyPathsButton) (and the document event
listener path) only when the sentinel was not previously set so copyPathsButton
and its listeners are registered exactly once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b720e9db-5118-4815-8d10-a006638ec8f4
📒 Files selected for processing (8)
public/js/copy-paths-button.jsresources/views/components/copy-paths-button.blade.phpresources/views/components/copy-paths-menu.blade.phpresources/views/components/diff/file-header.blade.phpresources/views/components/status-strip.blade.phpresources/views/pages/⚡review-page.blade.phptests/Browser/FileInteractionTest.phptests/Js/copy-paths-button.test.js
💤 Files with no reviewable changes (1)
- resources/views/components/copy-paths-menu.blade.php
![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 automated review suggestions for this pull request.
Reviewed commit: fc2dff4fe5
ℹ️ 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
- 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 address that feedback".
Comment on lines
+46
to
+47
| if (kind === 'name') return this.pathBase(p); | ||
| if (kind === 'full') return this.buildFullPath(p); |
There was a problem hiding this comment.
When the shared diff-file header is rendered outside the review page, e.g. in resources/views/pages/⚡context-page.blade.php, its ancestor Alpine scope does not define pathBase() or buildFullPath(); it only defines scrollToContextFile() and init(). Right-clicking the new single-file copy button there advertises “Copy file names” / “Copy full paths”, but choosing either option reaches these calls and throws instead of copying. The single-mode component needs local fallbacks or the required helpers/values passed in rather than assuming the review-page scope is always present.
Useful? React with 👍 / 👎.
The single-mode button is rendered inside <x-diff.file-header>, which is also used by ⚡context-page. That page's Alpine root only defines scrollToContextFile / init — no pathBase / buildFullPath / repoPath. With the previous factory, right-clicking and choosing "Copy file names" or "Copy full paths" would throw `this.pathBase is not a function` / `this.buildFullPath is not a function` (Codex P2). Fix: drop the factory's reliance on inherited helpers. Basename is a local lastIndexOf split. Full-path uses an init-param repoPath, falling back to inherited this.repoPath for bulk mode (which is review-page only). file-header now takes a :repo-path prop, fed from ⚡diff-file's $repoPath, and forwards it to the copy-paths-button. Also add the project's standard `__copyPathsButtonAttached` sentinel to autoInstall so SPA navigation doesn't double-register the Alpine component (CodeRabbit nit; matches diff-file.js / branch-explorer.js / keymap-store.js / smart-poll.js / etc.). JS tests gain a singleScope() helper with no parent helpers in scope, locking down the regression for "name", "full", empty repoPath fall-through, and trailing-slash normalization.
Reviewers can now see PR-scoped UI screenshots inline in a sticky comment. - tests/Screenshots/*ScreenshotTest.php captures named PNGs via Pest's browser plugin; calls global visit() so BrowserTestIdentifier picks them up outside tests/Browser/. Wired with the Browsable + CreatesTestRepo traits and a new Screenshots phpunit testsuite + composer test:screenshots. - .github/workflows/screenshots.yml runs the suite on PRs (skipped for forks since GITHUB_TOKEN is read-only there), then runs publish-screenshots.sh to push the PNGs to a screenshots orphan branch under pr-<n>/<sha>/ and post/update a sticky comment referencing them via raw.githubusercontent.com. - First consumer: copy-paths-button bulk + single-file menu states, to illustrate the pattern for future PRs.
ScreenshotsUpdated for
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/publish-screenshots.sh:
- Around line 78-87: Replace the word-splitting for-loop that uses command
substitution over ls with a null-delimited find/while read loop to safely
iterate filenames containing spaces or special characters: use find "$STAGE"
-maxdepth 1 -type f -name '*.png' -print0 piped into while IFS= read -r -d ''
png; do ... done, derive the basename and strip the .png suffix into name
(instead of parameter expansion on the ls output), and continue to build url
using REPO, BRANCH and SUBPATH as before; ensure you use read -r -d '' and quote
variable expansions (e.g., "$png", "$name") to avoid reintroducing
word-splitting.
In `@resources/views/components/copy-paths-button.blade.php`:
- Around line 33-37: The context menu is only reachable via pointer events; add
keyboard parity by wiring Enter/Space (and optionally ArrowDown or ContextMenu
key) to the same handlers: call openMenu() on keydown.enter/keydown.space and
invoke onClick($event) on keydown.enter/keydown.space where immediate copy is
expected, and ensure onKeyup/keydown handlers call cancelLongPress() as
appropriate; update the same for the other instance referenced (lines 44-45) so
keyboard users can open the copy-options menu and select name/full-path without
a mouse.
In `@tests/Js/copy-paths-button.test.js`:
- Around line 43-50: Pin document focus and visibility in these DOM-timer tests
by stubbing document.hasFocus and overriding document.hidden in the test
lifecycle: in the beforeEach that calls vi.useFakeTimers() and sets component =
attach(bulkScope(), 'bulk'), add a vi.spyOn(document,
'hasFocus').mockReturnValue(true) and Object.defineProperty(document, 'hidden',
{ value: false, configurable: true }) to force focused/visible state; in the
afterEach that calls vi.useRealTimers(), restore the hasFocus spy (or call
vi.restoreAllMocks()) and revert document.hidden (either by deleting the
configurable property or restoring the saved original value) so subsequent tests
aren’t affected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71cb4ffe-8d77-4482-a39d-308f0d4325b1
📒 Files selected for processing (13)
.github/scripts/publish-screenshots.sh.github/workflows/screenshots.yml.gitignorecomposer.jsonphpunit.xmlpublic/js/copy-paths-button.jsresources/views/components/copy-paths-button.blade.phpresources/views/components/diff/file-header.blade.phpresources/views/livewire/⚡diff-file.blade.phptests/Js/copy-paths-button.test.jstests/Pest.phptests/Screenshots/CLAUDE.mdtests/Screenshots/CopyPathsButtonScreenshotTest.php
Comment on lines
+43
to
+50
| beforeEach(() => { | ||
| vi.useFakeTimers(); | ||
| component = attach(bulkScope(), 'bulk'); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
Stabilize DOM tests by pinning focus/visibility state.
These tests use timers and UI gestures, but they don’t override document.hasFocus / document.hidden, which can introduce happy-dom-dependent flakes.
Suggested patch
beforeEach(() => {
vi.useFakeTimers();
+ vi.spyOn(document, 'hasFocus').mockReturnValue(false);
+ Object.defineProperty(document, 'hidden', {
+ configurable: true,
+ get: () => false,
+ });
component = attach(bulkScope(), 'bulk');
});
afterEach(() => {
+ vi.restoreAllMocks();
vi.useRealTimers();
});As per coding guidelines, “Override happy-dom's default focus and visibility states per test using vi.spyOn(document, 'hasFocus')... and Object.defineProperty(document, 'hidden', ...).”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| beforeEach(() => { | |
| vi.useFakeTimers(); | |
| component = attach(bulkScope(), 'bulk'); | |
| }); | |
| afterEach(() => { | |
| vi.useRealTimers(); | |
| }); | |
| beforeEach(() => { | |
| vi.useFakeTimers(); | |
| vi.spyOn(document, 'hasFocus').mockReturnValue(false); | |
| Object.defineProperty(document, 'hidden', { | |
| configurable: true, | |
| get: () => false, | |
| }); | |
| component = attach(bulkScope(), 'bulk'); | |
| }); | |
| afterEach(() => { | |
| vi.restoreAllMocks(); | |
| vi.useRealTimers(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Js/copy-paths-button.test.js` around lines 43 - 50, Pin document focus
and visibility in these DOM-timer tests by stubbing document.hasFocus and
overriding document.hidden in the test lifecycle: in the beforeEach that calls
vi.useFakeTimers() and sets component = attach(bulkScope(), 'bulk'), add a
vi.spyOn(document, 'hasFocus').mockReturnValue(true) and
Object.defineProperty(document, 'hidden', { value: false, configurable: true })
to force focused/visible state; in the afterEach that calls vi.useRealTimers(),
restore the hasFocus spy (or call vi.restoreAllMocks()) and revert
document.hidden (either by deleting the configurable property or restoring the
saved original value) so subsequent tests aren’t affected.
Fixes: - Menu items now read 'Copy file name' / 'Copy relative path' / 'Copy full path' in single mode and 'Copy N file names' / 'Copy N relative paths' / 'Copy N full paths' in bulk, matching the existing primaryLabel pattern. Implemented as a single _label(noun) helper exposed via three getters (nameLabel/relativeLabel/fullLabel) bound to the menu items via x-text. Updated browser/screenshot tests' name= filters and added JS unit tests covering both modes. Reviewer follow-ups: - Keyboard parity: Shift+F10 and the ContextMenu key now open the menu alongside right-click. Tooltip body updated. - publish-screenshots.sh: switched the screenshot iteration loop from ls + word-splitting to a null-delimited find loop (shellcheck SC2012).
`clicking line with existing draft re-opens it` was reading inputValue() in the same statement that triggered the form remount, racing the Livewire round-trip that hydrates `formBody`. The Pest browser wrapper's waitForFunction is single-shot (see Csrf419RecoveryTest), so use the project pattern: poll the locator's inputValue() from PHP until it populates, with a 5s ceiling. Confirmed stable under x5 local re-runs.
Previous attempt polled inputValue() but the textarea opened from a
fresh form (formBody='') because the line-mousedown handler reads
\$wire.fileComments *synchronously* to find the existing draft. The
draft-comment testid renders after the server morph but the wire-property
sync to the client-side \$wire proxy can race the next click on a loaded
CI shard. waitForEvent('networkidle') after the badge appears covers
both halves of the round-trip end-to-end. 3/3 local re-runs.
fgilio
added a commit
that referenced
this pull request
May 6, 2026
Sibling symlink convention enforced by the agents-symlinks pre-commit hook; missed when tests/Screenshots/CLAUDE.md landed in #87. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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
Consolidates copy-paths functionality into a single reusable Alpine component that supports both bulk (multi-file) and single-file modes, with improved gesture handling including left-click and long-press interactions.
Key Changes
New Alpine component (
copy-paths-button.js): Unified factory supporting two modes:bulkmode: copies filtered visible files from parent scopesinglemode: copies a fixed path passed as parameterGesture handling:
New Blade component (
copy-paths-button.blade.php): Replacescopy-paths-menu.blade.phpwith support for both modes and improved accessibilityRemoved legacy component:
copy-paths-menu.blade.phpno longer neededRemoved inline logic: Deleted
copyVisibleFilePaths()method from review-page Alpine root, now handled by the componentUpdated consumers:
Comprehensive test suite (
copy-paths-button.test.js): 151 lines covering:Implementation Details
<flux:dropdown>wrapping only the trigger button, allowing menu items to work normally_popoverable.setState()API for opening the dropdown menushowPopover()API if Flux integration unavailablehttps://claude.ai/code/session_01SSdfH6M5HLtWjeLJTDa3Ck
Summary by CodeRabbit
New Features
Tests
Chores / CI
Documentation