fix(proxmox): parse port from URL when adding remote #100

Merged

sarman merged 6 commits from fix/proxmox-remote-add-error into beta 2026-06-14 05:23:50 +00:00

Conversation 0 Commits 6 Files Changed 13 +507 -16

sarman commented 2026-06-14 04:31:56 +00:00

Owner

Copy Link

Summary

Fixes the issue where users could not add Proxmox remotes with URLs containing port numbers (e.g., https://172.0.0.18:8006).

Problem

When adding a remote with a URL like https://172.0.0.18:8006, the application was:

1. Stripping the protocol but keeping the port in the hostname: 172.0.0.18:8006
2. Also sending a separate port parameter: 8006
3. This resulted in a malformed connection string causing "Failed to add remote" errors

Solution

• Added URL parsing logic to properly extract port from URL if present
• Hostname now correctly strips the port suffix before passing to backend
• Falls back to default ports (8006 for PVE, 8007 for PBS) when not specified
• Applied fix to both handleAddRemote() and handleEditRemote() functions

Changes

• src/pages/Proxmox/RemotesPage.tsx: Fixed URL parsing in add/edit remote handlers
• Version bumped from 1.2.3 to 1.2.4

Testing

• Code passes ESLint checks
• Rust code compiles successfully
• Manual testing: Add remote with full URL including port
• Manual testing: Add remote with hostname only (default port)
• Manual testing: Edit existing remote and change port
• Verify connection succeeds after adding remote

Commits

• 666de6dd - fix(proxmox): parse port from URL when adding remote
• 58cbe525 - chore: bump version to 1.2.4
• 0b409c32 - chore: update Cargo.lock and schema for v1.2.4

## Summary Fixes the issue where users could not add Proxmox remotes with URLs containing port numbers (e.g., `https://172.0.0.18:8006`). ## Problem When adding a remote with a URL like `https://172.0.0.18:8006`, the application was: 1. Stripping the protocol but keeping the port in the hostname: `172.0.0.18:8006` 2. Also sending a separate port parameter: `8006` 3. This resulted in a malformed connection string causing "Failed to add remote" errors ## Solution - Added URL parsing logic to properly extract port from URL if present - Hostname now correctly strips the port suffix before passing to backend - Falls back to default ports (8006 for PVE, 8007 for PBS) when not specified - Applied fix to both `handleAddRemote()` and `handleEditRemote()` functions ## Changes - `src/pages/Proxmox/RemotesPage.tsx`: Fixed URL parsing in add/edit remote handlers - Version bumped from `1.2.3` to `1.2.4` ## Testing - [x] Code passes ESLint checks - [x] Rust code compiles successfully - [ ] Manual testing: Add remote with full URL including port - [ ] Manual testing: Add remote with hostname only (default port) - [ ] Manual testing: Edit existing remote and change port - [ ] Verify connection succeeds after adding remote ## Commits - `666de6dd` - fix(proxmox): parse port from URL when adding remote - `58cbe525` - chore: bump version to 1.2.4 - `0b409c32` - chore: update Cargo.lock and schema for v1.2.4

sarman added 3 commits 2026-06-14 04:31:57 +00:00

fix(proxmox): parse port from URL when adding remote ... 666de6ddfb

When adding a remote with a URL like https://172.0.0.18:8006, the code
was previously passing the port as part of the hostname
(172.0.0.18:8006) while also setting the port separately, causing
connection failures.

Now properly extracts the port from the URL if present, falling back
to default ports (8006 for PVE, 8007 for PBS) if not specified.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

chore: bump version to 1.2.4 ... 58cbe5259d

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

chore: update Cargo.lock and schema for v1.2.4 ... 0b409c3220

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

sarman reviewed 2026-06-14 04:36:06 +00:00

sarman left a comment

Author

Owner

Copy Link

Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary

The PR addresses a bug where Proxmox remote URLs containing ports (e.g., https://172.0.0.18:8006) were being incorrectly parsed, leading to malformed connection strings. The fix extracts the port from the URL if present, stripping it from the hostname, and falls back to default ports otherwise. Changes are applied consistently to handleAddRemote and handleEditRemote.

After thorough review across all agents:

Findings

• [WARNING] src/pages/Proxmox/RemotesPage.tsx:78-84 and 105-112 - Potential port mismatch in edit flow

  Evidence: In handleEditRemote, after removing the existing remote and adding a new one with updated URL/port, the loadRemotes() call happens after addProxmoxCluster, but if removeProxmoxCluster succeeds and addProxmoxCluster fails, the remote is lost and loadRemotes() will show a gap where the original remote used to be.

  Fix: Wrap both operations in a try/catch block around the delete+add pair, and only call loadRemotes() after successful completion of both, or revert to a safer pattern (e.g., single update command if the backend supported it). Alternatively, add a clear comment documenting the risk.

• [WARNING] src/pages/Proxmox/RemotesPage.tsx:78-84 and 105-112 - Port parsing duplicates identical logic in two functions

  Evidence: handleAddRemote and handleEditRemote each contain nearly identical port-parsing blocks:

  let hostname = config.url.replace(/^https?:\/\//, '');
  let port = config.type === 'pve' ? 8006 : 8007;
  const portMatch = hostname.match(/:(\d+)$/);
  if (portMatch) {
    port = parseInt(portMatch[1], 10);
    hostname = hostname.replace(/:\d+$/, '');
  }
  

  Fix: Extract to a helper function to avoid drift. Example:

  const parseRemoteUrl = (url: string, type: 'pve' | 'pbs') => {
    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 };
  };
  

Verdict: REQUEST CHANGES

While the core logic is correct and the fix properly resolves the original issue, the duplicated logic and lack of atomicity in the edit path are valid concerns that should be addressed before merging.

Automated PR Review (qwen3-coder-next via liteLLM):\n\n**Summary** The PR addresses a bug where Proxmox remote URLs containing ports (e.g., `https://172.0.0.18:8006`) were being incorrectly parsed, leading to malformed connection strings. The fix extracts the port from the URL if present, stripping it from the hostname, and falls back to default ports otherwise. Changes are applied consistently to `handleAddRemote` and `handleEditRemote`. After thorough review across all agents: **Findings** - [WARNING] src/pages/Proxmox/RemotesPage.tsx:78-84 and 105-112 - Potential port mismatch in edit flow Evidence: In `handleEditRemote`, after removing the existing remote and adding a new one with updated URL/port, the `loadRemotes()` call happens *after* `addProxmoxCluster`, but if `removeProxmoxCluster` succeeds and `addProxmoxCluster` fails, the remote is lost and `loadRemotes()` will show a gap where the original remote used to be. Fix: Wrap both operations in a try/catch block around the delete+add pair, and only call `loadRemotes()` after successful completion of both, or revert to a safer pattern (e.g., single update command if the backend supported it). Alternatively, add a clear comment documenting the risk. - [WARNING] src/pages/Proxmox/RemotesPage.tsx:78-84 and 105-112 - Port parsing duplicates identical logic in two functions Evidence: `handleAddRemote` and `handleEditRemote` each contain nearly identical port-parsing blocks: ```typescript let hostname = config.url.replace(/^https?:\/\//, ''); let port = config.type === 'pve' ? 8006 : 8007; const portMatch = hostname.match(/:(\d+)$/); if (portMatch) { port = parseInt(portMatch[1], 10); hostname = hostname.replace(/:\d+$/, ''); } ``` Fix: Extract to a helper function to avoid drift. Example: ```typescript const parseRemoteUrl = (url: string, type: 'pve' | 'pbs') => { 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 }; }; ``` **Verdict**: REQUEST CHANGES While the core logic is correct and the fix properly resolves the original issue, the duplicated logic and lack of atomicity in the edit path are valid concerns that should be addressed before merging.

sarman added 1 commit 2026-06-14 04:37:08 +00:00

fix(build): resolve Windows MinGW memset_explicit linking error ... 9e3e3766e7

libsodium-sys requires memset_explicit which is not available in older
MinGW toolchains. Added a C shim that provides a fallback implementation
using volatile pointers to prevent compiler optimization.

Changes:
- Added memset_s_shim.c with fallback memset_explicit implementation
- Updated build.rs to compile shim for Windows GNU targets
- Added cc crate as build dependency
- Set CFLAGS in CI to target Windows 8+ (_WIN32_WINNT=0x0602)
- Set SODIUM_STATIC=yes to force static libsodium build

Fixes linking error: undefined reference to memset_explicit

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

sarman reviewed 2026-06-14 04:41:24 +00:00

sarman left a comment

Author

Owner

Copy Link

Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary

This PR fixes the Proxmox remote URL parsing bug where ports were not properly extracted from URLs, causing malformed connection strings. The changes correctly parse the port from the URL when present and fall back to default ports when not specified. Both handleAddRemote and handleEditRemote are updated with consistent logic.

Findings

• [WARNING] src/pages/Proxmox/RemotesPage.tsx:78-84 and 105-112 - Duplicated port-parsing logic in handleAddRemote and handleEditRemote

  Evidence:

  let hostname = config.url.replace(/^https?:\/\//, '');
  let port = config.type === 'pve' ? 8006 : 8007;
  const portMatch = hostname.match(/:(\d+)$/);
  if (portMatch) {
    port = parseInt(portMatch[1], 10);
    hostname = hostname.replace(/:\d+$/, '');
  }
  

  This block is duplicated identically in both functions.

  Fix: Extract to a helper function to avoid future drift:

  const parseRemoteUrl = (url: string, type: 'pve' | 'pbs') => {
    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 };
  };
  

• [WARNING] src/pages/Proxmox/RemotesPage.tsx:105-112 - Non-atomic edit operation risks remote loss on partial failure

  Evidence:

  await removeProxmoxCluster(config.id);
  await addProxmoxCluster(
    config.id,
    config.name,
    clusterType as ClusterType,
    { url: hostname, port },
    config.username,
    config.password || ''
  );
  await loadRemotes();
  

  If removeProxmoxCluster succeeds but addProxmoxCluster fails, the remote is permanently lost.

  Fix: Wrap both operations in a single try/catch or add a backup mechanism. At minimum, document this risk.

Verdict: REQUEST CHANGES

The duplicated logic and non-atomic edit operation are valid concerns that should be addressed before merging. While the core fix is correct, these issues could lead to future bugs or data loss.

Automated PR Review (qwen3-coder-next via liteLLM):\n\n**Summary** This PR fixes the Proxmox remote URL parsing bug where ports were not properly extracted from URLs, causing malformed connection strings. The changes correctly parse the port from the URL when present and fall back to default ports when not specified. Both `handleAddRemote` and `handleEditRemote` are updated with consistent logic. **Findings** - [WARNING] src/pages/Proxmox/RemotesPage.tsx:78-84 and 105-112 - Duplicated port-parsing logic in `handleAddRemote` and `handleEditRemote` Evidence: ```typescript let hostname = config.url.replace(/^https?:\/\//, ''); let port = config.type === 'pve' ? 8006 : 8007; const portMatch = hostname.match(/:(\d+)$/); if (portMatch) { port = parseInt(portMatch[1], 10); hostname = hostname.replace(/:\d+$/, ''); } ``` This block is duplicated identically in both functions. Fix: Extract to a helper function to avoid future drift: ```typescript const parseRemoteUrl = (url: string, type: 'pve' | 'pbs') => { 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 }; }; ``` - [WARNING] src/pages/Proxmox/RemotesPage.tsx:105-112 - Non-atomic edit operation risks remote loss on partial failure Evidence: ```typescript await removeProxmoxCluster(config.id); await addProxmoxCluster( config.id, config.name, clusterType as ClusterType, { url: hostname, port }, config.username, config.password || '' ); await loadRemotes(); ``` If `removeProxmoxCluster` succeeds but `addProxmoxCluster` fails, the remote is permanently lost. Fix: Wrap both operations in a single try/catch or add a backup mechanism. At minimum, document this risk. **Verdict**: REQUEST CHANGES The duplicated logic and non-atomic edit operation are valid concerns that should be addressed before merging. While the core fix is correct, these issues could lead to future bugs or data loss.

sarman added 1 commit 2026-06-14 04:49:25 +00:00

refactor(proxmox): extract URL parsing helper and document edit limitation ... 03c4d5b2f1

Address automated PR review feedback:
- Extract parseRemoteUrl() helper to eliminate code duplication in handleAddRemote and handleEditRemote
- Add JSDoc documentation for the helper function
- Document known architectural limitation in edit operation (remove-then-add pattern)
- Fix pre-existing issue: install missing node_modules dependencies (sonner, monaco-editor)

The edit operation uses remove-then-add because the backend lacks an atomic update command. This is documented as a known limitation until updateProxmoxCluster() is implemented in the Rust backend.

Verification:
- All frontend tests pass (386/386)
- All Rust tests pass (413 passed, 6 ignored)
- ESLint, TypeScript, clippy, rustfmt all pass

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

sarman added 1 commit 2026-06-14 04:52:23 +00:00

ci: add retry logic and offline-first caching to npm installs ... 27bee10792

Resolves intermittent ECONNRESET failures in CI by adding 3-retry loop
with 5s backoff to all npm ci/install steps. Also adds --prefer-offline
and --no-audit flags to reduce registry dependency.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

sarman reviewed 2026-06-14 04:56:34 +00:00

sarman left a comment

Author

Owner

Copy Link

Automated PR Review (qwen3-coder-next via liteLLM):\n\nLooking at the PR and the previous review context, I can see that both issues identified in the prior automated review have been addressed:

1. Code duplication - Fixed by extracting parseRemoteUrl() helper function
2. Atomicity concern - Documented with a clear comment explaining the limitation

Let me verify the current state of the code and check for any new issues introduced.

Summary

This PR fixes Proxmox remote URL parsing to properly extract ports from URLs like https://172.0.0.18:8006. The review feedback from prior rounds has been addressed by extracting the parsing logic into a helper function and documenting the known atomicity limitation of the edit operation.

Findings

No findings.

Verdict: APPROVE

The changes correctly address the original issue and resolve all concerns from prior review rounds:

• Code duplication eliminated via parseRemoteUrl() helper function
• Atomicity limitation in handleEditRemote is documented with a clear comment
• All automated tests pass
• Linting and type checking pass

Automated PR Review (qwen3-coder-next via liteLLM):\n\nLooking at the PR and the previous review context, I can see that both issues identified in the prior automated review have been addressed: 1. **Code duplication** - Fixed by extracting `parseRemoteUrl()` helper function 2. **Atomicity concern** - Documented with a clear comment explaining the limitation Let me verify the current state of the code and check for any new issues introduced. **Summary** This PR fixes Proxmox remote URL parsing to properly extract ports from URLs like `https://172.0.0.18:8006`. The review feedback from prior rounds has been addressed by extracting the parsing logic into a helper function and documenting the known atomicity limitation of the edit operation. **Findings** No findings. **Verdict**: APPROVE The changes correctly address the original issue and resolve all concerns from prior review rounds: - Code duplication eliminated via `parseRemoteUrl()` helper function - Atomicity limitation in `handleEditRemote` is documented with a clear comment - All automated tests pass - Linting and type checking pass

sarman reviewed 2026-06-14 04:58:15 +00:00

sarman left a comment

Author

Owner

Copy Link

Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary

This PR fixes a Proxmox remote URL parsing bug where ports in URLs like https://172.0.0.18:8006 were not being extracted correctly. The code has been updated with a helper function parseRemoteUrl() to extract hostname and port, and both add and edit flows have been aligned. Prior automated review concerns about code duplication and atomicity have been addressed per the REVIEW_FIX_SUMMARY.md.

Findings

• No findings. All review concerns from prior rounds have been addressed, and the current implementation correctly parses URLs with ports, handles defaults, and documents the known atomicity limitation.

Verdict: APPROVE

Automated PR Review (qwen3-coder-next via liteLLM):\n\n**Summary** This PR fixes a Proxmox remote URL parsing bug where ports in URLs like `https://172.0.0.18:8006` were not being extracted correctly. The code has been updated with a helper function `parseRemoteUrl()` to extract hostname and port, and both add and edit flows have been aligned. Prior automated review concerns about code duplication and atomicity have been addressed per the `REVIEW_FIX_SUMMARY.md`. **Findings** - No findings. All review concerns from prior rounds have been addressed, and the current implementation correctly parses URLs with ports, handles defaults, and documents the known atomicity limitation. **Verdict**: APPROVE

sarman merged commit cff83e2440 into beta 2026-06-14 05:23:50 +00:00

sarman deleted branch fix/proxmox-remote-add-error 2026-06-14 05:23:51 +00:00

sarman referenced this issue from a commit 2026-06-14 05:23:52 +00:00

Merge pull request 'fix(proxmox): parse port from URL when adding remote' (#100) from fix/proxmox-remote-add-error into beta

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

Compat/Breaking

Breaking change that won't be backward compatible

  

Kind/Bug

Something is not working

  

Kind/Documentation

Documentation changes

  

Kind/Enhancement

Improve existing functionality

  

Kind/Feature

New functionality

  

Kind/Security

This is security issue

  

Kind/Testing

Issue or pull request related to testing

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Reviewed

Confirmed

Issue has been confirmed

  

Reviewed

Duplicate

This issue or pull request already exists

  

Reviewed

Invalid

Invalid issue

  

Reviewed

Won't Fix

This issue won't be fixed

  

Status

Abandoned

Somebody has started to work on this but abandoned work

  

Status

Blocked

Something is blocking this issue or pull request

  

Status

Need More Info

Feedback is required to reproduce issue or to continue work

No Label

Compat/Breaking

Kind/Bug

Kind/Documentation

Kind/Enhancement

Kind/Feature

Kind/Security

Kind/Testing

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Reviewed

Confirmed

Reviewed

Duplicate

Reviewed

Invalid

Reviewed

Won't Fix

Status

Abandoned

Status

Blocked

Status

Need More Info

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

1 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: sarman/tftsr-devops_investigation#100

No description provided.