Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ stay complete. This is mandatory, like logging — a route without `describeRout
- Connection pool: `max: 10` via `postgres-js` driver in `packages/db/src/index.ts`.
- Migrations: `deno task db:generate` (creates SQL) then `deno task db:migrate` (applies); seed is `deno run -A packages/db/src/seed.ts`.
- **Schema changes:** All schema changes (tables, columns, indexes, enums, constraints) MUST be made in `packages/db/src/schema.ts` (the Drizzle TypeScript schema). Then run `make db-generate && make db-migrate` to auto-generate and apply the migration. **Never manually edit the generated SQL migration files** — Drizzle's hash-based migration tracking depends on them being unmodified, and manual edits cause silent migration failures. The only exception is `make db-push` for lightweight rename/enum-addition syncs (but it does NOT detect new CHECK constraints or indexes).
- **`drizzle-kit generate --custom` workaround (TTY-less environments):** Drizzle Kit 0.31's interactive `generate` prompts for column renames require a TTY — non-interactive shells (CI, piped stdin, agents without a PTY) error out with "Interactive prompts require a TTY terminal". The non-interactive pivot is `drizzle-kit generate --custom --name=<tag>` which creates an empty SQL migration shell + a snapshot that is an EXACT COPY of the previous snapshot (NOT regenerated from the current schema). After writing the SQL by hand (per the design's reference template), you MUST also **manually update `meta/<NNNN>_snapshot.json`** to reflect EVERY schema change introduced by the migration — including but not limited to: renamed columns (both the JSON object key AND the `name` value inside), new enum values (under the `enums` section — Drizzle's snapshot stores them as a `values` array), new/dropped columns, new indexes, new constraints. After editing, **run `make db-generate` again and assert it outputs "No schema changes, nothing to migrate 😴"** — if it produces a new migration file, your snapshot is missing a change the schema declares; diff the new migration to find the gap, fold it into the snapshot, delete the stray migration + its snapshot + journal entry, and re-run until clean. CI runs `make db-generate` as a freshness check; a snapshot that doesn't match the schema fails the build. The hand-written SQL in the `.sql` file is NOT cross-checked against the snapshot — both must be authored to agree.
- **Seed idempotency:** `packages/db/src/seed.ts` must be safe to run repeatedly. All seed helpers that insert into tables with unique constraints MUST use `onConflictDoNothing({ target: [...] })` keyed on those constraints, or select-and-reuse existing rows for tables without usable unique keys. This lets `make db-seed` recover when containers are recreated but the Postgres named volume still holds previous seed data. The seed script entrypoint MUST be guarded with `if (import.meta.main)` so the file can be imported by tests without executing the full seed.
- **Full DB reset:** `make db-reset` drops and recreates the database, pushes the schema fresh, re-seeds, and flushes the Deno KV cache.

Expand Down
108 changes: 78 additions & 30 deletions apps/api/src/modules/comment/service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -663,22 +663,25 @@ describe('deleteComment — edge cases', () => {
//
// Exercises the REAL runCommentNotificationSideEffects exported by service.ts
// via the service-layer `deps` proxy (same idiom as notification/service.test.ts):
// model lookups and the two notification functions are replaced with in-memory
// recorders and restored after each test. Verifies:
// - notifyRecipeCommented remains gated on recipe.authorId !== userId
// model lookups and the notification functions are replaced with in-memory
// recorders and restored after each test. As of the comment/notification
// ownership refactor, createCommentNotification is the SOLE owner of both the
// in-app comment record AND the recipe-commented email; notifyRecipeCommented
// is no longer called from the comment service. Verifies:
// - createCommentNotification is gated on recipe.authorId !== userId
// - the mention scan ALWAYS runs (even when the commenter IS the recipe
// author) on the effective content, forwarding recipeAuthorId
// - each notification call has independent error handling: a rejection in
// one never prevents the other from running
// ---------------------------------------------------------------------------

type ModelDeps = typeof deps.model;
type NotifyRecipeCommentedParams = Parameters<typeof deps.notifyRecipeCommented>[0];
type MentionNotificationParams = Parameters<typeof deps.createMentionNotifications>[0];
type CommentNotificationParams = Parameters<typeof deps.createCommentNotification>[0];

const originalModel = deps.model;
const originalNotifyRecipeCommented = deps.notifyRecipeCommented;
const originalCreateMentionNotifications = deps.createMentionNotifications;
const originalCreateCommentNotification = deps.createCommentNotification;

describe('createComment — notification side-effects (F04 mention flow)', () => {
const recipe = {
Expand All @@ -688,8 +691,8 @@ describe('createComment — notification side-effects (F04 mention flow)', () =>
authorId: 'author-1',
};

let notifyCalls: NotifyRecipeCommentedParams[];
let mentionCalls: MentionNotificationParams[];
let commentCalls: CommentNotificationParams[];

/** Replace deps with in-memory recorders; pass null to simulate a missing recipe. */
function stubDeps(recipeResult: typeof recipe | null) {
Expand All @@ -702,25 +705,25 @@ describe('createComment — notification side-effects (F04 mention flow)', () =>
getCommenterById: (userId: string) => Promise.resolve({ id: userId, username: 'commenter' }),
};
deps.model = modelStub;
deps.notifyRecipeCommented = (params) => {
notifyCalls.push(params);
return Promise.resolve();
};
deps.createMentionNotifications = (params) => {
mentionCalls.push(params);
return Promise.resolve();
};
deps.createCommentNotification = (params) => {
commentCalls.push(params);
return Promise.resolve();
};
}

beforeEach(() => {
notifyCalls = [];
mentionCalls = [];
commentCalls = [];
});

afterEach(() => {
deps.model = originalModel;
deps.notifyRecipeCommented = originalNotifyRecipeCommented;
deps.createMentionNotifications = originalCreateMentionNotifications;
deps.createCommentNotification = originalCreateCommentNotification;
});

it('sends recipe-commented AND triggers the mention flow when the commenter is not the author', async () => {
Expand All @@ -731,8 +734,8 @@ describe('createComment — notification side-effects (F04 mention flow)', () =>
commentId: 'comment-1',
effectiveContent: 'Nice one @alice and @bob-2!',
});
expect(notifyCalls.length).toBe(1);
expect(notifyCalls[0].recipeAuthorId).toBe('author-1');
expect(commentCalls.length).toBe(1);
expect(commentCalls[0].recipeAuthorId).toBe('author-1');
expect(mentionCalls.length).toBe(1);
expect(mentionCalls[0].mentions).toEqual(['alice', 'bob-2']);
expect(mentionCalls[0].commentId).toBe('comment-1');
Expand All @@ -749,7 +752,7 @@ describe('createComment — notification side-effects (F04 mention flow)', () =>
commentId: 'comment-2',
effectiveContent: 'Thanks @alice for the tip',
});
expect(notifyCalls.length).toBe(0); // gated: author commenting on own recipe
expect(commentCalls.length).toBe(0); // gated: author commenting on own recipe
expect(mentionCalls.length).toBe(1); // mention scan must NOT be skipped
expect(mentionCalls[0].mentions).toEqual(['alice']);
expect(mentionCalls[0].recipeAuthorId).toBe('author-1');
Expand All @@ -775,7 +778,7 @@ describe('createComment — notification side-effects (F04 mention flow)', () =>
commentId: 'comment-4',
effectiveContent: 'no mentions here',
});
expect(notifyCalls.length).toBe(1);
expect(commentCalls.length).toBe(1);
expect(mentionCalls.length).toBe(1);
expect(mentionCalls[0].mentions).toEqual([]);
});
Expand All @@ -788,13 +791,13 @@ describe('createComment — notification side-effects (F04 mention flow)', () =>
commentId: 'comment-5',
effectiveContent: 'hello @alice',
});
expect(notifyCalls.length).toBe(0);
expect(commentCalls.length).toBe(0);
expect(mentionCalls.length).toBe(0);
});

it('still runs the mention flow when notifyRecipeCommented rejects', async () => {
it('still runs the mention flow when createCommentNotification rejects', async () => {
stubDeps(recipe);
deps.notifyRecipeCommented = () => Promise.reject(new Error('smtp down'));
deps.createCommentNotification = () => Promise.reject(new Error('db down'));
await runCommentNotificationSideEffects({
userId: 'commenter-1',
recipeId: 'recipe-1',
Expand All @@ -814,8 +817,62 @@ describe('createComment — notification side-effects (F04 mention flow)', () =>
commentId: 'comment-7',
effectiveContent: 'cc @alice',
});
expect(notifyCalls.length).toBe(1);
expect(notifyCalls[0].recipeAuthorId).toBe('author-1');
expect(commentCalls.length).toBe(1);
expect(commentCalls[0].recipeAuthorId).toBe('author-1');
});

// F05 — single-recipient comment fan-out (createCommentNotification) is
// gated on commenter != recipe author. The F04 mention flow is independent
// of that gate and must keep firing for @mentioned users regardless of the
// commenter/author relationship (regression guard).

it('createCommentNotification invoked when commenter != recipe author', async () => {
stubDeps(recipe);
await runCommentNotificationSideEffects({
userId: 'commenter-1',
recipeId: 'recipe-1',
commentId: 'comment-8',
effectiveContent: 'nice recipe',
});
expect(commentCalls.length).toBe(1);
expect(commentCalls[0]).toEqual({
commenterId: 'commenter-1',
commenterUsername: 'commenter',
recipeAuthorId: 'author-1',
recipeId: 'recipe-1',
recipeSlug: 'recipe-slug',
recipeTitle: 'Recipe Title',
commentId: 'comment-8',
});
});

it('createCommentNotification NOT invoked when commenter === recipe author (self-comment)', async () => {
stubDeps(recipe);
await runCommentNotificationSideEffects({
userId: 'author-1',
recipeId: 'recipe-1',
commentId: 'comment-9',
effectiveContent: 'thanks for the tips',
});
expect(commentCalls.length).toBe(0);
// The mention flow (F04) is NOT gated on the commenter/author relationship.
expect(mentionCalls.length).toBe(1);
});

it('createMentionNotifications STILL fires for @mentioned users (F04 regression)', async () => {
stubDeps(recipe);
await runCommentNotificationSideEffects({
userId: 'commenter-1',
recipeId: 'recipe-1',
commentId: 'comment-10',
effectiveContent: 'cc @alice agree',
});
// F05 comment fan-out fires (non-author commenter) ...
expect(commentCalls.length).toBe(1);
// ... AND the F04 mention fan-out still fires for the @mentioned user.
expect(mentionCalls.length).toBe(1);
expect(mentionCalls[0].mentions).toEqual(['alice']);
expect(mentionCalls[0].recipeAuthorId).toBe('author-1');
});
});

Expand All @@ -839,7 +896,6 @@ type GateRecipeModelDeps = typeof deps.recipeModel;

const gateOriginalModel = deps.model;
const gateOriginalRecipeModel = deps.recipeModel;
const gateOriginalNotify = deps.notifyRecipeCommented;
const gateOriginalMentions = deps.createMentionNotifications;
const gateOriginalBadges = deps.evaluateBadges;

Expand All @@ -857,7 +913,6 @@ describe('D99.9 — comment visibility gate', () => {
let createCalls: unknown[];
let incrementCalls: unknown[];
let findByRecipeCalls: unknown[];
let notifyCalls: unknown[];
let mentionCalls: unknown[];

/** Replace deps with in-memory recorders over `fixture` (null = missing recipe). */
Expand Down Expand Up @@ -885,10 +940,6 @@ describe('D99.9 — comment visibility gate', () => {
return Promise.resolve();
},
} as GateRecipeModelDeps;
deps.notifyRecipeCommented = (params) => {
notifyCalls.push(params);
return Promise.resolve();
};
deps.createMentionNotifications = (params) => {
mentionCalls.push(params);
return Promise.resolve();
Expand All @@ -905,22 +956,19 @@ describe('D99.9 — comment visibility gate', () => {
createCalls = [];
incrementCalls = [];
findByRecipeCalls = [];
notifyCalls = [];
mentionCalls = [];
});

afterEach(() => {
deps.model = gateOriginalModel;
deps.recipeModel = gateOriginalRecipeModel;
deps.notifyRecipeCommented = gateOriginalNotify;
deps.createMentionNotifications = gateOriginalMentions;
deps.evaluateBadges = gateOriginalBadges;
});

function expectZeroSideEffects() {
expect(createCalls.length).toBe(0);
expect(incrementCalls.length).toBe(0);
expect(notifyCalls.length).toBe(0);
expect(mentionCalls.length).toBe(0);
}

Expand Down
36 changes: 20 additions & 16 deletions apps/api/src/modules/comment/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ import * as model from './model.ts';
import * as recipeModel from '../recipe/model.ts';
import { canViewRecipe } from '../recipe/service.ts';
import { createLogger } from '../../utils/logger/index.ts';
import { notifyRecipeCommented } from '../../utils/notify/index.ts';
import { createMentionNotifications } from '../notification/service.ts';
import { createCommentNotification, createMentionNotifications } from '../notification/service.ts';
import { evaluateBadges } from '../badge/service.ts';

const logger = createLogger('comment-service');
Expand All @@ -26,8 +25,8 @@ const logger = createLogger('comment-service');
export const deps = {
model,
recipeModel,
notifyRecipeCommented,
createMentionNotifications,
createCommentNotification,
evaluateBadges,
};

Expand Down Expand Up @@ -128,8 +127,11 @@ export async function createComment(
* Flow:
* 1. Load the recipe (early return when it cannot be found).
* 2. Load the commenter (early return when no username is available).
* 3. Send the recipe-commented email ONLY when the commenter is not the
* recipe author (`recipe.authorId !== userId`).
* 3. `createCommentNotification` owns BOTH the in-app `comment` record AND
* the recipe-commented email — gated on `notifyRecipeCommented` prefs
* (single flag gates both). It self-skips when the commenter IS the
* recipe author, so the `recipe.authorId !== userId` guard here only
* avoids the call; the creator's own self-check is belt-and-braces.
* 4. ALWAYS run the mention flow — including when the commenter IS the
* recipe author. Mentions are parsed from the effective (post-prepend,
* sanitized) content; createMentionNotifications no-ops on empty lists.
Expand Down Expand Up @@ -157,18 +159,20 @@ export async function runCommentNotificationSideEffects(params: {
const commenter = await deps.model.getCommenterById(userId);
if (!commenter?.username) return;

// Recipe-commented email only when someone ELSE comments on the recipe.
// F05: createCommentNotification owns BOTH the in-app `comment` record AND
// the recipe-commented email (gated on `notifyRecipeCommented` prefs —
// single flag gates both). No direct notifyRecipeCommented call here —
// that would double-send and bypass the preference gate.
if (recipe.authorId !== userId) {
try {
await deps.notifyRecipeCommented({
recipeAuthorId: recipe.authorId,
commenterUsername: commenter.username,
recipeTitle: recipe.title,
recipeSlug: recipe.slug,
});
} catch (err) {
logger.error({ err, commentId }, 'notifyRecipeCommented failed');
}
deps.createCommentNotification({
commenterId: userId,
commenterUsername: commenter.username,
recipeAuthorId: recipe.authorId,
recipeId: recipe.id,
recipeSlug: recipe.slug,
recipeTitle: recipe.title,
commentId,
}).catch((err) => logger.error({ err, commentId }, 'createCommentNotification failed'));
}

// Mention notifications always run — including when the commenter IS the
Expand Down
Loading