Lessons Learned

Version: 0.2.32 Updated: 2026-04-15

63. Dead Code Can Hide Security Features

Lesson: passwordForStdin in svn.ts was declared, checked, and consumed — but never assigned. The --password-from-stdin path for SVN ≥1.10 was completely inert since the auth cache refactor. The if/else chain fell through silently.

Fix: Add the missing else branch. Also add tests that verify stdin is actually written to.

Rule: When refactoring branching logic, test every branch outcome explicitly — not just the new path, but that existing paths still fire.

62. Network Flags Should Be Opt-In, Not Always-On

Lesson: fetchLockStatus = true was hardcoded in run(), adding --show-updates (network call) to every status refresh — including file-watcher-triggered ones. This caused 20+ server round-trips during normal usage like opening files.

Fix: Extract shouldFetchLockStatus(operation) — returns true only for StatusRemote, Lock, Unlock. Regular status stays local-only.

Rule: Flags that hit the network must be opt-in per operation, never hardcoded to always-on.

61. Reuse Background Polling Results for User-Triggered Checks

Lesson: Pre-commit update called svn log -r BASE:HEAD even though background polling already checked seconds ago. On slow connections, this added 5-30s of pure wait.

Fix: Cache hasRemoteChanges result with timestamp. PreCommitUpdateService reuses it if within poll interval. Both hasRemoteChanges() and updateModelState(checkRemote=true) populate the cache.

Rule: When background polling already computes a value, cache it with TTL and let on-demand paths reuse it.

60. Deduplicate Cascading Refreshes in Compound Operations

Lesson: commitFiles() called updateRevision() post-commit, which fired repolog.fetch + itemlog.refresh. Then commitFiles() fired the same two commands again. 4 redundant history fetches per commit.

Fix: Add skipHistoryRefresh option to updateRevision(). Caller handles refresh once.

Rule: When operation A calls operation B, let A own the side effects — give B an opt-out flag.

58. Always Invalidate Cache After Cleanup

Lesson: cleanup() didn't call resetInfoCache(), only removeUnversioned()/removeIgnored() did. After lock repair (most common cleanup), UI showed stale "locked" status.

Fix: Call resetInfoCache() unconditionally after all cleanup variants.

Rule: Cache invalidation should match operation scope, not just the "destructive" subset.

59. Unify Error Handling Across Command Paths

Lesson: executeOnResources only offered "Run Cleanup" button. executeOnUrisOrResources (Explorer) offered no action buttons at all. handleRepositoryOperation had the full priority chain. Three paths, three behaviors.

Fix: Extract handleOperationError() with full priority chain (auth > lock > cleanup > update > conflict > output). All paths delegate to it.

Rule: Error handling is a crosscutting concern — extract once, reuse everywhere.

56. Legacy E2E Preflight: Use Suite-Ready Guard, Not suiteSetup Skip

Lesson: In mixed Mocha/Vitest harnesses, this.skip() inside suiteSetup can still leave tests running with uninitialized fixtures on some runners.

Fix:

Add suiteReady = false by default.
Wrap each test with testIfReady(...) and run this.skip() inside the test context when not ready.
Set suiteReady = true only after full preflight/setup success.

Rule: For binary/extension preflight in legacy E2E suites, guard each test via readiness flag; do not rely on suiteSetup skip alone.

55. Polling Tests: Await Explicit Signal, Not Fixed Sleep

Lesson: Fixed short sleeps (setTimeout(120)) in timer tests can flake under CI load and miss expected callback counts.

Fix:

Resolve a promise on the target callback invocation.
Await that promise with a bounded timeout.

Rule: For timer/poll assertions, wait on explicit condition/event + timeout; avoid fixed-sleep count checks.

54. Background Polls: Teardown Must Wait Before Repo Deletion

Lesson: Tests can pass yet CI still fail when Vitest sees unhandled rejections from in-flight background SVN polls during teardown.

Fix:

Dispose repositories first.
Wait briefly for in-flight poll/status promises to settle.
Remove temp repos after the settle delay.

Rule: In suites with polling/watchers, cleanup order is dispose → settle → delete temp resources.

53. Cross-Platform Unit Tests: Avoid Mocked-Home Prefix Assumptions

Lesson: os.homedir monkey-patching may not reliably drive imported module behavior across all runners; prefix assertions can become CI-only flakes.

Fix:

For cache-dir assertions, validate invariant suffix (.subversion/auth/svn.simple) + absolute path.
Avoid strict startsWith(testCacheDir) checks for default-home constructors.

Rule: In platform tests, assert stable invariants; avoid runner-dependent home-path assumptions.

52. Cross-Platform Paths/Auth: Match Real SVN + Windows Rules

Lesson: CI portability breaks when tests/runtime assume extension-specific auth paths or platform-local absolute-path semantics.

Fix:

Use canonical SVN auth cache dir: auth/svn.simple (not sven.simple).
Reject Windows-absolute path forms on all platforms (C:\..., \\server\share, \Windows\...).
In integration suites, skip early when required binaries/commands are unavailable.

Rule: Path/auth logic must be platform-invariant unless behavior is explicitly OS-specific.

51. Vitest+Sinon Stubs: Keep CommonJS Boundary for Stub Targets

Lesson: For modules stubbing built-in APIs (child_process.spawn, fs.watch) with sinon, ESM import shape can break stubs.

Fix:

Keep local CommonJS require binding for stubbed module targets in runtime/tests.
Use local lint suppression only on those boundary lines.

Rule: Prefer ESM generally, but keep require-boundary where sinon stub target identity must stay mutable.

50. Coverage Fail-Cluster: Hit Biggest Bucket First

Lesson: Coverage climbs faster when pass starts with highest uncovered-line file, not easiest file.

