Repository navigation
feat(ui): consolidate toggles into tools popover - #98
Conversation
Move web search, write mode, page tree context, and MCP toggles from separate buttons into a single "Tools" popover in the floating input footer. This provides a cleaner UI while keeping all toggle options easily accessible. - Add ToolsPopover component with all toggle options - Update InputFooter to use ToolsPopover instead of individual buttons - Add MCP toggle support to ChatInput and ChatLayout - Remove MCPToggle from GlobalAssistantView and AiChatView headers
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds MCP-related props (mcpEnabled, onMcpToggle, mcpRunningServers, showMcp) and forwards them through chat layout/input components, removes header MCPToggle UI, introduces a new ToolsPopover consolidating tool toggles (web search, write mode, page-tree, MCP), updates InputFooter to use ToolsPopover, and simplifies InputActions by removing the variant prop. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Comment |
![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 (2)
apps/web/src/components/ui/floating-input/ToolsPopover.tsx (2)
66-71: Consider excluding default states from active count.The
activeCountincludeswriteModewhich defaults totrue. This means the badge will typically show "1" even when the user hasn't actively enabled any special tools. Consider whether the badge should only reflect non-default/special states (like web search, page tree context, and MCP when enabled).🔎 Alternative approach - exclude default write mode:
// Count active tools for badge const activeCount = [ webSearchEnabled, - writeMode, showPageTree, showMcp && mcpEnabled, ].filter(Boolean).length;This would make the badge only appear when special tools are explicitly enabled, keeping the UI cleaner in the default state.
107-195: LGTM with minor API design note.The toggle implementations are well-structured with good visual feedback. The use of Switch components with the existing toggle callbacks works correctly.
Note: There's a minor API inconsistency where
webSearchEnabled,writeMode, andshowPageTreetoggles use callbacks with no parameters (() => void), while the Switch component'sonCheckedChangeprovides a boolean parameter. The callbacks ignore this parameter, which works but isn't ideal. This is inherited from existing code, not introduced by this PR.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/src/components/ai/chat/input/ChatInput.tsx(3 hunks)apps/web/src/components/ai/chat/layouts/ChatLayout.tsx(3 hunks)apps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsx(2 hunks)apps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsx(2 hunks)apps/web/src/components/ui/floating-input/InputFooter.tsx(5 hunks)apps/web/src/components/ui/floating-input/ToolsPopover.tsx(1 hunks)apps/web/src/components/ui/floating-input/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use
anytypes in TypeScript code - always use proper TypeScript types
**/*.{ts,tsx}: Noanytypes - always use proper TypeScript types
Use kebab-case for filenames (e.g.,image-processor.ts)
Use camelCase for variables and functions
Use UPPER_SNAKE_CASE for constants
Use PascalCase for types and enums
Use centralized permission logic: importgetUserAccessLevelandcanUserEditPagefrom@pagespace/lib/permissions
Use Drizzle client from@pagespace/dbfor all database access
Always structure message content using the message parts structure:{ parts: [{ type: 'text', text: '...' }] }
Use ESM modules and TypeScript strict mode
Files:
apps/web/src/components/ai/chat/layouts/ChatLayout.tsxapps/web/src/components/ui/floating-input/ToolsPopover.tsxapps/web/src/components/ui/floating-input/index.tsapps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsxapps/web/src/components/ai/chat/input/ChatInput.tsxapps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsxapps/web/src/components/ui/floating-input/InputFooter.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/**/*.{ts,tsx}: Use centralized permission functions from@pagespace/lib/permissionsfor access control, such asgetUserAccessLevel()andcanUserEditPage()
Always use Drizzle client from@pagespace/dbfor database access instead of direct database connections
Always use message parts structure withpartsarray containing objects withtypeandtextfields when constructing messages for AI
Files:
apps/web/src/components/ai/chat/layouts/ChatLayout.tsxapps/web/src/components/ui/floating-input/ToolsPopover.tsxapps/web/src/components/ui/floating-input/index.tsapps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsxapps/web/src/components/ai/chat/input/ChatInput.tsxapps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsxapps/web/src/components/ui/floating-input/InputFooter.tsx
apps/web/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/**/*.tsx: For document editing, register editing state usinguseEditingStore.getState().startEditing()andendEditing()to prevent unwanted UI refreshes
For AI streaming operations, register streaming state usinguseEditingStore.getState().startStreaming()andendStreaming()to prevent unwanted UI refreshes
When using SWR, checkuseEditingStorestate withisAnyActive()and setisPausedto prevent data refreshes during editing or streaming
Files:
apps/web/src/components/ai/chat/layouts/ChatLayout.tsxapps/web/src/components/ui/floating-input/ToolsPopover.tsxapps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsxapps/web/src/components/ai/chat/input/ChatInput.tsxapps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsxapps/web/src/components/ui/floating-input/InputFooter.tsx
**/{components,src/**/components}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for React component names and filenames
Files:
apps/web/src/components/ai/chat/layouts/ChatLayout.tsxapps/web/src/components/ui/floating-input/ToolsPopover.tsxapps/web/src/components/ui/floating-input/index.tsapps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsxapps/web/src/components/ai/chat/input/ChatInput.tsxapps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsxapps/web/src/components/ui/floating-input/InputFooter.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier and lint with ESLint using the configuration at
apps/web/eslint.config.mjs
Files:
apps/web/src/components/ai/chat/layouts/ChatLayout.tsxapps/web/src/components/ui/floating-input/ToolsPopover.tsxapps/web/src/components/ui/floating-input/index.tsapps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsxapps/web/src/components/ai/chat/input/ChatInput.tsxapps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsxapps/web/src/components/ui/floating-input/InputFooter.tsx
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/src/**/*.{ts,tsx}: Use Zustand for client-side state management
Use SWR for server state and caching
Files:
apps/web/src/components/ai/chat/layouts/ChatLayout.tsxapps/web/src/components/ui/floating-input/ToolsPopover.tsxapps/web/src/components/ui/floating-input/index.tsapps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsxapps/web/src/components/ai/chat/input/ChatInput.tsxapps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsxapps/web/src/components/ui/floating-input/InputFooter.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-18T05:22:42.263Z
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 96
File: apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarChatTab.tsx:641-657
Timestamp: 2025-12-18T05:22:42.263Z
Learning: In apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarChatTab.tsx, the provider/model selector buttons are intentionally non-functional placeholders in the compact sidebar view. The `hideModelSelector={true}` prop is passed to ChatInput to hide the full ProviderModelSelector. Users are expected to use the full GlobalAssistantView for model selection. A settings link may be added in a future iteration.
Applied to files:
apps/web/src/components/ai/chat/layouts/ChatLayout.tsxapps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsxapps/web/src/components/ai/chat/input/ChatInput.tsxapps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsxapps/web/src/components/ui/floating-input/InputFooter.tsx
🧬 Code graph analysis (2)
apps/web/src/components/ui/floating-input/ToolsPopover.tsx (3)
apps/web/src/components/ui/floating-input/index.ts (2)
ToolsPopoverProps(19-19)ToolsPopover(18-18)apps/web/src/components/ui/button.tsx (1)
Button(59-59)apps/web/src/components/ui/badge.tsx (1)
Badge(46-46)
apps/web/src/components/ui/floating-input/InputFooter.tsx (2)
apps/web/src/components/ui/floating-input/ToolsPopover.tsx (1)
ToolsPopover(51-245)apps/web/src/components/ui/floating-input/index.ts (1)
ToolsPopover(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests
🔇 Additional comments (11)
apps/web/src/components/ui/floating-input/ToolsPopover.tsx (2)
198-240: Excellent UX for MCP toggle.The conditional rendering of the MCP section with
showMcpand the disabled state whenmcpRunningServers === 0provides clear feedback to users. The "No MCP servers running" message is helpful, and the badge showing the count of running servers when enabled is a nice touch.
1-247: Well-implemented consolidation component.The ToolsPopover successfully consolidates the four toggle controls into a single, accessible popover UI. The implementation is clean with:
- Proper TypeScript typing with comprehensive JSDoc
- Consistent visual feedback for active/inactive states
- Appropriate use of conditional rendering
- Good disabled state handling throughout
This achieves the PR's objective of providing a consolidated UI while keeping toggle options accessible.
apps/web/src/components/ui/floating-input/index.ts (1)
17-20: LGTM - Clean export.The export follows the existing pattern in the module and properly exposes both the component and its props type.
apps/web/src/components/ai/chat/layouts/ChatLayout.tsx (2)
73-86: LGTM - Consistent prop additions.The MCP-related props are properly typed with clear documentation and appropriate optional flags. The prop names and types are consistent with the pattern used throughout the PR.
127-130: Correct prop forwarding.The MCP props are properly destructured with sensible defaults and correctly forwarded through the
renderInputcallback, maintaining the data flow pattern.Also applies to: 195-198
apps/web/src/components/ai/chat/input/ChatInput.tsx (2)
34-41: LGTM - Proper prop threading.The MCP-related props are correctly added to the component interface with clear documentation and passed through to InputFooter. The defaults are consistent with the other components in the chain.
Also applies to: 76-79
156-159: Clean prop forwarding to InputFooter.All four MCP-related props are correctly passed to InputFooter, completing the prop flow from parent layouts down to the UI component.
apps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsx (1)
551-554: LGTM - Proper MCP state propagation.The MCP-related state from
useMCPToolsis correctly passed through ChatLayout and into the ChatInput render path. This replaces the removed MCPToggle UI from the header, aligning with the PR's objective to consolidate toggles into the ToolsPopover.Also applies to: 567-570
apps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsx (1)
411-414: LGTM - Consistent MCP integration.The MCP props are properly integrated following the same pattern as GlobalAssistantView. The removal of the MCPToggle from the header and integration through the chat layout completes the consolidation into ToolsPopover.
Also applies to: 427-430
apps/web/src/components/ui/floating-input/InputFooter.tsx (2)
28-35: LGTM - Complete MCP prop integration.The MCP-related props are properly added to InputFooterProps with clear documentation and sensible defaults, completing the prop chain from the view components down to the UI layer.
Also applies to: 71-74
93-107: Excellent consolidation of toggle UI.The replacement of individual toggle buttons with the ToolsPopover component successfully achieves the PR's main objective. All props are correctly forwarded to ToolsPopover, including the new MCP-related props. The updated comment accurately describes the new consolidated structure.
This consolidation improves the UI by:
- Reducing visual clutter with a single "Tools" button
- Maintaining easy access to all toggles via the popover
- Providing a clear active count badge for user awareness
![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.
ℹ️ 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".
| <Switch | ||
| checked={webSearchEnabled} | ||
| onCheckedChange={onWebSearchToggle} | ||
| disabled={disabled} |
There was a problem hiding this comment.
In ToolsPopover, the new toggle rows wire the same handler to both the row <button> (e.g., lines 107–114) and the nested Switch (lines 128–131). A click on the switch bubbles to the parent button because the Radix switch doesn’t stop propagation, so onWebSearchToggle runs twice and the state flips back to its original value. The same pattern is used for write mode, page tree, and MCP, so clicking the switch UI appears to do nothing unless the user clicks outside the switch, breaking the new consolidated tools popover.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This issue has been resolved. The current code uses <div> elements for the toggle rows (not <button>), so there's no event bubbling that would cause double-toggles. Only the <Switch> component's onCheckedChange handler fires when clicked.
- Change toggle row wrappers from <button> to <div> to prevent click events from bubbling and triggering toggle twice - Exclude writeMode from activeCount since it defaults to true, so badge now only shows when non-default tools are enabled Addresses review feedback from Codex (P1 bug) and CodeRabbit (nitpick) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove variant-based styling from InputActions - the send button is now consistently blue (primary) in light mode and muted in dark mode across both main view and sidebar contexts. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/src/components/ui/floating-input/ToolsPopover.tsx (1)
15-40: Inconsistent callback signatures with Switch'sonCheckedChange.The callbacks
onWebSearchToggle,onWriteModeToggle, andonShowPageTreeToggleare typed as() => void, but Radix UI's Switch passes a boolean toonCheckedChange. While this works at runtime (the boolean is ignored), it creates an inconsistent API whereonMcpToggleaccepts the boolean but the others don't.Consider harmonizing the signatures for type safety and API consistency.
🔎 Suggested change
export interface ToolsPopoverProps { /** Whether web search is enabled */ webSearchEnabled?: boolean; /** Callback when web search is toggled */ - onWebSearchToggle?: () => void; + onWebSearchToggle?: (enabled: boolean) => void; /** Whether write mode is active (true = write, false = read only) */ writeMode?: boolean; /** Callback when write mode is toggled */ - onWriteModeToggle?: () => void; + onWriteModeToggle?: (enabled: boolean) => void; /** Whether to show workspace page tree context to AI */ showPageTree?: boolean; /** Callback when page tree context is toggled */ - onShowPageTreeToggle?: () => void; + onShowPageTreeToggle?: (enabled: boolean) => void; /** Whether MCP is enabled for this conversation */ mcpEnabled?: boolean; /** Callback when MCP is toggled */ onMcpToggle?: (enabled: boolean) => void;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/components/ui/floating-input/ToolsPopover.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use
anytypes in TypeScript code - always use proper TypeScript types
**/*.{ts,tsx}: Noanytypes - always use proper TypeScript types
Use kebab-case for filenames (e.g.,image-processor.ts)
Use camelCase for variables and functions
Use UPPER_SNAKE_CASE for constants
Use PascalCase for types and enums
Use centralized permission logic: importgetUserAccessLevelandcanUserEditPagefrom@pagespace/lib/permissions
Use Drizzle client from@pagespace/dbfor all database access
Always structure message content using the message parts structure:{ parts: [{ type: 'text', text: '...' }] }
Use ESM modules and TypeScript strict mode
Files:
apps/web/src/components/ui/floating-input/ToolsPopover.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/**/*.{ts,tsx}: Use centralized permission functions from@pagespace/lib/permissionsfor access control, such asgetUserAccessLevel()andcanUserEditPage()
Always use Drizzle client from@pagespace/dbfor database access instead of direct database connections
Always use message parts structure withpartsarray containing objects withtypeandtextfields when constructing messages for AI
Files:
apps/web/src/components/ui/floating-input/ToolsPopover.tsx
apps/web/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/**/*.tsx: For document editing, register editing state usinguseEditingStore.getState().startEditing()andendEditing()to prevent unwanted UI refreshes
For AI streaming operations, register streaming state usinguseEditingStore.getState().startStreaming()andendStreaming()to prevent unwanted UI refreshes
When using SWR, checkuseEditingStorestate withisAnyActive()and setisPausedto prevent data refreshes during editing or streaming
Files:
apps/web/src/components/ui/floating-input/ToolsPopover.tsx
**/{components,src/**/components}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for React component names and filenames
Files:
apps/web/src/components/ui/floating-input/ToolsPopover.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier and lint with ESLint using the configuration at
apps/web/eslint.config.mjs
Files:
apps/web/src/components/ui/floating-input/ToolsPopover.tsx
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/src/**/*.{ts,tsx}: Use Zustand for client-side state management
Use SWR for server state and caching
Files:
apps/web/src/components/ui/floating-input/ToolsPopover.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests
🔇 Additional comments (5)
apps/web/src/components/ui/floating-input/ToolsPopover.tsx (5)
1-14: LGTM!Imports are well-organized and appropriate for the component's functionality.
51-70: LGTM!Props destructuring with sensible defaults and the active count logic is well-documented and correctly excludes the default-true
writeMode.
72-98: LGTM!The trigger button implementation is clean with appropriate conditional badge rendering and accessible labeling.
104-188: Toggle rows are implemented correctly.The past review concern about double-toggle doesn't apply here since the rows use
<div>elements without click handlers, and only theSwitchcomponents handle the toggle viaonCheckedChange. This is a valid pattern.Note: The rows have hover styling but clicking outside the switch does nothing. If you want the entire row to be clickable, you could wrap with a
<button>and calle.stopPropagation()on the Switch's click—but the current approach is perfectly acceptable.
190-231: LGTM!The MCP section is well-implemented with:
- Proper conditional rendering based on
showMcp- Defensive disabling when no servers are running
- Clear user feedback with the "No MCP servers running" message
- Visual separation from other toggles
Update onWebSearchToggle, onWriteModeToggle, and onShowPageTreeToggle to accept (enabled: boolean) => void, matching onMcpToggle and the Switch component's onCheckedChange signature. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR Comment Review SummaryI've reviewed all review comments on this PR. Here's the status: 1. ❌ Double Toggle Bug (P1) - Already FixedThe concern about switch clicks toggling twice no longer applies. The current code uses 2. ❌ Active count includes writeMode - Already AddressedThe // Count active tools for badge (exclude writeMode since it's default true)
const activeCount = [
webSearchEnabled,
showPageTree,
showMcp && mcpEnabled,
].filter(Boolean).length;3. ❌ Inconsistent callback signatures - Already AddressedAll callbacks are now consistently typed as onWebSearchToggle?: (enabled: boolean) => void;
onWriteModeToggle?: (enabled: boolean) => void;
onShowPageTreeToggle?: (enabled: boolean) => void;
onMcpToggle?: (enabled: boolean) => void;All review comments have been addressed in subsequent commits. ✅ |
) * fix(security): harden integration OAuth callback against CWE-807 alerts (#98-101) Add Zod schema validation and log sanitization to break CodeQL taint chains in the integration OAuth callback. Alert #101 dismissed as S4 false positive — verifySignedState() provides HMAC-SHA256 verification. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(security): validate visibility enum and restore env in tests Replace unsafe type assertion for visibility with runtime validation against allowed values. Add afterEach env var cleanup in callback tests to prevent cross-test pollution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(security): address CodeRabbit review feedback - Persist visibility on reconnect by passing it to updateConnectionCredentials - Add optional visibility parameter to updateConnectionCredentials repository fn - Replace vi.clearAllMocks with vi.resetAllMocks for proper mock isolation - Use mockReturnValueOnce/mockResolvedValueOnce for per-test overrides - Add test coverage for visibility enum validation (valid, invalid, default) - Fix file count mismatches in CODEQL_ALERT_LOG.md subsection headings Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Move web search, write mode, page tree context, and MCP toggles from separate
buttons into a single "Tools" popover in the floating input footer. This
provides a cleaner UI while keeping all toggle options easily accessible.
Summary by CodeRabbit
New Features
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.