refactor(web): shrink LayoutShell.documentsPanel interface (fixes #1360)#1436
Conversation
…Setter#1360) The `documentsPanel` prop on `LayoutShell` declared `isDocked` and `onDockedChange` alongside `open` / `onOpenChange`, but the shell never forwarded those two extra fields to its consumers. `RightPanel` already defines its own interface accepting only `open` / `onOpenChange`, and `DocumentsSidebar` keeps both as optional props with safe fallbacks for the rare cases that pass them directly. Trim the interface to the two fields the shell actually plumbs through and drop the matching `isDocumentsDocked` state and prop entries from `LayoutDataProvider`, which was the only caller still populating the dead fields (`FreeLayoutDataProvider` already passed the minimal pair). `DocumentsSidebar` itself is untouched — its props remain optional so direct consumers that want docking behaviour can still wire it.
|
@suryo12 is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Removes the “docked documents” concept from the layout layer, simplifying the Documents panel state/props.
Changes:
- Dropped
isDocked/onDockedChangefromLayoutShellProps.documentsPanel. - Removed
isDocumentsDockedstate inLayoutDataProviderand stopped passing dock-related props downstream.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| surfsense_web/components/layout/ui/shell/LayoutShell.tsx | Removes dock-related fields from the documentsPanel prop contract. |
| surfsense_web/components/layout/providers/LayoutDataProvider.tsx | Removes local dock state and stops supplying dock props to LayoutShell. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Documents sidebar state (shared atom so Composer can toggle it) | ||
| const [isDocumentsSidebarOpen, setIsDocumentsSidebarOpen] = useAtom(documentsSidebarOpenAtom); | ||
| const [isDocumentsDocked, setIsDocumentsDocked] = useState(true); | ||
| const setIsRightPanelCollapsed = useSetAtom(rightPanelCollapsedAtom); | ||
|
|
||
| // Open documents sidebar by default on desktop (docked mode) |
Removes the dead
isDocked/onDockedChangeplumbing fromLayoutShell.documentsPanel— these fields were declared and populated byLayoutDataProviderbut never forwarded by the shell to any consumer, so the interface advertised a capability it never delivered.Description
components/layout/ui/shell/LayoutShell.tsx— Trims thedocumentsPanelprop type to the two fields the shell actually plumbs through (open,onOpenChange).RightPanelalready declares its own interface restricted to those two fields, andDocumentsSidebarkeeps the docking props on its own optional interface for the few callers that wire them directly.components/layout/providers/LayoutDataProvider.tsx— Drops theisDocumentsDocked/setIsDocumentsDockeduseStatepair and the matchingisDocked/onDockedChangeentries in thedocumentsPanelprop, since the shell never read them downstream.FreeLayoutDataProviderwas already passing the minimal pair, so it needs no change.components/layout/ui/sidebar/DocumentsSidebar.tsx— Left untouched per the issue notes. Its docking props remain optional so any direct caller that does want to drive docking can still do so without the shell as a middleman.Motivation and Context
The previous interface advertised four fields, but
LayoutShellonly ever forwarded two. New contributors reading the type would expect the shell to drive a docking flow that it doesn't, and the data provider was carrying auseStatethat nothing else in the tree consumed. Tightening the contract makes the shell's actual capability self-documenting and removes a dead plumbing path.FIX #1360
Screenshots
N/A — type / dead-code cleanup, no UI change.
API Changes
Change Type
Testing Performed
pnpm exec biome check ./components/layout/ui/shell/LayoutShell.tsx ./components/layout/providers/LayoutDataProvider.tsxpasses.pnpm exec tsc --noEmitintroduces no new type errors in the touched files. Verified there are no other callers ofLayoutShellpassingisDocked/onDockedChange(grep -racrosssurfsense_web/returned only the two files above plusDocumentsSidebar's own internal prop definitions, which are untouched).Runtime behaviour is unchanged:
LayoutShellnever read the removed fields, so removing them in the data provider has no downstream effect.Tested locally
Manual/QA verification
Checklist
High-level PR Summary
This PR cleans up dead code by removing unused docking-related props (
isDockedandonDockedChange) from theLayoutShell.documentsPanelinterface. The shell component never forwarded these fields to any consumers, so the interface was advertising a capability it didn't actually deliver. The changes remove the unused state management fromLayoutDataProviderand tighten the interface definition inLayoutShellto only include the two props that are actually used (openandonOpenChange).⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
surfsense_web/components/layout/ui/shell/LayoutShell.tsxsurfsense_web/components/layout/providers/LayoutDataProvider.tsx