Applied:

Pass 1: src/blame/blameProvider.ts (largest uncovered in blame/security/auth cluster)
Pass 2: src/security/errorSanitizer.ts

Result:

blameProvider line coverage moved to high-90s.
errorSanitizer line coverage moved to high-90s.

Rule: For aggressive coverage goals, rank uncovered lines first, then test in descending bucket order.

32. Vitest Migration: Avoid ESM Export Reassignment

Lesson: Legacy Mocha tests that assign imported module exports break under ESM.

Issue:

Patterns like (module as any).fn = ... fail with readonly namespace exports
Typical error: Cannot set property ... which has only a getter

Fix:

Replace export reassignment with vi.spyOn(module, "fn").mockImplementation(...)
For heavy per-test override patterns, route through one mutable local implementation function

Rule: In Vitest+ESM suites, mock with spies, never write to imported module exports.

33. VS Code Mock Fidelity: Bindings + Defaults Matter

Lesson: Legacy suite stability depends on mock parity with VS Code APIs.

Issue:

Command callbacks registered with thisArg broke when mock ignored binding.
Config reads without explicit default returned undefined in mock, unlike extension defaults.
Missing workspace.textDocuments tracking broke runtime code paths that inspect open docs.

Fix:

Implement registerCommand/registerTextEditorCommand with thisArg application.
Add persisted mock config store with key defaults for common sven.* settings.
Track opened documents in mock workspace (openTextDocument + textDocuments).

Rule: When modernizing harness, prioritize runtime API parity over minimal stubs.

34. XML Adapter Contract: Root Can Be Stripped

Lesson: Parsers using shared XML adapter must accept root-stripped and rooted shapes.

Issue:

parseXml() uses explicitRoot: false by default.
Legacy parser transforms expected only rooted forms (paths.path, list.entry).
Result: diff/list parser regressions on valid SVN XML.

Fix:

In parser transforms, accept both forms (result.paths?.path ?? result.path, result.lists?.list ?? result.list ?? { entry: result.entry }).

Rule: When adapter defaults change, parser tests must include both rooted and root-stripped fixtures.

Core Patterns

0. Path Guards Before SVN Operations

Lesson: Always check if file is inside repository before running SVN commands.

Bug (v0.1.2):

Opening external files (outside workspace) caused repo to disappear
BlameProvider called repository.getInfo() on external file
SVN returned "NotASvnRepository" error
Error handler set RepositoryState.Disposed → repo gone

Fix: Add isDescendant(workspaceRoot, file.fsPath) guard before any SVN operation.

Rule: Guard all SVN operations with path membership check first.

1. Build System: tsc over webpack

Lesson: Direct TypeScript compilation provides better type safety and debugging than bundling.

Benefits:

Full strict mode enforcement (found 21 hidden bugs)
Individual modules easier to debug
20% faster builds

Trade-offs:

Larger package size (+250KB acceptable)
Runtime dependencies must be in dependencies not devDependencies
.vscodeignore must include all compiled modules

Rule: Use tsc for extensions unless bundling truly needed.

2. Service Extraction Pattern

Lesson: Extract stateless services from god classes incrementally.

Approach:

Write 3 TDD tests first (core scenarios)
Create stateless service with parameter objects
Extract verbatim (preserve behavior)
Move decorators to caller
Refactor incrementally after tests pass

Results (Phase 2):

760 lines extracted (StatusService, ResourceGroupManager, RemoteChangeService)
Repository.ts: 1,179 → 923 lines (22% reduction)
Zero breaking changes

Rule: Multiple small extractions beat one big refactor.

3. Dependency Migration: Adapter Pattern

Lesson: Abstract old API behind compatibility layer when migrating dependencies.

Pattern (xml2js → fast-xml-parser):

Create adapter abstracting parser API
Add compatibility tests before migration
Migrate simplest parser first
Validate pattern, then migrate complex parsers

Results:

79% bundle reduction (45KB → 9.55KB)
Zero functionality changes
Can swap parser again in future

Critical Fix (v2.17.137):

Bug: Revision expansion failed - textNodeName was #text not _
Root cause: fast-xml-parser uses different defaults than xml2js
xml2js: text nodes → _ (when mergeAttrs: true)
fast-xml-parser: text nodes → configurable via textNodeName
Fix: Set textNodeName: "_" to match xml2js behavior
Test gap: No test verified _ property, only attributes

Rule: De-risk migrations with adapters + incremental rollout. Rule: Test text content extraction, not just attributes.

4. Type Safety: No Bypasses

Lesson: transpileOnly and any types hide bugs. Fix types, don't disable checker.

Found:

21 TypeScript errors masked by webpack transpileOnly
248 any types across 25 files compromise safety

Impact:

10× unknown error types
4× null/undefined checks missing
Array/readonly violations

Rule: Enable strict mode from start. No any without explicit justification.

5. Performance: Measure User Impact

Lesson: Prioritize optimizations by end-user impact, not code elegance.

High-impact fixes (Phases 8-16):

Debounce/throttle: 60-80% burst reduction
Config cache: 100% users, -10ms per command
Conditional index rebuild: 50-80% users, 5-15ms saved
Decorator removal: 1-2ms → <0.5ms

Low-ROI efforts:

God class refactoring: 6-8h effort, minimal user benefit
Over-optimization: Diminishing returns after P0/P1 fixes

Rule: Profile real usage. Fix P0 bottlenecks before refactoring.

6. Error Handling: Descriptive Messages

Lesson: Silent error handling is debugging nightmare.

Anti-pattern:

if (err) {
  reject();
} // ❌ No error message

Best practice:

