[LOW] validation.js analyseImport: seenNames dual-purpose accumulator and 'valid' includes duplicates — return contract non-obvious from code

[JuliaKalder]

Summary

analyseImport in modules/validation.js (lines 53–75) has two non-obvious behaviors that require careful re-reading to understand: the seenNames Map serves a dual purpose that its name does not convey, and the valid return value includes duplicates — contrary to what the name implies.

Impact

Maintenance burden. A developer modifying the import pipeline (e.g. adding a new merge mode, or adding logging for duplicates) will likely misread the return contract and introduce bugs:

• They may assume valid means "templates safe to import without conflict" and omit the duplicate re-check that executeImport currently performs.
• They may assume seenNames only tracks existing templates and inadvertently break the intra-import dedup logic by resetting or skipping the per-iteration seenNames.set() call.

Evidence

modules/validation.js lines 53–75:

export function analyseImport(importedTemplates, existingTemplates) {
  const seenNames = new Map(
    existingTemplates.map((t) => [t.name.toLowerCase(), t])
  );
  // ^ seenNames is initialized with EXISTING templates for storage-level dedup...

  const valid = [];
  let invalid = 0;
  const duplicates = new Map();

  for (const t of importedTemplates) {
    if (!t || typeof t.name !== "string" || !t.name.trim()) {
      invalid++;
      continue;
    }
    const key = t.name.trim().toLowerCase();
    if (seenNames.has(key)) {
      duplicates.set(key, t);
    }
    seenNames.set(key, t);  // ^ ...but also grows with each imported template
                             //   for intra-import dedup. "seenNames" reads as
                             //   "existing names" not "all names encountered so far".
    valid.push(t);           // ^ t is pushed even if it just landed in duplicates.
                             //   "valid" reads as "safe to insert" but means
                             //   "structurally well-formed".
  }

  return { valid, invalid, duplicates };
}

Effect on caller: executeImport in options.js (lines 682–713) does not rely on analysis.duplicates for import decisions — it rebuilds existingByName from storage and re-checks each template. The duplicates map is used only for the count shown in the dialog (analysis.duplicates.size). This works, but the mismatch between what analyseImport returns and what callers actually use is non-obvious.

Recommended Fix

Add two inline comments to make the dual behaviors explicit without changing logic:

export function analyseImport(importedTemplates, existingTemplates) {
  // Tracks all names seen so far: pre-seeded with existing template names
  // for storage-level dedup, then extended with each processed import for
  // intra-import dedup (so two imported templates with the same name both
  // land in `duplicates`).
  const allSeenNames = new Map(
    existingTemplates.map((t) => [t.name.toLowerCase(), t])
  );

  const valid = [];   // Structurally valid templates — includes duplicates.
  let invalid = 0;
  const duplicates = new Map();

  for (const t of importedTemplates) {
    if (!t || typeof t.name !== "string" || !t.name.trim()) {
      invalid++;
      continue;
    }
    const key = t.name.trim().toLowerCase();
    if (allSeenNames.has(key)) {
      duplicates.set(key, t);
    }
    allSeenNames.set(key, t);
    valid.push(t); // Always included; callers must consult `duplicates` for merge logic.
  }

  return { valid, invalid, duplicates };
}

The rename of seenNames → allSeenNames makes the accumulating nature explicit. The two short inline comments eliminate the re-reading required to verify the dedup semantics. No behavior changes.

References

• Single Responsibility Principle for local variables
• Related: issue [MEDIUM] executeImport merge strategy (append/skip/replace logic) embedded in options.js UI layer instead of template-store.js #94 (executeImport merge strategy in UI layer)
• Related: issue [LOW] executeImport: destructure-to-omit pattern produces unused variables that look like dead code #91 (destructure-to-omit pattern in executeImport producing unused variables)