Docs

Documentation

Design Philosophy

Why this document exists

The tactical docs in this folder (DI_PATTERNS.md, ERROR_HANDLING_GUIDE.md, IPC_CONTRACTS.md, DEPENDENCY_BOUNDARIES.md, CODE_QUALITY_STANDARDS.md) describe how we build things. This doc describes why — the strategic principles that those tactical patterns serve. It draws on John Ousterhout's A Philosophy of Software Design, calibrated to this codebase's reality.

Use this doc:

This doc is prescriptive, not aspirational. The codebase already follows most of these principles; the goal here is to make the principles explicit so they don't erode.

Core principle: complexity is the enemy

Complexity is anything about a system that makes it hard to understand or modify. It shows up as three symptoms:

Every other principle below exists to attack one of these three symptoms.

Principle 1: prefer deep modules

A deep module has a simple interface that hides substantial complexity. A shallow module exposes nearly as much surface area as it implements — it adds cognitive load without taking anything away.

Rule of thumb for services in this codebase: target ≥50 LOC of implementation per public method. The deep services we already have:

Service LOC Public methods LOC per method
OramaVectorService ~5,400 ~105 ~51
LlamaService ~4,300 ~40 ~107
SearchService ~3,700 ~43 ~86
SmartFolderWatcher ~4,100 ~35 ~117

(LOC as of 2026-10; method counts are from the last hand count. Both drift as the code evolves — treat them as illustrative, not exact.)

These are textbook deep modules: small public API, large hidden implementation (state machines, retries, caching, fallback paths). Do not split them to chase a LOC target — the splits would be the very pass-through layers we're trying to avoid.

Conversely, if a new service exposes 20 methods over 600 LOC, that's ~30 LOC/method and a yellow flag. Ask: is the interface too wide? Could most of those methods be private?

