Skip to content

MSD architecture cleanup: documentation and performance instrumentation guard#9

Closed
Copilot wants to merge 2 commits intomsd-globalisationfrom
copilot/document-deep-analysis-findings
Closed

MSD architecture cleanup: documentation and performance instrumentation guard#9
Copilot wants to merge 2 commits intomsd-globalisationfrom
copilot/document-deep-analysis-findings

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Nov 26, 2025

Deep analysis identified 8 optimization opportunities in MSD codebase. Addresses documentation gaps and adds performance guard for debug instrumentation.

HIGH Priority

  • Global card instance references (lcards-msd.js) - Documented the 3 global references, marked legacy ones deprecated, identified canonical reference window.lcards.debug.msd.cardInstance

  • Performance instrumentation guard (SystemsManager.js) - Skip instrumentation in production:

const debugEnabled = mergedConfig?.debug?.performance ||
                     mergedConfig?.debug?.rules ||
                     mergedConfig?.debug?.enabled;
if (!debugEnabled) {
  lcardsLog.debug('[SystemsManager] Performance instrumentation disabled (debug mode off)');
  return;
}
  • Empty render() method - Added JSDoc documenting no-op behavior and deprecation status

MEDIUM Priority

  • StyleResolver two-phase init (PipelineCore.js) - Documented circular dependency pattern: Phase 1 creates without ThemeManager, Phase 2 patches after SystemsManager init

  • Selective re-rendering TODO (SystemsManager.js) - Expanded with implementation plan for per-overlay re-render capability

  • Card registration failure (PipelineCore.js) - Documented non-fatal nature and which features are disabled

LOW Priority

  • dataSourceManager getter (index.js) - Documented fallback chain reasoning
  • ValidationService guard (PipelineCore.js) - Documented idempotent initialization pattern
Original prompt

I'll create a comprehensive issue documenting all the findings from the deep analysis with actionable tasks for an agent to address. Let me try creating the issue with the complete details:Let me create a comprehensive issue with all the findings and recommendations from the deep analysis:I'll create a comprehensive GitHub issue with all the findings from the deep analysis. Let me format this properly:

GitHub Issue: MSD Architecture Cleanup - Optimization Opportunities

🎯 Overview

Deep analysis of MSD codebase confirms successful refactoring (legacy code removed, singleton architecture implemented, SimpleCards integrated). However, identified 10 optimization opportunities for improved maintainability and performance.

Status: ✅ MSD codebase is production-ready and clean
Priority: Address HIGH priority items before next feature development
Estimated Effort: ~4-6 hours total


✅ Architecture Validation Results

CONFIRMED CLEAN:

  • ✅ All legacy custom-button-card dependencies removed
  • ✅ Global singleton architecture properly implemented
  • ✅ SimpleCards fully integrated (text/button/status overlays replaced)
  • ✅ MSD pipeline heavily refactored with excellent tombstone documentation
  • ✅ Only line + control overlays remain (MSD-specific, intentional)

🔴 HIGH PRIORITY Issues (Fix Before Next Feature)

1. Audit Global Card Instance References

File: src/cards/lcards-msd.js lines 1084-1089

Issue: Three different global references to the same card instance:

window.cb_lcars_card_instance = this;        // ← Why cb_ prefix?  Legacy?
window._currentCardInstance = this;          // ← Underscore = private? 
window.lcards.debug. msd.cardInstance = this; // ← Most organized

Problems:

  • Namespace pollution (3 globals for 1 card)
  • Multiple MSD cards: last one wins (overwrites previous)
  • cb_ prefix suggests legacy code remnant
  • Underscore prefix inconsistent with modern patterns

Recommendation:

// Option A: Use Map keyed by card GUID
if (! window.lcards.debug.msd.cardInstances) {
  window.lcards.debug.msd.cardInstances = new Map();
}
window.lcards.debug.msd.cardInstances.set(this._cardGuid, this);

// Option B: If only one card expected, consolidate to single reference
window.lcards.debug.msd.cardInstance = this; // Keep only this one

