Refactor/independent workspace document hook #586
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
Reviewer's GuideThis PR extracts all document and streaming state into a standalone useWorkspaceDocuments hook/provider, refactors the chat and main workspace hooks to delegate document logic to the new module, removes legacy workspace state fields and mutations, and applies a minor menu id fix. Sequence diagram for document update flow after refactorsequenceDiagram
participant WC as WorkspaceChatProvider
participant WDP as WorkspaceDocumentsProvider
participant DB as Database
WC->>WDP: handleDocumentUpdate(aiResponse, selectionRange)
WDP->>DB: updateDocumentContentConditional(project, document, newMarkdown)
DB-->>WDP: Document updated
WDP-->>WC: onSectionContentUpdate(sectionId, updatedContent)
Class diagram for refactored workspace document managementclassDiagram
class WorkspaceDocumentsProvider {
+documents: WorkspaceDocumentMetadata[]
+organizationData: OrganizationData[]
+organizationList: string[]
+departmentsByOrg: Record<string, string[]>
+projectsByDept: Record<string, Record<string, string[]>>
+documentList: Record<string, string[]>
+textDocuments: Record<string, string[]>
+imageDocuments: Record<string, string[]>
+spreadsheetDocuments: Record<string, string[]>
+templates: typeof workspaceDocTemplates
+addOrganization(name, chatbots)
+addDepartment(org, name)
+addProject(org, dept, name)
+addDocument(project, name, type, templateContent)
+updateChatbotActiveState(orgName, chatbotId, isActive)
+getOrganizationChatbots(orgName)
+documentData: content, sections
+isLoadingDocument: boolean
+workspaceProcessingState: 'idle' | 'analyzing' | 'generating' | 'updating'
+setWorkspaceProcessingState(state)
+activeWorkspaceSection: string | null
+setActiveWorkspaceSection(sectionId)
+selectionRange: start, end
+setSelectionRange(range)
+handleDocumentUpdate(aiResponse, selectionRange)
+onSectionContentUpdate(sectionId, content)
+setOnSectionContentUpdate(callback)
+onStreamingChunk()
+setOnStreamingChunk(callback)
}
class WorkspaceChatProvider {
-document and streaming state (removed)
+delegates document logic to useWorkspaceDocuments()
}
class WorkspaceProvider {
-document and streaming state fields (removed)
+delegates document logic to useWorkspaceDocuments()
}
WorkspaceChatProvider --> WorkspaceDocumentsProvider : uses
WorkspaceProvider --> WorkspaceDocumentsProvider : uses
Class diagram for removed legacy workspace state and mutationsclassDiagram
class WorkspaceContextType {
-organizationList: string[]
-departmentList: Record<string, string[]>
-projectList: string[]
-documentList: Record<string, string[]>
-textDocuments: Record<string, string[]>
-imageDocuments: Record<string, string[]>
-spreadsheetDocuments: Record<string, string[]>
-projectsByDept: Record<string, Record<string, string[]>>
-addOrganization(name, chatbots)
-addDepartment(org, name)
-addProject(org, dept, name)
-addDocument(project, name, type, templateContent)
-updateChatbotActiveState(orgName, chatbotId, isActive)
-getOrganizationChatbots(orgName)
}
class WorkspaceState {
-departmentsByOrg: Record<string, string[]>
-projectsByDept: Record<string, Record<string, string[]>>
-textDocuments: Record<string, string[]>
-imageDocuments: Record<string, string[]>
-spreadsheetDocuments: Record<string, string[]>
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors workspace document management by extracting it into a dedicated useWorkspaceDocuments hook and provider, separating concerns between workspace navigation state and document operations.
Key Changes
- Extracted document structure management (organizations, departments, projects, documents) and content operations into a standalone
WorkspaceDocumentsProvideranduseWorkspaceDocumentshook - Streamlined
WorkspaceProviderby removing document-related state and mutations, delegating to the new documents hook - Updated
WorkspaceChatProviderto consume document state and actions fromuseWorkspaceDocumentsinstead of managing them locally - Removed large portions of streaming document update logic from
use-workspace-chat.tsx, consolidating it in the documents hook
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/pro-web/lib/hooks/use-workspace.tsx | Removed document structure state/actions and mutations; now consumes from useWorkspaceDocuments hook; simplified to focus on workspace navigation state only |
| apps/pro-web/lib/hooks/use-workspace-documents.tsx | New file providing standalone document management with structure data, mutations, content state, and streaming update logic |
| apps/pro-web/lib/hooks/use-workspace-documents.ts | Old implementation deleted - replaced by new Provider-based approach |
| apps/pro-web/lib/hooks/use-workspace-chat.tsx | Refactored to use document state/actions from useWorkspaceDocuments; removed local streaming refs and document update logic |
| apps/pro-web/components/routes/workspace/workspace-tab-menu.tsx | Updated condition to only show history submenu for 'document-history' instead of generic 'history' |
Comments suppressed due to low confidence (1)
apps/pro-web/lib/hooks/use-workspace-documents.tsx:16
- Unused import useAutoSaveDocument.
import {
makeDocumentKey,
useAutoSaveDocument,
useDocumentContent,
useUpdateDocumentContent,
} from '@/lib/queries'
| const newMarkdown = `${preservedBeforeSelectionRef.current}${cleanedResponse} | ||
| ${preservedAfterSelectionRef.current}` | ||
|
|
There was a problem hiding this comment.
[nitpick] Formatting inconsistency: The template string has an extra blank line between ${cleanedResponse} and the closing of the preserved after content. This creates double line breaks which may not be intentional. The same pattern appears on line 562-564.
Consider removing the extra blank line:
const newMarkdown = `${preservedBeforeSelectionRef.current}${cleanedResponse}
${preservedAfterSelectionRef.current}`| const newMarkdown = `${preservedBeforeSelectionRef.current}${cleanedResponse} | |
| ${preservedAfterSelectionRef.current}` | |
| const newMarkdown = `${preservedBeforeSelectionRef.current}${cleanedResponse}${preservedAfterSelectionRef.current}` |
| start: number | ||
| end: number | ||
| } | null>(null) | ||
| const currentStreamingContentRef = useRef<string>('') |
There was a problem hiding this comment.
currentStreamingContentRef is defined and assigned (line 565) but never read or used anywhere in the code. This appears to be dead code that can be removed to improve maintainability.
| const currentStreamingContentRef = useRef<string>('') |
| const { | ||
| documents: legacyUserDocuments, | ||
| organizationList, | ||
| departmentsByOrg, | ||
| projectsByDept, | ||
| documentList, | ||
| textDocuments, | ||
| imageDocuments, | ||
| spreadsheetDocuments, | ||
| } = useWorkspaceDocuments() |
There was a problem hiding this comment.
The useWorkspaceDocuments() hook is being called but the WorkspaceDocumentsProvider is not added to the provider hierarchy. This will cause a runtime error because useWorkspaceDocuments() requires its context provider.
The WorkspaceDocumentsProvider should be added to the provider tree (likely in providers.tsx) wrapping or near the WorkspaceProvider, and it should receive the initialState prop with the appropriate document-related data.
| const { | ||
| activeOrganization, | ||
| activeDepartment, | ||
| activeProject, | ||
| activeDocument, | ||
| setActiveDocument, | ||
| } = useWorkspace() |
There was a problem hiding this comment.
Circular dependency issue: useWorkspaceDocuments (line 5) imports and uses useWorkspace (line 125-131), while useWorkspace imports and uses useWorkspaceDocuments (line 422-431). This creates a circular dependency that could lead to initialization issues or runtime errors.
Consider restructuring so that useWorkspaceDocuments doesn't depend on useWorkspace. You could pass the required values (activeOrganization, activeDepartment, activeProject, activeDocument, setActiveDocument) as parameters to the provider or extract shared state into a third, lower-level context that both can depend on.
| useUpdateChatbotActiveState, | ||
| } from '@/lib/queries/use-workspace-mutations' | ||
| import { | ||
| type WorkspaceStructure, |
There was a problem hiding this comment.
The imported WorkspaceStructure type is not used anywhere in this file. Consider removing this unused import.
| type WorkspaceStructure, |
| const isLast = index === menuOptions.length - 1 | ||
|
|
||
| return option.id === 'history' ? ( | ||
| // Only document-history has history items for now. For media-history we will have a pop-up, different UI/UX for it... |
There was a problem hiding this comment.
Grammar issue in comment: "For media-history we will have a pop-up" should be "For media-history, we will have a pop-up" (missing comma after the introductory clause).
| // Only document-history has history items for now. For media-history we will have a pop-up, different UI/UX for it... | |
| // Only document-history has history items for now. For media-history, we will have a pop-up, different UI/UX for it... |
| const initialSelectionRangeRef = useRef<{ | ||
| start: number | ||
| end: number | ||
| } | null>(null) |
There was a problem hiding this comment.
initialSelectionRangeRef is declared and assigned (lines 489, 554) but never read anywhere in the code. Consider removing this unused ref to improve code clarity.
| const initialSelectionRangeRef = useRef<{ | |
| start: number | |
| end: number | |
| } | null>(null) |
| } from '@/lib/markdown-utils' | ||
| import { | ||
| makeDocumentKey, | ||
| useAutoSaveDocument, |
There was a problem hiding this comment.
The imported useAutoSaveDocument is not used anywhere in this file. Consider removing this unused import.
| useAutoSaveDocument, |
| start: number | ||
| end: number | ||
| } | null>(null) | ||
| const [onSectionContentUpdate, setOnSectionContentUpdate] = useState< |
There was a problem hiding this comment.
Using useState for storing callback functions is problematic. The state setter pattern setOnSectionContentUpdate expects a value, but when you try to set a function, React will treat it as a state updater function. You need to wrap the function in another function like: setOnSectionContentUpdate(() => callback) when setting it.
Consider using useRef instead of useState for storing callback references, or ensure all usages properly wrap the callback: setOnSectionContentUpdate(() => actualCallback).
| const [onSectionContentUpdate, setOnSectionContentUpdate] = useState< | |
| const onSectionContentUpdateRef = useRef< |
| } else if ( | ||
| selectionRange && | ||
| selectionRange.start === selectionRange.end && | ||
| selectionRange.start > 0 |
There was a problem hiding this comment.
Logic issue: Lines 467-476 check if selectionRange exists and if start !== end, then if start === end with additional validation. However, there's a potential issue at line 473 where you check selectionRange.start > 0.
A selection at position 0 (start of section content) is valid, so this condition would incorrectly skip that case. The condition should likely be selectionRange.start >= 0 or the > 0 check should be removed entirely since cursor position at the beginning of content is a valid scenario.
| selectionRange.start > 0 | |
| selectionRange.start >= 0 |
TODO:
Summary by Sourcery
Extract the workspace document management logic into a standalone hook and provider, streamline the main workspace context, and update components to consume the new documents hook.
Enhancements: