feat: add archive on fork option with keyboard navigation (ENG-1937) #576
Conversation
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
…NG-1937) - Add checkbox to automatically archive source session after fork - Store preference in localStorage (defaults to checked) - Execute archive after successful fork with toast notifications - Add tab/shift-tab navigation between message list and checkbox - Make keyboard shortcuts context-aware based on focused section - Add visual feedback for focused sections
- Replace ring-based focus with background color change (bg-accent/10) - Match focus pattern used by message items for consistency - Suppress checkbox's own focus ring when section is focused - Ensure stable padding without negative margins
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to f68da4d in 1 minute and 10 seconds. Click for details.
- Reviewed
400lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. humanlayer-wui/src/components/internal/SessionDetail/SessionDetail.tsx:207
- Draft comment:
Good use of lazy initialization for archiveOnFork using getArchiveOnForkPreference. Ensure this aligns with session persistence needs. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. humanlayer-wui/src/components/internal/SessionDetail/SessionDetail.tsx:1042
- Draft comment:
Passing onArchiveOnForkChange to ForkViewModal enables two-way sync of the preference. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:80
- Draft comment:
Local archive checkbox state is correctly initialized. Consider using a ref for the checkbox element instead of querying by id for more robust focus management. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:200
- Draft comment:
Tab and Shift+Tab handlers for switching focus between messages and checkbox work well; consider adding appropriate ARIA roles/attributes for improved accessibility. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. humanlayer-wui/src/components/internal/SessionDetail/hooks/useSessionActions.ts:145
- Draft comment:
Archive action post-fork is non-blocking; ensure that a failed archive doesn’t affect fork completion. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. humanlayer-wui/src/lib/preferences.ts:7
- Draft comment:
The comparison 'stored !== "false"' defaults to true; consider explicit parsing for robustness if unexpected values are stored. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_o5IXGVndGqIkRgt1
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Contributor
Author
|
updating with some UI nits |
Contributor
Author
|
ui nits fixed and much more tasteful |
- Add compact styling with border to message list section - Add visual focus indicators for all interactive sections - Add Fork Session and Cancel buttons in DialogFooter - Implement Cmd/Ctrl+Enter hotkey for fork action - Fix Enter key to only select items, not trigger fork - Fix Tab/Shift+Tab navigation between message list, checkbox, and buttons - Prevent keyboard events from bubbling to background session view - Add proper event isolation to prevent "accept edits" mode trigger The modal now follows standard UI patterns with proper button placement, clear focus indicators, and predictable keyboard behavior.
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 4fbbd51 in 2 minutes and 21 seconds. Click for details.
- Reviewed
426lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:217
- Draft comment:
Focus management for the Tab key is handled in both useHotkeys and individual onKeyDown handlers on elements. Consider consolidating this logic to reduce duplication and potential conflicts. - Reason this comment was not posted:
Comment was on unchanged code.
2. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:177
- Draft comment:
The Enter key now only selects a message rather than immediately forking the session. Since fork execution now requires Cmd/Ctrl+Enter, please verify that this UX change is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:487
- Draft comment:
Directly accessing navigator.platform may cause issues in SSR setups. Consider adding a check to ensure navigator is defined before calling toLowerCase, or move this logic into a useEffect that runs client-side. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a React component in a web UI project. The code change is adding keyboard shortcut hints. While the SSR concern is technically valid, this is a client-side UI component that will only be rendered in the browser. The navigator check would add complexity without providing real value. The component already has many client-side only features like focus management, keyboard handlers, etc. The comment raises a valid technical point about SSR safety. There could be edge cases where this causes hydration mismatches. Given this is clearly a client-side only component (with keyboard handlers, focus management, etc.), the SSR safety check would be unnecessary defensive programming that adds complexity without benefit. Delete the comment. While technically correct, the SSR safety check is unnecessary in this clearly client-side UI component and would just add complexity.
4. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:469
- Draft comment:
The onKeyDown handler for the fork button prevents Tab navigation (except for Shift+Tab to move back). Confirm that trapping focus on the fork button is the intended behavior and that users won’t be inadvertently trapped in the button group. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_bqXZnOKAlvPo59ak
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| setFocusedSection('checkbox') | ||
| // Focus the checkbox element | ||
| setTimeout(() => { | ||
| const checkboxElement = document.getElementById('archive-on-fork') |
Contributor
There was a problem hiding this comment.
Consider using React refs for focus management (e.g. for the archive checkbox and fork button) instead of document.getElementById. Using refs would be more in line with React best practices and avoid potential issues with duplicate IDs.
- Add Enter key support for Fork Session button when focused - Add Enter key support for Cancel button when focused - Skip Cancel button in tab navigation (tabIndex=-1) - Direct tab navigation from checkbox to Fork Session button - Prevent all keyboard events from propagating to background
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 002a6d4 in 1 minute and 9 seconds. Click for details.
- Reviewed
61lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:232
- Draft comment:
Ensure that skipping the Cancel button in the tab navigation (by directly focusing the fork button) is an intentional design choice and is well-documented for accessibility. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:430
- Draft comment:
The updated comment clarifies that Tab now goes directly to the fork button (skipping Cancel). Validate that this behavior is consistent across the modal’s focus flow. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:459
- Draft comment:
Removing the Cancel button from the tab order (tabIndex = -1) is nonstandard. Confirm that keyboard users can still easily access cancel actions, for example via the Escape key. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:476
- Draft comment:
The fork button's onKeyDown handler handles Enter and Tab/Shift+Tab appropriately. Consider if allowing the Tab key to cycle focus (rather than doing nothing on Tab) might improve usability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_xlaodWfOQl4hCMmn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
…s mode - Use scope stealing pattern (like rename session) to fully isolate modal hotkeys - Add explicit true parameter to useStealHotkeyScope to ensure proper isolation - Restore cyclic tab navigation between messages, checkbox, and fork button - Add shift+tab capture to SessionLauncher to prevent bubbling - Ensures complete hotkey isolation when modals are open
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 472738d in 1 minute and 49 seconds. Click for details.
- Reviewed
239lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. humanlayer-wui/src/components/SessionLauncher.tsx:65
- Draft comment:
The new shift+Tab hotkey handler repeats the pattern of casting the event to 'any' and calling nativeEvent.stopImmediatePropagation. Consider extracting a utility/helper for this behavior to improve type safety and reduce repetition. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment suggests extracting a pattern, but the pattern only appears once in the new code. Creating a utility for a single use would actually increase complexity. The type casting to 'any' might be unavoidable due to the library's typing. A utility function wouldn't necessarily improve type safety since we'd still need the cast somewhere. I might be missing other files where this pattern is used. The type safety concern could be valid even if it's not repeated. Even if the pattern exists elsewhere, we should follow the rule to ignore cross-file issues. The type safety concern alone doesn't justify the refactor for a single use. The comment should be deleted as it suggests a refactor that would add complexity without clear benefit, based on a pattern that only appears once in this file.
2. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:232
- Draft comment:
The Tab navigation handler now directly calls element.focus() (e.g. focusing the 'archive-on-fork' checkbox) without using setTimeout. This approach can be fragile if the DOM update isn’t fully committed. Please verify that the target elements are reliably available when focus() is invoked. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:424
- Draft comment:
The Cancel button has been removed from the modal footer. Ensure that users who prefer mouse interactions have an accessible way to cancel the operation, rather than relying solely on the Escape key. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The Dialog component from @/components/ui/dialog likely provides standard modal behaviors including a close button in the top-right and click-outside-to-close. The Escape key functionality is preserved. The UI clearly shows "Esc Cancel" in the shortcuts list. Mouse users can still close the modal by clicking outside or the X button. The removal of the Cancel button actually simplifies the UI without reducing functionality. I could be wrong about the Dialog component's built-in close button - I'm assuming it exists but don't see it explicitly in the code. Some users might expect a Cancel button in the footer as it's a common pattern. Even if I'm wrong about the close button, clicking outside to close is a standard modal pattern, and the Escape key is clearly documented in the UI. The comment speculates about an issue without strong evidence that users are actually struggling. The comment should be deleted. While the Cancel button was removed, there are still multiple intuitive ways to cancel/close the modal for both keyboard and mouse users, and no evidence has been provided that this creates a real accessibility or usability problem.
Workflow ID: wflow_LqHQYcvzmvUmwNVk
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
- Move useStealHotkeyScope from inner ForkViewModalContent to outer ForkViewModal - Ensures scope stealing happens immediately when modal opens, before Dialog renders - Prevents shift+tab from reaching SessionDetail during the render gap - Fixes issue where accept edits was still being cycled by shift+tab
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 4bce9a5 in 46 seconds. Click for details.
- Reviewed
29lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:40
- Draft comment:
Removed redundant useStealHotkeyScope call. Confirm that the parent’s hook setup fully covers hotkey scope stealing before the dialog renders. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:467
- Draft comment:
Good addition of immediate scope stealing in the ForkViewModal parent. Verify that useStealHotkeyScope responds correctly to dynamic changes in the isOpen prop. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_rRHGgIn8QIMmyKHC
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
dexhorthy
reviewed
Sep 18, 2025
| // Steal hotkey scope when this component mounts | ||
| useStealHotkeyScope(ForkViewModalHotkeysScope) | ||
| // Note: Scope stealing is handled in parent ForkViewModal component to ensure | ||
| // it happens BEFORE the dialog renders, preventing any timing gaps |
Contributor
There was a problem hiding this comment.
Claude finished @K-Mistele's task —— View job
You're welcome, @K-Mistele!
I'm glad I could help with the implementation of the archive-on-fork feature and keyboard navigation improvements. The PR looks well-structured with good separation of concerns and proper error handling.
dexhorthy
reviewed
Sep 18, 2025
| } | ||
| }, | ||
| { scopes: [ForkViewModalHotkeysScope], enableOnFormTags: true }, | ||
| ) |
Contributor
There was a problem hiding this comment.
im surprised we don't get this out of the box but ok
Contributor
Author
There was a problem hiding this comment.
we should have but something was just-so-slightly borked where it took two tabs to jump from the checkbox to the fork button and you couldn't tell what was being focused in the meantime, so had claude extract it. same with shift+tab
spent way too much time trying to debug this and figure out why it wasn't behaving like the "create session" box
dexhorthy
approved these changes
Sep 18, 2025
… management - Replace document.getElementById with proper React refs (checkboxRef, forkButtonRef) - Fix TypeScript types - Radix Checkbox renders as HTMLButtonElement not HTMLInputElement - Address PR feedback from ellipsis-dev bot about using idiomatic React patterns - Apply prettier formatting to maintain code style consistency
Contributor
Author
✅ All UI issues resolvedFixed in recent commits:
Testing performed:
The fork modal now has robust keyboard navigation with complete hotkey isolation. Ready for final review! |
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a49658d in 1 minute and 47 seconds. Click for details.
- Reviewed
62lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:46
- Draft comment:
Ensure that the Checkbox and Button components forward refs properly so that these useRef instances are set. This is critical for the new focus management. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:233
- Draft comment:
Good refactor: replacing document.getElementById with ref-based focus is more React-friendly. Consider a fallback or logging if the ref is unexpectedly null. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:259
- Draft comment:
Consistent update: using forkButtonRef and checkboxRef for backward navigation improves maintainability. Verify that these elements are always mounted when navigating. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:398
- Draft comment:
Attaching ref to the Checkbox component is a neat improvement. Please ensure that the Checkbox component uses React.forwardRef so that the ref is forwarded to the underlying DOM element. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. humanlayer-wui/src/components/internal/SessionDetail/components/ForkViewModal.tsx:420
- Draft comment:
Assigning ref to the Button component aids keyboard navigation. Confirm that the Button component forwards refs correctly (e.g., via React.forwardRef). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_3C5f7eEwyX9smDKs
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Contributor
Author
|
lol is there a way to have the |
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.
What problem(s) was I solving?
Users frequently want to archive the source session after creating a fork, but currently have to:
Additionally, the fork modal lacked keyboard-only navigation for the archive checkbox, making it inaccessible to keyboard-only users.
Related issue:
What user-facing changes did I ship?
Archive on Fork Feature
Enhanced Keyboard Navigation
bg-accent/10)How I implemented it
Preference Management (
lib/preferences.ts)Created utility functions for managing user preferences:
getArchiveOnForkPreference(): Returns boolean (defaults to true)setArchiveOnForkPreference(): Persists to localStorageFork Modal UI Updates (
components/ForkViewModal.tsx)'messages' | 'checkbox')bg-accent/10matching message item selectionArchive Logic (
hooks/useSessionActions.ts)archiveOnForkparameter to hooktoast.loading()during operationSession Detail Integration
How to verify it
make check testpasses for humanlayer-wuiManual Testing
Fork with Archive:
Keyboard Navigation:
Preference Persistence:
Error Handling:
Focus Indication:
Description for the changelog
Added automatic archive option when forking sessions with persistent user preference. Enhanced fork modal with full keyboard navigation using Tab/Shift+Tab to switch between sections. Prevents layout shifts with consistent focus indication matching existing UI patterns.
Cute animal picture
Important
Adds automatic session archiving after forking with persistent preferences and enhanced keyboard navigation in the fork modal.
ForkViewModal.localStorage, defaulting to checked for new users.ForkViewModal.getArchiveOnForkPreference()andsetArchiveOnForkPreference()inpreferences.tsfor managing preferences.ForkViewModal.tsx: UI updates for checkbox and keyboard navigation.useSessionActions.ts: AddedarchiveOnForkparameter to handle archiving logic post-fork.This description was created by
for a49658d. You can customize this summary. It will automatically update as commits are pushed.