chore: allow floating sidebar to be attached#133
chore: allow floating sidebar to be attached#133iamEvanYT merged 2 commits intoMultiboxLabs:mainfrom
Conversation
|
Warning Rate limit exceeded@iamEvanYT has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 59 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 (2)
WalkthroughThe changes extend the sidebar component hierarchy to propagate the Changes
Suggested labels
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/renderer/src/components/browser-ui/sidebar/header/action-buttons.tsx (1)
163-163: Consider improving icon selection logic for better UX clarity.The current icon selection logic may confuse users:
- In floating mode when open, it shows
SidebarOpenIconbut clicking switches to sidebar variant (doesn't close)- In all other cases, it shows
SidebarCloseIconConsider making the icon represent the action that will be performed rather than the current state for better UX predictability.
For example:
-const SidebarIcon = variant === 'floating' && open ? SidebarOpenIcon : SidebarCloseIcon +const SidebarIcon = variant === 'floating' ? SidebarCloseIcon : (open ? SidebarCloseIcon : SidebarOpenIcon)Also applies to: 171-171
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/renderer/src/components/browser-ui/browser-sidebar.tsx(5 hunks)src/renderer/src/components/browser-ui/sidebar/header/action-buttons.tsx(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/renderer/src/components/browser-ui/sidebar/header/action-buttons.tsx (1)
src/renderer/src/components/browser-ui/main.tsx (1)
SidebarVariant(21-21)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04-arm)
🔇 Additional comments (7)
src/renderer/src/components/browser-ui/sidebar/header/action-buttons.tsx (4)
19-19: LGTM: Import addition is correct.The import of
SidebarVarianttype is properly added and will be used in the component props.
98-98: LGTM: Component signature updated correctly.The
NavigationControlscomponent now properly accepts thevariantandsetVariantprops needed to implement variant-aware behavior.
129-143: LGTM: Conditional rendering logic is correct.The updated logic correctly shows the open button only when the sidebar is closed AND the variant is 'sidebar'. This makes sense as floating sidebars should be controlled differently.
146-151: LGTM: Core bug fix implemented correctly.This is the key fix for the reported issue. The logic correctly handles both scenarios:
- When variant is 'sidebar': closes the sidebar normally
- When variant is 'floating': switches back to 'sidebar' variant (enabling reattachment)
This directly addresses the bug where floating sidebars couldn't be reverted without shortcut keys.
src/renderer/src/components/browser-ui/browser-sidebar.tsx (3)
120-120: LGTM: SidebarHeaderContent correctly updated for prop passing.The component signature and prop forwarding to
NavigationControlsare implemented correctly, enabling the variant state to reach the component that needs it.Also applies to: 124-124
140-142: LGTM: SidebarContent signature updated correctly.The component now properly accepts the
setVariantprop and includes it in the type definition. The prop drilling pattern is correctly implemented.Also applies to: 152-152
163-163: LGTM: Props correctly passed through component hierarchy.The
variantandsetVariantprops are properly passed down throughSidebarHeaderContentandSidebarContentto eventually reach theNavigationControlscomponent where they're needed.Also applies to: 258-258
iamEvanYT
force-pushed
the
fix/attach-floating-sidebar
branch
from
e1c0624 to
d62a6ce
Compare
iamEvanYT
force-pushed
the
fix/attach-floating-sidebar
branch
from
c2d2675 to
2f66f0b
Compare
2 participants
closes #86
Targets #86 with floating sidebar bug impossible to set back to normal sidebar mode without using shortcut keys
EDIT: also I was thinking about adding sth small like
Zustandto keep thinkgs likevariantandsetVariantin small contexts, make them more accessible and prevent props drillingSummary by CodeRabbit