Repository navigation
feat: restore documents and Git Sync support - #2491
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughThe change adds Markdown document storage, workspace-aware APIs, search, synchronization, SSE updates, runtime paths, and a document-management UI. Git Sync now models DAGs and documents as sync items. ChangesDocument management and sync-item support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/frontend/sse/app_stream.go (1)
244-266: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the file watcher on the early error returns in
Start.Two error paths return without releasing the watcher:
- Line 248-250:
w.addWatch(w.root)fails and returns.w.watcherstays open.- Line 262-264:
w.addWatch(path)fails and returns.w.watcherstays open.The
watchPathsForScopepath at Line 254-257 does call_ = w.watcher.Close(), so the handling is inconsistent.Stop()cannot recover the handle either, because theloopgoroutine has not started yet, soStop()only closesdoneand waits on an emptyWaitGroup.This path is now reachable on every fallback.
docsDirectoryWatcher.Startat Line 375-382 starts a recursive event watcher, and on failure callseventWatcher.Stop()before switching to the Markdown poller. The inotify handle from the failed attempt leaks.🔒️ Proposed fix
w.watcher, err = w.newFileWatcher() if err != nil { return err } if err := w.addWatch(w.root); err != nil { + _ = w.watcher.Close() return err } if w.scope == watchScopeOneLevel || w.scope == watchScopeRecursive { paths, err := watchPathsForScope(w.root, w.scope) if err != nil { _ = w.watcher.Close() return err } for _, path := range paths { if path == w.root { continue } if err := w.addWatch(path); err != nil { + _ = w.watcher.Close() return err } } }🤖 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 `@internal/service/frontend/sse/app_stream.go` around lines 244 - 266, Update Start to close w.watcher before returning when addWatch(w.root) fails or when adding any path from watchPathsForScope fails, matching the existing cleanup for watchPathsForScope errors; preserve the original error after cleanup.
🧹 Nitpick comments (18)
internal/service/frontend/sse/app_stream_test.go (1)
108-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider not starting the poll loop in this test.
Startspawns the polling goroutine, and the test then callswatcher.check()directly. Both touchw.snapshotwithout synchronization. The test is safe today only because the interval istime.Hour, so the loop goroutine never callscheck. A future change to the default interval or toStartwould make this a real race undergo test -race.Seed the snapshot without starting the loop, or keep the current form and add a short comment stating that the one-hour interval is what keeps the loop idle.
As per coding guidelines: "Go tests use race detection".
🤖 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 `@internal/service/frontend/sse/app_stream_test.go` around lines 108 - 112, Update the test around watcher.Start and watcher.check to avoid concurrent snapshot access: seed the watcher snapshot directly without starting the polling loop, or explicitly document that the one-hour interval keeps the loop idle. Prefer the no-loop setup so the test remains race-safe if the default interval or Start behavior changes.Source: Coding guidelines
internal/service/frontend/api/v1/docs.go (1)
465-482: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider pushing workspace scoping into the store call.
SearchDocscallsa.docStore.Searchwithout any scope, then discards non-matching and non-visible results in Go.SearchDocFeedininternal/service/frontend/api/v1/search.gopassesPathPrefixandExcludePathRootsto the store instead. Authorization is still correct here, because every hidden result is filtered before the response is built. However, the store reads and scores documents that the caller can never see.If
docs.DocStoregains a scoped variant ofSearch, align this handler withSearchDocFeed.🤖 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 `@internal/service/frontend/api/v1/docs.go` around lines 465 - 482, Update SearchDocs to use a scoped docs.DocStore search when the store exposes the scoped Search variant, passing workspaceName as PathPrefix and the visibility-derived roots as ExcludePathRoots, consistent with SearchDocFeed. Preserve the existing filtering and ID normalization behavior as a safety check before constructing the response.internal/service/frontend/sse/app_stream.go (1)
343-358: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIgnore
os.ErrNotExistduring the recursive walk.
addCreatedDirWatcheswalks a directory immediately after aCreateevent. If the directory or a child is removed before the walk reaches it,WalkDirreturns the error. The caller at Line 319-321 then callsw.onReset, which forces a full client refresh for a transient directory.Skip missing entries so a short-lived directory does not trigger a reset.
♻️ Proposed change
return filepath.WalkDir(path, func(childPath string, entry os.DirEntry, err error) error { if err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil + } return err }🤖 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 `@internal/service/frontend/sse/app_stream.go` around lines 343 - 358, Update the WalkDir callback in addCreatedDirWatches to ignore errors wrapping os.ErrNotExist and continue the recursive walk, including when the created directory or a child disappears before traversal. Preserve returning all other errors so genuine failures still reach the caller without triggering unnecessary behavior changes.internal/service/frontend/api/v1/search.go (1)
296-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why the aggregate-scope retry exists.
The retry compensates for a matches cursor that
SearchDocFeedminted in aggregate scope and that the client then replays withworkspaceset. The behavior depends on the store returningexec.ErrInvalidCursorwhen the cursorPathPrefixdoes not match the request. That coupling is not obvious from the code.Authorization is preserved, because
workspaceNameis only non-empty afterdocPointReadScopeForParamsauthorizes it, andaggregatePathkeeps that prefix. Add a short comment so a future change to cursor encoding does not remove this branch by accident.🤖 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 `@internal/service/frontend/api/v1/search.go` around lines 296 - 306, Add a short comment above the retry branch in the search handler around SearchMatches explaining that SearchDocFeed may mint an aggregate-scope cursor, which can be replayed with a workspace and yield exec.ErrInvalidCursor when PathPrefix differs; document that the retry restores aggregate scope while preserving the authorized aggregatePath.ui/src/contexts/DocTabContext.tsx (1)
196-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove persistence and ref writes out of state updater functions.
closeTabcallssetActiveTabId, mutatesdraftsRef/unsavedTabIdsRef, and callspersistStateinside thesetTabs,setDrafts, andsetUnsavedTabIdsupdater functions. The same pattern appears incloseOtherTabs,setDraft,clearDraft,markTabUnsaved, andmarkTabSaved. React expects updater functions to be pure. Under StrictMode the updaters run twice, so these writes run twice per call.The effect on Line 158 already persists state after every change to
tabs,activeTabId,drafts, andunsavedTabIds, and the refs are refreshed on every render at Lines 135-142. The in-updater persistence is therefore redundant. Compute the next value first, then call the setters and persist outside the updater.♻️ Example for
setDraftconst setDraft = useCallback( (tabId: string, content: string) => { - setDrafts((prev) => { - const next = new Map(prev); - next.set(tabId, content); - draftsRef.current = next; - persistState({ drafts: Array.from(next.entries()) }); - return next; - }); + const next = new Map(draftsRef.current); + next.set(tabId, content); + setDrafts(next); }, - [persistState] + [] );🤖 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 `@ui/src/contexts/DocTabContext.tsx` around lines 196 - 233, Update closeTab and the related mutators closeOtherTabs, setDraft, clearDraft, markTabUnsaved, and markTabSaved so their state updater callbacks only compute and return next values. Move setActiveTabId calls, draftsRef/unsavedTabIdsRef writes, and persistState calls outside updater functions, relying on the existing state-persistence effect and render-time ref synchronization to handle updates without duplicate side effects.ui/src/pages/docs/index.tsx (3)
203-205: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDepend on
setTitleinstead of the whole context object.
AppBarContextis provided as an inline object literal inui/src/App.tsx(lines 578-592), so its identity changes on everyAppInnerrender. This effect then re-runs on every parent render.ui/src/pages/home/index.tsxline 79 uses[setTitle], which is stable.♻️ Proposed refactor
- // Set page title - useEffect(() => { - appBarContext.setTitle('Docs'); - }, [appBarContext]); + // Set page title + const setTitle = appBarContext.setTitle; + useEffect(() => { + setTitle('Docs'); + }, [setTitle]);🤖 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 `@ui/src/pages/docs/index.tsx` around lines 203 - 205, Update the Docs page useEffect dependency array to depend on the stable setTitle function rather than the entire appBarContext object, matching the existing pattern in the Home page while preserving the title-setting behavior.
495-498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface the server error message on delete failure.
handleRenameModal(line 366) andhandlePathChange(line 429) showerror?.message. This branch discards it. Users lose the reason for the failure.♻️ Proposed refactor
if (error) { - showToast('Failed to delete document'); + showToast(error?.message || 'Failed to delete document'); return; }🤖 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 `@ui/src/pages/docs/index.tsx` around lines 495 - 498, Update the delete failure branch in the document deletion handler to include the server-provided error message via error?.message, matching handleRenameModal and handlePathChange, while retaining the existing fallback toast text when no message is available.
705-745: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated modal block.
Lines 705-745 and lines 763-803 declare the same four modals with identical props. Two copies can diverge when a prop changes. Assign the block to a variable once, then render it in both layouts.
♻️ Proposed refactor
+ const modals = ( + <> + <CreateDocModal + isOpen={createModalOpen} + onClose={() => setCreateModalOpen(false)} + onSubmit={handleCreate} + parentDir={createParentDir} + isLoading={createLoading} + externalError={createError} + /> + <RenameDocModal + isOpen={renameModalOpen} + onClose={() => setRenameModalOpen(false)} + onSubmit={handleRenameModal} + currentPath={renameDocPath} + isLoading={renameLoading} + externalError={renameError} + /> + <ConfirmModal + title="Delete Document" + buttonText="Delete" + visible={deleteConfirmOpen} + dismissModal={() => setDeleteConfirmOpen(false)} + onSubmit={handleDelete} + > + <p className="text-sm text-muted-foreground"> + Are you sure you want to delete <strong>{deleteDocTitle}</strong>? + This action cannot be undone. + </p> + </ConfirmModal> + <ConfirmModal + title="Delete Docs" + buttonText={`Delete ${batchDeleteTargets.length} items`} + visible={batchDeleteConfirmOpen} + dismissModal={() => setBatchDeleteConfirmOpen(false)} + onSubmit={handleBatchDelete} + > + <p className="text-sm text-muted-foreground"> + Are you sure you want to delete {batchDeleteTargets.length} items? + This cannot be undone. + </p> + </ConfirmModal> + </> + );Then render
{modals}in the mobile branch and in the desktop branch.🤖 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 `@ui/src/pages/docs/index.tsx` around lines 705 - 745, Extract the shared CreateDocModal, RenameDocModal, and both ConfirmModal declarations from the page component into a single modals variable, preserving all existing props and handlers. Replace the duplicated modal blocks in the mobile and desktop layout branches with {modals} so both branches render the same definitions.ui/src/pages/docs/lib/doc-validation.ts (1)
11-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate
trimmedconsistently, and name the length limit.Line 11 computes
trimmed, but lines 15, 18, 24, and 31 validate the rawpath. Both current callers trim before calling, so behavior is correct today. A future caller that passes an untrimmed value would get a misleading "cannot end with a space" error. Also extract252into a named constant that states it mirrors the backend document ID limit.♻️ Proposed refactor
+const MAX_DOC_PATH_LENGTH = 252; // Mirrors the backend document ID limit. + export function validateDocPath(path: string): { isValid: boolean; error?: string; } { const trimmed = path.trim(); if (!trimmed) { return { isValid: false, error: 'Path is required' }; } - if (path.length > 252) { - return { isValid: false, error: 'Path must be 252 characters or fewer' }; + if (trimmed.length > MAX_DOC_PATH_LENGTH) { + return { + isValid: false, + error: `Path must be ${MAX_DOC_PATH_LENGTH} characters or fewer`, + }; } - if (path.toLowerCase().endsWith('.md')) { + if (trimmed.toLowerCase().endsWith('.md')) { return { isValid: false, error: 'Path should not include the .md extension.', }; } - if (!DOC_PATH_PATTERN.test(path)) { + if (!DOC_PATH_PATTERN.test(trimmed)) { return { isValid: false, error: 'Invalid path. Use letters, numbers, underscores, dots, hyphens, and spaces. Use / for directories.', }; } - if (path.split('/').some((segment) => /[ .]$/.test(segment))) { + if (trimmed.split('/').some((segment) => /[ .]$/.test(segment))) {If you apply this, update
ui/src/pages/docs/lib/__tests__/doc-validation.test.tsline 23, becausevalidateDocPath('guide ')would then return{ isValid: true }.🤖 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 `@ui/src/pages/docs/lib/doc-validation.ts` around lines 11 - 31, Update the doc path validation function to use the existing trimmed value for all subsequent length, extension, pattern, and segment checks, while preserving the required-path check. Extract the 252-character limit into a named constant documenting that it mirrors the backend document ID limit, and update the affected test expectation for validateDocPath('guide ') to reflect the trimmed valid path result.ui/src/pages/docs/components/DocEditor.tsx (2)
334-347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImprove the accessible state and name of the toolbar controls.
The copy-file-path button renders an icon only and relies on
titlefor its name. Addaria-label. The mode toggle buttons expose no selected state to assistive technology. Addaria-pressed.♿ Proposed fix
<button type="button" onClick={copyFilePath} className="inline-flex items-center gap-1 px-1.5 py-0.5 text-xs rounded-md text-muted-foreground hover:text-foreground hover:bg-muted transition-all shrink-0" title={`Copy file path: ${doc.filePath}`} + aria-label="Copy file path" ><button type="button" + aria-pressed={mode === 'edit'} className={cn(<button type="button" + aria-pressed={mode === 'preview'} className={cn(Also applies to: 368-393
🤖 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 `@ui/src/pages/docs/components/DocEditor.tsx` around lines 334 - 347, Improve accessibility for the toolbar controls in DocEditor: add an aria-label to the icon-only copy-file-path button using copyFilePath, and add aria-pressed to both mode toggle buttons in the related controls, reflecting each button’s active/selected state.
284-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated clipboard routine.
copyFilePathandcopyContentrepeat the same logic: write to the clipboard, fall back to a hiddentextarea, then reset a flag after 2000 ms. Extract one helper and call it from both callbacks.♻️ Suggested shape
// e.g. ui/src/lib/clipboard.ts export async function copyText(text: string): Promise<void> { try { await navigator.clipboard.writeText(text); return; } catch { const textArea = document.createElement('textarea'); textArea.value = text; document.body.appendChild(textArea); textArea.select(); document.execCommand('copy'); document.body.removeChild(textArea); } }Each callback then reduces to
await copyText(value); setCopied(true); setTimeout(() => setCopied(false), 2000);.🤖 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 `@ui/src/pages/docs/components/DocEditor.tsx` around lines 284 - 320, Extract the shared clipboard write and textarea fallback from copyFilePath and copyContent into a reusable copyText helper, then have both callbacks call it while retaining their existing empty-value guards and copied-state reset behavior. Keep the helper responsible only for copying text; leave the file-path and content-specific state updates in their respective callbacks.ui/src/hooks/__tests__/useContentEditor.test.ts (1)
8-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
cancelSaveand for the conflict path.The suite covers the echo-accept path only.
cancelSaveis new public API and has no coverage. A failed save must restore conflict detection, so an external change aftercancelSavemust setconflict.hasConflict.💚 Suggested additional test
}); + + it('restores conflict detection after a cancelled save', () => { + const { result, rerender } = renderHook( + ({ serverContent }) => useContentEditor({ key: 'doc', serverContent }), + { initialProps: { serverContent: '' } } + ); + + act(() => { + result.current.setCurrentValue('# Local'); + result.current.beginSave('# Local'); + result.current.cancelSave(); + }); + + rerender({ serverContent: '# External' }); + + expect(result.current.conflict.hasConflict).toBe(true); + }); });As per coding guidelines: "Add or update tests appropriate to the changed code; ... frontend unit tests use Vitest".
🤖 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 `@ui/src/hooks/__tests__/useContentEditor.test.ts` around lines 8 - 31, Extend the useContentEditor test suite with Vitest coverage for cancelSave: begin a save, call result.current.cancelSave(), then simulate an external serverContent change and assert conflict.hasConflict becomes true. Keep the existing server-echo acceptance test unchanged and use the hook’s public API.Source: Coding guidelines
ui/src/pages/docs/components/DocExternalChangeDialog.tsx (1)
25-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe global Enter handler duplicates native button activation.
ignoreButtonRefanddiscardButtonRefpoint to native<button>elements. A focused button already firesonClickon Enter, soonIgnoreandonDiscardrun without this listener. The handler works today only becausee.preventDefault()suppresses the generated click. Remove the effect and rely on the buttons.♻️ Proposed simplification
function DocExternalChangeDialog({ visible, onDiscard, onIgnore }: Props) { - const ignoreButtonRef = React.useRef<HTMLButtonElement>(null); - const discardButtonRef = React.useRef<HTMLButtonElement>(null); - - React.useEffect(() => { - const handleKeyDown = (e: KeyboardEvent) => { - if (!visible) return; - - if (e.key === 'Enter') { - const activeElement = document.activeElement; - - if (activeElement === ignoreButtonRef.current) { - e.preventDefault(); - onIgnore(); - return; - } - - if (activeElement === discardButtonRef.current) { - e.preventDefault(); - onDiscard(); - return; - } - - // No default action — Enter only triggers when a specific button is focused - } - }; - - document.addEventListener('keydown', handleKeyDown); - return () => document.removeEventListener('keydown', handleKeyDown); - }, [visible, onDiscard, onIgnore]); - return (Then drop the
refprops on bothButtonelements.🤖 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 `@ui/src/pages/docs/components/DocExternalChangeDialog.tsx` around lines 25 - 50, Remove the global Enter-handling React.useEffect and its related keyboard logic from DocExternalChangeDialog, relying on native button activation for onIgnore and onDiscard. Remove the now-unused ignoreButtonRef and discardButtonRef declarations and ref props from both Button elements.internal/persis/file/doc/store.go (2)
81-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNormalize
baseDirinNew.
Newstores the caller-supplied path verbatim.safePath(line 103) andcleanEmptyParents(line 1710) compare raw strings againsts.baseDir. If the caller passes a path with a trailing separator or a.element, those comparisons no longer match the values produced byfilepath.Dirandfilepath.Join.cleanEmptyParentsdeletes directories, so an implicit invariant here has a wide blast radius. Clean the path once at construction and remove the assumption.♻️ Proposed normalization
func New(baseDir string) *Store { + baseDir = filepath.Clean(baseDir) _ = os.MkdirAll(baseDir, docDirPermissions) // best effort return &Store{ baseDir: baseDir,🤖 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 `@internal/persis/file/doc/store.go` around lines 81 - 89, Normalize baseDir in New with filepath.Clean before storing it in Store and before creating the directory, so safePath and cleanEmptyParents compare canonical paths consistently. Remove the implicit assumption that callers provide an already-clean path while preserving the existing initialization behavior.
242-267: 🚀 Performance & Scalability | 🔵 TrivialIndex refresh blocks all readers during a full directory walk.
ensureFreshIndextakes the exclusives.muwrite lock and callsrefreshIndexLocked, which walks the whole document tree and re-parses every file with a changed fingerprint. Every read path (List,ListFlat,listSearchCandidates) goes through this function. Once the 5-second interval expires, the next request performs the scan while all concurrent readers block on the same lock.This is acceptable for small document sets. For large trees consider one of these options:
- Run the refresh in a single background goroutine and let readers serve the current snapshot.
- Build the new index into a local map, then swap it in under a short write lock.
- Add a metric for refresh duration so the cost becomes visible before it degrades request latency.
🤖 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 `@internal/persis/file/doc/store.go` around lines 242 - 267, Refactor ensureFreshIndex so refreshIndexLocked does not hold the exclusive s.mu lock while walking and re-parsing the document tree. Build the refreshed index outside the write lock, then acquire s.mu only to atomically swap the completed snapshot and update its freshness metadata, while preserving safe behavior for concurrent readers and avoiding duplicate refresh work.internal/core/docs/doc.go (1)
138-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider rejecting Windows reserved device names in
ValidateDocID.The pattern accepts segments such as
con,nul,aux,com1, andlpt1. On Windows these names are reserved. Creatingdocs/con.mdfails or resolves to a device, so document creation and rename produce confusing errors on Windows builds. The trailing space and dot checks already handle the other Windows-specific naming rules, so this check completes the set.♻️ Proposed addition of a reserved-name check
+// windowsReservedNames lists device names that Windows rejects as file names. +var windowsReservedNames = map[string]struct{}{ + "con": {}, "prn": {}, "aux": {}, "nul": {}, + "com1": {}, "com2": {}, "com3": {}, "com4": {}, "com5": {}, + "com6": {}, "com7": {}, "com8": {}, "com9": {}, + "lpt1": {}, "lpt2": {}, "lpt3": {}, "lpt4": {}, "lpt5": {}, + "lpt6": {}, "lpt7": {}, "lpt8": {}, "lpt9": {}, +} + // ValidateDocID validates that id is a safe, well-formed doc identifier. func ValidateDocID(id string) error { if id == "" { return ErrInvalidDocID } @@ for segment := range strings.SplitSeq(id, "/") { if strings.HasSuffix(segment, " ") || strings.HasSuffix(segment, ".") { return fmt.Errorf("%w: path segments must not end with spaces or dots", ErrInvalidDocID) } + base, _, _ := strings.Cut(segment, ".") + if _, reserved := windowsReservedNames[strings.ToLower(base)]; reserved { + return fmt.Errorf("%w: %q is a reserved name", ErrInvalidDocID, base) + } } return nil }As per coding guidelines: "Use Go 1.26 and keep Go code compatible with Windows builds, since linting runs under
GOOS=windows."🤖 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 `@internal/core/docs/doc.go` around lines 138 - 155, Update ValidateDocID to reject path segments matching Windows reserved device names, including CON, NUL, AUX, COM1–COM9, and LPT1–LPT9, case-insensitively. Apply the check alongside the existing trailing-space and trailing-dot validation for every segment, returning ErrInvalidDocID with a clear reserved-name message while preserving all existing validation behavior.Source: Coding guidelines
internal/persis/file/doc/store_test.go (1)
955-966: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the expected case-insensitivity behavior instead of discarding the result.
The test discards
resultswith_ = resultsand asserts nothing beyond the absence of an error. It cannot fail if case handling regresses. The store's cursor search builds its pattern with a(?i)prefix indocSearchPattern, so the intended behavior is defined. Pin the expectation forSearchhere, or remove the test and cover case-insensitivity throughSearchCursor, where the behavior is explicit.💚 Proposed assertion
- // Search with different case — verify no error even if no match. - results, err := store.Search(ctx, "hello") - require.NoError(t, err) - // Grep may or may not be case-insensitive depending on implementation. - _ = results + // Cursor search lowercases the pattern with (?i), so a case difference must match. + results, err := store.SearchCursor(ctx, docs.SearchDocsOptions{ + Query: "hello", + Limit: 10, + MatchLimit: 1, + }) + require.NoError(t, err) + require.Len(t, results.Items, 1) + assert.Equal(t, "doc1", results.Items[0].ID)🤖 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 `@internal/persis/file/doc/store_test.go` around lines 955 - 966, Update TestSearchCaseDifferenceDoesNotError to assert that searching for “hello” returns the previously created “Hello World” document, using the expected result assertion helpers. Do not discard results; preserve the no-error check and pin the case-insensitive behavior implemented by docSearchPattern, or move coverage to SearchCursor if that is the explicit API path.internal/persis/file/doc/birthtime_linux.go (1)
24-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReconsider the
Ctimfallback.
Ctimis the inode change time. It updates on every content or metadata change, so it is usually later thanModTimeand is not a creation time. Whenstatxdoes not reportSTATX_BTIME(older kernels, or filesystems such as older ext4 or NFS), this branch reports a "created at" value that moves forward on each edit. The other platform variants fall back toModTime. UseModTimehere for consistent behavior.♻️ Proposed change
- if stat, ok := info.Sys().(*syscall.Stat_t); ok { - return time.Unix(int64(stat.Ctim.Sec), int64(stat.Ctim.Nsec)) - } return info.ModTime()Remove the now-unused
syscallimport if you apply this.🤖 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 `@internal/persis/file/doc/birthtime_linux.go` around lines 24 - 27, Update the birth-time fallback in the shown file to return info.ModTime() instead of reading stat.Ctim, matching the behavior of other platform variants. Remove the now-unused syscall import.
🤖 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 `@internal/gitsync/service.go`:
- Around line 535-567: Update scanDocFiles so itemID removes the matched file’s
actual extension by slicing filepath.ToSlash(relPath) using the length of
filepath.Ext(filePath), rather than strings.TrimSuffix with the lowercase
docExtension constant. Preserve case-insensitive document matching while
ensuring files such as deploy.MD produce an ID without the extension.
- Line 536: Update newSyncService and the Git Sync service configuration to
carry ctx.Config.Paths.DocsDir into serviceImpl, then use that configured path
when assigning docsDir in scanDocFiles instead of constructing it from s.dagsDir
and docsDir. Preserve scanning behavior for the configured documents directory,
including paths outside the DAGs directory.
In `@internal/gitsync/state.go`:
- Around line 217-227: Update normalizeTrackedItems to stop deleting entries
when itemState.Kind is nonempty but unrecognized. Preserve the existing state,
including sync and conflict metadata, and let the subsequent
KindForDAGID(itemID) assignment normalize the kind; retain deletion only for
other explicitly supported removal cases, if any.
In `@internal/persis/file/doc/metadata.go`:
- Around line 132-149: Update Store.renameDocCreatedAtPrefix to avoid inserting
or deleting entries in metadata.CreatedAt while ranging over it. Build a
separate result map from the original entries, remapping oldID and
oldID-prefixed keys to their newID equivalents, then assign the completed map
back to metadata.CreatedAt before calling saveMetadata.
- Around line 88-130: Guard all document-createdAt metadata mutations with the
existing Store.mutationMu by updating the callers or the methods
setDocCreatedAt, deleteDocCreatedAt, deleteDocCreatedAtPrefix,
renameDocCreatedAt, and renameDocCreatedAtPrefix to use the same in-process lock
around the full load, map mutation, and save sequence. Prefer the existing
public guarded API when available, and preserve the required local file-lock
behavior for shared document directories.
In `@internal/persis/file/doc/rename_noreplace_darwin.go`:
- Around line 15-21: Update renameNoReplace to detect unix.ENOTSUP from
unix.RenameatxNp and fall back to fileutil.Rename(oldPath, newPath) for
unsupported filesystems. Preserve the existing docs.ErrDocAlreadyExists handling
for unix.EEXIST and return other errors unchanged.
In `@internal/persis/file/doc/rename_noreplace_other.go`:
- Around line 15-21: Update renameNoReplace to use an atomic no-replace rename
primitive instead of fileutil.Rename, ensuring an existing destination is never
replaced; retain the existing pre-check for cross-process TOCTOU protection and
continue mapping destination-exists failures to docs.ErrDocAlreadyExists.
In `@internal/persis/file/doc/store_test.go`:
- Around line 1276-1285: Update the comment in TestListFlatMissingBaseDir to
describe the current behavior: rebuildIndexLocked returns early when os.Stat
finds that the base directory is missing, so ListFlat returns empty results
without an error. Remove the outdated reference to filepath.WalkDir and
swallowed errors.
In `@internal/persis/file/doc/store.go`:
- Around line 865-867: In internal/persis/file/doc/store.go at Create (865-867),
Update (903-905), Rename (1133-1135), and Rename (1182-1184), treat created-at
sidecar errors as best effort: log each setDocCreatedAt, renameDocCreatedAt, or
renameDocCreatedAtPrefix error with logger.Warn, then continue the existing
mutation cleanup and index-update calls instead of returning the error.
In `@internal/service/frontend/api/v1/docs.go`:
- Around line 705-716: Update GetDocTreeData to return the error from
url.ParseQuery instead of replacing invalid results with empty url.Values. Match
the existing error-handling behavior in GetDocContentData, while preserving the
current parameter parsing and docReadScopeForParams flow for successful queries.
In `@ui/src/api/v1/schema.ts`:
- Around line 15095-15114: Add a 400 response definition to the deleteDocBatch
operation alongside its existing 200 and default responses, using the
appropriate Error schema, then regenerate the API-derived files with make api so
clients distinguish validation failures.
In `@ui/src/hooks/useDocSSE.ts`:
- Around line 15-25: Update the useDocSSE call to useSSE so it forwards the
remoteNode argument along with the endpoint and enabled condition. Preserve the
existing endpoint construction and ensure the SSE subscription and docs endpoint
resolve the same node.
In `@ui/src/layouts/ContentNavigation.tsx`:
- Around line 114-122: Update the docs breadcrumb construction in the segments
loop so the final docs segment is added without a to value, matching the dags
and generic breadcrumb branches; retain links for preceding segments and the
existing Docs breadcrumb.
In `@ui/src/pages/docs/components/DocArboristNode.tsx`:
- Around line 177-192: Add an aria-label to the inline rename input in the
DocArboristNode component, matching the accessible naming approach used by the
nearby dropdown trigger. Keep the existing input behavior and styling unchanged.
In `@ui/src/pages/docs/components/DocEditor.tsx`:
- Around line 270-279: Update the keyboard shortcut effect in DocEditor to
recognize both lowercase and uppercase S, and only call preventDefault or
handleSaveRef.current when the current user can edit. Add and keep canEditRef
synchronized with canEdit alongside handleSaveRef so the stable keydown listener
uses the latest permission state without blocking the browser save shortcut for
read-only users.
In `@ui/src/pages/docs/components/DocTabBar.tsx`:
- Around line 54-82: Update the tab keyboard navigation in handleKeyDown and the
tab rendering logic to store refs for each tab element and focus the newly
selected tab after ArrowLeft or ArrowRight changes activeTab. Preserve the
existing wrapping behavior and ensure the ref targets remain synchronized with
the roving tabIndex so visible focus follows the active tab.
In `@ui/src/pages/docs/components/DocTreeSidebar.tsx`:
- Around line 424-452: Update handleMove to wrap each awaited onMove call in
try/catch, report failures through the component’s established error-reporting
mechanism, and deliberately choose whether to continue processing later dragIds
or stop after the first failure. Ensure the caught rejection does not escape the
react-arborist callback as an unhandled promise rejection.
In `@ui/src/pages/git-sync/sync-kind.ts`:
- Around line 58-60: Update deriveSyncKindFromItemId to accept the configured
document directory and classify IDs using that directory instead of hardcoded
docs/. Thread paths.docsDir through the caller into MoveDialog and its
classification logic, preserving the backend’s configured-path behavior. Add
Vitest coverage for both the default document directory and a non-default
directory.
---
Outside diff comments:
In `@internal/service/frontend/sse/app_stream.go`:
- Around line 244-266: Update Start to close w.watcher before returning when
addWatch(w.root) fails or when adding any path from watchPathsForScope fails,
matching the existing cleanup for watchPathsForScope errors; preserve the
original error after cleanup.
---
Nitpick comments:
In `@internal/core/docs/doc.go`:
- Around line 138-155: Update ValidateDocID to reject path segments matching
Windows reserved device names, including CON, NUL, AUX, COM1–COM9, and
LPT1–LPT9, case-insensitively. Apply the check alongside the existing
trailing-space and trailing-dot validation for every segment, returning
ErrInvalidDocID with a clear reserved-name message while preserving all existing
validation behavior.
In `@internal/persis/file/doc/birthtime_linux.go`:
- Around line 24-27: Update the birth-time fallback in the shown file to return
info.ModTime() instead of reading stat.Ctim, matching the behavior of other
platform variants. Remove the now-unused syscall import.
In `@internal/persis/file/doc/store_test.go`:
- Around line 955-966: Update TestSearchCaseDifferenceDoesNotError to assert
that searching for “hello” returns the previously created “Hello World”
document, using the expected result assertion helpers. Do not discard results;
preserve the no-error check and pin the case-insensitive behavior implemented by
docSearchPattern, or move coverage to SearchCursor if that is the explicit API
path.
In `@internal/persis/file/doc/store.go`:
- Around line 81-89: Normalize baseDir in New with filepath.Clean before storing
it in Store and before creating the directory, so safePath and cleanEmptyParents
compare canonical paths consistently. Remove the implicit assumption that
callers provide an already-clean path while preserving the existing
initialization behavior.
- Around line 242-267: Refactor ensureFreshIndex so refreshIndexLocked does not
hold the exclusive s.mu lock while walking and re-parsing the document tree.
Build the refreshed index outside the write lock, then acquire s.mu only to
atomically swap the completed snapshot and update its freshness metadata, while
preserving safe behavior for concurrent readers and avoiding duplicate refresh
work.
In `@internal/service/frontend/api/v1/docs.go`:
- Around line 465-482: Update SearchDocs to use a scoped docs.DocStore search
when the store exposes the scoped Search variant, passing workspaceName as
PathPrefix and the visibility-derived roots as ExcludePathRoots, consistent with
SearchDocFeed. Preserve the existing filtering and ID normalization behavior as
a safety check before constructing the response.
In `@internal/service/frontend/api/v1/search.go`:
- Around line 296-306: Add a short comment above the retry branch in the search
handler around SearchMatches explaining that SearchDocFeed may mint an
aggregate-scope cursor, which can be replayed with a workspace and yield
exec.ErrInvalidCursor when PathPrefix differs; document that the retry restores
aggregate scope while preserving the authorized aggregatePath.
In `@internal/service/frontend/sse/app_stream_test.go`:
- Around line 108-112: Update the test around watcher.Start and watcher.check to
avoid concurrent snapshot access: seed the watcher snapshot directly without
starting the polling loop, or explicitly document that the one-hour interval
keeps the loop idle. Prefer the no-loop setup so the test remains race-safe if
the default interval or Start behavior changes.
In `@internal/service/frontend/sse/app_stream.go`:
- Around line 343-358: Update the WalkDir callback in addCreatedDirWatches to
ignore errors wrapping os.ErrNotExist and continue the recursive walk, including
when the created directory or a child disappears before traversal. Preserve
returning all other errors so genuine failures still reach the caller without
triggering unnecessary behavior changes.
In `@ui/src/contexts/DocTabContext.tsx`:
- Around line 196-233: Update closeTab and the related mutators closeOtherTabs,
setDraft, clearDraft, markTabUnsaved, and markTabSaved so their state updater
callbacks only compute and return next values. Move setActiveTabId calls,
draftsRef/unsavedTabIdsRef writes, and persistState calls outside updater
functions, relying on the existing state-persistence effect and render-time ref
synchronization to handle updates without duplicate side effects.
In `@ui/src/hooks/__tests__/useContentEditor.test.ts`:
- Around line 8-31: Extend the useContentEditor test suite with Vitest coverage
for cancelSave: begin a save, call result.current.cancelSave(), then simulate an
external serverContent change and assert conflict.hasConflict becomes true. Keep
the existing server-echo acceptance test unchanged and use the hook’s public
API.
In `@ui/src/pages/docs/components/DocEditor.tsx`:
- Around line 334-347: Improve accessibility for the toolbar controls in
DocEditor: add an aria-label to the icon-only copy-file-path button using
copyFilePath, and add aria-pressed to both mode toggle buttons in the related
controls, reflecting each button’s active/selected state.
- Around line 284-320: Extract the shared clipboard write and textarea fallback
from copyFilePath and copyContent into a reusable copyText helper, then have
both callbacks call it while retaining their existing empty-value guards and
copied-state reset behavior. Keep the helper responsible only for copying text;
leave the file-path and content-specific state updates in their respective
callbacks.
In `@ui/src/pages/docs/components/DocExternalChangeDialog.tsx`:
- Around line 25-50: Remove the global Enter-handling React.useEffect and its
related keyboard logic from DocExternalChangeDialog, relying on native button
activation for onIgnore and onDiscard. Remove the now-unused ignoreButtonRef and
discardButtonRef declarations and ref props from both Button elements.
In `@ui/src/pages/docs/index.tsx`:
- Around line 203-205: Update the Docs page useEffect dependency array to depend
on the stable setTitle function rather than the entire appBarContext object,
matching the existing pattern in the Home page while preserving the
title-setting behavior.
- Around line 495-498: Update the delete failure branch in the document deletion
handler to include the server-provided error message via error?.message,
matching handleRenameModal and handlePathChange, while retaining the existing
fallback toast text when no message is available.
- Around line 705-745: Extract the shared CreateDocModal, RenameDocModal, and
both ConfirmModal declarations from the page component into a single modals
variable, preserving all existing props and handlers. Replace the duplicated
modal blocks in the mobile and desktop layout branches with {modals} so both
branches render the same definitions.
In `@ui/src/pages/docs/lib/doc-validation.ts`:
- Around line 11-31: Update the doc path validation function to use the existing
trimmed value for all subsequent length, extension, pattern, and segment checks,
while preserving the required-path check. Extract the 252-character limit into a
named constant documenting that it mirrors the backend document ID limit, and
update the affected test expectation for validateDocPath('guide ') to reflect
the trimmed valid path result.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c1c6b783-bc1d-4ced-8ee5-4beaa859cb29
⛔ Files ignored due to path filters (2)
assets/images/readme-documents-dark.pngis excluded by!**/*.pngui/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (100)
README.mdapi/v1/api.gen.goapi/v1/api.yamlinternal/cmd/config.gointernal/cmd/config_test.gointernal/cmd/process/frontend_store_factories.gointernal/cmn/config/config.gointernal/cmn/config/definition.gointernal/cmn/config/loader.gointernal/cmn/config/loader_test.gointernal/cmn/value/builtin_context_test.gointernal/cmn/value/template.gointernal/core/docs/doc.gointernal/core/exec/context.gointernal/core/exec/context_env.gointernal/core/exec/context_test.gointernal/core/exec/env.gointernal/core/spec/builder.gointernal/gitsync/errors.gointernal/gitsync/service.gointernal/gitsync/service_test.gointernal/gitsync/state.gointernal/persis/file/doc/birthtime_darwin.gointernal/persis/file/doc/birthtime_linux.gointernal/persis/file/doc/birthtime_other.gointernal/persis/file/doc/birthtime_windows.gointernal/persis/file/doc/metadata.gointernal/persis/file/doc/rename_noreplace_darwin.gointernal/persis/file/doc/rename_noreplace_linux.gointernal/persis/file/doc/rename_noreplace_other.gointernal/persis/file/doc/store.gointernal/persis/file/doc/store_test.gointernal/persis/file/service_stores.gointernal/runtime/builtin/harness/harness.gointernal/runtime/builtin/harness/root_container_external_test.gointernal/runtime/builtin_context_test.gointernal/runtime/env_test.gointernal/runtime/eval.gointernal/service/audit/entry.gointernal/service/frontend/api/v1/api.gointernal/service/frontend/api/v1/docs.gointernal/service/frontend/api/v1/docs_test.gointernal/service/frontend/api/v1/search.gointernal/service/frontend/api/v1/sync.gointernal/service/frontend/api/v1/sync_test.gointernal/service/frontend/persistence.gointernal/service/frontend/server.gointernal/service/frontend/sse/app_stream.gointernal/service/frontend/sse/app_stream_test.gointernal/service/frontend/sse/topic_parse.gointernal/service/frontend/sse/types.gointernal/service/frontend/templates.gointernal/service/frontend/templates/base.gohtmlui/package.jsonui/src/App.tsxui/src/__tests__/App.test.tsxui/src/api/v1/schema.tsui/src/contexts/ConfigContext.tsxui/src/contexts/DocTabContext.tsxui/src/features/search/components/SearchResult.tsxui/src/features/search/components/__tests__/SearchResult.test.tsxui/src/features/system-status/components/PathsCard.tsxui/src/hooks/SSEManager.tsui/src/hooks/__tests__/SSEManager.test.tsui/src/hooks/__tests__/useContentEditor.test.tsui/src/hooks/useContentEditor.tsui/src/hooks/useDocSSE.tsui/src/hooks/useDocTreeSSE.tsui/src/layouts/ContentNavigation.tsxui/src/menu.tsxui/src/pages/__tests__/DocArboristNode.test.tsxui/src/pages/audit-logs/index.tsxui/src/pages/docs/components/CreateDocModal.tsxui/src/pages/docs/components/DocArboristNode.tsxui/src/pages/docs/components/DocEditor.tsxui/src/pages/docs/components/DocExternalChangeDialog.tsxui/src/pages/docs/components/DocOutlinePanel.tsxui/src/pages/docs/components/DocTabBar.tsxui/src/pages/docs/components/DocTabEditorPanel.tsxui/src/pages/docs/components/DocTreeSidebar.tsxui/src/pages/docs/components/RenameDocModal.tsxui/src/pages/docs/index.tsxui/src/pages/docs/lib/__tests__/doc-mutation.test.tsui/src/pages/docs/lib/__tests__/doc-url.test.tsui/src/pages/docs/lib/__tests__/doc-validation.test.tsui/src/pages/docs/lib/doc-mutation.tsui/src/pages/docs/lib/doc-polling.tsui/src/pages/docs/lib/doc-url.tsui/src/pages/docs/lib/doc-validation.tsui/src/pages/git-sync/BatchDeleteDialog.tsxui/src/pages/git-sync/CleanupDialog.tsxui/src/pages/git-sync/DeleteDialog.tsxui/src/pages/git-sync/DeleteMissingDialog.tsxui/src/pages/git-sync/ForgetDialog.tsxui/src/pages/git-sync/MoveDialog.tsxui/src/pages/git-sync/index.tsxui/src/pages/git-sync/sync-kind.tsui/src/pages/git-sync/useSyncReconcile.tsui/src/pages/home/index.tsxui/src/pages/search/index.tsx
|
Addressed the review in Top-level findings:
Validation completed: full race-enabled Go suite, full UI suite, production UI build, TypeScript typecheck, macOS and Windows Go lint, and Windows cross-compilation. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/hooks/__tests__/useContentEditor.test.ts (1)
20-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTest
markAsSavedindependently.The matching server echo already makes the editor clean before
markAsSavedruns. The final assertion passes even ifmarkAsSavedstops updating the saved-content reference.Assert that the echo clears
hasUnsavedChanges. Then add a separate local edit, assert it is unsaved, callmarkAsSaved, rerender without changingserverContent, and assert that it is clean.Proposed test update
rerender({ serverContent: '# Saved' }); expect(result.current.conflict.hasConflict).toBe(false); + expect(result.current.hasUnsavedChanges).toBe(false); + + act(() => { + result.current.setCurrentValue('# Saved again'); + }); + expect(result.current.hasUnsavedChanges).toBe(true); act(() => { - result.current.markAsSaved('# Saved'); + result.current.markAsSaved('# Saved again'); }); rerender({ serverContent: '# Saved' }); expect(result.current.hasUnsavedChanges).toBe(false);As per coding guidelines, add or update tests appropriate to the changed code.
🤖 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 `@ui/src/hooks/__tests__/useContentEditor.test.ts` around lines 20 - 29, Update the test around the hook’s server-echo handling and markAsSaved flow so it first asserts the matching server content clears hasUnsavedChanges without calling markAsSaved. Then perform a separate local edit, verify it becomes unsaved, call result.current.markAsSaved with that content, rerender while keeping serverContent unchanged, and assert hasUnsavedChanges is false to independently validate markAsSaved.Source: Coding guidelines
🧹 Nitpick comments (1)
ui/src/hooks/__tests__/useContentEditor.test.ts (1)
10-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a props type for the
renderHookcallbacks.The test callbacks both destructure
{ serverContent }without a TypeScript annotation. Define one shared props type and use it for both callbacks.
ui/src/hooks/__tests__/useContentEditor.test.ts#L10-L12ui/src/hooks/__tests__/useContentEditor.test.ts#L33-L35🤖 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 `@ui/src/hooks/__tests__/useContentEditor.test.ts` around lines 10 - 12, Define one shared props type for the renderHook callbacks in ui/src/hooks/__tests__/useContentEditor.test.ts at lines 10-12 and 33-35, then annotate both destructured callback parameters with that type while preserving their existing serverContent behavior.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 `@ui/src/contexts/DocTabContext.tsx`:
- Around line 281-287: Update setDraft in DocTabContext to ignore drafts when
the direct tabId or its encoded key does not match any tab in tabsRef.current.
Only add the draft to the copied drafts map for an open tab, preventing
DocEditor unmount cleanup from persisting discarded content after closure.
---
Outside diff comments:
In `@ui/src/hooks/__tests__/useContentEditor.test.ts`:
- Around line 20-29: Update the test around the hook’s server-echo handling and
markAsSaved flow so it first asserts the matching server content clears
hasUnsavedChanges without calling markAsSaved. Then perform a separate local
edit, verify it becomes unsaved, call result.current.markAsSaved with that
content, rerender while keeping serverContent unchanged, and assert
hasUnsavedChanges is false to independently validate markAsSaved.
---
Nitpick comments:
In `@ui/src/hooks/__tests__/useContentEditor.test.ts`:
- Around line 10-12: Define one shared props type for the renderHook callbacks
in ui/src/hooks/__tests__/useContentEditor.test.ts at lines 10-12 and 33-35,
then annotate both destructured callback parameters with that type while
preserving their existing serverContent behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78456a52-69ea-42f7-9d22-0ab3c3cfca1c
📒 Files selected for processing (34)
api/v1/api.gen.goapi/v1/api.yamlinternal/cmd/sync.gointernal/core/docs/doc.gointernal/gitsync/pull_external_test.gointernal/gitsync/service.gointernal/gitsync/service_test.gointernal/persis/file/doc/birthtime_linux.gointernal/persis/file/doc/metadata.gointernal/persis/file/doc/rename_noreplace_darwin.gointernal/persis/file/doc/rename_noreplace_fallback.gointernal/persis/file/doc/rename_noreplace_linux.gointernal/persis/file/doc/rename_noreplace_other.gointernal/persis/file/doc/rename_noreplace_windows.gointernal/persis/file/doc/store.gointernal/persis/file/doc/store_test.gointernal/service/frontend/api/v1/docs.gointernal/service/frontend/api/v1/docs_test.gointernal/service/frontend/api/v1/search.gointernal/service/frontend/server.gointernal/service/frontend/sse/app_stream.gointernal/service/frontend/sse/app_stream_test.goui/src/api/v1/schema.tsui/src/contexts/DocTabContext.tsxui/src/hooks/__tests__/useContentEditor.test.tsui/src/hooks/useDocSSE.tsui/src/pages/docs/components/DocArboristNode.tsxui/src/pages/docs/components/DocEditor.tsxui/src/pages/docs/components/DocExternalChangeDialog.tsxui/src/pages/docs/components/DocTabBar.tsxui/src/pages/docs/components/DocTabEditorPanel.tsxui/src/pages/docs/index.tsxui/src/pages/docs/lib/__tests__/doc-validation.test.tsui/src/pages/docs/lib/doc-validation.ts
💤 Files with no reviewable changes (1)
- internal/persis/file/doc/birthtime_linux.go
🚧 Files skipped from review as they are similar to previous changes (20)
- internal/persis/file/doc/rename_noreplace_darwin.go
- internal/persis/file/doc/metadata.go
- internal/service/frontend/sse/app_stream_test.go
- internal/persis/file/doc/rename_noreplace_linux.go
- ui/src/pages/docs/lib/doc-validation.ts
- internal/gitsync/service_test.go
- ui/src/pages/docs/lib/tests/doc-validation.test.ts
- internal/core/docs/doc.go
- internal/service/frontend/server.go
- ui/src/hooks/useDocSSE.ts
- internal/service/frontend/api/v1/search.go
- ui/src/pages/docs/components/DocArboristNode.tsx
- internal/service/frontend/sse/app_stream.go
- ui/src/pages/docs/components/DocEditor.tsx
- ui/src/pages/docs/index.tsx
- internal/persis/file/doc/store.go
- internal/service/frontend/api/v1/docs.go
- ui/src/api/v1/schema.ts
- internal/service/frontend/api/v1/docs_test.go
- api/v1/api.gen.go
|
Follow-up on the latest pre-merge warnings:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Addressed all CodeRabbit findings from the e573eb1 review in 824a5ee.
Validation: make fmt; full race-enabled make test; all UI tests; TypeScript typecheck; changed-file ESLint; and production UI build. Individual replies were posted on both inline threads and both threads are resolved. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Pre-merge warning clarification:
The final incremental CodeRabbit review reported no actionable comments, and all review threads are resolved. |
|
Cleanup follow-up pushed in e9550ac..691e28b.
Validation: make fmt; make check; full race-enabled make test; frontend typecheck; all frontend unit tests; targeted Docs ESLint; and production frontend build. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/frontend/api/v1/sync.go (1)
696-704: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve the tracked Markdown extension for document file paths.
syncItemFilePathreturnsitemID + ".md"for every document. Git Sync preserves the local extension casing for documents;scanDocFilesstores.MDfordeploy.MD, andTestScanLocalDAGsasserts that value. For such an item the API reportsdocs/operations/deploy.md, which does not match the file on disk. Use the tracked extension when it is a Markdown extension.🔧 Proposed fix
func syncItemFilePath(itemID, fileExtension string) string { if gitsync.SyncItemKindForID(itemID) == gitsync.SyncItemKindDoc { + if strings.EqualFold(fileExtension, ".md") { + return itemID + fileExtension + } return itemID + ".md" }🤖 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 `@internal/service/frontend/api/v1/sync.go` around lines 696 - 704, Update syncItemFilePath to preserve the tracked Markdown extension for document items instead of always appending lowercase ".md". When fileExtension represents Markdown, return itemID with that extension and retain its casing; preserve the existing YAML/YML handling for non-document items.
🧹 Nitpick comments (7)
ui/src/pages/docs/hooks/__tests__/useDocDraftPersistence.test.tsx (1)
19-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth new test files rely on
vi.clearAllMocks()to reset mock behavior.vi.clearAllMocks()clears call history only. Return values and resolved values set withmockReturnValueormockResolvedValuepersist into later tests, so the tests become order dependent.
ui/src/pages/docs/hooks/__tests__/useDocDraftPersistence.test.tsx#L19-L22: replacevi.clearAllMocks()withvi.resetAllMocks()so themocks.getDraftreturn value from the restore test does not leak.ui/src/pages/docs/hooks/__tests__/useDocMutations.test.tsx#L40-L44: replacevi.clearAllMocks()withvi.resetAllMocks()so themocks.client.POSTresolved value from the rename test does not leak.🤖 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 `@ui/src/pages/docs/hooks/__tests__/useDocDraftPersistence.test.tsx` around lines 19 - 22, Replace vi.clearAllMocks() with vi.resetAllMocks() in the beforeEach blocks of ui/src/pages/docs/hooks/__tests__/useDocDraftPersistence.test.tsx lines 19-22 and ui/src/pages/docs/hooks/__tests__/useDocMutations.test.tsx lines 40-44, ensuring mock return and resolved values from tests such as the restore and rename tests do not persist between cases.internal/persis/file/doc/tree_test.go (1)
126-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the directory mtime assertion distinguish the two orderings.
dir-newprecedesdir-oldboth by descending propagated mtime and by ascending name.TestListTreeSortMtimeKeepsDirectoriesAlphabeticalshows that directories keep alphabetical order under mtime sort, so this test passes under either rule and does not prove propagation. Choose names whose alphabetical order opposes the expected mtime order, for examplea-diras the oldest andb-diras the newest.🤖 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 `@internal/persis/file/doc/tree_test.go` around lines 126 - 143, Update TestListTreeSortMtimeWithDirectories to use directory names whose alphabetical order opposes their modification-time order, such as making a-dir the older directory and b-dir the newer one. Adjust the created paths, setModTime targets, expected item IDs, and ModTime assertion so the test specifically verifies descending propagated directory mtime rather than alphabetical ordering.internal/cmd/sync.go (1)
246-257: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one sort helper in this file.
Line 252 uses
sort.Strings, while the new code at Lines 123, 433, and 527 usesslices.Sort. Useslices.Sorthere for consistency.🤖 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 `@internal/cmd/sync.go` around lines 246 - 257, Update the itemIDs sorting in the sync command flow around PublishAll to use slices.Sort instead of sort.Strings, matching the sorting approach already used elsewhere in the file. Ensure the corresponding sort package import is removed if no longer referenced.internal/persis/file/service_stores.go (1)
161-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local variable to avoid shadowing the
storepackage.The file imports a package named
storeand uses it inNewDAGSettingsStore. The local variablestorehere shadows that package inside this function. Rename it for clarity.♻️ Proposed change
- store, err := filedoc.New(cfg.Paths.DocsDir) + docStore, err := filedoc.New(cfg.Paths.DocsDir) if err != nil { return nil, fmt.Errorf("document store: %w", err) } - return store, nil + return docStore, nil🤖 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 `@internal/persis/file/service_stores.go` around lines 161 - 165, Rename the local variable assigned from filedoc.New in the document store constructor, and update the return statement to use the new name, avoiding shadowing of the imported store package while preserving the existing error handling.internal/service/frontend/api/v1/docs_search_test.go (1)
132-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse checked type assertions for the response values.
Lines 132 and 150 use unchecked type assertions. If the handler returns an error response, the test panics instead of reporting a clear failure. The other tests in this file use
require.True(t, ok). Apply the same pattern here.♻️ Proposed change
- feedPage := feedResp.(apigen.SearchDocFeed200JSONResponse) + feedPage, ok := feedResp.(apigen.SearchDocFeed200JSONResponse) + require.True(t, ok)- matchesPage := matchesResp.(apigen.SearchDocMatches200JSONResponse) + matchesPage, ok := matchesResp.(apigen.SearchDocMatches200JSONResponse) + require.True(t, ok)Also applies to: 150-150
🤖 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 `@internal/service/frontend/api/v1/docs_search_test.go` at line 132, Update the response assertions around feedPage and the corresponding response at the second unchecked assertion to use checked type assertions, then call require.True(t, ok) before using the asserted values so unexpected error responses produce a clear test failure instead of panicking.internal/gitsync/service_test.go (1)
130-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to match
scanLocalItems.The test now exercises
scanLocalItemsand asserts document classification, but the name still saysTestScanLocalDAGs. Rename it toTestScanLocalItemsso the coverage target is clear.🤖 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 `@internal/gitsync/service_test.go` around lines 130 - 157, Rename the test function TestScanLocalDAGs to TestScanLocalItems so its name matches the scanLocalItems method it exercises; leave the test body unchanged.internal/persis/file/doc/search.go (1)
41-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign
SearchwithSearchCursormatch semantics.
DefaultGrepOptionsuses regexp matching, butSearchsends the rawquerywhileSearchCursorusesdocSearchPattern()with(?i)plusregexp.QuoteMeta. Use the same pattern construction in both paths.♻️ Proposed change for `Search`
- matches, err := grep.Grep(data, query, grep.DefaultGrepOptions) + matches, err := grep.Grep(data, docSearchPattern(query), grep.GrepOptions{ + IsRegexp: true, + Before: grep.DefaultGrepOptions.Before, + After: grep.DefaultGrepOptions.After, + })🤖 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 `@internal/persis/file/doc/search.go` around lines 41 - 44, Update Search’s grep pattern construction around grep.Grep to use docSearchPattern(query), matching SearchCursor’s case-insensitive, regexp-escaped semantics instead of passing the raw query; leave the existing match handling unchanged.
🤖 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 `@internal/cmd/sync.go`:
- Line 660: Update the completion message in the sync item move flow to start
with an uppercase letter, changing it to “Sync item moved successfully” while
leaving the surrounding behavior unchanged.
- Around line 110-112: Update the counts-section header in the sync command to
replace “DAG Status Counts” with wording that reflects all included items,
including documents, and remains consistent with “Sync items with pending
changes.” Locate the header near the pending-change output in the sync flow.
In `@internal/gitsync/state.go`:
- Around line 217-228: Update normalizeTrackedItems to delete entries whose
itemState is nil from state.Items before continuing, rather than retaining them.
Preserve the existing validation and Kind normalization behavior for non-nil
entries.
In `@internal/persis/file/doc/store_test.go`:
- Around line 1280-1284: Update TestNewReturnsBaseDirCreationError to create a
temporary regular file, then pass a child path beneath that file to New so
directory creation reliably fails on Windows and other platforms. Replace the
POSIX-specific "/dev/null/impossible" fixture while preserving the existing
error and nil-store assertions.
---
Outside diff comments:
In `@internal/service/frontend/api/v1/sync.go`:
- Around line 696-704: Update syncItemFilePath to preserve the tracked Markdown
extension for document items instead of always appending lowercase ".md". When
fileExtension represents Markdown, return itemID with that extension and retain
its casing; preserve the existing YAML/YML handling for non-document items.
---
Nitpick comments:
In `@internal/cmd/sync.go`:
- Around line 246-257: Update the itemIDs sorting in the sync command flow
around PublishAll to use slices.Sort instead of sort.Strings, matching the
sorting approach already used elsewhere in the file. Ensure the corresponding
sort package import is removed if no longer referenced.
In `@internal/gitsync/service_test.go`:
- Around line 130-157: Rename the test function TestScanLocalDAGs to
TestScanLocalItems so its name matches the scanLocalItems method it exercises;
leave the test body unchanged.
In `@internal/persis/file/doc/search.go`:
- Around line 41-44: Update Search’s grep pattern construction around grep.Grep
to use docSearchPattern(query), matching SearchCursor’s case-insensitive,
regexp-escaped semantics instead of passing the raw query; leave the existing
match handling unchanged.
In `@internal/persis/file/doc/tree_test.go`:
- Around line 126-143: Update TestListTreeSortMtimeWithDirectories to use
directory names whose alphabetical order opposes their modification-time order,
such as making a-dir the older directory and b-dir the newer one. Adjust the
created paths, setModTime targets, expected item IDs, and ModTime assertion so
the test specifically verifies descending propagated directory mtime rather than
alphabetical ordering.
In `@internal/persis/file/service_stores.go`:
- Around line 161-165: Rename the local variable assigned from filedoc.New in
the document store constructor, and update the return statement to use the new
name, avoiding shadowing of the imported store package while preserving the
existing error handling.
In `@internal/service/frontend/api/v1/docs_search_test.go`:
- Line 132: Update the response assertions around feedPage and the corresponding
response at the second unchecked assertion to use checked type assertions, then
call require.True(t, ok) before using the asserted values so unexpected error
responses produce a clear test failure instead of panicking.
In `@ui/src/pages/docs/hooks/__tests__/useDocDraftPersistence.test.tsx`:
- Around line 19-22: Replace vi.clearAllMocks() with vi.resetAllMocks() in the
beforeEach blocks of
ui/src/pages/docs/hooks/__tests__/useDocDraftPersistence.test.tsx lines 19-22
and ui/src/pages/docs/hooks/__tests__/useDocMutations.test.tsx lines 40-44,
ensuring mock return and resolved values from tests such as the restore and
rename tests do not persist between cases.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b742bae-c9ce-4d66-a301-ed573c6a218a
📒 Files selected for processing (25)
internal/cmd/sync.gointernal/gitsync/pull_external_test.gointernal/gitsync/service.gointernal/gitsync/service_test.gointernal/gitsync/state.gointernal/persis/file/doc/search.gointernal/persis/file/doc/store.gointernal/persis/file/doc/store_test.gointernal/persis/file/doc/tree.gointernal/persis/file/doc/tree_test.gointernal/persis/file/service_stores.gointernal/service/frontend/api/v1/docs.gointernal/service/frontend/api/v1/docs_response.gointernal/service/frontend/api/v1/docs_search_test.gointernal/service/frontend/api/v1/docs_test.gointernal/service/frontend/api/v1/docs_visibility.gointernal/service/frontend/api/v1/sync.gointernal/service/frontend/api/v1/sync_test.goui/src/contexts/DocTabContext.tsxui/src/pages/docs/components/DocEditor.tsxui/src/pages/docs/hooks/__tests__/useDocDraftPersistence.test.tsxui/src/pages/docs/hooks/__tests__/useDocMutations.test.tsxui/src/pages/docs/hooks/useDocDraftPersistence.tsui/src/pages/docs/hooks/useDocMutations.tsui/src/pages/docs/index.tsx
💤 Files with no reviewable changes (2)
- internal/service/frontend/api/v1/docs_test.go
- internal/service/frontend/api/v1/docs.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/gitsync/pull_external_test.go
- ui/src/pages/docs/components/DocEditor.tsx
- ui/src/contexts/DocTabContext.tsx
- internal/service/frontend/api/v1/sync_test.go
- ui/src/pages/docs/index.tsx
|
Addressed the cleanup review in ff871a4.
The Search suggestion was intentionally not applied: Store.Search is the restored legacy pattern-search API (regex and case-sensitive), while SearchCursor/SearchMatches power global search and intentionally treat the user query as a case-insensitive literal. Existing contract tests cover that distinction. Validation: focused Go packages; all frontend unit tests; targeted ESLint; and clean Linux/Windows make check. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Final cleanup and review pass is complete.
|
Summary
docs/**alongside DAG definitionsWhy
Documents previously provided an integrated place to capture business logic and scripts next to workflows. This restores that workflow while preserving the current Git Sync safety model.
Closes #2368
Testing
make testgo test ./internal/gitsync ./internal/service/frontend/api/v1make bincd ui && pnpm testcd ui && pnpm typecheckcd ui && pnpm build.local/bin/golangci-lint run --timeout=10m ./...GOOS=windows .local/bin/golangci-lint run --timeout=10m ./...Summary by cubic
Restores Documents with a workspace-aware Markdown editor and tree, plus Git Sync for docs. Adds doc search, SSE live updates with a 30s fallback, and explicit config/runtime support.
New Features
paths.docs_dir(defaults to<dags_dir>/docs).react-arborist), tabbed Markdown + Mermaid preview, outline panel, external-change dialog, drafts/unsaved handling; System Paths anddagu configshow “Docs directory”; global search includes docs with match fetching.docsendpoints;/search/docsand match endpoints; SSE topicsdocanddoctreewith recursive watching and a 30s polling fallback.context.paths.docs_dirand envDAG_DOCS_DIR; addspaths.docs_dirand envDAGU_DOCS_DIR..mdunderdocs/**as kinddoc, writes to the configured docs dir; CLI/UI use generic “sync item” labels and filters.Stability & Refactors
doc/doctree; recursive watch includes nested dirs; low-volume invalidations use a 30s polling fallback.DocStoreFactory; document mutations centralized; rejects invalid path segments; filtersDAG_DOCS_DIRin shared containers; clarifies document search case behavior.Written for commit ff871a4. Summary will update on new commits.
Summary by CodeRabbit