Skip to content

Key record source icon/label off the real source type - #224

Open
grimicorn-agent wants to merge 5 commits into
mainfrom
agent/client-source-real-type
Open

Key record source icon/label off the real source type#224
grimicorn-agent wants to merge 5 commits into
mainfrom
agent/client-source-real-type

Conversation

@grimicorn-agent

Copy link
Copy Markdown
Collaborator

What changed and why

Records rendered the same zap icon and a garbled label for every source. sourceTypeIcon/formatSourceLabel keyed off a type/-style prefix in records.source, but that column stores the source's free-text display name (e.g. "My Zapier hook"), never a type prefix — so the type detection matched nothing and every record fell through to the default.

The real source type lives on sources.type, reachable only by joining records.sourceId → sources.uuid, and it was not exposed on the record read contract. This PR exposes it and drives the display off it.

Server

  • Added a tenant-scoped leftJoin on sources to the record list (GET /api/records) and detail (GET /api/records/:uuid) queries, selecting sources.type as a new sourceType attribute. The join is scoped AND sources.userId = userId so it can never surface another tenant's source type, even if a future write path sets sourceId without the ownership check.
  • recordSerializer now emits sourceType on the record resource.
  • Documented sourceType in public/openapi.json.

Client

  • sourceTypeIcon(sourceType) maps the real type to an icon via an exhaustive Record<SourceType, string> (compiler flags a missing icon if a source type is added).
  • formatSourceLabel(source, sourceType) shows the source's display name first (so two same-type sources like "Prod deploys" / "Staging deploys" stay distinguishable — the type is already conveyed by the icon), falling back to the resolved type name, then "unknown".
  • Inbox rows and the record detail modal pass record.attributes.sourceType.

Key decisions

  • Scope expansion approved on the issue. The maintainer authorized exposing sourceType on the record resource (server join + contract + client type) provided no related PR existed; none did.
  • Label prefers the source name over the type. The icon carries the type (the actual bug); the label carries identity so same-type sources are distinguishable, and records whose source was deleted (ON DELETE SET NULL) keep their stored name instead of collapsing to "unknown".
  • sourceType is null on create/patch responses. Those endpoints don't join sources; the field is documented (OpenAPI + code comment) as a write-receipt null — re-read via GET for the resolved type. Populating write responses is left as a follow-up (see below).

Where to view

  • Inbox list: /inbox — the source column (icon + label) per record row.
  • Record detail modal: open any record from /inbox.

Tests

  • Unit: sourceTypeIcon per type + fallback; formatSourceLabel name-first, type fallback, legacy-type, and unknown paths.
  • Server: list/detail join is tenant-scoped and surfaces sourceType; serializer emits it.
  • Snapshot: inbox rows and detail modal exercise the real wiring (icon resolved from sourceType, label from the name; reverting the wiring now fails).

Closes #153

Follow-up suggestions

  • Mirror sourceType in markpost-cli — the CLI hand-mirrors the record contract; add sourceType to its record type so consumers see the resolved type (suggested: P3, effort: S, evidence: cross-repo contract server/utils/response.ts)
  • Populate sourceType on create/patch responses — join/re-read sources on POST/PATCH/bulk-PATCH so the write responses carry the resolved type instead of a documented null (suggested: P3, effort: M, evidence: server/api/records/index.post.ts, [uuid].patch.ts, index.patch.ts)
  • Consolidate source-type→icon maps — SourceCard.vue's ICON_BY_TYPE and useRecords.ts's SOURCE_TYPE_ICONS duplicate the type→icon mapping; extract one shared map to prevent drift (suggested: P3, effort: S, evidence: app/components/SourceCard.vue, app/composables/useRecords.ts)

Join sources into the record list/detail read queries and expose
sourceType on the record resource, then drive sourceTypeIcon/
formatSourceLabel off it instead of a type prefix that the free-text
records.source column never carried.

Closes #153
@grimicorn-agent grimicorn-agent added the has-suggestions PR carries follow-up suggestions for the improvement digest label Aug 23, 2026
@grimicorn-agent

Copy link
Copy Markdown
Collaborator Author

Independent code review trail

Ran the independent reviewer (Opus) for 3 rounds. Findings and actions:

