refactor(node-replacement): reorganize domain components and expand comprehensive test suite#9301
Merged
christian-byrne merged 9 commits intomainfrom Feb 28, 2026
Merged
Conversation
…omprehensive test suite Resolves multiple issues related to node replacement and error handling logic (#9271, #9270, #9267, #9255, #9254, #9231). Key changes: - Relocated node replacement UI components and scanning logic to a domain-driven structure under src/platform/nodeReplacement/ for better maintainability. - Refactored SwapNodeGroupRow.vue to follow the emit pattern ( eplace event), centralizing replacement and cleanup logic within the parent TabErrors.vue. - Significantly expanded the test suite with the following additions: - **Component Tests**: Validated rendering, user interactions, and event contracts for MissingNodeCard, MissingPackGroupRow, SwapNodeGroupRow, and SwapNodesCard. - **Logic & Composable Unit Tests**: Verified graph scanning logic in missingNodeScan, placeholder detection criteria in useNodeReplacement, and computed grouping logic in useErrorGroups. - **Store Tests**: Added unit tests for executionErrorStore to ensure correct filtering and removal of missing nodes by type. - Updated import paths in �pp.ts and other relevant files to reflect the new directory structure.
dosubot
bot
added
the
size:XXL
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ✅ 543 passed, 0 failed · 7 flaky📊 Browser Reports
|
Contributor
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds extensive tests for missing-node and node-replacement features, refactors swap components to emit a Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User / Component (SwapRow)
participant SwapCard as SwapNodesCard
participant Tab as TabErrors
participant Replacement as useNodeReplacement
participant ExecStore as executionErrorStore
UI->>SwapCard: emit replace(group)
SwapCard->>Tab: emit replace(group)
Tab->>Replacement: call replaceGroup(group)
Replacement->>Replacement: replaceNodesInPlace(group)
Replacement->>ExecStore: removeMissingNodesByType(group.type)
ExecStore-->>Tab: missing nodes state updated
Tab-->>SwapCard: UI updates (rows removed/updated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
📦 Bundle: 4.46 MB gzip 🟢 -129 BDetailsSummary
Category Glance App Entry Points — 17.9 kB (baseline 17.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.03 MB (baseline 1.03 MB) • 🟢 -177 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 72.1 kB (baseline 72.1 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 435 kB (baseline 435 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 736 B (baseline 736 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 47 kB (baseline 47 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.56 MB (baseline 2.55 MB) • 🔴 +511 BStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 55.5 kB (baseline 55.5 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 11 added / 11 removed Vendor & Third-Party — 8.84 MB (baseline 8.84 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.87 MB (baseline 7.87 MB) • ⚪ 0 BBundles that do not match a named category
Status: 52 added / 52 removed |
⚡ Performance Report
Raw data{
"timestamp": "2026-02-28T14:03:30.814Z",
"gitSha": "dba53273f0da3eda8662b0e1a877690632843404",
"branch": "chore/node-replacement-tests-and-refactor",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2029.1149999999902,
"styleRecalcs": 124,
"styleRecalcDurationMs": 20.318999999999996,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 403.796,
"heapDeltaBytes": -3392184
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2097.126000000003,
"styleRecalcs": 189,
"styleRecalcDurationMs": 57.879,
"layouts": 12,
"layoutDurationMs": 3.885,
"taskDurationMs": 1107.942,
"heapDeltaBytes": -3719044
},
{
"name": "dom-widget-clipping",
"durationMs": 583.4609999999998,
"styleRecalcs": 43,
"styleRecalcDurationMs": 14.172999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 367.20599999999996,
"heapDeltaBytes": 8041248
}
]
} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/components/rightSidePanel/errors/MissingNodeCard.test.ts (1)
124-132: CountMissingPackGroupRowinstances by component, not stub class.At Line 126 and Line 131, using
.pack-rowties the test to mock template markup. Prefer component-level queries to validate parent rendering behavior directly.♻️ Suggested assertion update
- expect(wrapper.findAll('.pack-row')).toHaveLength(3) + expect( + wrapper.findAllComponents({ name: 'MissingPackGroupRow' }) + ).toHaveLength(3) - expect(wrapper.findAll('.pack-row')).toHaveLength(0) + expect( + wrapper.findAllComponents({ name: 'MissingPackGroupRow' }) + ).toHaveLength(0)As per coding guidelines: “Do not write tests dependent on non-behavioral features like utility classes or styles”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/MissingNodeCard.test.ts` around lines 124 - 132, The tests should count MissingPackGroupRow components instead of querying DOM utility classes; update the two assertions in MissingNodeCard.test.ts to use component-level queries (e.g., call mountCard({ missingPackGroups: ... }) then use wrapper.findAllComponents(MissingPackGroupRow) and expect its length to be 3 or 0 respectively), referencing the MissingPackGroupRow component and the mountCard helper to locate the code to change.src/platform/nodeReplacement/useNodeReplacement.test.ts (1)
693-723: ContainLiteGraph.registered_node_typesmutations to avoid cross-test bleed.At Line 695 and Line 720-Line 722, this test mutates shared mock state and relies on in-test cleanup. If an assertion throws before cleanup, later tests can inherit stale registrations.
♻️ Safer reset pattern
beforeEach(() => { vi.clearAllMocks() setActivePinia(createPinia()) + for (const key of Object.keys(LiteGraph.registered_node_types)) { + delete (LiteGraph.registered_node_types as Record<string, unknown>)[key] + } }) ... - // Cleanup - delete (LiteGraph.registered_node_types as Record<string, unknown>)[ - 'OldNode' - ]As per coding guidelines: “For mocking in Vitest, leverage Vitest's utilities where possible; keep module mocks contained without global mutable state; use
vi.hoisted()if necessary for per-test mock state manipulation”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/useNodeReplacement.test.ts` around lines 693 - 723, The test mutates shared state LiteGraph.registered_node_types which can leak between tests; update the test to save and restore that map (or clone and reassign) around the mutation using a safe per-test setup/teardown pattern (e.g., store const original = { ...LiteGraph.registered_node_types } at start of the it block or in beforeEach, perform the assignment to register 'OldNode', then in finally or afterEach restore LiteGraph.registered_node_types = original), or convert the mutation to a vitest-scoped mock via vi.hoisted/vi.mock so the registration is isolated; reference LiteGraph.registered_node_types and the test that assigns 'OldNode' to locate where to add the save/restore.src/platform/nodeReplacement/components/SwapNodeGroupRow.vue (1)
105-105: DecoupleSwapNodeGroupRowfrom right-side-panel internals.Line 105 imports
SwapNodeGroupfrom@/components/rightSidePanel/errors/useErrorGroups, which introduces reverse coupling fromplatform/nodeReplacementback into a panel-specific composable. Consider extractingSwapNodeGroupto a shared node-replacement type module.♻️ Suggested direction
- import type { SwapNodeGroup } from '@/components/rightSidePanel/errors/useErrorGroups' + import type { SwapNodeGroup } from '@/platform/nodeReplacement/types'Then have both
useErrorGroups.tsand node-replacement components import from that shared type module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/components/SwapNodeGroupRow.vue` at line 105, SwapNodeGroupRow.vue currently imports the SwapNodeGroup type from useErrorGroups which creates a reverse coupling; extract the SwapNodeGroup type into a new shared node-replacement type module (e.g., nodeReplacementTypes or types/nodeReplacement) and export it there, then update SwapNodeGroupRow.vue to import SwapNodeGroup from that new shared module and update useErrorGroups.ts to also import the type from the shared module so both consumers use the same centralized type instead of importing from the right-side-panel composable.src/platform/nodeReplacement/components/SwapNodesCard.test.ts (1)
61-74: Prefer component-based row assertions over mock CSS class checks.The row-count assertions are coupled to the mocked
.swap-rowclass rather than the component behavior. CountingSwapNodeGroupRowcomponents is more robust.♻️ Suggested refactor
- expect(wrapper.findAll('.swap-row')).toHaveLength(3) + expect( + wrapper.findAllComponents({ name: 'SwapNodeGroupRow' }) + ).toHaveLength(3) - expect(wrapper.findAll('.swap-row')).toHaveLength(0) + expect( + wrapper.findAllComponents({ name: 'SwapNodeGroupRow' }) + ).toHaveLength(0) - expect(wrapper.findAll('.swap-row')).toHaveLength(1) + expect( + wrapper.findAllComponents({ name: 'SwapNodeGroupRow' }) + ).toHaveLength(1)Based on learnings: Applies to src/**/*.test.ts : Do not write tests dependent on non-behavioral features like utility classes or styles.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/components/SwapNodesCard.test.ts` around lines 61 - 74, Update the three assertions in SwapNodesCard.test.ts to count actual SwapNodeGroupRow components instead of querying the mocked .swap-row CSS class: locate the tests that call mountCard(...) and replace expect(wrapper.findAll('.swap-row')).toHaveLength(n) with an assertion that uses wrapper.findAllComponents(SwapNodeGroupRow) (or wrapper.findAllComponents({ name: 'SwapNodeGroupRow' }) if using name lookup) to assert the component count; ensure SwapNodeGroupRow is imported into the test file if not already.src/components/rightSidePanel/errors/TabErrors.vue (1)
269-273: Use actual replaced types for cleanup in single-group replace.To keep behavior aligned with
handleReplaceAll(), passreplacedtoremoveMissingNodesByTypeinstead of[group.type].♻️ Suggested refactor
function handleReplaceGroup(group: SwapNodeGroup) { const replaced = replaceNodesInPlace(group.nodeTypes) if (replaced.length > 0) { - executionErrorStore.removeMissingNodesByType([group.type]) + executionErrorStore.removeMissingNodesByType(replaced) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/TabErrors.vue` around lines 269 - 273, In handleReplaceGroup, after calling replaceNodesInPlace(group.nodeTypes) you currently call executionErrorStore.removeMissingNodesByType([group.type]); change it to pass the actual replaced array (the replaced variable) to executionErrorStore.removeMissingNodesByType so the cleanup mirrors handleReplaceAll—i.e., use replaced instead of [group.type] to remove missing node types that were actually replaced.src/platform/nodeReplacement/components/SwapNodeGroupRow.test.ts (1)
30-35: Replace icon-markup matching with semantic button queries.Selectors based on
html().includes('chevron'|'repeat'|'locate')are brittle. Prefer role/name or visible-label based queries so tests validate user-visible behavior instead of icon implementation details.Based on learnings: In test files, prefer selecting or asserting on accessible properties (text content, aria-label, role, accessible name) over implementation-detail selectors.
Also applies to: 99-103, 174-177, 187-190, 199-200, 213-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/components/SwapNodeGroupRow.test.ts` around lines 30 - 35, Replace brittle icon-markup matching in findChevron and the other occurrences (lines referenced around 99-103, 174-177, 187-190, 199-200, 213-214) with semantic queries that target accessible button properties: use the mounted wrapper helpers (e.g., mountRow) to find buttons by aria-label, title, visible text, or role/accessible name instead of html().includes('chevron'|'repeat'|'locate'); update findChevron to search wrapper.findAll('button').find(b => b.attributes('aria-label') === 'Chevron' || b.text().includes('Chevron') || b.props('aria-label') === 'Chevron') (or the app's actual accessible name) and do the same for the other helper/finders so tests assert on user-visible labels rather than icon markup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/rightSidePanel/errors/MissingPackGroupRow.test.ts`:
- Around line 151-159: The tests rely on icon/class selectors (e.g.,
findChevron, `.icon-[lucide--check]`, `.dot-spinner`) which are brittle; update
findChevron(wrapper) to locate the toggle button by its accessible attributes
instead of markup (e.g., find a button whose aria-label matches
/expand|collapse/i or use getByRole('button', { name: /expand|collapse/i }) on
the mounted wrapper), and replace other assertions in the referenced ranges
(lines ~187-190, 225-244, 271-303, 319-321, 348-378) to use role, visible text,
or aria-label queries (getByRole/getByText/getByLabelText or
wrapper.find('button[aria-label="..."]')) so tests assert user-observable
behavior rather than icon/class names; keep the same semantic checks
(expanded/collapsed state, presence of download/search/info actions, spinner vs
success state) but select elements by accessible labels or roles.
---
Nitpick comments:
In `@src/components/rightSidePanel/errors/MissingNodeCard.test.ts`:
- Around line 124-132: The tests should count MissingPackGroupRow components
instead of querying DOM utility classes; update the two assertions in
MissingNodeCard.test.ts to use component-level queries (e.g., call mountCard({
missingPackGroups: ... }) then use
wrapper.findAllComponents(MissingPackGroupRow) and expect its length to be 3 or
0 respectively), referencing the MissingPackGroupRow component and the mountCard
helper to locate the code to change.
In `@src/components/rightSidePanel/errors/TabErrors.vue`:
- Around line 269-273: In handleReplaceGroup, after calling
replaceNodesInPlace(group.nodeTypes) you currently call
executionErrorStore.removeMissingNodesByType([group.type]); change it to pass
the actual replaced array (the replaced variable) to
executionErrorStore.removeMissingNodesByType so the cleanup mirrors
handleReplaceAll—i.e., use replaced instead of [group.type] to remove missing
node types that were actually replaced.
In `@src/platform/nodeReplacement/components/SwapNodeGroupRow.test.ts`:
- Around line 30-35: Replace brittle icon-markup matching in findChevron and the
other occurrences (lines referenced around 99-103, 174-177, 187-190, 199-200,
213-214) with semantic queries that target accessible button properties: use the
mounted wrapper helpers (e.g., mountRow) to find buttons by aria-label, title,
visible text, or role/accessible name instead of
html().includes('chevron'|'repeat'|'locate'); update findChevron to search
wrapper.findAll('button').find(b => b.attributes('aria-label') === 'Chevron' ||
b.text().includes('Chevron') || b.props('aria-label') === 'Chevron') (or the
app's actual accessible name) and do the same for the other helper/finders so
tests assert on user-visible labels rather than icon markup.
In `@src/platform/nodeReplacement/components/SwapNodeGroupRow.vue`:
- Line 105: SwapNodeGroupRow.vue currently imports the SwapNodeGroup type from
useErrorGroups which creates a reverse coupling; extract the SwapNodeGroup type
into a new shared node-replacement type module (e.g., nodeReplacementTypes or
types/nodeReplacement) and export it there, then update SwapNodeGroupRow.vue to
import SwapNodeGroup from that new shared module and update useErrorGroups.ts to
also import the type from the shared module so both consumers use the same
centralized type instead of importing from the right-side-panel composable.
In `@src/platform/nodeReplacement/components/SwapNodesCard.test.ts`:
- Around line 61-74: Update the three assertions in SwapNodesCard.test.ts to
count actual SwapNodeGroupRow components instead of querying the mocked
.swap-row CSS class: locate the tests that call mountCard(...) and replace
expect(wrapper.findAll('.swap-row')).toHaveLength(n) with an assertion that uses
wrapper.findAllComponents(SwapNodeGroupRow) (or wrapper.findAllComponents({
name: 'SwapNodeGroupRow' }) if using name lookup) to assert the component count;
ensure SwapNodeGroupRow is imported into the test file if not already.
In `@src/platform/nodeReplacement/useNodeReplacement.test.ts`:
- Around line 693-723: The test mutates shared state
LiteGraph.registered_node_types which can leak between tests; update the test to
save and restore that map (or clone and reassign) around the mutation using a
safe per-test setup/teardown pattern (e.g., store const original = {
...LiteGraph.registered_node_types } at start of the it block or in beforeEach,
perform the assignment to register 'OldNode', then in finally or afterEach
restore LiteGraph.registered_node_types = original), or convert the mutation to
a vitest-scoped mock via vi.hoisted/vi.mock so the registration is isolated;
reference LiteGraph.registered_node_types and the test that assigns 'OldNode' to
locate where to add the save/restore.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/components/rightSidePanel/errors/MissingNodeCard.test.tssrc/components/rightSidePanel/errors/MissingPackGroupRow.test.tssrc/components/rightSidePanel/errors/TabErrors.vuesrc/components/rightSidePanel/errors/swapNodeGroups.test.tssrc/components/rightSidePanel/errors/useErrorGroups.test.tssrc/platform/nodeReplacement/components/SwapNodeGroupRow.test.tssrc/platform/nodeReplacement/components/SwapNodeGroupRow.vuesrc/platform/nodeReplacement/components/SwapNodesCard.test.tssrc/platform/nodeReplacement/components/SwapNodesCard.vuesrc/platform/nodeReplacement/missingNodeScan.test.tssrc/platform/nodeReplacement/missingNodeScan.tssrc/platform/nodeReplacement/useNodeReplacement.test.tssrc/scripts/app.tssrc/stores/executionErrorStore.test.ts
src/components/rightSidePanel/errors/MissingPackGroupRow.test.ts
Outdated
Show resolved
Hide resolved
… clean up test code - Move replaceGroup() and replaceAllGroups() helpers into useNodeReplacement, removing inline replace logic from TabErrors.vue - Remove unused useExecutionErrorStore import from TabErrors.vue - Clean up section dividers and non-null assertions in executionErrorStore.test.ts - Add useExecutionErrorStore mock to useNodeReplacement.test.ts
…omponents - Replace brittle icon/class-based selectors with semantic aria-label and text-content queries (e.g., �utton[aria-label='Expand'], .toContain()) - Populate i18n messages with actual translation strings instead of empty objects so assertions reflect real user-visible text - Remove custom indChevron / indButtonByAriaLabel helper functions; use wrapper.get() / wrapper.findAll() directly per project style - Update DotSpinner stub to use ole='status' and query via [role='status'] instead of class-based .dot-spinner selector - Replace non-null assertion operators (!) with optional chaining (?.) Affected files: - MissingPackGroupRow.test.ts - SwapNodeGroupRow.test.ts - MissingNodeCard.test.ts - SwapNodesCard.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/platform/nodeReplacement/useNodeReplacement.ts (1)
10-10: Decouple node-replacement domain code from right-side-panel UI types.
useNodeReplacementnow depends onSwapNodeGroupfrom a UI module path. Consider moving this contract into a node-replacement domain type (or defining a minimal local interface) to keep dependency direction clean.♻️ Suggested refactor
-import type { SwapNodeGroup } from '@/components/rightSidePanel/errors/useErrorGroups' +interface ReplacementGroup { + type: string + nodeTypes: MissingNodeType[] +} ... - function replaceGroup(group: SwapNodeGroup): void { + function replaceGroup(group: ReplacementGroup): void { ... - function replaceAllGroups(groups: SwapNodeGroup[]): void { + function replaceAllGroups(groups: ReplacementGroup[]): void {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/useNodeReplacement.ts` at line 10, The hook useNodeReplacement currently imports the UI type SwapNodeGroup from '@/components/rightSidePanel/errors/useErrorGroups', creating a UI→domain dependency; remove that import and instead declare a domain-level type (or a minimal local interface) that captures only the fields useNodeReplacement needs, then update all references in useNodeReplacement to use that new type (or export it from a node-replacement domain module) so the hook no longer depends on right-side-panel UI code.src/platform/nodeReplacement/components/SwapNodesCard.test.ts (1)
61-74: Prefer component queries over mock CSS-class selectors for row counts.The
.swap-rowassertions are coupled to mock template markup. UsefindAllComponents({ name: 'SwapNodeGroupRow' })so the test asserts component behavior rather than mock styling details.As per coding guidelines: `src/**/*.test.ts`: Do not write tests dependent on non-behavioral features like utility classes or styles.♻️ Suggested update
- expect(wrapper.findAll('.swap-row')).toHaveLength(3) + expect( + wrapper.findAllComponents({ name: 'SwapNodeGroupRow' }) + ).toHaveLength(3) ... - expect(wrapper.findAll('.swap-row')).toHaveLength(0) + expect( + wrapper.findAllComponents({ name: 'SwapNodeGroupRow' }) + ).toHaveLength(0) ... - expect(wrapper.findAll('.swap-row')).toHaveLength(1) + expect( + wrapper.findAllComponents({ name: 'SwapNodeGroupRow' }) + ).toHaveLength(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/components/SwapNodesCard.test.ts` around lines 61 - 74, Replace CSS-class based assertions that use wrapper.findAll('.swap-row') with component queries to assert real behavior: use wrapper.findAllComponents({ name: 'SwapNodeGroupRow' }) in the three tests inside SwapNodesCard.test.ts (the tests that call mountCard and currently assert lengths 3, 0, and 1). Keep the same expected lengths but change the selector to target the SwapNodeGroupRow component by name so tests rely on component presence rather than styling.src/components/rightSidePanel/errors/MissingNodeCard.test.ts (1)
124-132: Use child-component queries instead of mock class selectors for row-count tests.Counting
.pack-rowcouples these assertions to mock template details. PreferfindAllComponents({ name: 'MissingPackGroupRow' })to keep tests behavior-focused.As per coding guidelines: `src/**/*.test.ts`: Do not write tests dependent on non-behavioral features like utility classes or styles.♻️ Suggested update
- expect(wrapper.findAll('.pack-row')).toHaveLength(3) + expect( + wrapper.findAllComponents({ name: 'MissingPackGroupRow' }) + ).toHaveLength(3) ... - expect(wrapper.findAll('.pack-row')).toHaveLength(0) + expect( + wrapper.findAllComponents({ name: 'MissingPackGroupRow' }) + ).toHaveLength(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/MissingNodeCard.test.ts` around lines 124 - 132, Tests in MissingNodeCard.test.ts are asserting row counts by querying the mock CSS selector '.pack-row', which couples tests to template implementation; update the two assertions to use component queries instead—use mountCard(...) as before but replace wrapper.findAll('.pack-row') with wrapper.findAllComponents({ name: 'MissingPackGroupRow' }) (or the equivalent named component query) so the tests count MissingPackGroupRow components produced when calling mountCard({ missingPackGroups: makePackGroups(3) }) and mountCard({ missingPackGroups: [] }).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/nodeReplacement/components/SwapNodeGroupRow.test.ts`:
- Around line 130-135: The test currently asserts on the utility class
"rotate-180" which is fragile; update the "toggles chevron rotation when
expanded" spec to assert user-observable behavior instead by using mountRow and
the Expand button: trigger the click on button[aria-label="Expand"] and then
assert the accessible/visible state change (for example check the button's
aria-expanded attribute toggles or that the expanded panel/content for
SwapNodeGroupRow becomes present/hidden) rather than checking for the
"rotate-180" CSS class.
---
Nitpick comments:
In `@src/components/rightSidePanel/errors/MissingNodeCard.test.ts`:
- Around line 124-132: Tests in MissingNodeCard.test.ts are asserting row counts
by querying the mock CSS selector '.pack-row', which couples tests to template
implementation; update the two assertions to use component queries instead—use
mountCard(...) as before but replace wrapper.findAll('.pack-row') with
wrapper.findAllComponents({ name: 'MissingPackGroupRow' }) (or the equivalent
named component query) so the tests count MissingPackGroupRow components
produced when calling mountCard({ missingPackGroups: makePackGroups(3) }) and
mountCard({ missingPackGroups: [] }).
In `@src/platform/nodeReplacement/components/SwapNodesCard.test.ts`:
- Around line 61-74: Replace CSS-class based assertions that use
wrapper.findAll('.swap-row') with component queries to assert real behavior: use
wrapper.findAllComponents({ name: 'SwapNodeGroupRow' }) in the three tests
inside SwapNodesCard.test.ts (the tests that call mountCard and currently assert
lengths 3, 0, and 1). Keep the same expected lengths but change the selector to
target the SwapNodeGroupRow component by name so tests rely on component
presence rather than styling.
In `@src/platform/nodeReplacement/useNodeReplacement.ts`:
- Line 10: The hook useNodeReplacement currently imports the UI type
SwapNodeGroup from '@/components/rightSidePanel/errors/useErrorGroups', creating
a UI→domain dependency; remove that import and instead declare a domain-level
type (or a minimal local interface) that captures only the fields
useNodeReplacement needs, then update all references in useNodeReplacement to
use that new type (or export it from a node-replacement domain module) so the
hook no longer depends on right-side-panel UI code.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/rightSidePanel/errors/MissingNodeCard.test.tssrc/components/rightSidePanel/errors/MissingPackGroupRow.test.tssrc/components/rightSidePanel/errors/TabErrors.vuesrc/platform/nodeReplacement/components/SwapNodeGroupRow.test.tssrc/platform/nodeReplacement/components/SwapNodesCard.test.tssrc/platform/nodeReplacement/useNodeReplacement.test.tssrc/platform/nodeReplacement/useNodeReplacement.tssrc/stores/executionErrorStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/platform/nodeReplacement/useNodeReplacement.test.ts
src/platform/nodeReplacement/components/SwapNodeGroupRow.test.ts
Outdated
Show resolved
Hide resolved
…ection When last_serialization.type is absent (old serialization format), the predicate falls back to n.type, which app.ts may have already passed through sanitizeNodeName — stripping HTML special chars (& < > ' =). In that case, targetTypes.has(n.type) would silently fail because targetTypes holds the original unsanitized names. Fix by also adding sanitized variants of each type into targetTypes at construction time. For the common case (no special chars), the Set deduplicates them and behavior is identical to before. - Add sanitized variants to targetTypes Set in replaceNodesInPlace - Add regression test covering the edge case: last_serialization.type absent + live type already sanitized
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/platform/nodeReplacement/components/SwapNodeGroupRow.test.ts (1)
182-192: Consider using explicit edge-case typing instead ofas never.The
as nevercasts bypass type checking entirely. For edge-case testing, a more explicit approach signals intent better:Optional: More explicit edge-case typing
it('does not render Locate button for nodeTypes without nodeId', async () => { const wrapper = mountRow({ group: makeGroup({ - nodeTypes: [{ type: 'NoIdNode', isReplaceable: true } as never] + // Intentionally malformed: missing nodeId to test graceful handling + nodeTypes: [{ type: 'NoIdNode', isReplaceable: true }] as unknown as typeof makeGroup extends () => infer G ? G['nodeTypes'] : never }) })Alternatively, if
MissingNodeType.nodeIdis actually optional, update the test data to match the real type signature.Also applies to: 229-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/components/SwapNodeGroupRow.test.ts` around lines 182 - 192, The test uses an unsafe cast (`as never`) when passing nodeTypes to makeGroup; replace that with an explicit edge-case typed object or construct a proper test fixture matching the actual type signature instead of silencing the compiler. Update the test in "does not render Locate button for nodeTypes without nodeId" to call mountRow(makeGroup(...)) with a nodeTypes entry shaped to the real NodeType (e.g., a partial/Optional type or a dedicated MissingNodeType variant) rather than using `as never`; ensure other occurrences (the similar case around lines 229-237) are updated the same way so tests reflect real types and preserve type safety for functions makeGroup and mountRow.src/platform/nodeReplacement/useNodeReplacement.test.ts (1)
661-833: Add behavioral tests for the new cleanup helpers (replaceGroup/replaceAllGroups).This suite thoroughly checks predicate behavior, but it does not assert the new
store-cleanup side effects fromreplaceGroupandreplaceAllGroups. Please
add tests that verifyremoveMissingNodesByTypereceives all successfully
replaced types (including multi-type group scenarios).As per coding guidelines: "Do not write tests that just test the mocks; ensure tests fail when code behaves unexpectedly" and "aim for behavioral coverage of critical and new features."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/useNodeReplacement.test.ts` around lines 661 - 833, Add behavioral tests that exercise replaceGroup and replaceAllGroups from useNodeReplacement and assert the store cleanup helper removeMissingNodesByType is called with the exact set of types actually replaced (including multi-type group cases). Specifically: mock or spy on removeMissingNodesByType to capture its arguments, construct a group payload (and a multi-type group) via makeMissingNodeType entries, ensure collectAllNodes returns the nodes to be replaced, call replaceGroup and replaceAllGroups, then expect removeMissingNodesByType to have been invoked with an array containing all original old_node_id types that were successfully replaced; also reset/cleanup the spy between tests. Reference functions/helpers: replaceGroup, replaceAllGroups, removeMissingNodesByType, useNodeReplacement, makeMissingNodeType, and collectAllNodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/nodeReplacement/useNodeReplacement.ts`:
- Around line 331-335: The cleanup in replaceGroup currently always removes only
group.type even though replaceNodesInPlace(group.nodeTypes) returns the actual
list of successfully replaced types; update replaceGroup to call
useExecutionErrorStore().removeMissingNodesByType with the replaced array (the
value returned by replaceNodesInPlace) instead of [group.type] so you clear
stale missing-node errors for all types that were actually replaced; locate this
logic inside the replaceGroup function and change the removeMissingNodesByType
argument from [group.type] to replaced.
---
Nitpick comments:
In `@src/platform/nodeReplacement/components/SwapNodeGroupRow.test.ts`:
- Around line 182-192: The test uses an unsafe cast (`as never`) when passing
nodeTypes to makeGroup; replace that with an explicit edge-case typed object or
construct a proper test fixture matching the actual type signature instead of
silencing the compiler. Update the test in "does not render Locate button for
nodeTypes without nodeId" to call mountRow(makeGroup(...)) with a nodeTypes
entry shaped to the real NodeType (e.g., a partial/Optional type or a dedicated
MissingNodeType variant) rather than using `as never`; ensure other occurrences
(the similar case around lines 229-237) are updated the same way so tests
reflect real types and preserve type safety for functions makeGroup and
mountRow.
In `@src/platform/nodeReplacement/useNodeReplacement.test.ts`:
- Around line 661-833: Add behavioral tests that exercise replaceGroup and
replaceAllGroups from useNodeReplacement and assert the store cleanup helper
removeMissingNodesByType is called with the exact set of types actually replaced
(including multi-type group cases). Specifically: mock or spy on
removeMissingNodesByType to capture its arguments, construct a group payload
(and a multi-type group) via makeMissingNodeType entries, ensure collectAllNodes
returns the nodes to be replaced, call replaceGroup and replaceAllGroups, then
expect removeMissingNodesByType to have been invoked with an array containing
all original old_node_id types that were successfully replaced; also
reset/cleanup the spy between tests. Reference functions/helpers: replaceGroup,
replaceAllGroups, removeMissingNodesByType, useNodeReplacement,
makeMissingNodeType, and collectAllNodes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/platform/nodeReplacement/components/SwapNodeGroupRow.test.tssrc/platform/nodeReplacement/useNodeReplacement.test.tssrc/platform/nodeReplacement/useNodeReplacement.ts
…-class selectors in tests - Replace SwapNodeGroup import in useNodeReplacement.ts with a local ReplacementGroup interface to keep dependency direction clean - Replace .swap-row and .pack-row class selectors in tests with findAllComponents() to assert component behavior, not mock markup
replaceGroup was removing group.type unconditionally on any success, which could leave stale errors if only a subset of node types in the group was actually replaced. Use the returned replaced list instead, consistent with replaceAllGroups.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/platform/nodeReplacement/components/SwapNodesCard.test.ts (1)
9-16: Consider simplifying the mock template.The
class="swap-row"in the mock template is never used in the tests. You could simplify to just<div />.♻️ Optional simplification
vi.mock('./SwapNodeGroupRow.vue', () => ({ default: { name: 'SwapNodeGroupRow', - template: '<div class="swap-row" />', + template: '<div />', props: ['group', 'showNodeIdBadge'], emits: ['locate-node', 'replace'] } }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/components/SwapNodesCard.test.ts` around lines 9 - 16, The mock for SwapNodeGroupRow can be simplified by removing the unused class attribute: in the vi.mock block that defines the default export for 'SwapNodeGroupRow' (the object with name: 'SwapNodeGroupRow', template, props and emits), change the template from '<div class="swap-row" />' to a plain '<div />' so the mock remains minimal and tests remain unaffected.src/components/rightSidePanel/errors/MissingNodeCard.test.ts (1)
17-38: Minor inconsistency in mock value containers.
mockIsRestartingusesref()(line 18) whilemockShouldShowManagerButtonsuses a plain object with avalueproperty (line 33). Both work, but usingref()consistently would be slightly cleaner since Vue Test Utils handles refs appropriately.♻️ Optional: use ref() consistently
-const mockShouldShowManagerButtons = { value: false } +const mockShouldShowManagerButtons = ref(false) vi.mock('@/workbench/extensions/manager/composables/useManagerState', () => ({ useManagerState: () => ({ shouldShowManagerButtons: mockShouldShowManagerButtons }) }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/MissingNodeCard.test.ts` around lines 17 - 38, Replace the plain object container for the manager buttons mock with a Vue ref to match the pattern used by mockIsRestarting: change mockShouldShowManagerButtons (used in the useManagerState mock) to be a ref(false) so tests consistently expose a reactive .value and align with how mockIsRestarting is provided by useApplyChanges.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/rightSidePanel/errors/MissingNodeCard.test.ts`:
- Around line 17-38: Replace the plain object container for the manager buttons
mock with a Vue ref to match the pattern used by mockIsRestarting: change
mockShouldShowManagerButtons (used in the useManagerState mock) to be a
ref(false) so tests consistently expose a reactive .value and align with how
mockIsRestarting is provided by useApplyChanges.
In `@src/platform/nodeReplacement/components/SwapNodesCard.test.ts`:
- Around line 9-16: The mock for SwapNodeGroupRow can be simplified by removing
the unused class attribute: in the vi.mock block that defines the default export
for 'SwapNodeGroupRow' (the object with name: 'SwapNodeGroupRow', template,
props and emits), change the template from '<div class="swap-row" />' to a plain
'<div />' so the mock remains minimal and tests remain unaffected.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/rightSidePanel/errors/MissingNodeCard.test.tssrc/platform/nodeReplacement/components/SwapNodesCard.test.tssrc/platform/nodeReplacement/useNodeReplacement.ts
…havioral tests - Replace �s never type casts in SwapNodeGroupRow.test.ts with �s unknown as MissingNodeType[] and intent-clarifying comments - Add shared mockRemoveMissingNodesByType spy to executionErrorStore mock so store cleanup side-effects can be asserted across call sites - Add behavioral tests for replaceGroup and replaceAllGroups covering: - successful replacement calls removeMissingNodesByType with replaced types - no replacement skips the cleanup call - partial failure removes only the types that actually succeeded
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/nodeReplacement/components/SwapNodeGroupRow.test.ts`:
- Around line 111-114: The test "starts collapsed — node list not visible" is
non-diagnostic because mountRow() defaults showNodeIdBadge: false; update the
test to mountRow with showNodeIdBadge: true (e.g., mountRow({ showNodeIdBadge:
true })) and then assert collapse behavior by verifying '#1' is not present
initially and becomes present after triggering the expand action (e.g., call the
row's expand/toggle handler or simulate the click). Use the mountRow helper and
the component's expand/toggle method to prove behavior rather than relying on
default props.
In `@src/platform/nodeReplacement/useNodeReplacement.test.ts`:
- Around line 47-52: The module factory is closing over a module-scope spy
(mockRemoveMissingNodesByType); replace that top-level vi.fn with a hoisted spy
using vi.hoisted() and return it from the mocked module factory so the mock is
created in a hoisted context; update the code around
mockRemoveMissingNodesByType and the vi.mock of useExecutionErrorStore to
reference the hoisted spy so tests follow the project convention and avoid
global mutable state.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/platform/nodeReplacement/components/SwapNodeGroupRow.test.tssrc/platform/nodeReplacement/useNodeReplacement.test.ts
…store spy - Add showNodeIdBadge: true to the 'starts collapsed' test so that the assertion actually exercises collapse behavior (without it, #1 never renders regardless of expand state) - Replace module-scope const spy with vi.hoisted() per project convention to ensure the spy is initialized before the vi.mock() factory runs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/platform/nodeReplacement/useNodeReplacement.test.ts (1)
702-732: Harden test isolation aroundregistered_node_typesmutation.Line 704 mutates global mocked state, and cleanup at Line 729 only runs if the test reaches the end. If an assertion fails early, later tests can inherit polluted state. Wrap mutation/cleanup in
try/finally.As per coding guidelines: "For mocking in Vitest, leverage Vitest's utilities where possible; keep module mocks contained without global mutable state; use `vi.hoisted()` if necessary for per-test mock state manipulation".Suggested patch
- ;(LiteGraph.registered_node_types as Record<string, unknown>)['OldNode'] = - {} + const registeredNodeTypes = + LiteGraph.registered_node_types as Record<string, unknown> + registeredNodeTypes['OldNode'] = {} - const placeholder = createPlaceholderNode(1, 'OldNode') - const graph = createMockGraph([placeholder]) - placeholder.graph = graph - Object.assign(app, { rootGraph: graph }) - - vi.mocked(collectAllNodes).mockReturnValue([]) - - const { replaceNodesInPlace } = useNodeReplacement() - replaceNodesInPlace([ - makeMissingNodeType('OldNode', { - new_node_id: 'NewNode', - old_node_id: 'OldNode', - old_widget_ids: null, - input_mapping: null, - output_mapping: null - }) - ]) - - const predicate = capturedPredicate() - expect(predicate(placeholder)).toBe(true) - - // Cleanup - delete (LiteGraph.registered_node_types as Record<string, unknown>)[ - 'OldNode' - ] + try { + const placeholder = createPlaceholderNode(1, 'OldNode') + const graph = createMockGraph([placeholder]) + placeholder.graph = graph + Object.assign(app, { rootGraph: graph }) + + vi.mocked(collectAllNodes).mockReturnValue([]) + + const { replaceNodesInPlace } = useNodeReplacement() + replaceNodesInPlace([ + makeMissingNodeType('OldNode', { + new_node_id: 'NewNode', + old_node_id: 'OldNode', + old_widget_ids: null, + input_mapping: null, + output_mapping: null + }) + ]) + + const predicate = capturedPredicate() + expect(predicate(placeholder)).toBe(true) + } finally { + delete registeredNodeTypes['OldNode'] + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/useNodeReplacement.test.ts` around lines 702 - 732, The test mutates global state by assigning to LiteGraph.registered_node_types['OldNode'] and only deletes it at the end, risking test pollution; wrap the mutation and the assertion block in a try/finally so the deletion always runs (or use a per-test mocked copy via vi mocks), i.e. save any original value from LiteGraph.registered_node_types['OldNode'], set the test stub, run the logic that obtains replaceNodesInPlace and asserts predicate(placeholder), and in finally restore the original value (delete if undefined) to ensure isolation for useNodeReplacement(), createPlaceholderNode(), replaceNodesInPlace(), and collectAllNodes() interactions.src/platform/nodeReplacement/components/SwapNodeGroupRow.test.ts (2)
186-190: Removeas unknown as MissingNodeType[]casts in fixturesLine 189 and Line 237 use double assertions that weaken type checks. These fixtures appear directly representable as
MissingNodeType[]without casting (or viasatisfies).Proposed cleanup
- nodeTypes: [ - { type: 'NoIdNode', isReplaceable: true } - ] as unknown as MissingNodeType[] + nodeTypes: [{ type: 'NoIdNode', isReplaceable: true }] @@ - nodeTypes: ['StringType'] as unknown as MissingNodeType[] + nodeTypes: ['StringType']Based on learnings: "In test files matching **/*.test.ts under src, when creating test helper functions that construct mock objects implementing an interface, prefer using satisfies InterfaceType for shape validation instead of type assertions."
Also applies to: 236-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/components/SwapNodeGroupRow.test.ts` around lines 186 - 190, Remove the unsafe double-cast "as unknown as MissingNodeType[]" used for the nodeTypes fixture in SwapNodeGroupRow.test.ts and instead construct the fixture to directly satisfy the MissingNodeType shape (or use the TypeScript "satisfies MissingNodeType[]" operator) so the compiler validates the object shape; update the two occurrences around the nodeTypes array (the fixture that contains { type: 'NoIdNode', isReplaceable: true }) to either be typed as const and use satisfies MissingNodeType[] or to explicitly include the MissingNodeType properties without any "as unknown as" casts.
46-51: TypemountRowprops from the component to avoid prop-shape driftThe helper currently duplicates the prop shape manually. Import
ComponentPropsfromvue-component-type-helpersand type the helper as:function mountRow( props: Partial<ComponentProps<typeof SwapNodeGroupRow>> = {} )This keeps tests aligned when the component's props evolve.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/components/SwapNodeGroupRow.test.ts` around lines 46 - 51, Replace the manual prop typing for the test helper with the component's actual props: import ComponentProps from 'vue-component-type-helpers' and change the mountRow signature to use Partial<ComponentProps<typeof SwapNodeGroupRow>> so mountRow(props = {}) is typed against SwapNodeGroupRow; update any references to the previous inline type and ensure the import and type usage reference the SwapNodeGroupRow symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/platform/nodeReplacement/components/SwapNodeGroupRow.test.ts`:
- Around line 186-190: Remove the unsafe double-cast "as unknown as
MissingNodeType[]" used for the nodeTypes fixture in SwapNodeGroupRow.test.ts
and instead construct the fixture to directly satisfy the MissingNodeType shape
(or use the TypeScript "satisfies MissingNodeType[]" operator) so the compiler
validates the object shape; update the two occurrences around the nodeTypes
array (the fixture that contains { type: 'NoIdNode', isReplaceable: true }) to
either be typed as const and use satisfies MissingNodeType[] or to explicitly
include the MissingNodeType properties without any "as unknown as" casts.
- Around line 46-51: Replace the manual prop typing for the test helper with the
component's actual props: import ComponentProps from
'vue-component-type-helpers' and change the mountRow signature to use
Partial<ComponentProps<typeof SwapNodeGroupRow>> so mountRow(props = {}) is
typed against SwapNodeGroupRow; update any references to the previous inline
type and ensure the import and type usage reference the SwapNodeGroupRow symbol.
In `@src/platform/nodeReplacement/useNodeReplacement.test.ts`:
- Around line 702-732: The test mutates global state by assigning to
LiteGraph.registered_node_types['OldNode'] and only deletes it at the end,
risking test pollution; wrap the mutation and the assertion block in a
try/finally so the deletion always runs (or use a per-test mocked copy via vi
mocks), i.e. save any original value from
LiteGraph.registered_node_types['OldNode'], set the test stub, run the logic
that obtains replaceNodesInPlace and asserts predicate(placeholder), and in
finally restore the original value (delete if undefined) to ensure isolation for
useNodeReplacement(), createPlaceholderNode(), replaceNodesInPlace(), and
collectAllNodes() interactions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/platform/nodeReplacement/components/SwapNodeGroupRow.test.tssrc/platform/nodeReplacement/useNodeReplacement.test.ts
christian-byrne
deleted the
chore/node-replacement-tests-and-refactor
branch
DrJKL
pushed a commit
that referenced
this pull request
Feb 28, 2026
…omprehensive test suite (#9301) ## Summary Resolves six open issues by reorganizing node replacement components into a domain-driven folder structure, refactoring event handling to follow the emit pattern, and adding comprehensive test coverage across all affected modules. ## Changes - **What**: - Moved `SwapNodeGroupRow.vue` and `SwapNodesCard.vue` from `src/components/rightSidePanel/errors/` to `src/platform/nodeReplacement/components/` (Issues #9255) - Moved `useMissingNodeScan.ts` from `src/composables/` to `src/platform/nodeReplacement/missingNodeScan.ts`, renamed to reflect it is a plain function not a Vue composable (Issues #9254) - Refactored `SwapNodeGroupRow.vue` to emit a `'replace'` event instead of calling `useNodeReplacement()` and `useExecutionErrorStore()` directly; replacement logic now handled in `TabErrors.vue` (Issue #9267) - Added unit tests for `removeMissingNodesByType` (`executionErrorStore.test.ts`), `scanMissingNodes` (`missingNodeScan.test.ts`), and `swapNodeGroups` computed (`swapNodeGroups.test.ts`, `useErrorGroups.test.ts`) (Issue #9270) - Added placeholder detection tests covering unregistered-type detection when `has_errors` is false, and exclusion of registered types (`useNodeReplacement.test.ts`) (Issue #9271) - Added component tests for `MissingNodeCard` and `MissingPackGroupRow` covering rendering, expand/collapse, events, install states, and edge cases (Issue #9231) - Added component tests for `SwapNodeGroupRow` and `SwapNodesCard` (Issues #9255, #9267) ## Additional Changes (Post-Review) - **Edge case guard in placeholder detection** (`useNodeReplacement.ts`): When `last_serialization.type` is absent (old serialization format), the predicate falls back to `n.type`, which the app may have already run through `sanitizeNodeName` — stripping HTML special characters (`& < > " ' \` =`). In that case, a `Set.has()` lookup against the original unsanitized type name would silently miss, causing replacement to be skipped. Fixed by including sanitized variants of each target type in the `targetTypes` Set at construction time. For the overwhelmingly common case (no special characters in type names), the Set deduplicates the entries and runtime behavior is identical to before. A regression test was added to cover the specific scenario: `last_serialization.type` absent + live `n.type` already sanitized. ## Review Focus - `TabErrors.vue`: confirm the new `@replace` event handler correctly replaces nodes and removes them from missing nodes list (mirrors the old inline logic in `SwapNodeGroupRow`) - `missingNodeScan.ts`: filename/export name change from `useMissingNodeScan` — verify all call sites updated via `app.ts` - Test mocking strategy: module-level `vi.mock()` factories use closures over `ref`/plain objects to allow per-test overrides without global mutable state - Fixes #9231 - Fixes #9254 - Fixes #9255 - Fixes #9267 - Fixes #9270 - Fixes #9271
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves six open issues by reorganizing node replacement components into a domain-driven folder structure, refactoring event handling to follow the emit pattern, and adding comprehensive test coverage across all affected modules.
Changes
SwapNodeGroupRow.vueandSwapNodesCard.vuefromsrc/components/rightSidePanel/errors/tosrc/platform/nodeReplacement/components/(Issues Refactor: Move node replacement UI components to domain-driven folder structure #9255)useMissingNodeScan.tsfromsrc/composables/tosrc/platform/nodeReplacement/missingNodeScan.ts, renamed to reflect it is a plain function not a Vue composable (Issues Refactor: Move useMissingNodeScan.ts to domain-driven location #9254)SwapNodeGroupRow.vueto emit a'replace'event instead of callinguseNodeReplacement()anduseExecutionErrorStore()directly; replacement logic now handled inTabErrors.vue(Issue Refactor SwapNodeGroupRow to emit 'replace' event instead of calling stores directly #9267)removeMissingNodesByType(executionErrorStore.test.ts),scanMissingNodes(missingNodeScan.test.ts), andswapNodeGroupscomputed (swapNodeGroups.test.ts,useErrorGroups.test.ts) (Issue Add unit tests for new error handling functions from PR#9253#9270)has_errorsis false, and exclusion of registered types (useNodeReplacement.test.ts) (Issue Add test coverage for placeholder detection logic in node replacement #9271)MissingNodeCardandMissingPackGroupRowcovering rendering, expand/collapse, events, install states, and edge cases (Issue Add component tests for MissingNodeCard and MissingPackGroupRow #9231)SwapNodeGroupRowandSwapNodesCard(Issues Refactor: Move node replacement UI components to domain-driven folder structure #9255, Refactor SwapNodeGroupRow to emit 'replace' event instead of calling stores directly #9267)Additional Changes (Post-Review)
Edge case guard in placeholder detection (
useNodeReplacement.ts): Whenlast_serialization.typeis absent (old serialization format), the predicate falls back ton.type, which the app may have already run throughsanitizeNodeName— stripping HTML special characters (& < > " ' \=). In that case, aSet.has()` lookup against the original unsanitized type name would silently miss, causing replacement to be skipped.Fixed by including sanitized variants of each target type in the
targetTypesSet at construction time. For the overwhelmingly common case (no special characters in type names), the Set deduplicates the entries and runtime behavior is identical to before.A regression test was added to cover the specific scenario:
last_serialization.typeabsent + liven.typealready sanitized.Review Focus
TabErrors.vue: confirm the new@replaceevent handler correctly replaces nodes and removes them from missing nodes list (mirrors the old inline logic inSwapNodeGroupRow)missingNodeScan.ts: filename/export name change fromuseMissingNodeScan— verify all call sites updated viaapp.tsTest mocking strategy: module-level
vi.mock()factories use closures overref/plain objects to allow per-test overrides without global mutable stateFixes Add component tests for MissingNodeCard and MissingPackGroupRow #9231
Fixes Refactor: Move useMissingNodeScan.ts to domain-driven location #9254
Fixes Refactor: Move node replacement UI components to domain-driven folder structure #9255
Fixes Refactor SwapNodeGroupRow to emit 'replace' event instead of calling stores directly #9267
Fixes Add unit tests for new error handling functions from PR
#9253#9270Fixes Add test coverage for placeholder detection logic in node replacement #9271