Model VisitControlMessage as a discriminated union on action (+ fix unsound cast for legacy bare visit id)

[vringar]

Summary

VisitControlMessage in Extension/src/loggingdb.ts has the same type-modelling weakness as JSLogMessageContent (#1218): every field is optional even though field presence is really governed by the action discriminant. It additionally has an unsound as cast — the socket callback casts a payload that may be a bare number to the struct type. This is type-only modelling polish, not a bug (the runtime is correct); filed as a follow-up.

Current shape

// loggingdb.ts
interface VisitControlMessage {
  action?: "Initialize" | "Finalize";
  visit_id?: number;
  browser_id?: number;
  success?: boolean;
}

const listeningSocketCallback = async (data: unknown) => {
  const message = data as VisitControlMessage;   // <-- also true for a bare int
  const action = message.action;
  let newVisitID = message.visit_id ?? null;
  switch (action) {
    case "Initialize": /* uses visit_id; sets browser_id */ ...
    case "Finalize":   /* uses visit_id; sets browser_id + success = true */ ...
    default:           /* legacy: data is a bare number/numeric string */
      newVisitID = parseInt(String(data), 10);
      ...
  }
};

Problem 1 — all fields optional; presence is really keyed on action

• Initialize and Finalize both require visit_id.
• success is only ever set for Finalize (message.success = true).
• browser_id is not received — it's added by the handler before forwarding as meta_information.

The blanket ? on every field doesn't express any of this, so callers can't rely on visit_id being present for a known action, and the visit_id ?? null guard exists partly to paper over the loose type.

Problem 2 — data as VisitControlMessage is unsound for the legacy path

The listening socket may deliver a bare visit id (number or numeric string), not an object (see the interface's own doc comment). The code casts it to VisitControlMessage anyway and reads .action off it — which happens to be undefined and falls through to the default (legacy) case. It works by luck, not by type. The payload is genuinely VisitControlMessage | number | string, and the cast hides that.

Proposed fix — discriminated union on action + explicit legacy scalar

interface InitializeMessage { action: "Initialize"; visit_id: number; browser_id?: number; }
interface FinalizeMessage   { action: "Finalize";   visit_id: number; browser_id?: number; success?: true; }
type VisitControlMessage = InitializeMessage | FinalizeMessage;

// what the socket can actually deliver:
type ListeningSocketPayload = VisitControlMessage | number | string; // legacy bare visit id

Then the callback narrows honestly:

const listeningSocketCallback = async (data: ListeningSocketPayload) => {
  if (typeof data !== "object") {              // legacy bare visit id
    visitID = parseInt(String(data), 10);
    return;
  }
  switch (data.action) {                        // data: VisitControlMessage
    case "Initialize": /* data.visit_id: number, no `??` needed */ ...
    case "Finalize":   /* data.success settable, data.visit_id: number */ ...
  }
};

Benefits: visit_id is required (not ?) inside each action branch, success only exists on Finalize, and the bare-int legacy path is a real, checked branch rather than a cast that reads .action off a number.

Note on the received-vs-forwarded shape

browser_id/success are mutated onto the message before forwarding to the storage controller as meta_information, so the incoming and outgoing shapes differ. A fuller version could separate the received type from the forwarded meta_information payload type; at minimum, model the received discriminated union above and keep the mutation explicit.

Scope / acceptance

• Split VisitControlMessage into InitializeMessage | FinalizeMessage; type the callback param as VisitControlMessage | number | string.
• Replace data as VisitControlMessage + .action access with a typeof data !== "object" narrowing for the legacy scalar path.
• Drop now-unnecessary visit_id ?? null inside the action branches (visit_id is non-optional there).
• tsc --noEmit clean; no runtime change; existing socket/visit-lifecycle behavior unchanged.

Related

• Model JS-instrument log message as a discriminated union on operation (honest args/value types) #1218 — the sibling discriminated-union modelling issue for JSLogMessageContent (same pattern, JS-instrument message).
• Audit NULL vs empty-string fidelity in extension data capture #1216 — NULL-vs-empty-string capture-fidelity audit.