-
Notifications
You must be signed in to change notification settings - Fork 11
🤖 Review: Move Files Changed to top and remove prefix subtraction #382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6d8ad16 to
9299a76
Compare
- Position file tree on top/left instead of bottom/right - Remove common prefix extraction logic - Show full directory structure without automatic prefix subtraction - Users can now collapse/expand the entire tree hierarchy In wide layout, file tree now appears on the left side. In narrow layout, file tree appears at the top. Changed border from border-left to border-right to match new layout. Net -43 LoC (55 deletions, 12 insertions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| `; | ||
|
|
||
| const HunkList = styled.div` | ||
| flex: 1; | ||
| min-height: 0; | ||
| overflow-y: auto; | ||
| padding: 12px; | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 12px; | ||
| `; | ||
|
|
||
| const FileTreeSection = styled.div` | ||
| /* Default: Wide layout - fixed width on right side */ | ||
| width: 300px; | ||
| flex-shrink: 0; | ||
| border-left: 1px solid #3e3e42; | ||
| /* Part of scrollable content, not sticky */ | ||
| width: 100%; | ||
| flex: 0 0 auto; /* Size based on content */ | ||
| display: flex; | ||
| flex-direction: column; | ||
| overflow: hidden; | ||
| min-height: 0; | ||
| order: 2; /* Come after HunksSection in wide mode */ | ||
| /* Narrow layout: full width, grow to fit full tree, above hunks */ | ||
| @container review-panel (max-width: 800px) { | ||
| width: 100%; | ||
| height: auto; /* Let it grow to show full tree */ | ||
| flex: 0 0 auto; /* Fixed size based on content */ | ||
| border-left: none; | ||
| border-bottom: 1px solid #3e3e42; | ||
| order: 0; /* Come before HunksSection in narrow mode */ | ||
| } | ||
| border-bottom: 1px solid #3e3e42; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep file tree as a left sidebar on wide layouts
The new styling sets ContentContainer to always use flex-direction: column and gives FileTreeSection 100% width with only a bottom border, removing the previous container query and fixed width that placed the file tree in a separate column. As a result, even on large viewports the file tree stacks above the hunks and scrolls away with them, so users lose the persistent navigation pane that existed before and that the change description says should now be on the left. This is a regression in wide layouts because the file navigator is no longer visible while reviewing hunks.
Useful? React with 👍 / 👎.
The original PR #382 was accidentally reverted by commit 00e5b17 during the Tailwind migration. This commit re-applies those changes to the Tailwind-based codebase. Changes: - Search bar moved outside scrollable area (always visible at top) - Created single scrollable container for file tree + hunks - File tree now at top in all viewports (not side-by-side) - File tree no longer sticky - scrolls away naturally with content - Removed common prefix extraction (already done, maintained here) Layout: - Search bar: Fixed at top, not sticky - ScrollableContent: Single overflow-y-auto container - FileTree: First child, sizes to content (flex-[0_0_auto]) - Hunks: Below file tree, sizes to content Fixes: File tree remaining visible when scrolling through hunks
The original PR #382 was accidentally lost during the Tailwind v4 migration (commit 7e7d5c71b79). This commit re-applies those changes to the current Tailwind-based codebase. Changes: - Search bar moved outside scrollable area (always visible at top) - Created single scrollable container for file tree + hunks - File tree now at top in all viewports (not side-by-side) - File tree no longer sticky - scrolls away naturally with content Layout: - Search bar: Fixed at top - ScrollableContent: Single overflow-y-auto container - FileTree: First child, sizes to content (flex-[0_0_auto]) - Hunks: Below file tree, sizes to content Fixes: File tree remaining visible when scrolling through hunks
…402) ## Problem PR #382 was merged but was accidentally reverted by commit `00e5b179` (🤖 Restore ReviewPanel and FileTree from good state) during the Tailwind migration. This PR re-applies those changes to the current Tailwind-based codebase. ## Changes ### Layout Restructuring - **Search bar moved outside scrollable area** - Always visible at top, not part of the scrolling content - **Single scrollable container** - Created one `overflow-y-auto` div containing both file tree and hunks - **File tree on top in all viewports** - No longer side-by-side on wide screens - **No sticky behavior** - File tree scrolls away naturally with content ### Technical Implementation - Search bar: Fixed position at top with `border-b` - ScrollableContent: `flex min-h-0 flex-1 flex-col overflow-y-auto` - FileTree: First child with `flex-[0_0_auto]` (sizes to content, doesn't grow) - Hunks: Below file tree with `flex-[0_0_auto]` (sizes to content) ## Before vs After **Before (reverted state):** - File tree in sidebar (wide) or top section (narrow) - Two separate scrollable areas (tree + hunks) - File tree stayed visible when scrolling hunks **After (this PR):** - File tree always at top, full width - Single scrollable area containing both - File tree scrolls away naturally ## Related - Original PR: #382 - Reverted by: `00e5b179` (Tailwind migration cleanup) - Maintained: Common prefix extraction removal (already absent in current code) _Generated with `cmux`_
Repositions the file tree and simplifies the directory structure display.
Changes
Layout Changes
order: 2toorder: 1to ensure Files Changed is always firstTechnical Details
Removed:
extractCommonPrefixlogic from ReviewPanelcommonPrefixstate and propCommonPrefixstyled componentNet -43 LoC (55 deletions, 12 insertions)
Generated with
cmux