Step 10: Code review, testing & documentation
Code Review (REVIEW.md): - Fixed parseSearchResponse skipping first result (critical bug) - Fixed trashbin/versions/chunked paths missing /remote.php/dav prefix - Fixed trash_restore to use original file location instead of /restore endpoint - Fixed createTask/updateTask missing iCal text escaping - Added 409 handling for createFolder (parent missing) - Extracted duplicate decodeXmlText to utils.ts - Extracted duplicate generateUID to utils.ts (shared with calendar/tasks) - Removed 5 dead code functions (parseVEVENT, extractVEventBlocks, unfoldICalLines, getCalDAVXmlHeaders, local decodeXmlText) - Cleaned unused imports across all tool files Testing (RESULTS.md): - 35 tests passed, 1 skipped (trash_empty), 1 server limitation (bulk_upload) - Tested all 21+ file tools, edge cases (spaces, unicode, overwrite, empty folders) - Verified chunked upload end-to-end Documentation (README.md): - Complete tool reference (21 file + 10 other tools) - Quick start, CLI usage, size limits, troubleshooting - Architecture overview
This commit is contained in:
@@ -0,0 +1,92 @@
|
||||
# Code Review — nextcloud-mcp
|
||||
|
||||
Date: 2026-05-11
|
||||
|
||||
## Summary
|
||||
|
||||
Reviewed all source files in `src/`. Found and fixed **7 bugs**, removed **5 dead code functions**, consolidated **3 duplicate functions**, and cleaned up unused imports.
|
||||
|
||||
## Bugs Found & Fixed
|
||||
|
||||
### 🔴 Critical
|
||||
|
||||
#### 1. `parseSearchResponse` skipped first result
|
||||
**File:** `src/webdav.ts`
|
||||
**Issue:** `parseSearchResponse` delegated to `parsePropfindFilesResponse`, which always skips the first `<response>` element (intended for PROPFIND root folder). SEARCH responses don't have a root element — every element is a result. The first search result was silently dropped.
|
||||
**Fix:** Implemented `parseSearchResponse` as a standalone parser that doesn't skip the first element.
|
||||
|
||||
#### 2. Trashbin paths missing `/remote.php/dav` prefix
|
||||
**File:** `src/tools/files.ts`
|
||||
**Issue:** All trashbin operations used `/trashbin/{user}/trash` instead of `/remote.php/dav/trashbin/{user}/trash`. Same for versions paths (`/versions/...` instead of `/remote.php/dav/versions/...`) and chunked upload paths (`/uploads/...` instead of `/remote.php/dav/uploads/...`).
|
||||
**Fix:** Added the missing `/remote.php/dav` prefix to all trashbin, versions, and chunked upload paths.
|
||||
|
||||
#### 3. `trash_restore` used wrong endpoint
|
||||
**File:** `src/tools/files.ts`
|
||||
**Issue:** `trash_restore` moved trash items to `/remote.php/dav/trashbin/{user}/restore`, which returned 412/403 errors. The correct approach is to move the trash item to its original file location.
|
||||
**Fix:** Now fetches the trash item's `originalLocation` via PROPFIND, then moves to the full DAV path of the original location.
|
||||
|
||||
### 🟡 Medium
|
||||
|
||||
#### 4. `createTask` didn't escape iCal text
|
||||
**File:** `src/tools/tasks.ts`
|
||||
**Issue:** Task `summary` and `description` were inserted raw into iCal format. Characters like `;`, `,`, `\`, and newlines would break the iCal parser.
|
||||
**Fix:** Applied `escapeICalText()` to summary and description in both `createTask` and `updateTask`.
|
||||
|
||||
#### 5. Duplicate `decodeXmlText` in webdav.ts and caldav.ts
|
||||
**Files:** `src/webdav.ts`, `src/caldav.ts`, `src/utils.ts`
|
||||
**Issue:** Identical function defined in two files. Any fix to one would need to be replicated.
|
||||
**Fix:** Moved to `src/utils.ts` as shared export, removed local copies.
|
||||
|
||||
#### 6. Duplicate `generateUID` in calendar.ts and tasks.ts
|
||||
**Files:** `src/tools/calendar.ts`, `src/tools/tasks.ts`
|
||||
**Issue:** Same UUID generation function duplicated.
|
||||
**Fix:** Both now use `generateUUID` from `src/utils.ts` (aliased as `generateUID`).
|
||||
|
||||
#### 7. `createFolder` didn't handle HTTP 409
|
||||
**File:** `src/tools/files.ts`
|
||||
**Issue:** When parent folder doesn't exist, MKCOL returns 409. The code only handled 405 (already exists).
|
||||
**Fix:** Added 409 handling with clear error message.
|
||||
|
||||
### 🟢 Low / Style
|
||||
|
||||
#### 8. Removed unused `getCalDAVXmlHeaders`
|
||||
**Files:** `src/caldav.ts`, `src/tools/tasks.ts`, `src/tools/calendar.ts`
|
||||
**Issue:** Exported function never imported anywhere.
|
||||
**Fix:** Removed from caldav.ts and import lists.
|
||||
|
||||
#### 9. Removed dead code in caldav.ts
|
||||
**File:** `src/caldav.ts`
|
||||
**Removed functions:**
|
||||
- `parseVEVENT()` — unused (events parsed via ical.js in `parseEventsFromCalDAV`)
|
||||
- `extractVEventBlocks()` — unused regex helper
|
||||
- `unfoldICalLines()` — unused iCal line unfolding
|
||||
|
||||
#### 10. Removed unused imports
|
||||
**Files:** Multiple
|
||||
- `TrashedFile` import in files.ts (type used only indirectly via `parseTrashbinResponse`)
|
||||
- `getCalDAVXmlHeaders` imports in tasks.ts and calendar.ts
|
||||
|
||||
## Issues Documented (Not Fixed)
|
||||
|
||||
### ⚠️ `resolveRelativePathFromHref` has unused `basePath` parameter
|
||||
**File:** `src/webdav.ts`
|
||||
**Status:** Left as-is — the function works correctly, `basePath` is dead code. Removing it would be a breaking change if external consumers exist.
|
||||
|
||||
### ⚠️ XML parsing is regex-based
|
||||
**Files:** `src/webdav.ts`, `src/caldav.ts`
|
||||
**Status:** Works for standard Nextcloud responses but could fail on edge cases (CDATA, deeply nested XML). Not worth fixing — adding an XML parser library would increase bundle size for minimal benefit.
|
||||
|
||||
### ⚠️ `bulk_upload` may not work on all Nextcloud versions
|
||||
**File:** `src/tools/files.ts`
|
||||
**Status:** The `/remote.php/dav/bulk` endpoint may not be available or may not accept POST on older Nextcloud versions. Test showed 400 on the target server. Documented in README.
|
||||
|
||||
## Files Changed
|
||||
|
||||
| File | Changes |
|
||||
|------|---------|
|
||||
| `src/utils.ts` | Added `decodeXmlText()`, `escapeICalText()` |
|
||||
| `src/webdav.ts` | Fixed `parseSearchResponse`, imported shared `decodeXmlText`, removed local copy |
|
||||
| `src/caldav.ts` | Imported shared `decodeXmlText`, removed local copy + 3 dead functions + unused export |
|
||||
| `src/tools/files.ts` | Fixed trashbin/versions/chunked paths, fixed trash_restore, added 409 handling, cleaned imports |
|
||||
| `src/tools/tasks.ts` | Added iCal escaping, shared `generateUID`, cleaned imports |
|
||||
| `src/tools/calendar.ts` | Shared `escapeICalText`/`generateUID`, removed local copies, cleaned imports |
|
||||
Reference in New Issue
Block a user