Round 1 (6 findings)

  • Label lost source identity / deleted-source records showed "unknown" — fixed: formatSourceLabel now takes (source, sourceType).
  • POST/PATCH return sourceType: nulldocumented (see round 3 Bearer Token Authentication #6); populating write paths deferred to a follow-up.
  • OpenAPI spec missing sourceTypefixed: added to components.schemas.Record.
  • No list-join test coverage — fixed: added join + serialized-sourceType tests.
  • Misleading list-test mock comments — fixed: comments now match the router.
  • Record row cardinality — verified no fan-out (sourceId → unique sources.uuid).

Round 2 (4 findings)

  • Write-path sourceType: null inconsistency — skipped (see decision below).
  • Label should be name-first — fixed: label prefers the source display name (icon carries the type).
  • Icon wiring untested (constant mock / swallowed prop) — fixed: inbox mock now echoes its arg; modal AppIcon stub renders data-icon; added a null-type fallback assertion. Reverting the wiring now fails.
  • Join not tenant-scoped — fixed: both joins add AND sources.userId = userId, asserted in the list test.

Round 3 (6 findings)

  • Two type→icon maps (SourceCard vs useRecords) — skipped/flagged: rule-of-three not met (2 occurrences) and consolidation touches out-of-scope SourceCard.vue; recorded as a follow-up suggestion.
  • Label discarded a valid legacy type (e.g. rss) — fixed: dropped the isSourceType guard in the label path (kept it on the icon path); added a legacy-type test.
  • Legacy type/-prefixed source values now render raw — skipped (deliberate): re-adding slash-stripping reintroduces the exact fragile prefix-parsing this issue removed; the icon now conveys the type and the raw name is honest. Flagged an optional prefix-strip follow-up.
  • Detail-endpoint join not asserted — fixed: added a test pinning the joined sources table (predicate detail is covered in the list test's mocked-drizzle assertion).
  • makeRecordResource fixture missing required sourceType (TS2741) — fixed.
  • Misleading create/patch nullfixed (doc): tightened the OpenAPI description to mark it a non-authoritative write-receipt null.

Deliberate decision — write-path sourceType: Kept sourceType resolved only on the list/detail GET endpoints (the display paths this issue is about and the surface the maintainer approved). Create/patch/bulk-patch return a documented write-receipt null. This app never renders write responses as source icons; populating them is recorded as a follow-up suggestion.

All 1756 unit/snapshot tests pass; npm run lint:ci (prettier + eslint + fallow gate) is clean.

@grimicorn grimicorn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix conflicts

@grimicorn-agent

Copy link
Copy Markdown
Collaborator Author

Independent code review trail (re-run after merging main)

Ran the independent reviewer (Opus) for 3 rounds on the post-merge diff. The prior review trail (before the main merge) still stands; this covers what the merge + fresh review surfaced.

Round 1 (5 findings)

  • RecordAttributes.sourceType typed SourceType | null but the server contract (response.ts) and the free-text sources.type column (no CHECK) are string | nullfixed: widened the client type to string | null with a comment; isSourceType stays the single narrowing point.
  • OpenAPI sourceType used oneOf: [SourceType, null], over-narrow vs the free-text column — fixed: type: ["string","null"], description still names the canonical set.
  • Detail-join tenant-scoping asserted by comment only (deleting sources.userId = userId kept the show test green) — fixed: show.test.ts now walks the real drizzle predicate and pins records.sourceId, sources.uuid, sources.userId, and the bound userId. Verified it reds when the clause is removed.
  • sourceType: null on create/patch responses — skipped (deliberate, pre-existing decision): resolved only on the display GET endpoints (the approved scope); write responses return a documented write-receipt null. Tracked as an existing follow-up suggestion.
  • Legacy type/-prefixed source values render raw — skipped (deliberate) + hardened: re-adding slash-stripping reintroduces the fragile prefix-parsing this issue removed. Added a test pinning formatSourceLabel("webhook/github", "webhook") === "webhook/github" so the raw rendering is chosen, not inherited.

Round 2 (3 live findings)

  • Create/patch null — same deliberate decision as above; skipped.
  • Prefix back-compat — addressed via the pinning test above.
  • list.test.ts mocked getTableColumns: () => ({}), so nothing asserted the page select's record columns (a dropped spread would return sourceType-only rows and stay green) — fixed: mock echoes the table, added an assertion pinning uuid/createdAt/status/sourceType in the page select. Verified it reds when the spread is dropped.

Round 3 (4 findings)

  • Detail select's column set unpinned like the list's — fixed: added the mirror assertion to show.test.ts (uuid/status/sourceType). Verified it reds when the spread is dropped.
  • Duplicate source-type→icon maps (useRecords vs SourceCard.vue vs AddSourceModal.vue) — skipped (out of scope): consolidation touches components unrelated to Client source icon/label ignores real type #153; already captured as an existing follow-up suggestion in the PR body.
  • Create/patch null — deliberate decision; skipped (as above).
  • formatSourceLabel(source, sourceType) two-param swap hazard — skipped: the signature was a deliberate prior-review choice and is fully tested; an object-param refactor would touch template call sites and snapshots beyond this issue's scope. Behavior is correct; noted for a possible future ergonomics pass.

All 1852 unit/snapshot tests pass; npm run lint:fix and npm run lint:ci (prettier + eslint + fallow gate) are clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

has-suggestions PR carries follow-up suggestions for the improvement digest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Client source icon/label ignores real type

2 participants