feat(frontend): improve knowledge document folder navigation view - #1977
Open
Qinxl0921 wants to merge 2 commits into
Open
feat(frontend): improve knowledge document folder navigation view#1977Qinxl0921 wants to merge 2 commits into
Qinxl0921 wants to merge 2 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughDocument browsing now supports folder-scoped navigation for larger knowledge bases, with breadcrumbs, direct-child fetching, virtualized folder/document rows, and separate recursive expand-all behavior for smaller collections. ChangesKnowledge folder navigation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
frontend/src/features/knowledge/document/components/DocumentList.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. frontend/src/features/knowledge/document/components/FolderTree.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. frontend/src/features/knowledge/document/components/knowledge-folder-breadcrumb.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
frontend/src/features/knowledge/document/hooks/useFolderNavigation.ts (1)
25-30: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdjust state during rendering instead of using
useEffect.Using an effect to adjust state in response to a prop change causes an unnecessary double-render and a potential flash of stale state. In React, deriving state from props is better handled during the render phase.
♻️ Proposed refactor
- const [navFolderId, setNavFolderId] = useState<number | null>(null) - - // Reset to root when switching knowledge bases - useEffect(() => { - setNavFolderId(null) - }, [knowledgeBaseId]) + const [navFolderId, setNavFolderId] = useState<number | null>(null) + const [prevKbId, setPrevKbId] = useState(knowledgeBaseId) + + // Reset to root when switching knowledge bases + if (knowledgeBaseId !== prevKbId) { + setPrevKbId(knowledgeBaseId) + setNavFolderId(null) + }Note: You can also remove
useEffectfrom the imports on line 5 after this change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/knowledge/document/hooks/useFolderNavigation.ts` around lines 25 - 30, Update the state-reset logic in useFolderNavigation so navFolderId is reset during rendering when knowledgeBaseId changes, rather than through useEffect. Track the previously rendered knowledgeBaseId as needed, preserve the root reset behavior, and remove the now-unused useEffect import.frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx (1)
82-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract shared resource-grid helpers and column cells.
File-size/date formatting, URL handling, download behavior, and most document/folder cells duplicate
knowledge-document-tree-grid.tsx. A shared column/cell abstraction will prevent these parallel grids from drifting.As per coding guidelines, “Use clear names and reuse existing abstractions instead of duplicating logic” and “Keep functions focused, preferably under 50 lines.”
Also applies to: 154-707
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx` around lines 82 - 117, The knowledge-folder navigation view duplicates resource-grid helpers and cell behavior already implemented in knowledge-document-tree-grid.tsx. Extract or reuse shared abstractions for formatFileSize, formatDateTime, canOpenExternalUrl, download handling, and document/folder column cells, then update both grids to consume them while preserving their existing behavior and keeping each function focused.Source: Coding guidelines
frontend/src/features/knowledge/document/components/DocumentList.tsx (1)
1216-1450: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the display-mode views from
DocumentList.The new compact/normal and expand-all/folder-nav matrix further expands an already 1,000+ line component. Extract focused view components and pass an explicit shared action contract.
As per coding guidelines, “Keep functions focused, preferably under 50 lines” and “Split source files over 1000 lines.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/knowledge/document/components/DocumentList.tsx` around lines 1216 - 1450, Extract the compact/normal and expand-all/folder-nav rendering branches from DocumentList into focused view components, keeping DocumentList responsible for state and orchestration. Define and pass an explicit shared action contract containing the required selection, navigation, document, folder, pagination, and capability handlers, and preserve all existing behavior across the four display-mode combinations. Keep each extracted component focused and move the view implementations into separate source files so DocumentList is no longer over 1,000 lines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/features/knowledge/document/components/DocumentList.tsx`:
- Around line 286-290: Update the selection-reset effect in DocumentList to
track navFolderId alongside or instead of activeFolderId, so navigating between
folders clears selections from the previous folder. Preserve the existing reset
behavior for other relevant dependencies and ensure batch operations cannot
retain invisible document selections.
- Around line 315-323: Update the useDocuments call in DocumentList to pass
paginationEnabled only when displayMode is 'folder-nav' and paginationEnabled is
otherwise true. Keep pagination disabled for expand-all and compact modes so
they load beyond the first page when pagination controls are hidden.
In
`@frontend/src/features/knowledge/document/components/knowledge-folder-breadcrumb.tsx`:
- Around line 53-56: Update the breadcrumb button in the navigation rendering to
provide a minimum 44px height on mobile, while preserving its existing desktop
styling and behavior. Adjust the button’s class list near the onClick handler,
using responsive utilities if needed so the mobile touch target meets the
requirement.
- Around line 34-37: Update the breadcrumb `<nav>` and its nested navigation
button classes in the folder breadcrumb component to provide at least 44px touch
targets on mobile, replacing the current 36px height constraint with the
appropriate `h-11` sizing while preserving existing responsive behavior and
spacing.
In
`@frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx`:
- Around line 819-845: Add descriptive, stable data-testid selectors for the new
interactions: use distinct folder-row and document-row values on the interactive
rows in knowledge-folder-nav-view.tsx (lines 819-845), add a document-specific
selector to each checkbox in knowledge-folder-nav-view.tsx (lines 200-206), and
add a navigation-row selector to FolderRow in DocumentList.tsx (line 1324) only
when onActivateFolder is supplied.
- Around line 600-674: Ensure every mobile control meets the 44×44px
touch-target requirement: update document action buttons at
frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx:600-674,
external-link controls at :277-289, quick-edit controls at :309-320, folder
actions at :536-560, sortable headers and resize handles at :779-805, the
compact expand-all selector at
frontend/src/features/knowledge/document/components/DocumentList.tsx:1224-1234,
and compact folder-nav selector at :1275-1289; enlarge their interactive hit
areas without changing their existing actions or labels.
---
Nitpick comments:
In `@frontend/src/features/knowledge/document/components/DocumentList.tsx`:
- Around line 1216-1450: Extract the compact/normal and expand-all/folder-nav
rendering branches from DocumentList into focused view components, keeping
DocumentList responsible for state and orchestration. Define and pass an
explicit shared action contract containing the required selection, navigation,
document, folder, pagination, and capability handlers, and preserve all existing
behavior across the four display-mode combinations. Keep each extracted
component focused and move the view implementations into separate source files
so DocumentList is no longer over 1,000 lines.
In
`@frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx`:
- Around line 82-117: The knowledge-folder navigation view duplicates
resource-grid helpers and cell behavior already implemented in
knowledge-document-tree-grid.tsx. Extract or reuse shared abstractions for
formatFileSize, formatDateTime, canOpenExternalUrl, download handling, and
document/folder column cells, then update both grids to consume them while
preserving their existing behavior and keeping each function focused.
In `@frontend/src/features/knowledge/document/hooks/useFolderNavigation.ts`:
- Around line 25-30: Update the state-reset logic in useFolderNavigation so
navFolderId is reset during rendering when knowledgeBaseId changes, rather than
through useEffect. Track the previously rendered knowledgeBaseId as needed,
preserve the root reset behavior, and remove the now-unused useEffect import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62c757aa-1645-427d-87d4-2def235e8b61
📒 Files selected for processing (9)
frontend/src/features/knowledge/document/components/DocumentList.tsxfrontend/src/features/knowledge/document/components/FolderTree.tsxfrontend/src/features/knowledge/document/components/knowledge-document-tree-grid.tsxfrontend/src/features/knowledge/document/components/knowledge-folder-breadcrumb.tsxfrontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsxfrontend/src/features/knowledge/document/hooks/useFolderNavigation.tsfrontend/src/features/knowledge/document/utils/resource-tree.tsfrontend/src/i18n/locales/en/knowledge.jsonfrontend/src/i18n/locales/zh-CN/knowledge.json
Comment on lines
315
to
+323
| } = useDocuments({ | ||
| knowledgeBaseId: knowledgeBase.id, | ||
| paginationEnabled, | ||
| folderId: activeFolderId, | ||
| includeSubfolders: activeFolderId !== undefined, | ||
| folderScopeIds: activeFolderScopeIds, | ||
| // folder-nav mode (no search): scope to current folder's direct documents only. | ||
| // folder-nav mode (searching): folderId=undefined → global search across all docs. | ||
| // expand-all / compact modes: use activeFolderId (legacy filter behavior). | ||
| folderId: isFolderNavMode && !searchQuery ? (navFolderId ?? 0) : activeFolderId, | ||
| includeSubfolders: isFolderNavMode ? false : activeFolderId !== undefined, | ||
| folderScopeIds: isFolderNavMode ? undefined : activeFolderScopeIds, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='frontend/src/features/knowledge/document/hooks/useDocuments.ts'
ast-grep outline "$file" --items all --match 'useDocuments|paginationEnabled|pageSize'
rg -n -C4 '\b(paginationEnabled|pageSize|limit|offset)\b' "$file"Repository: wecode-ai/Wegent
Length of output: 4429
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='frontend/src/features/knowledge/document/components/DocumentList.tsx'
ast-grep outline "$file" --items all --match 'displayMode|paginationEnabled|useDocuments|folder-nav|expand-all|compact'
rg -n -C6 '\b(displayMode|paginationEnabled|folder-nav|expand-all|compact|useDocuments|hasMore|pageSize|totalPages)\b' "$file"
echo '--- useDocuments relevant slice ---'
file2='frontend/src/features/knowledge/document/hooks/useDocuments.ts'
sed -n '119,320p' "$file2"Repository: wecode-ai/Wegent
Length of output: 1918
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='frontend/src/features/knowledge/document/hooks/useDocuments.ts'
rg -n -C4 '\b(DEFAULT_PAGE_SIZE|LOCAL_METADATA_LIMIT|SERVER_REQUEST_LIMIT|paginationEnabled|localSnapshotModeRef|fetchServerPage|fetchLocalSnapshot|goToPage|changePageSize)\b' "$file"
echo '--- DocumentList mode selection ---'
file2='frontend/src/features/knowledge/document/components/DocumentList.tsx'
rg -n -C5 '\b(displayMode|paginationEnabled|isFolderNavMode|searchQuery|navFolderId|activeFolderId|activeFolderScopeIds)\b' "$file2"Repository: wecode-ai/Wegent
Length of output: 1918
Disable pagination when pagination controls are hidden. Pass paginationEnabled: displayMode === 'folder-nav' && paginationEnabled here so expand-all/compact modes don’t stop at the first page.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/features/knowledge/document/components/DocumentList.tsx` around
lines 315 - 323, Update the useDocuments call in DocumentList to pass
paginationEnabled only when displayMode is 'folder-nav' and paginationEnabled is
otherwise true. Keep pagination disabled for expand-all and compact modes so
they load beyond the first page when pagination controls are hidden.
Comment on lines
+600
to
+674
| <button | ||
| className="p-1.5 rounded-md text-text-muted hover:text-primary hover:bg-primary/10 transition-colors" | ||
| onClick={event => { | ||
| event.stopPropagation() | ||
| onMove(document) | ||
| }} | ||
| title={moveLabel} | ||
| aria-label={moveLabel} | ||
| data-testid={`move-document-${document.id}`} | ||
| > | ||
| <FolderInput className="w-3.5 h-3.5" /> | ||
| </button> | ||
| )} | ||
| {isWeb && onRefresh && ( | ||
| <button | ||
| className={`p-1.5 rounded-md transition-colors ${ | ||
| refreshingDocId === document.id | ||
| ? 'text-primary cursor-not-allowed' | ||
| : 'text-text-muted hover:text-primary hover:bg-primary/10' | ||
| }`} | ||
| onClick={event => { | ||
| event.stopPropagation() | ||
| onRefresh(document) | ||
| }} | ||
| disabled={refreshingDocId === document.id} | ||
| title={refreshLabel} | ||
| aria-label={refreshLabel} | ||
| data-testid={`refresh-document-${document.id}`} | ||
| > | ||
| <CloudDownload | ||
| className={`w-4 h-4 ${refreshingDocId === document.id ? 'animate-pulse' : ''}`} | ||
| /> | ||
| </button> | ||
| )} | ||
| {canReindex && ( | ||
| <button | ||
| className="p-1.5 rounded-md text-text-muted hover:text-primary hover:bg-primary/10 transition-colors" | ||
| onClick={event => { | ||
| event.stopPropagation() | ||
| onReindex?.(document) | ||
| }} | ||
| title={reindexLabel} | ||
| aria-label={reindexLabel} | ||
| data-testid={`reindex-document-${document.id}`} | ||
| > | ||
| <RotateCcw className="w-4 h-4" /> | ||
| </button> | ||
| )} | ||
| {showDownload && ( | ||
| <button | ||
| className="p-1.5 rounded-md text-text-muted hover:text-primary hover:bg-primary/10 transition-colors" | ||
| onClick={event => { | ||
| event.stopPropagation() | ||
| handleDocumentDownload(document) | ||
| }} | ||
| title={downloadLabel} | ||
| aria-label={downloadLabel} | ||
| data-testid={`download-document-${document.id}`} | ||
| > | ||
| <Download className="w-4 h-4" /> | ||
| </button> | ||
| )} | ||
| {onDelete && ( | ||
| <button | ||
| className="p-1.5 rounded-md text-text-muted hover:text-error hover:bg-error/10 transition-colors" | ||
| onClick={event => { | ||
| event.stopPropagation() | ||
| onDelete(document) | ||
| }} | ||
| title={deleteLabel} | ||
| aria-label={deleteLabel} | ||
| data-testid={`delete-document-${document.id}`} | ||
| > | ||
| <Trash2 className="w-4 h-4" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Meet the 44×44px mobile touch-target requirement across the new navigation UI.
frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L600-L674: enlarge document action controls or use a mobile action menu.frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L277-L289: enlarge the external-link control.frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L309-L320: enlarge the quick-edit control.frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L536-L560: enlarge folder action controls.frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L779-L805: enlarge sortable headers and touch resize handles.frontend/src/features/knowledge/document/components/DocumentList.tsx#L1224-L1234: enlarge the compact expand-all selector.frontend/src/features/knowledge/document/components/DocumentList.tsx#L1275-L1289: enlarge the compact folder-nav selector.
As per coding guidelines, “Mobile controls must be at least 44px × 44px.”
📍 Affects 2 files
frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L600-L674(this comment)frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L277-L289frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L309-L320frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L536-L560frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L779-L805frontend/src/features/knowledge/document/components/DocumentList.tsx#L1224-L1234frontend/src/features/knowledge/document/components/DocumentList.tsx#L1275-L1289
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx`
around lines 600 - 674, Ensure every mobile control meets the 44×44px
touch-target requirement: update document action buttons at
frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx:600-674,
external-link controls at :277-289, quick-edit controls at :309-320, folder
actions at :536-560, sortable headers and resize handles at :779-805, the
compact expand-all selector at
frontend/src/features/knowledge/document/components/DocumentList.tsx:1224-1234,
and compact folder-nav selector at :1275-1289; enlarge their interactive hit
areas without changing their existing actions or labels.
Source: Coding guidelines
Comment on lines
+819
to
+845
| <div | ||
| className={`grid items-center gap-4 px-4 py-3 transition-colors border-b border-border min-w-[880px] ${ | ||
| isFolder | ||
| ? `bg-surface/50 hover:bg-surface cursor-pointer` | ||
| : `bg-base hover:bg-surface group ${onViewDetail ? 'cursor-pointer' : ''}` | ||
| }`} | ||
| style={{ gridTemplateColumns }} | ||
| onClick={() => { | ||
| if (isFolder) { | ||
| onNavigateFolder(item.folder.id) | ||
| } else if (canClick) { | ||
| onViewDetail?.(item.document) | ||
| } | ||
| }} | ||
| role="button" | ||
| tabIndex={0} | ||
| onKeyDown={event => { | ||
| if (event.currentTarget !== event.target) return | ||
| if (event.key === 'Enter' || event.key === ' ') { | ||
| event.preventDefault() | ||
| if (isFolder) { | ||
| onNavigateFolder(item.folder.id) | ||
| } else if (canClick) { | ||
| onViewDetail?.(item.document) | ||
| } | ||
| } | ||
| }} |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add stable selectors for all new folder-navigation interactions.
frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L819-L845: add folder/document-specificdata-testidvalues to interactive rows.frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L200-L206: add a document-specificdata-testidto each checkbox.frontend/src/features/knowledge/document/components/DocumentList.tsx#L1324-L1324: add a navigation-rowdata-testidinFolderRowwhenonActivateFolderis supplied.
As per coding guidelines, “All new interactive elements must have descriptive data-testid values.”
📍 Affects 2 files
frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L819-L845(this comment)frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx#L200-L206frontend/src/features/knowledge/document/components/DocumentList.tsx#L1324-L1324
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@frontend/src/features/knowledge/document/components/knowledge-folder-nav-view.tsx`
around lines 819 - 845, Add descriptive, stable data-testid selectors for the
new interactions: use distinct folder-row and document-row values on the
interactive rows in knowledge-folder-nav-view.tsx (lines 819-845), add a
document-specific selector to each checkbox in knowledge-folder-nav-view.tsx
(lines 200-206), and add a navigation-row selector to FolderRow in
DocumentList.tsx (line 1324) only when onActivateFolder is supplied.
Source: Coding guidelines
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.
知识库文档数量较多时，会进行分页，因为只对所有文档分页，文件夹全部展示，文件夹和文档是混合在一棵树里渲染，所以会出现一个文件夹下的文档分在多页显示的情况。对此进行优化，当文档总数<100时，全量展示所有文件夹和文档；当文档总数>=100时，只展示根目录下文件夹，由用户手动点击一层层展开，并在顶部添加面包屑组件，便于返回到任意父层级文件夹中。
Summary by CodeRabbit