catch (err) {
  console.error("parseInfoXml error:", err);
  reject(new Error(`Failed to parse: ${err.message}`));
}

Impact: Previous migration failures debuggable only after adding logging.

Rule: Always include context in error messages.

7. Testing: TDD + 3 End-to-End Tests

Lesson: Write tests before implementation. 3 core scenarios sufficient per feature.

Coverage progress:

138 → 844 → 930+ tests (+792, +574%)
21-23% → 50-55% → 60-65% coverage ✅ EXCEEDED TARGET
Phase 22 (v2.17.235): +41 e2e tests covering critical gaps

Pattern:

3 end-to-end tests (happy path + 2 edge cases)
Unit tests for critical logic only
Don't overtest implementation details
Real SVN/file system for e2e (no mocks)

Coverage additions (v2.17.235):

Core layer: svn.ts (3), svnFinder (3), resource.ts (3)
Services: StatusService (3), ResourceGroupManager (3), RemoteChangeService (3)
Commands: add (3), remove (3), commitAll (3), upgrade (3), pullIncomingChange (3)
File system: mkdir (2), write_file (2), read_file (2), stat (2)

Rule: Test behavior, not implementation. Stop at 50-60% coverage.

8. Incremental Commits

Lesson: Small focused commits enable easy review, revert, cherry-pick.

Examples:

Phase 1: 13 commits (10 lines avg)
Phase 2: 1 commit per service extraction
XML migration: 9 commits (one per parser)

Benefits:

Clear version history
Bisectable (find regressions fast)
Low review overhead

Rule: Version bump per commit. One concern per commit.

9. Critical Path Mapping

Lesson: Map failure cascades before refactoring critical code.

Found: Extension activation depends on parseInfoXml() in 2 places:

source_control_manager.ts:295 - workspace scan
svnRepository.ts:86 - repository init

Risk: Parse failure = silent repository failure (no error shown to user).

Mitigation: Test critical paths extensively. Add diagnostic logging.

Rule: Know where failures cascade before touching critical code.

10. Code Review: Type Safety Checks

Lesson: Code review catches gaps missed during extraction.

Blockers found (Phase 2):

Unsafe cast: groups as any as ResourceGroup[]
Encapsulation leak: Exposed internal _groups Map
Incomplete interfaces: Missing config types

Rule: Review for type safety, encapsulation, performance after extraction.

11. Batch Operations: Trade Bandwidth for Latency

Lesson: Fetching extra data is faster than multiple network calls.

Example (v2.17.210 - Batch SVN log):

Before: 50 sequential svn log -r REV:REV commands = 5-10s
After: 1 command svn log -r MIN:MAX = 0.1-0.2s
Trade-off: 2x bandwidth for 50x speed

Pattern:

Find min/max range of requested items
Fetch entire range in single call
Filter results to requested items
Cache all fetched data for future use

When to use:

✅ High latency operations (network calls)
✅ Cheap filtering on client side
✅ Sparse data across small range
❌ Huge ranges (>1000x requested items)

Rule: For N network calls, consider single batch + filter if latency >> bandwidth.

17. Cache Eviction: LRU Policy

Lesson: Unbounded caches cause memory leaks; use LRU eviction to cap growth.

Example (v2.17.215 - Blame cache):

Before: Unlimited blameCache + messageCache = 700KB+ per 100 files
After: MAX_CACHE_SIZE=20, MAX_MESSAGE_CACHE_SIZE=500 = bounded memory
Implementation: cacheAccessOrder Map tracks access time, evictOldestCache()

Pattern:

Add Map<key, timestamp> to track access order
Update timestamp on cache hit/miss
Evict oldest entry when size exceeds limit
For immutable caches, simple FIFO eviction works (batch 25%)

When to use:

✅ Long-running sessions (editors, servers)
✅ Large cache entries (>1KB per item)
✅ Unpredictable access patterns
❌ Short-lived processes (startup scripts)

Rule: Cache without eviction = eventual memory leak. Set explicit limits.

12. VS Code Dynamic Icons: No Static Icon in Command

Lesson: For context-based dynamic icons, remove icon from command definition.

Issue: Static icon in command definition overrides dynamic menu icons.

Example (v2.17.222 - Blame toggle icon):

Before: "command": "svn.blame.toggleBlame", "icon": "$(eye)" (static)
After: Removed icon from command, only in menu contributions with when clauses
Result: Icon changes based on context (eye → eye-closed → circle-slash)

Pattern:

Command definition: NO icon property
Menu contributions: Multiple entries with same command, different when clauses
Each menu entry has different icon based on context variables

Anti-pattern:

"commands": [
  {"command": "foo", "icon": "$(eye)"}  // ❌ Overrides dynamic icons
]

Best practice:

"menus": {
  "editor/title": [
    {"command": "foo", "when": "ctx == true", "icon": "$(eye)"},
    {"command": "foo", "when": "ctx == false", "icon": "$(eye-closed)"}
  ]
}

Rule: Dynamic icons require context-based menu entries, no command icon.

13. Remote Development: extensionKind Required

Lesson: SCM extensions must declare extensionKind: ["workspace"] for remote development.

Issue (v2.17.237):

Extension activated on both local and remote in SSH sessions
Caused "command already exists" error, breaking all functionality
Silent failure: no error shown to user

Fix:

"extensionKind": ["workspace"]

Why:

SCM extensions need access to file system and CLI on remote
workspace = run only on remote, not local
Prevents duplicate command registration

Rule: Always add extensionKind: ["workspace"] for extensions needing remote file/CLI access.

14. Error Detection: Check Specific Patterns First

Lesson: When detecting error types, check specific patterns before generic ones.

Issue (v2.17.238):

SVN returns E170013 (network) with E215004 (no credentials) on auth failure
getSvnErrorCode() loop found E170013 first, returned "network error"
Auth retry logic never triggered because error wasn't detected as auth error
Users saw "Network error" when issue was actually missing credentials

Fix:

// BEFORE: Generic loop finds E170013 first
for (const code of errorCodes) {
  if (stderr.includes(code)) return code; // E170013 wins
}
if (stderr.includes("No more credentials")) return "E170001"; // Never reached!

// AFTER: Check specific patterns FIRST
if (stderr.includes("E215004") || stderr.includes("No more credentials")) {
  return "E170001"; // Auth error takes priority
}
for (const code of errorCodes) { ... } // Generic codes

Why:

SVN errors can have multiple codes in same stderr output
Generic patterns (E170013) may mask specific ones (E215004)
User action depends on correct error classification

Rule: Order error checks by specificity - specific patterns before generic ones.

15. Remote SSH: Respect Native Credential Stores

Lesson: Don't disable system credential managers (gpg-agent, gnome-keyring) in remote environments.

Issue (v2.18.0):

Extension always passed --config-option config:auth:password-stores= to SVN
This disabled gpg-agent/gnome-keyring, breaking remote SSH workflows
Password prompt cycled endlessly because:
1. Extension credential cache needs realmUrl from svn info
2. But svn info itself needs auth → chicken-egg problem
3. Without cached credentials, SVN rejected --password on command line
4. Auth failed, prompted again → infinite loop

Fix (v2.26.0 - simplified):

// Use credentialMode setting with auto-detection
const mode = configuration.get("auth.credentialMode", "auto");
const isRemote = !!env.remoteName;
const useSystemKeyring =
  mode === "systemKeyring" || (mode === "auto" && !isRemote);

if (!useSystemKeyring) {
  args.push("--config-option", "config:auth:password-stores=");
  args.push("--config-option", "servers:global:store-auth-creds=no");
}

Why auto mode works:

Detects local vs remote (SSH/WSL/container) automatically
Local: Uses system keyring (gpg-agent, Keychain, Credential Manager)
Remote: Uses VS Code SecretStorage (no keyring setup needed)

Security (v2.26.0):

SVN 1.10+ uses --password-from-stdin (hides from process list)
SVN < 1.10: Password NOT passed - must use system keyring
Removed insecure --password command-line fallback

Rule: Default to auto mode - it handles most scenarios correctly.

Quick Reference

Starting extension: tsc + strict mode + Positron template Migrating deps: Adapter pattern + TDD + incremental Extracting services: Stateless + parameter objects + decorators at caller Optimizing: Profile user impact, fix P0 first Testing: 3 E2E tests per feature, 50-60% coverage target Committing: Small focused commits, version bump each

Anti-Patterns to Avoid

❌ transpileOnly (bypasses type checking) ❌ Silent error handling (no context) ❌ Big bang refactors (high risk) ❌ Premature optimization (fix bottlenecks first) ❌ Over-testing (diminishing returns >60%) ❌ God commits (hard to review/revert)

16. Testing: Vitest for Unit Tests

Lesson: Use Vitest for unit tests, keep Mocha for VS Code E2E tests.

Migration (v2.23.0):

Unit tests: Vitest (104 tests, runs in 1.5s)
E2E tests: Mocha + @vscode/test-electron (needs VS Code API)
Dual framework: Both run in parallel in CI

Benefits:

6-10x faster unit tests (no TypeScript compilation needed)
Better error messages with object diffs
Watch mode for development
Built-in coverage (replaces c8)

Pattern:

// Vitest (unit tests)
import { describe, it, expect, vi } from "vitest";
describe("Parser", () => {
  it("parses XML", () => {
    expect(result).toHaveLength(1);
  });
});

// Mocha (E2E tests needing VS Code)
suite("Extension", () => {
  test("activates", async () => {
    const ext = vscode.extensions.getExtension("...");
  });
});

Key insight: Mock vscode module via vitest.config.ts alias for unit tests.

Rule: Use Vitest for pure logic, Mocha only when VS Code API required.

18. SVN Locking for Large Files

Lesson: SVN's lock-modify-unlock workflow mitigates "poor SVN usage" in data science repos.

Problem domains:

Monolithic checkouts (sparse checkout needed)
Binary bloat (large CSVs, models, datasets)
Locking ambiguity (unclear who has lock)
Disconnected history (vague commit messages)

Implementation (v2.24.0):

Commands: lock, unlock, breakLock
Lock info in tooltips (🔒 Locked by )
Explorer context menu integration
Directory locking support

Pattern:

// Lock with optional comment
await repository.lock(files, { comment: "Editing dataset" });

// Break lock owned by another user
await repository.unlock(files, { force: true });

// Check lock status
const lockInfo = await repository.getLockInfo(filePath);
if (lockInfo) console.log(`Locked by ${lockInfo.owner}`);

Rule: Lock files before editing large binaries. Use lock comments for coordination.

19. CLI vs GUI Feature Parity

Lesson: Not all GUI options map to CLI flags. Research before implementing.

Example (v2.30.0 - Cleanup):

TortoiseSVN has "Break locks" checkbox (calls C API directly)
SVN CLI always breaks locks (hardcoded in svn_client_cleanup2())
No --break-locks flag exists - it's always on

Pattern:

Check SVN CLI --help for actual flags
Check if feature is hardcoded in CLI source
For GUI-only features, document limitation
Don't create "phantom" options that do nothing

Reference: TortoiseSVN Cleanup

Rule: When porting from GUI to extension, verify CLI supports the feature.

20. Cleanup Error Detection: Multiple Codes

Lesson: SVN cleanup is needed for multiple error types, not just E155004.

Error codes requiring cleanup (v2.31.0):

