NEW @W-22393677@ - User Not Logged In — Skip ApexGuru, Show Non-Blocking Connect Org Banner

[nikhil-mittal-165]

Summary

When a user is not logged in to a Salesforce org, the ApexGuru engine is skipped gracefully, and a non-blocking "Connect Org" banner is displayed to guide the user to authenticate.

GUS Ticket W-22393677 — User Not Logged In — Skip ApexGuru, Show Non-Blocking "Connect Org" Banner

Changes

• Add InsightsHandler to gracefully handle undefined insights when ApexGuru is skipped due to missing authentication
• Wire InsightsHandler into CodeAnalyzerRunAction.run() flow at line 115
• Update ResultsJson type to support optional insights field for backward compatibility
• Display non-blocking banner when SFAP authentication is not configured

Dependencies

• NEW @W-22393694@ - Gracefully skip ApexGuru when user is not authenticated code-analyzer-core#478

Test Evidence

Unit tests: 362/362 pass (24 suites, 97.19% coverage). Integration baseline: dreamhouse scan 637 violations, no crash, CLI exit 0. Feature-specific: CLI correctly omits insights when SFAP not configured (AG engine returns 0 rules, graceful skip). VS Code extension handles undefined insights via optional field in ResultsJson type - backward compat confirmed by unit tests. InsightsHandler correctly wired into CodeAnalyzerRunAction.run() flow at line 115. No fixes needed.

Test Status

PASS

Fix attempts: 0

Integration: PASS - baseline scan succeeds (637 violations), CLI does not crash with AG absent, extension compiles and all unit tests pass with mocked insights scenarios covering all error codes

Known external failures (not blocking): ApexGuru engine disabled in test env (SFAP API base URL not configured) - cannot produce live insights output

[git2gus]

Git2Gus App is installed but the .git2gus/config.json doesn't have right values. You should add the required configuration.

[namrata111f]

Comprehensive PR Review - PR #360

✅ Overall Assessment: APPROVE with minor recommendations

This PR implements a graceful handling mechanism for ApexGuru engine skips when users are not authenticated. The implementation is well-structured, thoroughly tested, and follows VSCode extension best practices.

---

Security Review ✅

Positive Security Aspects:

1. No credential exposure - Error messages sanitized appropriately
2. No command injection risks - All user interactions via VSCode API
3. Secure external URL handling - GitHub issues URL hardcoded, not user-controlled
4. Session-based suppression - Error code suppression stored in memory only (Set), no persistence
5. Optional insights field - Backward compatible with older CLI versions (insights?: Record<string, EngineInsight>)

Security Recommendations:

• ✅ ISSUES_URL is hardcoded - good practice
• ✅ No eval/exec usage detected
• ✅ No workspace file writes without validation

---

Code Quality Review ✅

Architecture & Design:

1. Well-separated concerns: InsightsHandler is a focused, single-responsibility class
2. Dependency injection: Clean constructor injection pattern
3. Type safety: Strong TypeScript typing with EngineInsight, ScanResults types
4. Backward compatibility: Optional insights field supports older CLI versions

Implementation Highlights:

1. Error code discrimination: Switch statement handles NO_ORG_CONNECTION, API_UNAVAILABLE, UNEXPECTED_ERROR
2. Session suppression: Prevents banner spam via suppressedErrorCodes Set
3. Retry mechanism: Callback pattern for retriggerScan maintains context
4. Graceful degradation: Falls back to manual instructions if Core Extension unavailable

Code Quality Concerns:

MINOR: Line 280 has unused parameter prefix (_remediation) in handleUnexpectedError

• Consider logging remediation or remove if truly unused

MINOR: Display.displayInfo signature changed to accept buttons

• ✅ All call sites updated correctly in tests
• ✅ Backward compatible (buttons optional with ...buttons)

---

UI/UX Review ✅

VSCode UI Patterns:

1. ✅ Non-blocking info banners - Correct choice for non-critical notifications
2. ✅ Action buttons - "Connect Org", "Retry Scan", "View Details", "Report Issue"
3. ✅ Output channel integration - windowManager.showLogOutputWindow() for details
4. ✅ External link handling - windowManager.showExternalUrl() for issue reporting

User Experience:

1. Progressive disclosure: Details hidden in log output, accessible via button
2. Contextual actions:
   • NO_ORG_CONNECTION → Connect Org button triggers sfdx.authorize.org
   • API_UNAVAILABLE → Retry Scan + Details buttons
   • UNEXPECTED_ERROR → View Details + Report Issue buttons
3. Fallback messaging: If Core Extension unavailable, shows manual CLI command

UX Recommendations:

