deep-merge configs, remove redundant YAML parsing, add collision warnings & robustness fixes #30

stonegray · 2026-01-14T13:52:33Z

Overview
Comprehensive review and refactoring of backends with focus on the Traefik templating engine. 7 commits address bugs, performance issues, dead code, and type safety.

Changes
🐛 Bug Fixes

Fix shallow merge bug in register() (6f30e9a)

Issue: Shallow merge was overwriting nested config sections entirely instead of merging them
Impact: Calling register() twice for the same app with different routers would lose the first app's routers
Fix: Implemented deep merge that properly combines routers, services, and middlewares sections
2. Fix unsafe non-null assertions (061dfb1)

Issue: activeBackend! assertions after initialize() could throw confusing errors if initialization failed
Impact: Poor error diagnostics if backend setup failed
Fix: Added ensureBackend() helper that explicitly checks the backend and throws a clear error message if missing
♻️ Refactoring
3. Eliminate double YAML parse-dump cycle (9299cf5)

Issue: Template rendering performed parse→dump→parse: renderTemplate() parsed YAML then dumped it back to string, only for the caller to parse it again
Impact: Unnecessary overhead and redundant operations
Fix: Split into two functions:
renderTemplate() - returns raw rendered string (for testing/debugging)
renderTemplateParsed() - returns parsed YAML object (for production)
Result: Single parse operation, cleaner API
🧹 Code Quality
4. Remove unused lastUserData tracking (b708a20)

Issue: Dead code - lastUserData variable and _getLastUserData() function never used
Fix: Removed variable, accessor function, and related cleanup code
Result: -3 lines of unused code
5. Fix lint violations (fcdc242)

Issue: 3 ESLint warnings for explicit any types
Fix: Replaced with specific Record<string, string | Record<string, string>> types
Result: Linting now passes with zero warnings
✨ New Features & Performance
6. Warn on config name collisions (dd62a74)

Feature: Log warnings when multiple apps define the same router/service/middleware name
Benefit: Helps catch misconfigurations where one app silently overwrites another's config
Implementation: Enhanced mergeRecord() to detect and log collisions per section (http.routers, tcp.services, etc.)
7. Optimize temp file cleanup (542afcc)

Issue: cleanupTempFiles() ran on every config flush, causing unnecessary filesystem operations
Fix: Tracks cleanup state per output file, runs only once, resets when file changes
Result: Reduced filesystem churn during rapid config updates
Testing
✅ All 203 tests passing
✅ Linting clean
✅ No breaking changes

Summary Stats
7 commits across traefik backend components
6 files modified
~50 lines changed
0 tests broken
3 bugs fixed, 1 perf improvement, 1 new feature, 2 code quality improvements

The shallow merge was overwriting nested config sections entirely instead of merging them. Now properly merges routers, services, and middlewares when updating an existing app's configuration.

Split renderTemplate into two functions: - renderTemplate() returns raw rendered string - renderTemplateParsed<T>() returns parsed YAML object This eliminates the redundant parse->dump->parse cycle where templates were parsed to YAML, dumped back to string, then parsed again by the caller.

The lastUserData variable and _getLastUserData() function were never used by any tests or production code. Removed to reduce dead code.

When building the combined config, log a warning if multiple apps define the same router, service, or middleware name. This helps catch misconfigurations where one app's config would silently overwrite another's.

if initialization failed, the next line would throw a confusing error. Added ensureBackend() helper that explicitly checks and throws a clear error message if the backend failed to initialize.

Previously cleanupTempFiles() ran on every config flush, causing unnecessary filesystem operations. Now tracks cleanup state and only runs once per output file, resetting when the output file changes.

Changed buildContext and getContextValue to use specific Record<string, ...> types instead of 'any' to satisfy ESLint no-explicit-any rules.

Copilot

Pull request overview

This PR refactors the Traefik backend to address several bugs, performance issues, and code quality concerns. The main changes include implementing deep merge for config registration, eliminating redundant YAML parsing, adding collision warnings, and improving error handling.

Changes:

Fixed shallow merge bug in register() by implementing deep merge of routers, services, and middlewares
Eliminated double YAML parse cycle by splitting template rendering into two functions
Added collision detection warnings when multiple apps define the same config names
Optimized temp file cleanup to run only once per output file
Improved error handling by replacing unsafe non-null assertions with explicit checks
Removed unused lastUserData tracking code

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
test/unit/traefik/user-data-substitution.test.ts	Updated test expectation to reflect raw template output instead of YAML-normalized output
test/unit/traefik/template-error-handling.test.ts	Updated to use new `renderTemplateParsed` function for YAML validation tests
src/backends/traefik/traefikManager.ts	Implemented deep merge in `register()`, added collision warnings, optimized temp file cleanup
src/backends/traefik/traefik.ts	Updated to use new `renderTemplateParsed` function, removed unused `lastUserData` tracking
src/backends/traefik/templateParser.ts	Split rendering into `renderTemplate()` and `renderTemplateParsed()` to eliminate redundant parsing
src/backends/backendPlugin.ts	Replaced unsafe non-null assertions with `ensureBackend()` helper for better error messages

Comments suppressed due to low confidence (1)

src/backends/traefik/traefikManager.ts:246

The deep merge functionality described as the main bug fix in the PR lacks test coverage. The PR description mentions that calling register twice for the same app with different routers would lose the first app's routers with shallow merge. Consider adding a test that calls register twice for the same app with different router configurations and verifies that both routers are preserved in the final merged config.

export function register(appName: string, config: Partial<TraefikConfigYamlFormat>): void {
    const existing = registry.get(appName) ?? {};

    // Deep merge each section to preserve existing routers/services/middlewares
    const merged: TraefikConfigYamlFormat = {
        http: (existing.http || config.http) ? {
            routers: { ...existing.http?.routers, ...config.http?.routers },
            services: { ...existing.http?.services, ...config.http?.services },
            middlewares: { ...existing.http?.middlewares, ...config.http?.middlewares },
        } : undefined,
        tcp: (existing.tcp || config.tcp) ? {
            routers: { ...existing.tcp?.routers, ...config.tcp?.routers },
            services: { ...existing.tcp?.services, ...config.tcp?.services },
        } : undefined,
        udp: (existing.udp || config.udp) ? {
            services: { ...existing.udp?.services, ...config.udp?.services },
        } : undefined,
    };

    registry.set(appName, merged);
    log.debug({ message: 'App registered', data: { appName } });
}

Copilot · 2026-01-14T13:57:51Z

src/backends/traefik/traefik.ts

        const rendered = renderTemplate(templateContent, appName, data);
        lastRendered = rendered;
-        return yaml.load(rendered) as TraefikConfigYamlFormat;
+        return renderTemplateParsed<TraefikConfigYamlFormat>(templateContent, appName, data);


The template is being rendered twice - once on line 58 to store in lastRendered, and again on line 60 by renderTemplateParsed (which internally calls renderTemplate). This defeats the purpose of the performance optimization described in the PR. Consider calling renderTemplateParsed first, then storing the result from a single renderTemplate call for lastRendered, or modify renderTemplateParsed to optionally return both the parsed result and the raw string.

Copilot · 2026-01-14T13:57:52Z

src/backends/traefik/traefikManager.ts

+        http: (existing.http || config.http) ? {
+            routers: { ...existing.http?.routers, ...config.http?.routers },
+            services: { ...existing.http?.services, ...config.http?.services },
+            middlewares: { ...existing.http?.middlewares, ...config.http?.middlewares },
+        } : undefined,


The deep merge implementation creates empty objects for routers, services, and middlewares even when neither the existing config nor the incoming config has values for those fields. For example, if existing.http is an empty object and config.http is also an empty object, this will create http.routers = {}, http.services = {}, and http.middlewares = {} which will then be included in the final YAML output. Consider only creating these nested properties when at least one of the sources has actual values, or use a helper function that filters out empty objects after the merge.

Copilot · 2026-01-14T13:57:53Z

src/backends/traefik/traefikManager.ts

+        tcp: (existing.tcp || config.tcp) ? {
+            routers: { ...existing.tcp?.routers, ...config.tcp?.routers },
+            services: { ...existing.tcp?.services, ...config.tcp?.services },
+        } : undefined,


The deep merge implementation creates empty objects for routers and services even when neither the existing config nor the incoming config has values for those fields. Similar to the http section, this will create tcp.routers = {} and tcp.services = {} which will then be included in the final YAML output. Consider only creating these nested properties when at least one of the sources has actual values.

Copilot · 2026-01-14T13:57:53Z

src/backends/traefik/traefikManager.ts

+        udp: (existing.udp || config.udp) ? {
+            services: { ...existing.udp?.services, ...config.udp?.services },
+        } : undefined,


The deep merge implementation creates an empty services object even when neither the existing config nor the incoming config has values for that field. This will create udp.services = {} which will then be included in the final YAML output. Consider only creating this nested property when at least one of the sources has actual values.

Copilot · 2026-01-14T13:57:53Z

src/backends/traefik/traefikManager.ts

+function mergeRecord<T>(target: Record<string, T> = {}, source: Record<string, T> = {}, section?: string): Record<string, T> {
+    if (section) {
+        const collisions = Object.keys(source).filter(key => key in target);
+        if (collisions.length > 0) {
+            log.warn({
+                message: 'Config name collision detected - values will be overwritten',
+                data: { section, collisions }
+            });
+        }
+    }


The new collision warning feature lacks test coverage. Consider adding a test that registers two different apps with overlapping router/service/middleware names and verifies that the warning is logged. This helps ensure the feature continues to work as expected and provides documentation of the intended behavior.

…ad code 1. Fix double rendering issue: - renderTemplateParsed now returns { raw, parsed } to avoid calling renderTemplate twice (was rendering once to store, then again to parse) 2. Extract reusable error helper: - Added getErrorMessage(err) to templateParser.ts - Used in traefik.ts, templateParser.ts, and validators.ts - Eliminates duplicate 'err instanceof Error ? err.message : String(err)' 3. Remove dead code: - Removed lastRendered variable (never used) - Removed _getLastRendered() function (never called from tests) - Removed duplicate debug log in makeAppConfig (templateParser already logs)

Define a dedicated Context type and use safe unknown-based lookup in getContextValue to satisfy TypeScript without resorting to 'any'.

- Add with and - Use helpers across parser, validators, and manager - Minor readability improvements

- Remove duplicate ComposeFileReference from docker.d.ts (use provider's version) - Remove TraefikConfigYamlFormat re-export from types/index.ts (keep in backend) - Remove duplicate FieldData type from api/types.ts - Extract shared getClientIP utility from auth.ts and logging.ts - Clean up config.ts comments (fix typos, improve clarity) - Remove unnecessary re-export in templateParser.ts

- Reorganize logger.ts imports (group and move type import to top) - Add JSDoc comments to logging functions - Clean up verbose comments and simplify code - Export getClientIP from middleware index - Simplify consoleFormat data source selection

- Reorganize config.ts imports at top - Extract VALID_BACKENDS as const array - Add JSDoc comments to exported functions - Convert BackendModule and BackendStatus to exported interfaces - Add JSDoc comments to backend plugin functions

- Move detectCollisions import to top of file with other imports - Remove redundant section headers (Flush Debouncing) - Simplify comments on module state variables

- Remove unused xMagicProxySchema alias export - Rewrite backends/readme.md with cleaner formatting and updated references

- Add unit tests for getErrorMessage and detectCollisions helpers - Add regression test for {{ userData.port }} nested dot notation syntax Bug report evaluation: - Comment 1 (critical): FALSE POSITIVE - nested userData access works correctly - Comment 2 (moderate): FIXED - added tests for helpers.ts functions - Comment 3 (nit): No action needed - optimization is correct - Comment 4 (nit): No action needed - TypeScript handles this correctly

Add detailed JSDoc with examples showing how dot notation traversal works for nested access like {{ userData.port }}. Rename variable from 'value' to 'current' to better convey the traversal behavior.

Add comprehensive tests to prevent future regressions where intermediate objects might be incorrectly substituted in templates. These tests verify: - Rejects {{ userData }} (the object itself) - Accepts {{ userData.port }} (nested string value) - Handles numeric and null userData values correctly - Fails on undefined intermediate paths - Type safety for template substitution All 7 new tests pass as part of user-data-substitution.test.ts. This ensures the false-positive bug from Comment 1 can never be accidentally reintroduced by future refactoring.

stonegray · 2026-01-15T18:28:39Z

@copilot resolve conflicts, test/lint/build and review

Copilot · 2026-01-15T18:28:48Z

@stonegray I've opened a new pull request, #33, to work on those changes. Once the pull request is ready, I'll request review from you.

Stone Gray added 7 commits January 14, 2026 08:11

fix(traefik): deep merge routers/services/middlewares in register()

6f30e9a

The shallow merge was overwriting nested config sections entirely instead of merging them. Now properly merges routers, services, and middlewares when updating an existing app's configuration.

chore(traefik): remove unused lastUserData tracking

b708a20

The lastUserData variable and _getLastUserData() function were never used by any tests or production code. Removed to reduce dead code.

feat(traefik): warn on router/service/middleware name collisions

dd62a74

When building the combined config, log a warning if multiple apps define the same router, service, or middleware name. This helps catch misconfigurations where one app's config would silently overwrite another's.

fix(backend): replace unsafe ! assertions with ensureBackend()

061dfb1

if initialization failed, the next line would throw a confusing error. Added ensureBackend() helper that explicitly checks and throws a clear error message if the backend failed to initialize.

perf(traefik): only cleanup temp files on first write

542afcc

Previously cleanupTempFiles() ran on every config flush, causing unnecessary filesystem operations. Now tracks cleanup state and only runs once per output file, resetting when the output file changes.

fix(lint): replace explicit 'any' types with specific Record types

fcdc242

Changed buildContext and getContextValue to use specific Record<string, ...> types instead of 'any' to satisfy ESLint no-explicit-any rules.

Copilot AI review requested due to automatic review settings January 14, 2026 13:52

Copilot started reviewing on behalf of stonegray January 14, 2026 13:52 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

Stone Gray added 12 commits January 14, 2026 09:24

fix(traefik): tighten Context typing and safe lookup in template parser

85fbe58

Define a dedicated Context type and use safe unknown-based lookup in getContextValue to satisfy TypeScript without resorting to 'any'.

refactor(traefik): extract shared helpers and improve readability

eeadb9e

- Add with and - Use helpers across parser, validators, and manager - Minor readability improvements

chore(merge): bring refactor/traefik-readability into dev

a1d817e

refactor: improve config and backend plugin modules

dd997b1

- Reorganize config.ts imports at top - Extract VALID_BACKENDS as const array - Add JSDoc comments to exported functions - Convert BackendModule and BackendStatus to exported interfaces - Add JSDoc comments to backend plugin functions

refactor: clean up traefik manager imports

2963195

- Move detectCollisions import to top of file with other imports - Remove redundant section headers (Flush Debouncing) - Simplify comments on module state variables

refactor: remove dead code and update documentation

ba0a578

- Remove unused xMagicProxySchema alias export - Rewrite backends/readme.md with cleaner formatting and updated references

docs: clarify getContextValue traversal behavior

1e2a5d9

Add detailed JSDoc with examples showing how dot notation traversal works for nested access like {{ userData.port }}. Rename variable from 'value' to 'current' to better convey the traversal behavior.

Copilot AI mentioned this pull request Jan 15, 2026

Merge main branch and resolve conflicts in templateParser and helpers tests #33

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

deep-merge configs, remove redundant YAML parsing, add collision warnings & robustness fixes #30

deep-merge configs, remove redundant YAML parsing, add collision warnings & robustness fixes #30

Uh oh!

stonegray commented Jan 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 14, 2026

Uh oh!

Copilot AI Jan 14, 2026

Uh oh!

Copilot AI Jan 14, 2026

Uh oh!

Copilot AI Jan 14, 2026

Uh oh!

Copilot AI Jan 14, 2026

Uh oh!

stonegray commented Jan 15, 2026

Uh oh!

Copilot AI commented Jan 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

deep-merge configs, remove redundant YAML parsing, add collision warnings & robustness fixes #30

Are you sure you want to change the base?

deep-merge configs, remove redundant YAML parsing, add collision warnings & robustness fixes #30

Uh oh!

Conversation

stonegray commented Jan 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

stonegray commented Jan 15, 2026

Uh oh!

Copilot AI commented Jan 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants