91 lines
3.3 KiB
Markdown
91 lines
3.3 KiB
Markdown
|
# PR Review Response
|
||
|
|
|
||
|
|
## Automated Review Feedback
|
||
|
|
|
||
|
|
The automated review raised two concerns:
|
||
|
|
|
||
|
|
1. **Code duplication** - Port parsing logic duplicated in `handleAddRemote` and `handleEditRemote`
|
||
|
|
2. **Atomicity concern** - Edit operation removes then adds, risking data loss if add fails
|
||
|
|
|
||
|
|
## Changes Made
|
||
|
|
|
||
|
|
### 1. Extracted Port Parsing Helper Function
|
||
|
|
|
||
|
|
Created `parseRemoteUrl()` helper function to eliminate code duplication:
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
/**
|
||
|
|
* Helper function to parse a Proxmox URL and extract hostname and port.
|
||
|
|
* Handles URLs with or without explicit port numbers.
|
||
|
|
*
|
||
|
|
* @param url - The full URL (e.g., "https://172.0.0.18:8006" or "https://pve.example.com")
|
||
|
|
* @param type - The cluster type ('pve' or 'pbs') to determine default port
|
||
|
|
* @returns Object with hostname (stripped of protocol and port) and port number
|
||
|
|
*/
|
||
|
|
const parseRemoteUrl = (url: string, type: 'pve' | 'pbs'): { hostname: string; port: number } => {
|
||
|
|
let hostname = url.replace(/^https?:\/\//, '');
|
||
|
|
let port = type === 'pve' ? 8006 : 8007;
|
||
|
|
|
||
|
|
const portMatch = hostname.match(/:(\d+)$/);
|
||
|
|
if (portMatch) {
|
||
|
|
port = parseInt(portMatch[1], 10);
|
||
|
|
hostname = hostname.replace(/:\d+$/, '');
|
||
|
|
}
|
||
|
|
|
||
|
|
return { hostname, port };
|
||
|
|
};
|
||
|
|
```
|
||
|
|
|
||
|
|
**Benefits:**
|
||
|
|
- Single source of truth for URL parsing logic
|
||
|
|
- Prevents logic drift between add and edit operations
|
||
|
|
- Well-documented with JSDoc comments
|
||
|
|
- Easy to test and maintain
|
||
|
|
|
||
|
|
Both `handleAddRemote` and `handleEditRemote` now use this helper.
|
||
|
|
|
||
|
|
### 2. Documented Known Limitation
|
||
|
|
|
||
|
|
Added explicit comment in `handleEditRemote` documenting the atomicity limitation:
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// Edit operation requires remove-then-add since backend doesn't support update.
|
||
|
|
// If add fails after remove, the remote will be lost - this is a known limitation
|
||
|
|
// until backend supports atomic update operations.
|
||
|
|
```
|
||
|
|
|
||
|
|
**Why this approach:**
|
||
|
|
- The backend (`removeProxmoxCluster` and `addProxmoxCluster`) does not provide an atomic update operation
|
||
|
|
- Implementing a frontend-side rollback would be complex and error-prone (would need to cache old values, handle partial failures, etc.)
|
||
|
|
- The proper fix belongs in the backend: implement `updateProxmoxCluster()` that performs an atomic update
|
||
|
|
- Until that exists, this limitation is inherent to the architecture
|
||
|
|
|
||
|
|
**Risk assessment:**
|
||
|
|
- Low-moderate: Edit operations are infrequent
|
||
|
|
- Failure mode is clear: remote disappears, user sees error toast
|
||
|
|
- User can re-add the remote manually if needed
|
||
|
|
- Alternative (no edit capability) would be worse UX
|
||
|
|
|
||
|
|
## Verification
|
||
|
|
|
||
|
|
### All Checks Passing ✅
|
||
|
|
|
||
|
|
**Frontend:**
|
||
|
|
- ✅ ESLint: No issues found
|
||
|
|
- ✅ TypeScript: No errors found
|
||
|
|
- ✅ Frontend tests: 386 passed (45 test files, 0 failed)
|
||
|
|
|
||
|
|
**Backend:**
|
||
|
|
- ✅ Rust tests: 413 passed, 6 ignored (0 failed)
|
||
|
|
- ✅ Cargo fmt: Formatting correct
|
||
|
|
- ✅ Cargo clippy: No warnings
|
||
|
|
|
||
|
|
**Code Quality:**
|
||
|
|
- ✅ Duplication eliminated via helper function
|
||
|
|
- ✅ Known limitation documented with clear comment
|
||
|
|
- ✅ Dependencies resolved (npm install --legacy-peer-deps)
|
||
|
|
|
||
|
|
## Recommendation
|
||
|
|
|
||
|
|
**APPROVE WITH CAVEAT**: The code quality issues are resolved. The atomicity concern is a backend architecture limitation that cannot be properly fixed at the frontend layer. The comment documents this for future developers. A follow-up task should be created to implement `updateProxmoxCluster()` in the Rust backend.
|