E155004: Working copy is locked
E155037: Previous operation interrupted
E200030: SQLite database busy/locked/corrupt
E155032: Working copy database problem

Text patterns:

"locked", "previous operation", "run 'cleanup'", "sqlite"

E155015 (conflict) does NOT need cleanup:

Conflicts require svn resolve, not cleanup
"Obstructed" status often needs delete + update, not cleanup

UX Pattern:

if (needsCleanup(error)) {
  const choice = await window.showErrorMessage(msg, "Run Cleanup");
  if (choice === "Run Cleanup") {
    await commands.executeCommand("svn.cleanup");
  }
}

Rule: Offer actionable buttons for recoverable errors.

21. User-Friendly Errors: Show Code for Transparency

Lesson: Include error codes in user messages for transparency and googlability.

Pattern (v2.32.0):

"Working copy locked (E155004). Run cleanup to fix."

Benefits:

User can Google the error code for more details
Technical users get context they need
Non-technical users get actionable guidance

Implementation:

// Extract code, format message
const code = extractErrorCode(stderr); // E155004
return `Working copy locked (${code}). Run cleanup to fix.`;

Action Buttons: Match error type to action:

Cleanup errors → "Run Cleanup" button
Out-of-date → "Update" button
Conflicts → "Resolve Conflicts" button

Rule: Always include error code in parentheses. Offer one actionable button per error type.

22. Timer Disposal: Store References

Lesson: Always store timer references and clear them in dispose().

Issue (v2.32.12):

setInterval() and setTimeout() not cleared on extension reload
Timers accumulate over reloads, causing memory leaks
Especially problematic during development with frequent reloads

Pattern:

// BAD: Timer created but never cleared
setInterval(() => this.cleanup(), FIVE_MINUTES);

// GOOD: Store reference, clear in dispose
private cleanupInterval?: ReturnType<typeof setInterval>;

constructor() {
  this.cleanupInterval = setInterval(() => this.cleanup(), FIVE_MINUTES);
}

dispose(): void {
  if (this.cleanupInterval) {
    clearInterval(this.cleanupInterval);
  }
}

Rule: Every setInterval/setTimeout needs corresponding clearInterval/clearTimeout in dispose().

Document Version: 0.2.8 Last Updated: 2025-02-03

23. VS Code TreeView Providers: Listen for Repository Open/Close Events

Lesson: TreeDataProviders created during extension activation may miss initial data if async initialization isn't complete.

Issue (v2.32.17):

Repository Log and Selective Download treeviews displayed empty at startup
Required manual refresh to show data
Root cause: SourceControlManager scans workspace folders asynchronously (fire-and-forget)
Providers created AFTER SourceControlManager constructor but BEFORE repositories discovered
sourceControlManager.repositories was empty when providers called getChildren()

Fix:

// In constructor, subscribe to lifecycle events
this._disposables.push(
  this.sourceControlManager.onDidOpenRepository(() => {
    this.refresh();
  }),
  this.sourceControlManager.onDidCloseRepository(() => {
    this.refresh();
  })
);

Why this pattern is needed:

VS Code extensions often use async initialization for performance (non-blocking activation)
TreeDataProviders may be created before data is available
onDidOpenRepository fires when async discovery completes
Without listening to this event, providers never know data is ready

When to apply:

✅ TreeDataProvider depends on async-discovered resources (repos, files, configs)
✅ Provider created during activate() before async init completes
✅ Data source fires lifecycle events (open, close, change)
❌ Static data available immediately at construction time

Rule: TreeDataProviders must subscribe to data source lifecycle events, not just change events.

24. SVN Peg Revision: Pass Separately, Don't Embed in Path

Lesson: Never manually embed SVN peg revisions into paths that will be processed by escaping functions.

Issue (v2.32.19):

Repository Log diff commands failed with "path not found" error
SVN error: svn: E160013: '.../file.txt@367' path not found
Actual command sent: svn log ... 'path@367@' (double @)
Root cause: Caller constructed path@revision then passed to log() method
log() method called fixPegRevision() which added another @ when it detected existing @

Why this happens:

// BAD: Manual peg revision embedding
const pathWithPeg = `${remotePath}@${revision}`; // "file.txt@367"
await repo.log(..., pathWithPeg);
// log() calls fixPegRevision("file.txt@367") → "file.txt@367@"
// SVN interprets trailing @ as empty peg revision (HEAD)

// GOOD: Pass peg revision separately
await repo.log(..., remotePath, revision);
// log() calls fixPegRevision("file.txt") → "file.txt"
// Then appends "@367" → "file.txt@367"

SVN Peg Revision Syntax:

file.txt@100 = view file.txt at revision 100
file@2024.txt@100 = view file named "file@2024.txt" at revision 100
file.txt@100@ = view file named "file.txt@100" at HEAD (empty peg)
fixPegRevision() adds trailing @ to escape any @ in filename

Fix Pattern:

// Add optional pegRevision parameter to methods that call SVN
async log(rfrom, rto, limit, target?, pegRevision?: string) {
  let targetPath = fixPegRevision(targetStr);  // Escape @ in filename
  if (pegRevision) {
    targetPath += "@" + pegRevision;  // Add peg revision last
  }
  args.push(targetPath);
}

Rule: Pass peg revision as a separate parameter; let the SVN-calling method construct the final path.

25. Credential Lock Deadlock: No Nested retryRun Calls

Lesson: credentialLock in retryRun() causes deadlock when operations are nested within callbacks.

Issue (v2.32.20):

"Update Revision" notification hung indefinitely after svn update and svn info completed
Root cause: updateRevision callback called await this.status() which triggered nested retryRun()
Parent retryRun held credentialLock, child retryRun waited for same lock = DEADLOCK