• ✅ Messages are user-friendly and actionable
• ✅ Session suppression prevents notification fatigue
• SUGGESTION: Consider adding telemetry for insight error rates to detect systemic issues

---

VSCode API Compatibility Review ✅

API Usage:

1. ✅ vscode.window.showInformationMessage - Standard API, stable
2. ✅ vscode.commands.executeCommand('sfdx.authorize.org') - Depends on Salesforce Core Extension
3. ✅ No deprecated APIs detected
4. ✅ Engine version: package.json specifies "vscode": "^1.90.0" (appropriate)

Dependency Check:

• ✅ No new external dependencies added
• ✅ Uses existing WindowManager abstraction for testability

---

Test Coverage Review ✅ EXCELLENT

Unit Test Coverage (test/lib/insights-handler.test.ts):

• ✅ 12 test cases covering all error codes and edge cases
• ✅ Session suppression verified
• ✅ Button callbacks tested
• ✅ Async fallback handling tested (Connect Org when Core Extension unavailable)
• ✅ Mock isolation - SpyDisplay, SpyLogger, StubExternalServiceProvider

Integration Test Coverage (test/lib/insights-handler.integration.test.ts):

• ✅ 7 test cases covering full lifecycle
• ✅ Backward compatibility - undefined insights field
• ✅ Retry scan flow - callback triggers re-run
• ✅ Banner suppression - cross-scan persistence
• ✅ Different error codes - independent suppression

Test Coverage Stats:

• 362/362 tests pass ✅
• 97.19% coverage ✅
• Integration baseline: dreamhouse scan 637 violations ✅

Test Quality Assessment:

EXCELLENT - Comprehensive unit + integration tests, edge cases covered

---

Documentation Review ✅

Code Documentation:

• ✅ JSDoc comments on public methods
• ✅ Type definitions (EngineInsight, ScanResults) self-documenting
• ✅ messages.ts contains user-facing strings (good i18n preparation)

PR Documentation:

• ✅ Clear summary in PR body
• ✅ Test evidence provided
• ✅ Dependency link to code-analyzer-core PR #478
• ✅ GUS ticket W-22393677 referenced

Documentation Recommendations:

• ✅ README.md not modified (appropriate - internal feature)
• SUGGESTION: Consider adding CHANGELOG entry for 1.21.0-SNAPSHOT

---

Additional Observations

Performance:

• ✅ Minimal overhead - handleInsights() early returns if no insights/apexguru
• ✅ No blocking operations on main thread
• ✅ Async operations (isOrgConnectionServiceAvailable, executeCommand) properly handled

Maintainability:

• ✅ Clear separation of concerns
• ✅ Extensible switch statement for future error codes
• ✅ Testable via dependency injection

Backward Compatibility:

• ✅ CRITICAL: insights field is optional (insights?: Record<string, EngineInsight>)
• ✅ Older CLI versions without insights field work correctly
• ✅ Extension compiles with both old and new CLI output formats

---

Final Recommendations

Required Changes: NONE ✅

Optional Improvements:

1. MINOR: Remove or use _remediation parameter in handleUnexpectedError (line 280)
2. SUGGESTION: Add telemetry for insight error tracking
3. SUGGESTION: Consider CHANGELOG entry for version 1.21.0

Merge Readiness: ✅ READY TO MERGE

This PR is well-implemented, thoroughly tested, and follows all VSCode extension best practices. The graceful degradation and backward compatibility ensure a smooth rollout.

---

Review Confidence: HIGH
Recommendation: APPROVE

[aruntyagiTutu]

APPROVED — Meets Code Analyzer guidelines. Gracefully handles undefined ApexGuru insights when user not authenticated.

Review Against Guidelines ✅

3. Validation & Error Handling ✅

• InsightsHandler validates insights at boundary
• Distinct handling per error code (NO_ORG_CONNECTION, API_UNAVAILABLE, UNEXPECTED_ERROR)
• Non-blocking banner guides user to authenticate

4. Logging ✅

• Appropriate log levels used

5. Architecture ✅

• InsightsHandler properly separated
• Wired into CodeAnalyzerRunAction.run() at line 115
• ResultsJson type supports optional insights for backward compat

6. Testing ✅

• 362/362 tests pass (97.19% coverage)
• Integration baseline: dreamhouse scan 637 violations, no crash
• Feature-specific: CLI omits insights when SFAP not configured
• Mocked insights scenarios cover all error codes

8. PR Hygiene ✅

• Title follows convention: NEW @W-22393677@
• Depends on forcedotcom/code-analyzer-core#478 (already approved)
• Test evidence included

No blocking issues.