# 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 |