Code path:

run(Update, callback)
  → retryRun(callback)      // Acquires credentialLock
    → callback()
      → this.status()
        → run(Status)
          → retryRun()      // BLOCKED waiting for credentialLock!

Fix:

// BEFORE: Nested run() call causes deadlock
await this.status(); // Calls run(Status) → retryRun()

// AFTER: Let parent run() handle model update
// run(Operation.Update) automatically calls updateModelState() after callback
void this.updateRemoteChangedFiles(); // Debounced, fires in background

Why this works:

Every run() call with a non-read-only operation already calls updateModelState() after the callback
Calling this.status() inside the callback was redundant AND caused the deadlock
The fix removes the redundant call, letting the parent handle status refresh

Rule: Never call run() / retryRun() from within a run() callback. Let the parent handle post-callback work.

35. VS Code UX: Multi-Step QuickPick Patterns

Lesson: Use native VS Code QuickPick/InputBox for wizard-like flows, not webviews.

Issue (v2.33.0):

Original commit UX required webview for commit message input
Webview is heavy, context-switching, non-native feel
Users expected Ctrl+Enter to commit like VS Code Git extension

Fix:

// Multi-step QuickPick with step indicators
await window.showQuickPick(items, {
  title: "Commit (1/3): Select type", // Step indicator in title
  placeHolder: "Choose commit type"
});

// acceptInputCommand for Ctrl+Enter
sourceControl.acceptInputCommand = {
  command: "svn.commitFromInputBox",
  title: "Commit",
  arguments: [this]
};

Pattern:

Use QuickPick for selection steps (commit type, files)
Use InputBox for text entry (scope, description)
Title shows step progress ("1/3", "2/3", "3/3")
Final step shows confirmation with file preview
Wire acceptInputCommand for Ctrl+Enter from SCM input box

Benefits:

Native VS Code look and feel
Keyboard-driven (no mouse required)
Faster than webview (no DOM rendering)
Familiar to Git extension users

Services:

ConventionalCommitService: Parse/format conventional commit messages
CommitFlowService: Orchestrate multi-step QuickPick flow
PreCommitUpdateService: Run SVN update in parallel with commit message input

Rule: Prefer native VS Code UI (QuickPick, InputBox, Progress) over webviews for simple workflows.

26. Optimistic UI Updates: Skip Status Refresh for SCM Operations

Lesson: For stage/unstage operations, update UI immediately without waiting for full svn status refresh.

Issue (v2.33.x):

Stage/unstage called full svn status --xml after each SVN changelist operation
Each status call: ~100-500ms depending on working copy size
Multiple rapid stage operations = cumulative delay (user sees lag)

Fix:

// BEFORE: Full refresh after every operation
await repository.addChangelist(files, STAGING_CHANGELIST);
await repository.updateModelState(); // Full svn status --xml

// AFTER: Optimistic UI update
await repository.addChangelist(files, STAGING_CHANGELIST);
groupManager.moveToStaged(files); // Move Resource objects directly

Pattern:

Execute SVN operation (addChangelist/removeChangelist)
Directly manipulate Resource groups in memory
Skip updateModelState() refresh
Eventual consistency: next status poll corrects any drift

Implementation:

moveToStaged(paths): Remove from changes/changelists, add to staged
moveFromStaged(paths, targetChangelist?): Remove from staged, add to target
Both methods update staging cache via syncFromChangelist()

When to use:

✅ Operations with predictable outcome (stage = move to staged group)
✅ User expects instant feedback (<100ms)
✅ Background refresh will eventually correct state
❌ Operations with uncertain outcome (merge, update with conflicts)
❌ Critical operations where accuracy > speed

Rule: For predictable SCM operations, update UI optimistically. Let background refresh handle edge cases.

27. Native Resources: Track and Dispose Separately

Lesson: Native Node.js resources (FSWatcher, process handlers) must be explicitly tracked and disposed.

Issue (v2.33.2):

fs.watch() watcher created but never added to disposables array
Process event handlers (exit, SIGINT, SIGTERM) never removed
File handles leaked when repository closed
Handlers accumulated during development reloads

Fix:

// BAD: Native watcher not tracked
const watcher = watch(path, callback);
watcher.on("error", handleError);
// Never closed!

// GOOD: Track and close in dispose
private nativeWatcher?: FSWatcher;

constructor() {
  this.nativeWatcher = watch(path, callback);
}

dispose(): void {
  if (this.nativeWatcher) {
    this.nativeWatcher.close();
    this.nativeWatcher = undefined;
  }
}

Process handlers:

// BAD: Handlers never removed
process.on("exit", cleanup);
process.on("SIGINT", handler);

// GOOD: Store references, remove on dispose
const sigintHandler = () => {
  cleanup();
  process.exit();
};
process.on("SIGINT", sigintHandler);
disposables.push(
  toDisposable(() => {
    process.removeListener("SIGINT", sigintHandler);
  })
);

Rule: Native resources don't implement IDisposable. Track them separately and clean up in dispose().

46. Proposed APIs: Proceed with Extreme Caution

Lesson: VS Code proposed APIs can have internal bugs that make them unusable, even with correct implementation.

Example (SourceControlHistoryProvider - ABANDONED):

We implemented scmHistoryProvider for native Graph view support. Despite correct implementation:

Runtime checks passed
Provider registered successfully
Methods implemented correctly

But VS Code's internal Graph view threw "Tree input not set" errors due to their own tree initialization bugs. No amount of timing adjustments (setImmediate, setTimeout, waiting for status updates) could fix VS Code's internal race condition.

What We Tried:

Immediate registration → Error
setImmediate delay → Error
setTimeout(100ms) → Error
Wait for first status update → Error