Questions for Maintainer:

  • Are all three references actively used?
  • What's the intended behavior with multiple MSD cards?
  • Can we deprecate cb_lcars_card_instance and _currentCardInstance?

2. Performance Instrumentation Always Active

File: src/msd/pipeline/SystemsManager.js lines 454-492

Issue: Performance counters run in production even when not needed:

const perfStore = W.__msdDebug.__perfStore = W.__msdDebug.__perfStore || { counters:{}, timings:{} };
function perfCount(k,inc=1){ perfStore.counters[k]=(perfStore.counters[k]||0)+inc; }
// ← Executes on EVERY rule evaluation

Impact:

  • Unnecessary object creation/manipulation overhead
  • Memory usage for counters never read
  • Production perf hit for dev-only feature

Recommendation:

_instrumentRulesEngine(mergedConfig) {
  // Only instrument if debug mode enabled
  if (! mergedConfig?.debug?.performance) {
    lcardsLog.debug('[SystemsManager] Performance instrumentation disabled');
    return;
  }
  
  // Existing instrumentation code here... 
  try {
    const depIndex = new Map();
    // ... rest of implementation
  }
}

3. Document Empty render() Method

File: src/msd/pipeline/SystemsManager.js lines 449-452

Issue: Empty method with vague comment:

render() {
  // Rendering is handled automatically via pipeline
  // This method exists for API compatibility
}

Problems:

  • "API compatibility" - which API? External or internal?
  • Not clear if method is safe to remove
  • Could confuse future maintainers

Recommendation:

/**
 * Render method (no-op)
 * 
 * Rendering is handled automatically by the MSD pipeline via reRender() callback. 
 * This method exists for backward compatibility with legacy code that may call
 * systemsManager.render() directly.  New code should use pipelineApi.reRender() instead.
 * 
 * @deprecated Use pipelineApi.reRender() for manual re-renders
 * @returns {void}
 */
render() {
  // No-op: Rendering handled by pipeline
  lcardsLog.trace('[SystemsManager] render() called (no-op, use reRender() instead)');
}

🟡 MEDIUM PRIORITY Issues (Next Sprint)

4. StyleResolver Two-Phase Initialization

File: src/msd/pipeline/PipelineCore.js lines 45-76 and 96-104

Issue: StyleResolver created before ThemeManager, then manually patched:

// Phase 2. 5: Create WITHOUT ThemeManager
styleResolver = new StyleResolverService(null, { /* config */ });

// Later: Patch in ThemeManager after it's ready
if (styleResolver && systemsManager. theme...

</details>



<!-- START COPILOT CODING AGENT TIPS -->
---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).

…ion improvements

Co-authored-by: snootched <11273374+snootched@users.noreply.github.com>
Copilot AI changed the title [WIP] Create issue for MSD architecture cleanup findings MSD architecture cleanup: documentation and performance instrumentation guard Nov 26, 2025
Copilot AI requested a review from snootched November 26, 2025 22:43
@snootched snootched closed this Nov 28, 2025
@snootched snootched deleted the copilot/document-deep-analysis-findings branch January 19, 2026 13:32
snootched added a commit that referenced this pull request Mar 29, 2026
… template support

- Gauge memoization hash: add classifiedState, resolvedMarkerValues, resolvedRangeBounds
  so state-driven range colors and template-bound range extents correctly bust the cache

- Per-band pill opacity (#12): add getRangeOpacity() alongside getPillColor() in
  _generatePillsSVG(); embed resolved opacity as data-unfilled-opacity on each pill
  rect; _updatePillOpacities() reads per-pill opacity so band-specific opacity values
  are honoured during live updates

- Attribute template support (#9): new _resolveControlAttribute() method handles
  {entity.attributes.xxx}, {entity.state}, and [[[JS]]] templates; used in
  _getEntityValue() so the slider can read from a dynamically-resolved attribute name

- Open GitHub issue #309 for Group 7 #10 drag interaction visual state (deferred)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants