Learning clean code with Markwork

Rules the build checks

The README has always described Markwork's layers: markdown/ knows no GTK, agent/ knows no widgets, controllers never see MainWindow. A rule that only lives in prose holds as long as every contributor remembers it. scripts/layers.mjs turns it into a check that runs in npm run build and npm run typecheck.

Each rule is three things: which files it applies to, which imports are forbidden, and why. The "why" is printed when the build fails, so the message teaches instead of just refusing.

// scripts/layers.mjs
const rules = [
    [f => f.startsWith('markdown/'), t => t.startsWith('gi://') || !t.startsWith('markdown/'),
        'markdown/ only holds string → data functions: no GI, nothing outside markdown/'],
    [f => f.startsWith('agent/'), t => GTK.test(t) || /^(ui|editor|window)\//.test(t) || t === 'window.ts',
        'agent/ is free of GTK and knows nothing about the editor, widgets, or the window'],
    [f => f.startsWith('ui/') || f.startsWith('window/') || f === 'actions.ts', t => t === 'window.ts',
        'only app.ts composes MainWindow; components and controllers get a host interface'],
    // …
];

The same script also limits the cyclomatic complexity of every function to 15 (chapter 2). It needs a TypeScript parser, and TypeScript 7 has no JavaScript API, so it uses the oxc parser that Vite already ships (rolldown/parseAst): no new dependency.

Small functions: complexity as a signal

Cyclomatic complexity counts the paths through a function: 1, plus one for every if, ?:, &&, ||, ??, case, loop, and catch. It is the minimum number of tests needed to run every branch once. The usual reading:

The worst case: one method that did a whole turn

ChatSession.ask was 242 lines with a complexity of 132: it built the context, recovered saved work, ran the model loop, counted usage, and executed every kind of tool inline. The fix was not to cut it into arbitrary pieces, but to notice that all those pieces shared the state of one turn. That state became a class, and each step a method.

async ask(input, provider, model, handlers, cancellable) {
    // context, recovery messages, journal …
    for (let round = 0; round < MAX_ROUNDS; round++) {
        // call the model, count usage …
        for (const call of result.toolCalls) {
            if (useTools && call.name === VERIFY_TOOL.name …) { … continue; }
            if (useTools && canPropose && … BATCH_TOOL …) { … continue; }
            if (useTools && … WORK_TOOL …) { … continue; }
            if (useTools && canPropose && isChangeTool(…)) { … continue; }
            // read and git tools …
        }
    }
}

ask(input, provider, model, handlers, cancellable) {
    const generation = this.generation;
    return new TurnRun(this, () => generation !== this.generation,
        input, provider, model, handlers, cancellable).run();
}

private async runTool(call: ToolCall, useTools: boolean) {
    const project = this.input.options.project;
    if (useTools && project && call.name === VERIFY_TOOL.name) return this.verify(call);
    if (useTools && this.canPropose && … BATCH_TOOL.name) return this.proposeBatch(call, …);
    if (useTools && project && call.name === WORK_TOOL.name) return this.setWork(call);
    if (useTools && this.canPropose && isChangeTool(call.name)) return this.proposeChange(call);
    return this.read(call, useTools);
}

A turn whose conversation was cleared meanwhile used to return early from five places deep inside the loop. Now any step calls checkStale(), which throws a private StaleTurn; run() catches it once and drops the result. One rule, one place.

A table instead of a long switch

planChange (45), planKanban (34), PiReader.line (54), and describeCall (17) all dispatched on a name. Each became a lookup table of small handlers:

const KANBAN_ACTIONS: Record<string, (k: KanbanTarget) => Board | PlanResult> = {
    add: k => { … return addCard(k.board, column, k.cardText); },
    move: k => { … return moveCard(k.board, from, { column, index: Infinity }); },
    mark: k => { … },
    // …
};
const action = lookup(KANBAN_ACTIONS, asText(args.action));
if (!action) return fail('The "action" argument must be one of: …', 'invalid action');

Prove the refactor changed nothing

"The tests still pass" says that the tested paths still work. A refactor of a function with 42 paths needs more than that. Markwork's refactors used differential testing: bundle the old version (from git show HEAD:…) and the new one side by side, feed both the same random inputs, and compare the outputs exactly.

// node eq.mjs <src path> <generator.mjs>
for (let i = 0; i < count; i++) {
    const input = gen.input(rnd, pick);
    const a = run(Old, input), b = run(New, input);
    if (a !== b) { console.log('DIFF for', input); process.exit(1); }
}

Check what the generator really reached

The first generator for parseChat passed 20,000 files, but counting its outputs showed only 49 with a saved work plan and none with a verification or the status complete: parseWork requires a note string the generator rarely produced. The branches that mattered most had not run at all. After fixing the generator: 2,586 plans, 1,839 with a verification, 116 complete, still identical.

Cohesion: one job per class

LCOM4 measures cohesion: draw a line between two methods when they use the same field or one calls the other, then count the groups. One group means the class does one thing; two groups are usually two classes sharing a file. Static helpers, callback hooks set from outside, and methods that do not touch this are left out, because they say nothing about cohesion.

export class HighlightCache {
    private snapshot: Parsed | null = null;
    private readonly lineCache = new LineCache();
    constructor(private readonly indent?: ListIndent) {}
    …
}
export class LineCache { begin() get() put() end() }

A view that only draws

ChatPanel sent requests, cancelled them, refused proposals after a folder change, saved and reopened history, undid changes, and drew all of it. The life cycle moved into agent/chatcontroller.ts, which has no GTK; the panel implements a small ChatView interface and draws what it is told. Its popovers and proposal cards then became helpers that own their template widgets.

export interface ChatView {
    noKey(): void;
    saveFailed(error: string): void;
    busyChanged(busy: boolean): void;
    startTurn(question: string): TurnView;   // text, reasoning, steps, work, end
    review(change, apply, cancelled): Promise<ProposalResult>;
}

The payoff is in the tests: a missing key, a folder change in the middle of a request, stopping, and a reset during a turn are now tested with a recording view and a fake provider, without opening a window.

Today 53 of the 60 classes with three or more methods have LCOM4 1. The other seven are small and read fine: StatusBar has a left and a right half, Sidebar a page and its contents, ProposalViewer a dark-mode setter. One is the counter's own blind spot: DocumentController.setFile only calls a helper that does not touch this.

Coupling: a narrow host, and one place that knows everyone

Markwork has no import cycle. Its fan-out is concentrated where it belongs: window.ts imports 33 modules because it is the composition root, the one place that creates the widgets and connects their callbacks. Everything else stays narrow.

A controller never receives MainWindow. It declares what it needs as a host interface, and the window builds that host from closures:

// window/views.ts
export interface ViewHost extends DocumentHost {
    readonly content: Gtk.Stack;
    readonly board: KanbanBoard;
    readonly inbox: InboxView;
    readonly statusBar: StatusBar;
    newDocument(text: string): void;
    // …
}

Reading ViewHost tells you everything the view controller can touch; nothing else is reachable. The layer check (chapter 1) forbids importing window.ts from window/, so this cannot erode.

The same shape for a small feature

The commit message writer (1bac3b8) needs git, the API key, the model client, and the chosen model. None of those is imported. The function lists them as a parameter, and the composition root fills it in:

// agent/commitmessage.ts: only type imports
export interface CommitSources {
    keyStore: KeyStore;
    makeProvider: (key: string) => Provider;
    model: () => string;
    diff: (file: string) => Promise<…>;
    subjects: (file: string, limit: number) => Promise<string[]>;
}

// window.ts: the one place that knows the Assistant and git
private connectWriter(writer: MessageWriter): void {
    writer.write = (files, onText, cancellable) => writeCommitMessage({
        keyStore: this.chat.keyStore, makeProvider: this.chat.makeProvider, model: () => this.chat.model,
        diff: workingDiff, subjects: recentSubjects,
    }, files, onText, cancellable);
}

The tests pass a fake key store and a fake provider, with no network and no repository. The button, ui/messagewriter.ts, is shared by the History tab and the single-file window. It knows neither of them: it gets an entry, a status label, and a files() getter, and reports back through onBusy so each owner locks its own form.

Local reasoning: one owner per piece of state

You can reason locally when the code in front of you is enough to know what a value means. Markwork's biggest obstacle was the Doc record for an open tab: ten fields, written by five modules. To know when textOverride is reset, or what file === null means after an agent deleted a file, you had to read all five.

export interface Doc {
    id: number; editor: MarkdownView;
    file: string | null; home: boolean;
    textOverride: boolean; boardText: string; reloadQueued: boolean;
    autosaveTimer: number; lastChange: number; changes: number;
}
// window/agentwrites.ts
if (open) { open.file = to; host.refreshTitle(open); }

export interface Doc {
    readonly id: number;
    readonly editor: MarkdownView;
    readonly file: string | null;   // DocumentController only
    readonly home: boolean;         // DocumentController only
}
// window/agentwrites.ts
if (open) host.fileMoved(open, to);

readonly makes the rule enforceable: an assignment from anywhere else is a compile error. The owner keeps a private writable view (type MutableDoc = { -readonly [K in keyof Doc]: Doc[K] }) used in exactly one method.

No hidden inputs

The highlighter used to find the editor's list indentation through a module-level WeakMap that MarkdownView filled: a parse depended on a registration done elsewhere, invisible in its signature. Now HighlightCache takes the ListIndent in its constructor. Likewise, the line parser's methods took (i, off, lineEnd, nl) positionally, four numbers easy to swap; they now take one Row.

Make time and order explicit

Autosave writes in a worker thread so typing never waits for fsync. The first version started every write at once and only counted how many were pending. Two overlapping writes could finish in the wrong order, leaving an older version on disk. Waiting for them pumped the whole main loop, so timers and input could run in the middle of a save.

The fix made order a data structure instead of luck: per path, one running write and at most one waiting, which a newer request replaces. Completions report on their own GLib.MainContext, so flushWrites() iterates only that context; no unrelated callback can run while it waits.

export function writeTextFileAsync(path: string, text: string, done: Done): void {
    const queue = queues.get(path);
    if (queue) queue.next = { text, done: [...queue.next?.done ?? [], done] };   // the newest text wins
    else { const fresh = { running: [done], next: null }; queues.set(path, fresh); start(path, text, fresh); }
    ensurePump();
}

Freshness as a parameter

Reading the work folder has the same shape of problem: how current must the answer be? WorkspaceRepository makes the caller say it, instead of every caller re-walking the folder or silently trusting a cache:

Checklist for a change in Markwork

• npm run typecheck passes: it includes the layer rules and the complexity limit of 15.
• A new module has a place in the layer diagram in the README, and a rule in scripts/layers.mjs if its place is not obvious from its folder.
• A function above 10 has a reason; a dispatcher on names is a guarded table (Object.hasOwn).
• A new controller receives a host interface that lists what it uses, never MainWindow.
• New per-document state lives with the feature that owns it, not on Doc; a shared field is readonly outside its owner.
• A refactor of a pure function is compared with the old version on generated inputs, and the generator's coverage of the changed branches is counted.
• Decisions live outside widgets, so they can be tested without a window; npm test runs unit and GUI tests under Xvfb.

The journey

1. c4e0a49Background writes queued per path and flushed explicitly (chapter 7).
2. 28b9badThe layer rules checked on every build (chapter 1).
3. e4e3d91WorkspaceRepository: a monitored snapshot and explicit freshness.
4. 2dbf902ChatController: the conversation life cycle without GTK (chapter 4).
5. 95f68f4DocumentController: tabs and documents out of the window.
6. 76acf0dChatSession.ask (132) split into a TurnRun (chapter 2).
7. 07b4862Complexity limit 20, with an exception list that may only shrink.
8. a45026aThe last ten functions above 20 split; each compared with its old version (chapter 3).
9. a8dc264Limit lowered to 15; eleven more functions split.
10. 84dccddChatPanel split into its parts; every class with LCOM4 above 1 reviewed.
11. 85f18bbOne owner per piece of document state; RunQueue owns run status (chapter 6).
12. 1bac3b8A new feature built to the rules from the start: CommitSources filled by the composition root, one button shared by two views through callbacks, and no function above complexity 7 (chapter 5).