Conclusion: The bug was in VS Code's WorkbenchCompressibleAsyncDataTree2._updateChildren - their tree view wasn't initialized before they tried to update it.

Rule: Proposed APIs are unstable by definition. Even correct implementations can fail due to VS Code internal bugs. Always have a stable fallback (TreeView worked perfectly while Graph view failed).

Recommendation: Avoid proposed APIs unless absolutely necessary. Wait for them to become stable APIs before adoption.

47. History Filtering: Hybrid Server/Client Approach

Lesson: Leverage immutable data characteristics for aggressive caching.

Context (v2.35.0 - History Filtering):

SVN revision history is immutable and linear. This enables:

LRU cache with infinite TTL - Entries never invalidate (only evict by LRU)
Server-side filtering - Use SVN --search for text, -r {DATE} for dates
Client-side filtering - Action types (A/M/D/R) not supported by SVN

Pattern:

Check if filter uses server-supported features (--search, -r)
If yes → Call logWithFilter() with SVN args
If no → Use regular log() + client-side filter
Cache both raw and filtered results keyed by filter hash

Benefits:

First query: SVN call → cache
Same filter: instant cache hit
Different filter on same data: client-side filter from cache

Trade-off: Memory usage grows with unique filter combinations (capped at 50 entries).

Rule: Immutable data enables aggressive caching. Split filtering between server (what it supports) and client (what it doesn't).

48. VS Code Native UX: Multi-Step QuickPick

Lesson: Prefer VS Code native UI patterns over custom solutions.

Context (v2.35.0 - History Filtering UX):

Initial implementation: 8 separate filter commands in toolbar menu. Overwhelming and non-standard.

Better Pattern:

Single entry point - One "Filter" button opens multi-step QuickPick
Step 1: Select filter type (shows current values in detail line)
Step 2: Enter value (pre-populated with current value)
TreeView.description - Shows active filter summary (not title change)
Context variable - commands.executeCommand('setContext', 'key', value)
Dynamic visibility - Clear button visible only when filter active

VS Code UX Guidelines:

Use QuickPick for selection, InputBox for text entry
Use description property for state display
Use when clauses for dynamic menu items
Avoid webviews when native UI suffices

Rule: Fewer toolbar buttons = better UX. Use multi-step QuickPick for complex operations.

49. Extension Rebrand: Uninstall Old Version

Context (v0.1.1 - Duplicate Extensions):

After rebranding extension (svn-scm → sven), having both old and new versions installed causes "already registered" errors for schemes/views.

Symptoms:

"a provider for the scheme 'svn' is already registered"
"Cannot register multiple views with same id"
Double activation logs

Root cause: Two different extensions trying to register same schemes/views.

Solution: Uninstall old extension. That's it.

Anti-pattern avoided: Over-engineering activation guards based on speculation. We initially implemented multi-layered guards (module flags + command checks) when the fix was simply uninstalling the duplicate.

Rule: Diagnose root cause before implementing fixes. Trust VS Code's context.subscriptions pattern for cleanup.

28. DRY Helpers in Command Base Class

Lesson: Extract common patterns to base class helpers to reduce duplication across commands.

Issue (v0.2.1):

11 commands had .filter(s => s instanceof Resource) as Resource[]
22+ commands had .map(r => r.resourceUri)
7+ commands had .map(u => u.fsPath)
Type casts (as Resource[]) are unsafe, lose type guard benefits

Fix:

// In Command base class
protected filterResources(states: SourceControlResourceState[]): Resource[] {
  return states.filter((s): s is Resource => s instanceof Resource);
}
protected toUris(resources: Resource[]): Uri[] {
  return resources.map(r => r.resourceUri);
}
protected toPaths(uris: Uri[]): string[] {
  return uris.map(u => u.fsPath);
}
protected resourcesToPaths(resources: Resource[]): string[] {
  return resources.map(r => r.resourceUri.fsPath);
}

Usage in commands:

// BEFORE: Inline patterns with unsafe cast
const selection = states.filter(s => s instanceof Resource) as Resource[];
const uris = selection.map(r => r.resourceUri);
await this.runByRepository(uris, async (repo, resources) => {
  const paths = resources.map(r => r.fsPath);
});

// AFTER: Concise, type-safe helpers
const selection = this.filterResources(states);
await this.runByRepository(this.toUris(selection), async (repo, resources) => {
  const paths = this.toPaths(resources);
});

Benefits:

Type-safe: filterResources() uses type guard, not unsafe cast
Concise: One-liners instead of inline patterns
Discoverable: IDE autocomplete shows available helpers
Consistent: All commands use same patterns

Rule: When you see the same pattern 3+ times across commands, extract to base class helper.

29. SVN --limit Applies BEFORE --search Filter

Lesson: SVN --limit restricts commits searched, not results returned.

Issue (v0.2.8):

Author filter returned nothing if author had no commits in first 50
Root cause: svn log --limit=50 --search john only searches first 50 commits
If john's commits are older than those 50, search finds nothing

SVN Documentation:

"if --limit is used, it restricts the number of log messages searched, rather than restricting the output to a particular number of matching log messages."

Fix:

// BAD: --limit applied before --search
const args = ["log", `--limit=${limit}`, "--search", author];
// Searches only first 50, filters those

// GOOD: No --limit with text search, apply client-side
const args = hasTextSearchFilter(filter)
  ? ["log", "--search", author] // Search full history
  : ["log", `--limit=${limit}`]; // Use --limit when no text search

// After fetch, apply client-side limit
if (useTextSearch && entries.length > limit) {
  entries = entries.slice(0, limit);
}

When to omit --limit:

✅ Text search filters: --search (author, message, path)
❌ Revision/date range: -r 100:200 (OK to use --limit)
❌ No filter: (OK to use --limit)

Rule: Never combine SVN --limit with --search. Search full history, truncate client-side.

30. Cache Validation: Use Document Version for Staleness Detection

Lesson: Caches tied to file content must validate against document version, not just URI.

Issue (v0.2.8):

Blame annotations showed wrong line numbers after svn update
Cache key was URI only - no version tracking
External file changes (svn update, git pull) didn't invalidate cache
Stale blame data had old line numbers that no longer matched

Fix:

// BAD: Cache keyed by URI only
const cached = this.blameCache.get(uri.toString());
if (cached) {
  return cached.data; // Might be stale!
}

// GOOD: Validate document version
const currentVersion = editor.document.version;
const cached = this.blameCache.get(key);
if (cached && cached.version === currentVersion) {
  return cached.data; // Guaranteed fresh
}

// When caching, store version
this.blameCache.set(key, { data, version: currentVersion });

VS Code document.version:

Increments on every change (edits, undo, external reload)
Reset when document is closed and reopened
Reliable indicator of content changes

Rule: For caches tied to file content, always store and validate document.version.

31. Line Mapping for Modified Files: LCS Algorithm

Lesson: When displaying per-line annotations for modified files, use diff-based line mapping.

Issue (v0.2.8):

Blame decorations appeared on wrong lines when file had local modifications
SVN blame returns line numbers for BASE (committed) version
Working copy has different line numbers due to added/removed lines
User sees blame info on content that doesn't match

Solution: LCS (Longest Common Subsequence) based line mapping

// Compute mapping from BASE line numbers to working copy line numbers
const baseLines = await svn.cat(file, "BASE").split("\n");
const workingLines = editor.document.getText().split("\n");
const mapping = computeLineMapping(baseLines, workingLines);

// Apply mapping when creating decorations
for (const blameLine of blameData) {
  const mappedLine = mapping.get(blameLine.lineNumber);
  if (mappedLine === undefined) continue; // Line deleted
  const lineIndex = mappedLine - 1; // Apply decoration here
}

Algorithm details:

LCS finds exact line matches (unchanged lines)
Lines not in LCS checked for similarity (modified lines)
Context anchoring: if surrounding lines match, treat as modified
Deleted lines: mapping returns undefined, skip decoration

Performance:

O(m*n) for LCS where m,n are line counts
Cached per document version
Negligible for typical file sizes (<10ms for 1000 lines)

Rule: For annotations on committed data displayed in working copy, compute line mapping between BASE and working copy.

Uh oh!

FilesExpand file tree

LESSONS_LEARNED.md

Latest commit

History

LESSONS_LEARNED.md

File metadata and controls

Lessons Learned

63. Dead Code Can Hide Security Features

62. Network Flags Should Be Opt-In, Not Always-On

61. Reuse Background Polling Results for User-Triggered Checks

60. Deduplicate Cascading Refreshes in Compound Operations

58. Always Invalidate Cache After Cleanup

59. Unify Error Handling Across Command Paths

56. Legacy E2E Preflight: Use Suite-Ready Guard, Not suiteSetup Skip

55. Polling Tests: Await Explicit Signal, Not Fixed Sleep

54. Background Polls: Teardown Must Wait Before Repo Deletion

53. Cross-Platform Unit Tests: Avoid Mocked-Home Prefix Assumptions

52. Cross-Platform Paths/Auth: Match Real SVN + Windows Rules

51. Vitest+Sinon Stubs: Keep CommonJS Boundary for Stub Targets

50. Coverage Fail-Cluster: Hit Biggest Bucket First

32. Vitest Migration: Avoid ESM Export Reassignment

33. VS Code Mock Fidelity: Bindings + Defaults Matter

34. XML Adapter Contract: Root Can Be Stripped

Core Patterns

0. Path Guards Before SVN Operations

1. Build System: tsc over webpack

2. Service Extraction Pattern

3. Dependency Migration: Adapter Pattern

4. Type Safety: No Bypasses

5. Performance: Measure User Impact

6. Error Handling: Descriptive Messages

7. Testing: TDD + 3 End-to-End Tests

8. Incremental Commits

9. Critical Path Mapping

10. Code Review: Type Safety Checks

11. Batch Operations: Trade Bandwidth for Latency

17. Cache Eviction: LRU Policy

12. VS Code Dynamic Icons: No Static Icon in Command

13. Remote Development: extensionKind Required

14. Error Detection: Check Specific Patterns First

15. Remote SSH: Respect Native Credential Stores

Quick Reference

Anti-Patterns to Avoid

16. Testing: Vitest for Unit Tests

18. SVN Locking for Large Files

19. CLI vs GUI Feature Parity

20. Cleanup Error Detection: Multiple Codes

21. User-Friendly Errors: Show Code for Transparency

22. Timer Disposal: Store References

23. VS Code TreeView Providers: Listen for Repository Open/Close Events

24. SVN Peg Revision: Pass Separately, Don't Embed in Path

25. Credential Lock Deadlock: No Nested retryRun Calls

35. VS Code UX: Multi-Step QuickPick Patterns

26. Optimistic UI Updates: Skip Status Refresh for SCM Operations

27. Native Resources: Track and Dispose Separately

46. Proposed APIs: Proceed with Extreme Caution

47. History Filtering: Hybrid Server/Client Approach

48. VS Code Native UX: Multi-Step QuickPick

49. Extension Rebrand: Uninstall Old Version

28. DRY Helpers in Command Base Class

29. SVN --limit Applies BEFORE --search Filter

30. Cache Validation: Use Document Version for Staleness Detection

31. Line Mapping for Modified Files: LCS Algorithm