fix: remove Review plus button - #88
Closed
Astro-Han wants to merge 1 commit into
Closed
Conversation
Astro-Han
added
bug
Contributor
📝 WalkthroughWalkthroughA new helper function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces logic to conditionally hide the file-open button in the session side panel when the 'review' tab is active, supported by new unit tests. The implementation refactors the button into a reusable helper function to reduce duplication. Feedback suggests refactoring this helper function into a standard SolidJS component to better align with idiomatic practices and maintain consistency with the rest of the codebase.
Comment on lines
+195
to
+214
| const fileOpenButton = (className: string) => ( | ||
| <Show when={shouldShowReviewFileOpenButton(activeTab())}> | ||
| <div class={className}> | ||
| <TooltipKeybind | ||
| title={language.t("command.file.open")} | ||
| keybind={command.keybind("file.open")} | ||
| class="flex items-center" | ||
| > | ||
| <IconButton | ||
| icon="plus-small" | ||
| variant="ghost" | ||
| iconSize="large" | ||
| class="!rounded-md" | ||
| onClick={() => openFilePicker(showAllFiles)} | ||
| aria-label={language.t("command.file.open")} | ||
| /> | ||
| </TooltipKeybind> | ||
| </div> | ||
| </Show> | ||
| ) |
There was a problem hiding this comment.
In SolidJS, it is more idiomatic to define reusable UI pieces as components (capitalized) rather than functions that return JSX. This improves readability and follows the project's convention (e.g., RightPanelShellIcon). Additionally, using the class prop name is more consistent with SolidJS than className.
Suggested change
| const fileOpenButton = (className: string) => ( | |
| <Show when={shouldShowReviewFileOpenButton(activeTab())}> | |
| <div class={className}> | |
| <TooltipKeybind | |
| title={language.t("command.file.open")} | |
| keybind={command.keybind("file.open")} | |
| class="flex items-center" | |
| > | |
| <IconButton | |
| icon="plus-small" | |
| variant="ghost" | |
| iconSize="large" | |
| class="!rounded-md" | |
| onClick={() => openFilePicker(showAllFiles)} | |
| aria-label={language.t("command.file.open")} | |
| /> | |
| </TooltipKeybind> | |
| </div> | |
| </Show> | |
| ) | |
| const FileOpenButton = (props: { class: string }) => ( | |
| <Show when={shouldShowReviewFileOpenButton(activeTab())}> | |
| <div class={props.class}> | |
| <TooltipKeybind | |
| title={language.t("command.file.open")} | |
| keybind={command.keybind("file.open")} | |
| class="flex items-center" | |
| > | |
| <IconButton | |
| icon="plus-small" | |
| variant="ghost" | |
| iconSize="large" | |
| class="!rounded-md" | |
| onClick={() => openFilePicker(showAllFiles)} | |
| aria-label={language.t("command.file.open")} | |
| /> | |
| </TooltipKeybind> | |
| </div> | |
| </Show> | |
| ) |
| /> | ||
| </TooltipKeybind> | ||
| </div> | ||
| fileOpenButton("w-full bg-background-stronger flex items-center justify-end px-3 py-1.5") |
There was a problem hiding this comment.
Use the refactored FileOpenButton component here.
Suggested change
| fileOpenButton("w-full bg-background-stronger flex items-center justify-end px-3 py-1.5") | |
| <FileOpenButton class="w-full bg-background-stronger flex items-center justify-end px-3 py-1.5" /> |
Comment on lines
+370
to
+372
| {fileOpenButton( | ||
| "bg-background-stronger h-full shrink-0 sticky right-0 z-10 flex items-center justify-center pr-3", | ||
| )} |
There was a problem hiding this comment.
Use the refactored FileOpenButton component here.
Suggested change
| {fileOpenButton( | |
| "bg-background-stronger h-full shrink-0 sticky right-0 z-10 flex items-center justify-center pr-3", | |
| )} | |
| <FileOpenButton | |
| class="bg-background-stronger h-full shrink-0 sticky right-0 z-10 flex items-center justify-center pr-3" | |
| /> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app/src/pages/session/session-side-panel.test.tsx (1)
148-162: Add anundefinedtest case to match the helper contract.
shouldShowReviewFileOpenButtonacceptsstring | undefined, but tests currently cover only string inputs. Addingundefinedkeeps the public helper contract explicit.Proposed test addition
describe("shouldShowReviewFileOpenButton", () => { test("hides the file-open button while the Review tab is active", async () => { const { shouldShowReviewFileOpenButton } = await import("./session-side-panel") expect(shouldShowReviewFileOpenButton("review")).toBe(false) }) test("keeps the file-open button for non-review tabs in the same panel", async () => { const { shouldShowReviewFileOpenButton } = await import("./session-side-panel") expect(shouldShowReviewFileOpenButton("empty")).toBe(true) expect(shouldShowReviewFileOpenButton("context")).toBe(true) expect(shouldShowReviewFileOpenButton("file:///tmp/example.ts")).toBe(true) + expect(shouldShowReviewFileOpenButton(undefined)).toBe(true) }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/pages/session/session-side-panel.test.tsx` around lines 148 - 162, Tests for shouldShowReviewFileOpenButton only cover string inputs but the function signature accepts string | undefined; add a test case calling shouldShowReviewFileOpenButton(undefined) and assert the expected boolean (likely true or false per helper contract) to make the test suite match the public contract. Locate the helper import in session-side-panel.test.tsx (the existing tests that import shouldShowReviewFileOpenButton) and add a third test assertion that passes undefined and checks the correct return value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/app/src/pages/session/session-side-panel.test.tsx`:
- Around line 148-162: Tests for shouldShowReviewFileOpenButton only cover
string inputs but the function signature accepts string | undefined; add a test
case calling shouldShowReviewFileOpenButton(undefined) and assert the expected
boolean (likely true or false per helper contract) to make the test suite match
the public contract. Locate the helper import in session-side-panel.test.tsx
(the existing tests that import shouldShowReviewFileOpenButton) and add a third
test assertion that passes undefined and checks the correct return value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e5f9bb2d-97e5-4fa6-9c06-09bc8e721c1f
📒 Files selected for processing (2)
packages/app/src/pages/session/session-side-panel.test.tsxpackages/app/src/pages/session/session-side-panel.tsx
Owner
Author
|
Closing because #85 was updated with additional required changes. I will open a fresh PR covering the updated scope. |
13 tasks
Astro-Han
added a commit
that referenced
this pull request
Jul 27, 2026
Root cause: - The repository pinned vulnerable parser, site, updater, packaging, router, CSS-processing, glob-expansion, and native image-processing dependencies. - The remaining brace-expansion advisory had no compatible backport for the older minimatch releases retained by lint and packaging tooling. Change boundary: - Upgrade pypdf and Astro to patched releases. - Override vulnerable site and root transitive dependencies to patched versions. - Upgrade electron-updater and the Electron Builder family together, including the explicit Windows Squirrel peer. - Resolve every brace-expansion chain to 5.0.8 and patch only its CommonJS export shape so minimatch 3, 5, and 9 remain callable while minimatch 10 retains the named expand export. - Add a compatibility regression test covering every installed minimatch release. Verification: - Root and site Bun audits report zero high or critical findings. - Frozen installs, site production build, desktop typecheck, root lint, desktop production build, and a real macOS Electron startup passed. - PDF parser smoke passed with pypdf and pdfplumber. - Targeted desktop updater and packaging tests passed. - Full PR CI passed across macOS, Windows, E2E, CodeQL, dependency review, and CodeRabbit. Review follow-ups and residual risk: - No unresolved review threads remain. - sharp 0.35.3 is intentionally forced above Astro 6.4.6's optional range. The site does not use astro:assets and its production build passes; revisit this constraint if the image pipeline is adopted. - Full signed installer production remains release-pipeline coverage. Related work: - No product issue. This work was driven by Dependabot alerts #88, #87, and #46 plus the repository-wide Bun audit.
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
1 participant
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
Remove the standalone file-open plus button while the Review tab is active.
Why
The Review Git changes surface should only show actions that are meaningful for the review workflow. The extra plus button could be clicked by mistake and had no clear purpose in that state.
Related Issue
Fixes #85
How To Verify
Fresh-eyes review was run twice. The final review reported no Critical, Important, or Minor issues.
Screenshots or Recordings
Not attached. This environment did not run a manual desktop UI capture.
Checklist
devbranchSummary by CodeRabbit
Bug Fixes
Refactor
Tests