feat: create a slack post when a video is created or updated#9078
Merged
tanflem merged 36 commits intoMay 18, 2026
Merged
Conversation
…create-a-slack-post-when-a-video-is-created-or-updated
|
Ran Plan for dir: Plan Error |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Slack notifications and weekly summaries to api-media: immediate video mutation alerts and weekly per-video and bulk Slack reports, plus config/helpers, report loaders/renderers, worker scheduling and CLI runners, tests, and infra/Nx entries for env vars and run targets. Changes
Sequence Diagram(s)sequenceDiagram
participant GraphQL as GraphQL Resolver
participant Notifier as notifyVideoSlackOfMutation
participant Config as getMediaDataLangSlackConfig
participant Msg as slackChatPostMessage
participant SlackAPI as Slack API
GraphQL->>Notifier: call with video, user, kind
Notifier->>Config: load token & channel from env
Config-->>Notifier: config | null
alt Config available
Notifier->>Msg: build blocks & POST payload
Msg->>SlackAPI: POST https://slack.com/api/chat.postMessage
SlackAPI-->>Msg: JSON response (ok / ts / error)
Msg-->>Notifier: ts | undefined
else Config missing
Notifier-->>GraphQL: log warning & return
end
sequenceDiagram
participant Worker as Worker/CLI
participant Summary as sendWeeklyVideoSummary
participant Report as loadWeeklyVideoReport
participant PrismaMedia as Prisma Media
participant PrismaLang as Prisma Languages
participant Renderer as postWeeklyVideoSlackMessages
participant SlackAPI as Slack API
Worker->>Summary: invoke with date window
Summary->>Report: load rows & counts
Report->>PrismaMedia: query videos/variants/updates
PrismaMedia-->>Report: rows
Report->>PrismaLang: fetch language names
PrismaLang-->>Report: name mappings
Report-->>Summary: rows & counts
alt has changes
Summary->>Renderer: format messages (group by month, chunk)
Renderer->>SlackAPI: POST message(s) sequentially
SlackAPI-->>Renderer: ts responses
alt a post fails (no ts)
Renderer-->>Summary: abort further posting
end
else no changes
Summary-->>Worker: log info & exit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
View your CI Pipeline Execution ↗ for commit 314b36e
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
apis/api-media/src/schema/video/video.spec.ts (1)
2591-2596: Consider also asserting theuserpayload passed tonotifyVideoSlackOfMutation.The resolvers forward
context.userinto the notification call, but neither assertion verifies that the user is propagated. Not critical — the tests pass as-is — but addinguser: expect.anything()(or an explicit shape) would guard against regressions where the context is dropped from the resolver signature.Also applies to: 2801-2806
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/schema/video/video.spec.ts` around lines 2591 - 2596, Update the test assertions that mock notifyVideoSlackOfMutation to also assert the forwarded user payload: when calling expect(notifyVideoSlackOfMutation).toHaveBeenCalledWith(...), include a user property in the objectContaining (e.g., user: expect.anything() or a precise shape like expect.objectContaining({ id: ..., email: ... })) so the test verifies the resolver forwards context.user; update both occurrences that assert notifyVideoSlackOfMutation (the one around the create assertion and the similar one near the other block referenced) to include this user check.apis/api-media/src/lib/videoSlack.ts (1)
22-59: Short-circuit on missing Slack config before running the report queries.
loadWeeklyVideoReportissues three Prisma queries plus a language-name fetch. When Slack env vars are missing, the config check only happens insidepostWeeklyVideoSlackMessages(videoSlackRenderer.tsline 197-200), so every scheduled weekly run in an environment without the Slack secrets still hits the DB just to discard the result. Moving thegetMediaDataLangSlackConfigcheck to the top ofsendWeeklyVideoSummary(in addition to the one inside the renderer) avoids wasted query work and keeps parity withnotifyVideoSlackOfMutation, which also guards early.♻️ Sketch
+import { getMediaDataLangSlackConfig } from './slack/config' ... const childLogger = currentLogger.child({ report: 'weekly-video-summary' }) + if (getMediaDataLangSlackConfig(childLogger) == null) { + return + } const { startDate, endDate } = resolveWeeklyVideoSummaryWindow({ ... })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/lib/videoSlack.ts` around lines 22 - 59, Move the Slack config guard to the top of sendWeeklyVideoSummary so we short-circuit before running DB work: call getMediaDataLangSlackConfig (the same config used by postWeeklyVideoSlackMessages/videoSlackRenderer) at the start of sendWeeklyVideoSummary and return early when config is missing, in addition to keeping the existing check inside postWeeklyVideoSlackMessages; this prevents loadWeeklyVideoReport from executing Prisma queries when Slack env vars are not present and mirrors notifyVideoSlackOfMutation's early-guard behavior.apis/api-media/src/scripts/run-weekly-video-slack-summary.ts (1)
31-58: Argument parsing silently accepts missing flag values.When
--startor--endis the last argv token (or--start=with empty value),argv[i + 1]isundefinedand gets assigned toparsed.startIso/parsed.endIso. DownstreamparseIsothen throwsInvalid start datewith an ISO example, which misleads a user who actually forgot to supply the value. Consider validating that the next token exists and isn't another flag, and emitting a clearer "--startrequires a value" error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/scripts/run-weekly-video-slack-summary.ts` around lines 31 - 58, The parseCliArgs function silently accepts missing or empty values for the --start/--end flags (assigning undefined or empty strings to parsed.startIso/parsed.endIso); update parseCliArgs to validate that when encountering '--start', '--start=' (and similarly '--end', '--end=') the value exists and is not another flag or empty, and throw a clear error like "`--start` requires a value" or "`--end` requires a value" instead of allowing downstream parseIso to fail; reference the parseCliArgs function and the parsed.startIso/parsed.endIso assignments when adding these checks and error messages.apis/api-media/src/lib/slack/chatPostMessage.ts (1)
55-57: Use Pino's conventionalerrkey for caught errors.Pino's default error serializer is keyed on
err, noterror. Logging{ error }bypasses the serializer, so stack traces and cause chains don't get expanded in structured output.Changes needed in two locations:
apis/api-media/src/lib/slack/chatPostMessage.tsline 56: Changelog.warn({ error }, errorMessage)tolog.warn({ err: error }, errorMessage)apis/api-media/src/lib/videoSlack.tslines 69-71: Change{ error }to{ err: error }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/lib/slack/chatPostMessage.ts` around lines 55 - 57, Replace the logged error property to use Pino's standard "err" key so the error serializer expands stacks: in chatPostMessage.ts update the catch block's log.warn call from logging { error } to { err: error } (the catch in function chatPostMessage where log.warn({ error }, errorMessage) is used), and similarly in videoSlack.ts change the log invocation that currently passes { error } to pass { err: error } instead; keep the same log message string and return behavior unchanged.apis/api-media/src/lib/videoSlackReport.ts (4)
441-445: Redundant type guard —videoIdis alreadystring.
UpdatedVariantRow.videoIdis typed asstring(and the Prisma schema declares it non-nullable), so thetypeof id === 'string' && id.length > 0filter is unreachable noise. A plainnew Set(updatedVariants.map(v => v.videoId))is sufficient — and if empty-string defense is intentionally wanted, keep only the length check with a comment.♻️ Proposed simplification
- const videoIdsWithVariantUpdates = new Set( - updatedVariants - .map((variant) => variant.videoId) - .filter((id): id is string => typeof id === 'string' && id.length > 0) - ) + const videoIdsWithVariantUpdates = new Set( + updatedVariants.map((variant) => variant.videoId) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/lib/videoSlackReport.ts` around lines 441 - 445, The filter using typeof id === 'string' is redundant because UpdatedVariantRow.videoId is already a non-nullable string; simplify the logic that builds videoIdsWithVariantUpdates by removing the type check and using new Set(updatedVariants.map(v => v.videoId)), or if you want to guard against empty strings only, replace the current filter with id.length > 0 and add a comment explaining why empty-string defense is needed; update the code around the videoIdsWithVariantUpdates declaration and references to updatedVariants/videoId accordingly.
267-349: Add a brief docstring explaining "package totals".This function encodes non-obvious domain logic (parent = series/featureFilm, package = parent + episode/segment children, "total" = number of package members having a variant in the row's language, with a floor of 1). A short doc comment at the signature would save future readers a lot of reverse-engineering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/lib/videoSlackReport.ts` around lines 267 - 349, Add a concise docstring to getLanguagePackageTotals describing its domain logic: explain that "parent" refers to series/featureFilm identified by packageParentLabels, "package" includes the parent plus children matching packageChildLabels, and "total" is the count of package members that have a VideoVariant for the row's language (with a minimum of 1). Place this comment above the getLanguagePackageTotals signature and mention key inputs/outputs (rowSeeds: ReportRowSeed[], returns Map keyed by `${mediaComponentId}:${languageId}` mapping to the package total) and note reliance on packageMembersByParentId, variantVideoIdsByLanguage, packageParentLabels and packageChildLabels so future readers understand the computation without reversing the code.
193-207: PreferPrisma.VideoWhereInputover hand-rolled where shape.Hand-typing the
whereshape duplicates Prisma's generated types and will drift if the schema/filters evolve. UsingPrisma.VideoWhereInputaligns with the existing pattern in this module (e.g.,apis/api-media/src/schema/video/video.ts) and keeps IDE autocomplete working.♻️ Proposed refactor
-import { prisma } from '@core/prisma/media/client' +import { Prisma, prisma } from '@core/prisma/media/client' @@ - const where: { - label: { not: 'collection' } - createdAt: { lt: Date } - updatedAt: { gte: Date; lte: Date } - id?: { notIn: string[] } - } = { + const where: Prisma.VideoWhereInput = { label: { not: 'collection' }, createdAt: { lt: startDate }, updatedAt: { gte: startDate, lte: endDate } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/lib/videoSlackReport.ts` around lines 193 - 207, The manually declared type for the where object should be replaced with Prisma.VideoWhereInput to avoid drifting from the generated schema: change the type of the where constant (variable named where in videoSlackReport.ts) to Prisma.VideoWhereInput, ensure the same shape (label.not = 'collection', createdAt.lt = startDate, updatedAt.gte = startDate, updatedAt.lte = endDate, and optional id.notIn array) is preserved, and add an import for Prisma from '@prisma/client' if it isn’t already imported so the type resolves correctly.
288-311: Push child-label filter into Prisma query.Children are fetched without a label filter and then filtered in JS at line 304. For series/featureFilm parents with many unrelated children, this ships extra rows across the wire every run. A Prisma
whereon the nestedchildrenrelation keeps the filter at the DB.♻️ Proposed refactor
const parentVideos = await prisma.video.findMany({ where: { id: { in: parentIds } }, select: { id: true, children: { + where: { label: { in: [...packageChildLabels] as any } }, select: { id: true, label: true } } } })Note:
as anyis only needed ifpackageChildLabelsstays aSet<string>. Prefer typing it asSet<VideoLabel>so the Prisma call stays fully typed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/lib/videoSlackReport.ts` around lines 288 - 311, The parentVideos query is fetching all children then filtering in JS (lines around parentVideos, packageChildLabels), which sends unnecessary rows; modify the Prisma query that builds parentVideos to push the child-label filter into the nested children select (use a where: { label: { in: Array.from(packageChildLabels) } } on the children relation) so only matching children are returned, then keep the existing logic that builds packageMembersByParentId and packageVideoIds from parent.id and returned children; also change packageChildLabels to be typed as Set<VideoLabel> (instead of Set<string>) to avoid needing any casts so the Prisma call remains fully typed.apis/api-media/src/lib/slack/videoMutationNotification.spec.ts (1)
1-159: Coverage looks solid; consider a fetch-rejection case.The suite covers success, env-gating, and Slack error responses well. One gap worth considering: a
fetchrejection (network error) path to confirm the module still logs instead of surfacing an unhandled promise rejection — important givennotifyVideoSlackOfMutationis invoked fire-and-forget.🧪 Example test
+ it('logs a warn when fetch rejects (network error)', async () => { + mockFetch.mockRejectedValueOnce(new Error('network down')) + + notifyVideoSlackOfMutation({ + kind: 'create', + video: { id: 'video-6', label: 'segment' }, + user: { id: 'user-6', email: 'editor@example.com' } + }) + + await flushAsync() + + expect(mockLoggerWarn).toHaveBeenCalled() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/lib/slack/videoMutationNotification.spec.ts` around lines 1 - 159, Add a new test that simulates a network error by having mockFetch.mockRejectedValueOnce(new Error('network error')), then call notifyVideoSlackOfMutation with a sample payload and await flushAsync; assert that mockFetch was attempted and that mockLoggerWarn was called with an error object (or expect.any(Error)) and the same 'Video mutation Slack notification failed' message—use the existing mockFetch, mockLoggerWarn, notifyVideoSlackOfMutation, and flushAsync identifiers to locate and implement the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apis/api-media/src/lib/slack/chatPostMessage.ts`:
- Around line 31-58: The Slack POST call in chatPostMessage.ts has no timeout
causing hung promises; update the fetch(slackApiUrl, {...}) call inside the try
block to include a node-fetch v2 compatible timeout option (e.g., timeout:
10000) so the request fails fast on network stalls, and adjust any
typing/payload handling as needed; also change the catch logging to use the Pino
idiom by passing the error as { err: error } (or rename the caught variable to
err and use { err }) when calling log.warn(errorMessage) so error serialization
is correct.
In `@apis/api-media/src/lib/slack/videoMutationNotification.ts`:
- Around line 79-94: formatUser currently returns raw PII (email or full name)
into Slack posts; update formatUser to avoid leaking editor PII unless the
channel is explicitly approved: change the function (formatUser) to either
return a masked email (e.g., keep local-part initial and replace remainder with
ellipsis like "a…@domain" or "a.…@domain"), or return first-initial+surname, or
simply user.id based on a new flag or environment/config setting (e.g.,
SLACK_INCLUDE_PII) so callers can opt-in when channel retention/audience is
approved; ensure the function still handles null/empty values the same and
document the config toggle so default behavior is non-PII (return masked
identifier or user.id).
In `@apis/api-media/src/lib/videoSlack.spec.ts`:
- Around line 11-34: Replace the hand-rolled jest.fn() Prisma mocks with deep
typed mocks from jest-mock-extended: create mockDeep<PrismaClient>() instances
and return those from the jest.mock calls instead of the current objects that
only stub prisma.language.findMany, prisma.video.findMany and
prisma.videoVariant.findMany; update the test to use the deep-mock instances (so
new model/method calls like prisma.video.count are auto-stubbed) and remove the
need for casting to any, ensuring the mocks maintain Prisma types and can be
configured via .language.findMany.mockResolvedValue(...) /
.video.findMany.mockResolvedValue(...) as needed.
In `@apis/api-media/src/lib/videoSlack.ts`:
- Around line 61-76: The try/catch only wraps postWeeklyVideoSlackMessages so
failures in resolveWeeklyVideoSummaryWindow or loadWeeklyVideoReport escape;
modify sendWeeklyVideoSummary so the try begins before calling
resolveWeeklyVideoSummaryWindow and loadWeeklyVideoReport (or wrap the entire
function body) to catch any errors from resolveWeeklyVideoSummaryWindow,
loadWeeklyVideoReport, and postWeeklyVideoSlackMessages, then log the error via
childLogger.warn (including the error object) and return gracefully instead of
letting the exception propagate to the worker; reference sendWeeklyVideoSummary,
resolveWeeklyVideoSummaryWindow, loadWeeklyVideoReport, and
postWeeklyVideoSlackMessages when making the change.
In `@apis/api-media/src/lib/videoSlackReport.ts`:
- Around line 332-346: The code in the loop that computes totals (variables:
totals, eligibleRows, packageMembersByParentId, variantVideoIdsByLanguage)
forces a minimum of 1 when a language has no variants by setting
countForLanguage to 1 and using Math.max(1, …), which over-reports; change the
logic so that when variantVideoIdsByLanguage.get(row.languageId) is undefined
you set countForLanguage = 0 (not 1) and remove the unconditional Math.max(1, …)
so totals.set(`${row.mediaComponentId}:${row.languageId}`, countForLanguage) can
be zero, or if you do intend to count the production as at least one, keep the
Math.max but add an explanatory comment near this block referencing
countForLanguage and the packageMembers logic to prevent future regressions.
---
Nitpick comments:
In `@apis/api-media/src/lib/slack/chatPostMessage.ts`:
- Around line 55-57: Replace the logged error property to use Pino's standard
"err" key so the error serializer expands stacks: in chatPostMessage.ts update
the catch block's log.warn call from logging { error } to { err: error } (the
catch in function chatPostMessage where log.warn({ error }, errorMessage) is
used), and similarly in videoSlack.ts change the log invocation that currently
passes { error } to pass { err: error } instead; keep the same log message
string and return behavior unchanged.
In `@apis/api-media/src/lib/slack/videoMutationNotification.spec.ts`:
- Around line 1-159: Add a new test that simulates a network error by having
mockFetch.mockRejectedValueOnce(new Error('network error')), then call
notifyVideoSlackOfMutation with a sample payload and await flushAsync; assert
that mockFetch was attempted and that mockLoggerWarn was called with an error
object (or expect.any(Error)) and the same 'Video mutation Slack notification
failed' message—use the existing mockFetch, mockLoggerWarn,
notifyVideoSlackOfMutation, and flushAsync identifiers to locate and implement
the test.
In `@apis/api-media/src/lib/videoSlack.ts`:
- Around line 22-59: Move the Slack config guard to the top of
sendWeeklyVideoSummary so we short-circuit before running DB work: call
getMediaDataLangSlackConfig (the same config used by
postWeeklyVideoSlackMessages/videoSlackRenderer) at the start of
sendWeeklyVideoSummary and return early when config is missing, in addition to
keeping the existing check inside postWeeklyVideoSlackMessages; this prevents
loadWeeklyVideoReport from executing Prisma queries when Slack env vars are not
present and mirrors notifyVideoSlackOfMutation's early-guard behavior.
In `@apis/api-media/src/lib/videoSlackReport.ts`:
- Around line 441-445: The filter using typeof id === 'string' is redundant
because UpdatedVariantRow.videoId is already a non-nullable string; simplify the
logic that builds videoIdsWithVariantUpdates by removing the type check and
using new Set(updatedVariants.map(v => v.videoId)), or if you want to guard
against empty strings only, replace the current filter with id.length > 0 and
add a comment explaining why empty-string defense is needed; update the code
around the videoIdsWithVariantUpdates declaration and references to
updatedVariants/videoId accordingly.
- Around line 267-349: Add a concise docstring to getLanguagePackageTotals
describing its domain logic: explain that "parent" refers to series/featureFilm
identified by packageParentLabels, "package" includes the parent plus children
matching packageChildLabels, and "total" is the count of package members that
have a VideoVariant for the row's language (with a minimum of 1). Place this
comment above the getLanguagePackageTotals signature and mention key
inputs/outputs (rowSeeds: ReportRowSeed[], returns Map keyed by
`${mediaComponentId}:${languageId}` mapping to the package total) and note
reliance on packageMembersByParentId, variantVideoIdsByLanguage,
packageParentLabels and packageChildLabels so future readers understand the
computation without reversing the code.
- Around line 193-207: The manually declared type for the where object should be
replaced with Prisma.VideoWhereInput to avoid drifting from the generated
schema: change the type of the where constant (variable named where in
videoSlackReport.ts) to Prisma.VideoWhereInput, ensure the same shape (label.not
= 'collection', createdAt.lt = startDate, updatedAt.gte = startDate,
updatedAt.lte = endDate, and optional id.notIn array) is preserved, and add an
import for Prisma from '@prisma/client' if it isn’t already imported so the type
resolves correctly.
- Around line 288-311: The parentVideos query is fetching all children then
filtering in JS (lines around parentVideos, packageChildLabels), which sends
unnecessary rows; modify the Prisma query that builds parentVideos to push the
child-label filter into the nested children select (use a where: { label: { in:
Array.from(packageChildLabels) } } on the children relation) so only matching
children are returned, then keep the existing logic that builds
packageMembersByParentId and packageVideoIds from parent.id and returned
children; also change packageChildLabels to be typed as Set<VideoLabel> (instead
of Set<string>) to avoid needing any casts so the Prisma call remains fully
typed.
In `@apis/api-media/src/schema/video/video.spec.ts`:
- Around line 2591-2596: Update the test assertions that mock
notifyVideoSlackOfMutation to also assert the forwarded user payload: when
calling expect(notifyVideoSlackOfMutation).toHaveBeenCalledWith(...), include a
user property in the objectContaining (e.g., user: expect.anything() or a
precise shape like expect.objectContaining({ id: ..., email: ... })) so the test
verifies the resolver forwards context.user; update both occurrences that assert
notifyVideoSlackOfMutation (the one around the create assertion and the similar
one near the other block referenced) to include this user check.
In `@apis/api-media/src/scripts/run-weekly-video-slack-summary.ts`:
- Around line 31-58: The parseCliArgs function silently accepts missing or empty
values for the --start/--end flags (assigning undefined or empty strings to
parsed.startIso/parsed.endIso); update parseCliArgs to validate that when
encountering '--start', '--start=' (and similarly '--end', '--end=') the value
exists and is not another flag or empty, and throw a clear error like "`--start`
requires a value" or "`--end` requires a value" instead of allowing downstream
parseIso to fail; reference the parseCliArgs function and the
parsed.startIso/parsed.endIso assignments when adding these checks and error
messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 74788101-07db-4628-85ed-3358cdb8daa8
📒 Files selected for processing (18)
apis/api-media/infrastructure/locals.tfapis/api-media/project.jsonapis/api-media/src/lib/slack/chatPostMessage.tsapis/api-media/src/lib/slack/config.tsapis/api-media/src/lib/slack/index.tsapis/api-media/src/lib/slack/videoMutationNotification.spec.tsapis/api-media/src/lib/slack/videoMutationNotification.tsapis/api-media/src/lib/videoSlack.spec.tsapis/api-media/src/lib/videoSlack.tsapis/api-media/src/lib/videoSlackRenderer.tsapis/api-media/src/lib/videoSlackReport.tsapis/api-media/src/schema/video/video.spec.tsapis/api-media/src/schema/video/video.tsapis/api-media/src/scripts/run-weekly-video-slack-summary.tsapis/api-media/src/workers/server.tsapis/api-media/src/workers/videoSlackSummary/config.tsapis/api-media/src/workers/videoSlackSummary/index.tsapis/api-media/src/workers/videoSlackSummary/service/service.ts
…n-a-video-is-created-or-updated
|
Ran Plan for dir: Plan Error |
…s-created-or-updated' of https://github.com/JesusFilm/core into tannerfleming/vmt-38-create-a-slack-post-when-a-video-is-created-or-updated
|
Ran Plan for dir: Plan Error |
|
Ran Plan for dir: Plan Error |
|
Ran Plan for dir: Plan Error |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apis/api-media/src/lib/videoSlackBulkReport.ts (2)
48-87: DuplicatedloadLanguageNamesfunction.This function is nearly identical to the one in
videoSlackReport.ts(lines 224-264). Consider extracting it to a shared utility module to reduce duplication and ensure consistent behavior.♻️ Suggested approach
Create a shared module, e.g.,
apis/api-media/src/lib/languageUtils.ts:import { prisma as languagesPrisma } from '@core/prisma/languages/client' import { ENGLISH_LANGUAGE_ID } from './algolia/languages/languageLists' import { logger } from '../logger' export async function loadLanguageNames( ids: string[], warnContext: string ): Promise<Map<string, string>> { // ... shared implementation }Then import and use in both report files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/lib/videoSlackBulkReport.ts` around lines 48 - 87, Extract the duplicated async function loadLanguageNames into a shared utility (e.g., languageUtils.ts) and export it for reuse; update both callers (loadLanguageNames usages in videoSlackBulkReport.ts and videoSlackReport.ts) to import the shared function, preserve behavior (unique filtering, languagesPrisma.language.findMany, fallback to id, and logger.warn with a contextual message) and consider adding a small parameter like warnContext or caller name so the logger message can remain specific when called from different reports; ensure the exported function signature and return type (Promise<Map<string,string>>) match the existing implementations so callers need only swap the local function call for the shared import.
7-7: Use the existingENGLISH_LANGUAGE_IDconstant instead of hardcoding'529'.The codebase already defines this constant in
apis/api-media/src/lib/algolia/languages/languageLists.ts. Using it improves maintainability and consistency.♻️ Suggested fix
+import { ENGLISH_LANGUAGE_ID } from './algolia/languages/languageLists' import { prisma as languagesPrisma } from '@core/prisma/languages/client' import { prisma } from '@core/prisma/media/client' import { logger } from '../logger' -const englishLanguageIdForNames = '529' +const englishLanguageIdForNames = ENGLISH_LANGUAGE_ID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/lib/videoSlackBulkReport.ts` at line 7, Replace the hardcoded string assigned to englishLanguageIdForNames ('529') with the shared ENGLISH_LANGUAGE_ID constant: import ENGLISH_LANGUAGE_ID from the languages constant module (where it's defined) and use that constant in place of '529' so englishLanguageIdForNames = ENGLISH_LANGUAGE_ID; ensure the import name matches the exported symbol and remove the literal.apis/api-media/src/lib/videoSlackBulkRenderer.ts (1)
15-52: Duplicated utility functions withvideoSlackRenderer.ts.
formatMonthYearUtc,getMonthKey,truncateEnd, andpadCellare duplicated between the bulk and weekly renderers. Consider extracting these to a shared formatting utility.♻️ Suggested approach
Create a shared module, e.g.,
apis/api-media/src/lib/slackFormatUtils.ts:export function formatMonthYearUtc(value: Date): string { ... } export function getMonthKey(value: Date): string { ... } export function truncateEnd(value: string, maxLength: number): string { ... } export function padCell(value: string, width: number): string { ... }Then import in both renderer files to reduce duplication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/lib/videoSlackBulkRenderer.ts` around lines 15 - 52, The four functions formatMonthYearUtc, getMonthKey, truncateEnd, and padCell are duplicated; extract them into a single shared module (e.g., slackFormatUtils) and export each function, then replace the local implementations in videoSlackBulkRenderer and videoSlackRenderer with imports from that module; ensure the signatures and behavior remain identical and update any references to call the imported functions.apis/api-media/src/scripts/run-weekly-video-slack-bulk-summary.ts (1)
100-101: Inconsistent logging: mix ofconsole.log/console.errorand Pino logger.The script uses Pino logger at lines 105-111 but falls back to
console.log/console.errorelsewhere. Per coding guidelines, use Pino logger everywhere for consistent structured logging.♻️ Suggested fix
- console.log( - `Running weekly bulk summary window: ${startDate.toISOString()} -> ${endDate.toISOString()}` - ) + logger.info( + { windowStart: startDate.toISOString(), windowEnd: endDate.toISOString() }, + 'Running weekly bulk summary window' + ) const rows = await loadBulkVideoReport({ startDate, endDate }) ... await postBulkVideoSlackMessages({ rows, startDate, endDate, childLogger: logger }) - console.log('postBulkVideoSlackMessages finished') + logger.info('postBulkVideoSlackMessages finished') } main().catch((error) => { - console.error(error) + logger.error({ error }, 'Weekly bulk summary failed') process.exit(1) })As per coding guidelines: "Use Pino logger everywhere —
nestjs-pinoin legacy, direct Pino in modern APIs, with pretty-print in dev and structured JSON in production".Also applies to: 120-120, 123-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/scripts/run-weekly-video-slack-bulk-summary.ts` around lines 100 - 101, Replace the ad-hoc console.log/console.error calls with the script's Pino logger instance (the same logger used in the block around lines 105-111) so all output is structured; for the startup message use logger.info with the startDate and endDate (e.g., logger.info({ startDate: startDate.toISOString(), endDate: endDate.toISOString() }, "Running weekly bulk summary window")), and convert other console.error uses (including the ones around lines 120 and 123-125) to logger.error, passing the error object as metadata for proper structured logs.apis/api-media/src/lib/videoSlackReport.ts (1)
8-8: Use the existingENGLISH_LANGUAGE_IDconstant instead of hardcoding'529'.Same issue as in
videoSlackBulkReport.ts. Import fromapis/api-media/src/lib/algolia/languages/languageLists.tsfor consistency.♻️ Suggested fix
+import { ENGLISH_LANGUAGE_ID } from './algolia/languages/languageLists' import { prisma as languagesPrisma } from '@core/prisma/languages/client' import { prisma } from '@core/prisma/media/client' import { logger } from '../logger' const oneWeekInDays = 7 -const englishLanguageIdForNames = '529' +const englishLanguageIdForNames = ENGLISH_LANGUAGE_ID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-media/src/lib/videoSlackReport.ts` at line 8, Replace the hardcoded const englishLanguageIdForNames = '529' with the shared ENGLISH_LANGUAGE_ID constant: remove or stop using englishLanguageIdForNames and import ENGLISH_LANGUAGE_ID from the languageLists module (languageLists.ts), then use ENGLISH_LANGUAGE_ID wherever englishLanguageIdForNames was referenced (same change as in videoSlackBulkReport.ts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apis/api-media/src/scripts/run-weekly-video-slack-bulk-summary.ts`:
- Around line 29-56: parseCliArgs currently reads argv[i+1] for the
--start/--end flags without checking bounds, which can yield undefined; update
parseCliArgs to validate that i+1 exists and is not another flag before
assigning parsed.startIso/parsed.endIso (for both the '--start' and '--end'
branches), and if the value is missing or looks like a flag, throw a clear error
or return a validation error indicating the flag is missing a value so
downstream parseIso isn't given undefined.
---
Nitpick comments:
In `@apis/api-media/src/lib/videoSlackBulkRenderer.ts`:
- Around line 15-52: The four functions formatMonthYearUtc, getMonthKey,
truncateEnd, and padCell are duplicated; extract them into a single shared
module (e.g., slackFormatUtils) and export each function, then replace the local
implementations in videoSlackBulkRenderer and videoSlackRenderer with imports
from that module; ensure the signatures and behavior remain identical and update
any references to call the imported functions.
In `@apis/api-media/src/lib/videoSlackBulkReport.ts`:
- Around line 48-87: Extract the duplicated async function loadLanguageNames
into a shared utility (e.g., languageUtils.ts) and export it for reuse; update
both callers (loadLanguageNames usages in videoSlackBulkReport.ts and
videoSlackReport.ts) to import the shared function, preserve behavior (unique
filtering, languagesPrisma.language.findMany, fallback to id, and logger.warn
with a contextual message) and consider adding a small parameter like
warnContext or caller name so the logger message can remain specific when called
from different reports; ensure the exported function signature and return type
(Promise<Map<string,string>>) match the existing implementations so callers need
only swap the local function call for the shared import.
- Line 7: Replace the hardcoded string assigned to englishLanguageIdForNames
('529') with the shared ENGLISH_LANGUAGE_ID constant: import ENGLISH_LANGUAGE_ID
from the languages constant module (where it's defined) and use that constant in
place of '529' so englishLanguageIdForNames = ENGLISH_LANGUAGE_ID; ensure the
import name matches the exported symbol and remove the literal.
In `@apis/api-media/src/lib/videoSlackReport.ts`:
- Line 8: Replace the hardcoded const englishLanguageIdForNames = '529' with the
shared ENGLISH_LANGUAGE_ID constant: remove or stop using
englishLanguageIdForNames and import ENGLISH_LANGUAGE_ID from the languageLists
module (languageLists.ts), then use ENGLISH_LANGUAGE_ID wherever
englishLanguageIdForNames was referenced (same change as in
videoSlackBulkReport.ts).
In `@apis/api-media/src/scripts/run-weekly-video-slack-bulk-summary.ts`:
- Around line 100-101: Replace the ad-hoc console.log/console.error calls with
the script's Pino logger instance (the same logger used in the block around
lines 105-111) so all output is structured; for the startup message use
logger.info with the startDate and endDate (e.g., logger.info({ startDate:
startDate.toISOString(), endDate: endDate.toISOString() }, "Running weekly bulk
summary window")), and convert other console.error uses (including the ones
around lines 120 and 123-125) to logger.error, passing the error object as
metadata for proper structured logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5b844828-0949-436c-a6ff-8e39d0749a1a
📒 Files selected for processing (10)
apis/api-media/project.jsonapis/api-media/src/lib/videoSlack.spec.tsapis/api-media/src/lib/videoSlackBulk.spec.tsapis/api-media/src/lib/videoSlackBulkRenderer.tsapis/api-media/src/lib/videoSlackBulkReport.tsapis/api-media/src/lib/videoSlackRenderer.tsapis/api-media/src/lib/videoSlackReport.tsapis/api-media/src/schema/video/video.spec.tsapis/api-media/src/scripts/run-weekly-video-slack-bulk-summary.tsapis/api-media/src/scripts/run-weekly-video-slack-summary.ts
✅ Files skipped from review due to trivial changes (1)
- apis/api-media/project.json
🚧 Files skipped from review as they are similar to previous changes (2)
- apis/api-media/src/schema/video/video.spec.ts
- apis/api-media/src/scripts/run-weekly-video-slack-summary.ts
…create-a-slack-post-when-a-video-is-created-or-updated
|
Ran Plan for dir: Plan Error |
|
Ran Plan for dir: Plan Error |
…s-created-or-updated' of https://github.com/JesusFilm/core into tannerfleming/vmt-38-create-a-slack-post-when-a-video-is-created-or-updated
|
Ran Plan for dir: Plan Error |
|
Ran Plan for dir: Plan Error |
Removes the per-video create/update Slack notification (called directly from the videoCreate/videoUpdate resolvers) so QA-378 is scoped to the weekly summary only. The per-video work moves to QA-452 on a stacked branch. Shared infra (chatPostMessage, config) stays — the weekly summary uses it.
…Slack/ Mirrors the project's folder convention (cf. lib/algolia/) and distinguishes the video-domain feature code from the generic Slack platform primitives in lib/slack/. - Move videoSlack.ts, videoSlackRenderer.ts, videoSlackReport.ts and the spec into lib/videoSlack/. - Add lib/videoSlack/index.ts barrel. - Update relative imports inside the moved files. - Collapse the script's two imports into one barrel import.
|
Ran Plan for dir: Plan Error |
4 tasks
…n-a-video-is-created-or-updated
|
Ran Plan for dir: Plan Error |
- Add blank line between import groups in videoSlackRenderer.ts - Disable no-unnecessary-type-assertion where the cast is load-bearing for cross-file type inference (prismaMock.ts, video.ts, videoSlack.spec.ts) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Ran Plan for dir: Plan Error |
…create-a-slack-post-when-a-video-is-created-or-updated
|
Ran Plan for dir: Plan Error |
…create-a-slack-post-when-a-video-is-created-or-updated
|
Ran Plan for dir: Plan Error |
|
Ran Plan for dir: Plan Error |
|
Ran Plan for dir: Plan Error |
Kneesal
approved these changes
May 14, 2026
tanflem
deleted the
tannerfleming/vmt-38-create-a-slack-post-when-a-video-is-created-or-updated
branch
tanflem
added a commit
that referenced
this pull request
May 19, 2026
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores
Bug Fixes