Caveat: count async methods. A naive regex that matches ^ methodName( will miss async methodName( and produce misleadingly low ratios. An earlier shallow-service review flagged three services as "shallow" using such a regex; once async methods were counted, all three were within the deep-module band (43–71 LOC/method). When in doubt, count by hand or use an AST.

Caveat: orchestrators get a pass. A coordinator class whose public methods each front a substantial sub-module legitimately has many small public methods. AnalysisHistoryServiceCore (35 LOC/method) is below the ≥50 threshold but each method orchestrates ≥2 cohesive submodules — it earns the depth at the subsystem level, not the per-method level.

Principle 2: information hiding before abstraction

A method's signature should reveal as little about its implementation as possible.

The strict main/renderer/shared boundary in DEPENDENCY_BOUNDARIES.md is a concrete expression of this principle — the IPC layer is the only sanctioned leak point.

Principle 3: pass-through layers are a red flag

A pass-through is a module that mostly forwards calls to another module without adding meaningful abstraction. Two pass-throughs in series quadruple cognitive load while adding zero value.

The pass-through test: if you removed this module and inlined its callers into its callees, would the system be meaningfully worse? If no, you have a pass-through.

Legitimate thin layers we keep (these are not pass-throughs):

Anti-patterns to avoid:

Principle 4: prefer general-purpose code

Over-specialization causes complexity. A function tailored to exactly one caller will be wrong for the second caller, and rewriting it during integration costs more than designing it generally up front.

The two-caller question. Before extracting a utility, ask: "Will this realistically have two callers within the next month?" If no, inline it. If yes, design the interface around both callers from the start.

The "no specific caller in the API" test. A general-purpose method's name and signature should make sense without knowing which caller it was extracted for. A method named computeBatchSizeForOrganizeFlow() is over-specialized; computeBatchSize(itemCount, memoryBudget) is general.

Our heavy-hitter utilities (ParallelEmbeddingService, WhisperService, atomicFileOperations) earn their generality — each has many callers and a name that describes capability, not workflow.

Principle 5: design it twice

For any non-trivial change, the first design is rarely the best one. Spending 15 minutes on a second alternative — even a brief sketch — almost always exposes assumptions baked into the first.

The rule: for any PR larger than ~300 LOC, the description includes a short "Alternatives considered" section (3–5 lines per alternative). It can be terse. The point is to prove the chosen design was a choice, not a default.

This applies especially to:

Principle 6: comments explain why, not what

Well-named identifiers describe what the code does. Comments earn their keep only when they explain something the code can't:

Don't write:

See CODE_QUALITY_STANDARDS.md for naming conventions that reduce the need for comments in the first place.

Principle 7: errors are designed, not added

Every error path is part of the interface. An error type that escapes a module's boundary becomes the caller's problem.

This codebase enforces this via:

Don't catch what you can't handle. A bare try/catch that logs and swallows is rarely better than letting the error propagate to a place that knows what to do. When you do catch, classify (recoverable / user-facing / fatal) and route accordingly.

Complexity budget heuristics

Numeric triggers for a "should we split this?" review comment. Hitting one isn't an automatic split; it's an automatic conversation.

Trigger Action
Single file > 1,500 LOC and > 20 public methods Discuss split
React component > 800 LOC Discuss extraction (sub-components, hooks)
IPC handler file > 1,500 LOC Discuss helper extraction (the createHandler / registerHandlers pattern)
New utility with exactly one caller Inline or justify in the PR
PR diff > 300 LOC of non-mechanical change Include "Alternatives considered"
Method body > 60 lines Consider extracting helpers (not always — sometimes a state machine is genuinely one block)

These are guides, not gates. CI does not enforce them. Reviewers do.

Red flags to call out in review

When you see one of these, the right comment is "this looks like a [name]; can we restructure?"

What is not in scope for this doc

These belong to other docs; this one points at them rather than duplicating:

Hard constraints (these survive every refactor)

The principles above are guidance. The following are non-negotiable: any change that violates one is reverted regardless of how well it scores on the principles.

Silent catches: convention

Empty catch {} blocks and .catch(() => {}) / .catch(() => null) discard the rejection without a trace. Sometimes that's correct (mutex-chain guards, fire-and-forget telemetry, defensive finally-body guards) and sometimes it's a forgotten error path that costs ops a debugging session when something goes wrong.

To make the intent legible at review time, every silent-catch site in src/main/** and src/shared/** must satisfy one of these:

  1. Migrate to swallow() from src/shared/promiseUtils.js:

    const { swallow } = require('../shared/promiseUtils');
await swallow(fs.unlink(tempPath), logger, 'cleanup:atomic-temp');
// ↑ debug-logs rejections with the context tag; always resolves to undefined.

    Pass 'warn' as the fourth argument for sites where a failure deserves production visibility.

  2. Add a // why-silent: comment above the catch explaining the reason silence is correct:

    this._writeChain = next.catch(() => {
  // why-silent: catch breaks chain poisoning; the original error already
  // surfaced to the caller via `return next` above.
});

    Reviewers can git grep "why-silent:" to audit every documented silent site.

ESLint enforces this with no-empty: ['warn', { allowEmptyCatch: false }] — new empty catches are flagged in the IDE and PR review. Use // eslint-disable-next-line no-empty -- why-silent: <reason> when the empty body is the deliberate intent and swallow() is the wrong fit.

The .catch(() => null) and .catch(() => []) patterns are not flagged by ESLint (they're not "empty" — they return a fallback) but the convention still applies: prefer swallow() when the caller will treat the rejection differently from the success-with-empty-result path.

Verification

Most principles are reviewer-enforced. The hard constraints above are CI-enforced. Run:

npm run ci            # the canonical gate
npm run local:gate    # ci + verify:native-modules (pre-push)

If a PR proposes to change anything in Hard constraints, the PR description must say so explicitly and be reviewed by someone other than the author.

Edit this page on GitHub