fix(clients): render all tunnel configs for multi-inbound client (#6346) - #6349
fix(clients): render all tunnel configs for multi-inbound client (#6346)#6349TheMRVX wants to merge 1 commit into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| it('findAmneziaWGInbounds returns all matching inbounds in order', () => { | ||
| const awgInbounds = findAmneziaWGInbounds(multiClient, inboundsById); | ||
| expect(awgInbounds).toHaveLength(2); | ||
| expect(awgInbounds[0]?.id).toBe(101); | ||
| expect(awgInbounds[1]?.id).toBe(102); | ||
| expect(findAmneziaWGInbound(multiClient, inboundsById)?.id).toBe(101); | ||
| }); | ||
|
|
||
| it('findWireguardInbounds returns all matching inbounds in order', () => { | ||
| const wgInbounds = findWireguardInbounds(multiClient, inboundsById); | ||
| expect(wgInbounds).toHaveLength(2); | ||
| expect(wgInbounds[0]?.id).toBe(201); | ||
| expect(wgInbounds[1]?.id).toBe(202); | ||
| expect(findWireguardInbound(multiClient, inboundsById)?.id).toBe(201); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🟡 The new test does not cover the defect this PR fixes.
The bug in #6346 is in the render loops — the modals only emitted the first matching tunnel inbound. That logic now lives at ClientInfoModal.tsx#L794-L818 and ClientQrModal.tsx#L188-L217, but this file imports neither modal — it only calls the four pure helpers directly. Revert both map(...) loops back to rendering index [0] and every assertion here still passes.
REVIEW.md: "A bug fix carries a test that would fail without the fix. A test that cannot tell the broken behaviour from the fixed one passes before and after, so it certifies nothing and is itself the finding."
The two findXInbounds cases (L111-L125) are additionally the kind CLAUDE.md rejects outright: "No test for a getter, a constant, a rename, a pure map lookup" — findWireguardInbounds is a .map().filter() over a record.
The repo already renders modals under test (frontend/src/test/client-form-modal.test.tsx), so a .test.tsx asserting "two ConfigBlocks for a client attached to two AmneziaWG inbounds" would drive the actual broken path.
There was a problem hiding this comment.
Updated in caef652: replaced the unit tests with multi-tunnel-client-config.test.tsx which directly renders ClientInfoModal and ClientQrModal under React Testing Library, verifying multiple distinct ConfigBlocks and QR panels are rendered for multi-inbound clients.
| export function findWireguardInbound( | ||
| client: ClientRecord | null | undefined, | ||
| inboundsById: Record<number, InboundOption>, | ||
| ): InboundOption | undefined { | ||
| return findWireguardInbounds(client, inboundsById)[0]; | ||
| } |
There was a problem hiding this comment.
🟡 Dead production code: this wrapper (and its AmneziaWG twin) now has no caller outside the new test.
Both modals were fully migrated to the plural finders, so a grep over frontend/src returns only three hits for findWireguardInbound: this definition, the test import, and multi-tunnel-client-config.test.ts#L123-L125. Same for findAmneziaWGInbound at amneziawgConfig.ts#L29-L35.
CLAUDE.md: "A small bug does not earn new columns, jobs, abstractions, config knobs or helper layers." These are helper layers kept alive purely so the test has something to assert on.
Suggested: delete both singular wrappers plus the two assertions at multi-tunnel-client-config.test.ts:116 and :124 and their imports. (No suggestion block — the fix spans three files.)
Note amneziawgConfig.ts:5-9 still documents findAmneziaWGInbound's filter as the thing that disambiguates the two protocols; if the wrapper goes, that comment should name findAmneziaWGInbounds.
There was a problem hiding this comment.
Resolved in caef652: deleted findWireguardInbound and findAmneziaWGInbound and updated references to the plural finders.
|
|
||
| const outDir = path.resolve(import.meta.dirname, '../internal/web/dist'); | ||
| const BACKEND_TARGET = 'http://localhost:2053'; | ||
| const BACKEND_TARGET = process.env.BACKEND_TARGET || 'http://localhost:2053'; |
There was a problem hiding this comment.
🟡 Unrelated to the fix, and the new knob is only half-wired.
BACKEND_TARGET feeds the three HTTP proxy entries but the WebSocket entry right below still hardcodes its target:
Lines 288 to 297 in 40bd0d7
So BACKEND_TARGET=http://otherhost:2053 npm run dev proxies the panel to the remote host while /ws silently stays on localhost — a dev session with a dead live-update socket and no error to explain it.
It only appears under server.proxy, never in build, so nothing reaches the shipped bundle. But it is scope creep on a client-config fix: CLAUDE.md — "Diff is focused; refactors are separate from feature work."
Either drop this hunk from the PR, or route the ws target through the same variable in the same change.
There was a problem hiding this comment.
Resolved in caef652: reverted vite.config.js back to upstream main to eliminate unrelated scope changes.
| const suffix = wgConfigs.length > 1 ? `-${inbound.remark || inbound.tag || inbound.id}` : ''; | ||
| const safeSuffix = suffix.replace(/[^\w.-]/g, '_'); |
There was a problem hiding this comment.
🟡 The filename suffix prefers remark, which carries no uniqueness constraint — so the "distinct download filenames" goal is not actually met.
Inbound.Remark is plain free text with no unique index (model.go#L52-L54). Two AmneziaWG inbounds both remarked DE give both panels downloadName="user-DE.conf"; the operator downloads two files the browser disambiguates as user-DE.conf / user-DE (1).conf, with nothing saying which node each belongs to. inbound.id is unique by construction and is already what the Collapse keys use one line below.
Same expression at ClientQrModal.tsx#L220, ClientInfoModal.tsx#L801 and #L831 — appending -${inbound.id} (or falling back to it) at all four sites makes the name collision-free. No suggestion block since the change spans four locations in two files.
There was a problem hiding this comment.
Resolved in caef652: unified filename generation with inbound.id fallback in formatTunnelConfigMeta to guarantee unique filenames.
Code reviewNo blocking issues — The core change is sound. I traced the WireGuard The five 🟡 nits are posted inline. In short: the new test does not exercise the modal render loops that actually held the bug; 🟣 Pre-existing —
|
…anaei#6346) When a client belongs to multiple AmneziaWG or WireGuard inbounds (e.g. across remote nodes), findAmneziaWGInbounds and findWireguardInbounds only returned the first matching inbound. Consequently, ClientInfoModal and ClientQrModal rendered only one config block, making other inbounds' configs unreachable. - Add findAmneziaWGInbounds and findWireguardInbounds returning all matching inbounds - Add formatTunnelConfigMeta helper to unify label, fileName, and qrRemark resolution - Support addressOverride in buildWireguardClientConfig from tunnelAllowedIPs - Render all tunnel configs in ClientInfoModal and ClientQrModal with node remarks - Distinguish download filenames with inbound remark suffix to avoid collisions - Add component integration tests covering multi-inbound modal rendering
40bd0d7 to
caef652
Compare
|
All review items have been addressed in commit
All tests, linters, and format checks pass cleanly. Ready for re-review! |
Code reviewNo blocking issues — Round two. Round one ran against Round-one items I confirmed closed on this head: the singular One item is only partly closed. Stating it as fact rather than re-filing it: Coverage — head
|
1 participant
Summary
Fixes #6346.
When a client is attached to multiple AmneziaWG or WireGuard inbounds (such as across remote DE and FI nodes),
ClientInfoModalandClientQrModalpreviously only returned the first matching inbound. Subsequent inbounds' configurations, node-specific endpoints, and QR codes were unreachable.Type of change
Areas affected
Changes
findAmneziaWGInboundsandfindWireguardInboundsreturning all matching inbounds.formatTunnelConfigMetainlabel.tsto unifylabel,fileName, andqrRemarkderivation across modals with collision-free naming.addressOverridesupport tobuildWireguardClientConfigmatchingbuildAmneziaWGClientConfig.ClientInfoModaland distinct QR panels inClientQrModalfor every attached tunnel inbound.multi-tunnel-client-config.test.tsxtestingClientInfoModalandClientQrModalrendering and verifying distinct endpoints and keys.Checklist
go build ./...and the test suite pass locally.npm run lint,npm run typecheck, andnpm run buildpass.