From f70002d9a6d8894566ef96ac8c74c06e428470b5 Mon Sep 17 00:00:00 2001 From: Kuzz007 Date: Sun, 26 Jul 2026 19:14:56 +0300 Subject: [PATCH] Revert "fix(routing): insert new rules after api instead of appending at the end" This reverts commit 638458bd. Reordering new rules to the top by creation-time was a blunt heuristic: it fixed a rule silently shadowed by an unrestricted catch-all above it, but breaks the opposite, equally valid setup -- e.g. a pre-existing "youtube -> interfaceA" rule (no inboundTag restriction, meant to apply everywhere) followed by a new "amneziawg -> interfaceB" rule. Under the reverted logic the newer, broader-matching amneziawg rule would jump above the youtube rule and shadow it for amneziawg-sourced traffic -- the exact same class of bug this was meant to fix, just aimed the other way. Xray's rule order encodes real admin intent (which rule should win for overlapping traffic) that creation time can't safely stand in for. Back to plain append-at-the-end; admins reorder manually with the existing moveUp/moveDown controls, same as upstream's own behavior. Co-Authored-By: Claude Sonnet 5 --- .../src/pages/xray/routing/RoutingTab.tsx | 18 +----- .../routing-new-rule-insert-position.test.tsx | 62 ------------------- 2 files changed, 3 insertions(+), 77 deletions(-) delete mode 100644 frontend/src/test/routing-new-rule-insert-position.test.tsx diff --git a/frontend/src/pages/xray/routing/RoutingTab.tsx b/frontend/src/pages/xray/routing/RoutingTab.tsx index 95cc1d165..1b54a6213 100644 --- a/frontend/src/pages/xray/routing/RoutingTab.tsx +++ b/frontend/src/pages/xray/routing/RoutingTab.tsx @@ -21,7 +21,7 @@ import RuleFormModal from './RuleFormModal'; import type { RoutingRule } from './RuleFormModal'; import RuleCardList from './RuleCardList'; import { useRoutingColumns } from './useRoutingColumns'; -import { arrJoin, originalRuleIndex, isApiRule } from './helpers'; +import { arrJoin, originalRuleIndex } from './helpers'; import type { RuleRow } from './types'; import type { XraySettingsValue, SetTemplate } from '@/hooks/useXraySetting'; import type { RuleObject } from '@/schemas/routing'; @@ -209,20 +209,8 @@ export default function RoutingTab({ if (!tt.routing) tt.routing = { rules: [] }; if (!Array.isArray(tt.routing.rules)) tt.routing.rules = []; const typed = rule as unknown as RuleObject; - if (editingIndex == null) { - // Rules match top-to-bottom, first hit wins, so a brand-new rule - // appended at the end is silently shadowed by any earlier - // broader/catch-all rule that also happens to match its traffic — - // a real trap: the rule looks saved and enabled, but never actually - // fires. Insert it as early as possible instead (right after the - // pinned api rule, if present) so a newly created rule takes effect - // by default; the admin can still drag it lower with moveDown if - // that's genuinely what they want. - const insertAt = isApiRule(tt.routing.rules[0] as RuleObject) ? 1 : 0; - tt.routing.rules.splice(insertAt, 0, typed); - } else { - tt.routing.rules[editingIndex] = typed; - } + if (editingIndex == null) tt.routing.rules.push(typed); + else tt.routing.rules[editingIndex] = typed; }); setRuleModalOpen(false); } diff --git a/frontend/src/test/routing-new-rule-insert-position.test.tsx b/frontend/src/test/routing-new-rule-insert-position.test.tsx deleted file mode 100644 index 609dc5813..000000000 --- a/frontend/src/test/routing-new-rule-insert-position.test.tsx +++ /dev/null @@ -1,62 +0,0 @@ -import { describe, it, expect, vi } from 'vitest'; -import { fireEvent, screen } from '@testing-library/react'; -import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; - -import RoutingTab from '@/pages/xray/routing/RoutingTab'; -import type { XraySettingsValue } from '@/hooks/useXraySetting'; - -import { renderWithProviders } from './test-utils'; - -function settingsWithApiAndBlockRules(): XraySettingsValue { - return { - routing: { - rules: [ - { type: 'field', inboundTag: ['api'], outboundTag: 'api', enabled: true }, - { type: 'field', ip: ['ext:geoip_RU.dat:ru'], outboundTag: 'blocked', enabled: true }, - { type: 'field', protocol: ['bittorrent'], outboundTag: 'blocked', enabled: true }, - ], - }, - } as unknown as XraySettingsValue; -} - -// Rules match top-to-bottom, first hit wins. A brand-new rule used to be -// appended at the end, where a pre-existing broader/catch-all rule (e.g. a -// block rule with no inboundTag restriction, like the ones here) silently -// shadows it forever -- the rule looks saved and enabled but never actually -// fires. See RoutingTab.tsx onRuleConfirm. -describe('RoutingTab new-rule insert position', () => { - it('inserts a newly created rule right after the pinned api rule, not at the end', () => { - const setTemplateSettings = vi.fn(); - const initial = settingsWithApiAndBlockRules(); - - const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); - renderWithProviders( - - - , - ); - - fireEvent.click(screen.getByRole('tab', { name: /Routing Rules/ })); - fireEvent.click(screen.getByRole('button', { name: /Routing Rules/ })); - fireEvent.click(screen.getByRole('button', { name: 'Create' })); - - expect(setTemplateSettings).toHaveBeenCalledTimes(1); - const updater = setTemplateSettings.mock.calls[0][0] as (prev: XraySettingsValue) => XraySettingsValue; - const next = updater(initial); - const rules = (next.routing as { rules: Array<{ inboundTag?: string[]; outboundTag?: string }> }).rules; - - expect(rules.length).toBe(4); - expect(rules[0].outboundTag).toBe('api'); - // The two pre-existing block rules must have been pushed down, not the - // new rule appended after them. - expect(rules[1].outboundTag).not.toBe('blocked'); - expect(rules[2].outboundTag).toBe('blocked'); - expect(rules[3].outboundTag).toBe('blocked'); - }); -});