[dev] [Marfuen] mariano/compliance-timeline-feature

[github-actions]

This is an automated pull request to merge mariano/compliance-timeline-feature into dev.
It was created by the [Auto Pull Request] action.

[cubic-dev-ai]

0 issues found across 11 files (changes from recent commits).

_{Requires human review: Auto-approval blocked by 17 unresolved issues from previous reviews.}

[cubic-dev-ai]

0 issues found across 11 files (changes from recent commits).

_{Requires human review: Auto-approval blocked by 7 unresolved issues from previous reviews.}

[cubic-dev-ai]

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/api/src/frameworks/frameworks-timeline.helper.ts">

<violation number="1" location="apps/api/src/frameworks/frameworks-timeline.helper.ts:338">
P1: Reconciliation call is unreachable when it's needed most. The function returns early at `if (phases.length === 0) return` when no auto phases are `IN_PROGRESS`. But the whole point of `reconcileAutoPhasesForOrganization` is to detect regressions on *COMPLETED* phases. When all auto phases are completed and a metric drops, reconciliation is skipped.

Move the reconciliation call before the early returns (or add it as a separate `finally`-style block) so it always runs.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.}

[cubic-dev-ai]

0 issues found across 1 file (changes from recent commits).

_{Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.}

[Marfuen]

@cubic-dev-ai review this

[cubic-dev-ai]

@cubic-dev-ai review this

@Marfuen I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

14 issues found across 111 files

Note: This PR contains a large number of files. During the trial, cubic reviews up to 50 files per PR. Paid plans get a higher limit.

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/api/src/timelines/dto/unlock-timeline.dto.ts">

<violation number="1" location="apps/api/src/timelines/dto/unlock-timeline.dto.ts:12">
P2: A whitespace-only `unlockReason` (e.g. `"   "`) passes `@IsNotEmpty()` because class-validator doesn't trim. Add a `@Transform` to trim, matching the pattern used in `UploadAttachmentDto`.</violation>
</file>

<file name="apps/api/src/people/people-invite.service.ts">

<violation number="1" location="apps/api/src/people/people-invite.service.ts:108">
P2: Silent `.catch(() => {})` swallows errors without logging. The same module's `PeopleService` logs these failures with `this.logger.warn(...)` in all 4 of its `checkAutoCompletePhases` calls. Follow the same pattern here to maintain debuggability.</violation>
</file>

<file name="apps/app/src/app/(app)/[orgId]/admin/timeline-templates/[templateId]/components/TemplateEditorPage.tsx">

<violation number="1" location="apps/app/src/app/(app)/[orgId]/admin/timeline-templates/[templateId]/components/TemplateEditorPage.tsx:26">
P2: API `error` from `useAdminTimelineTemplate` is not destructured or handled. If the fetch fails (network error, server error), the user sees "Template not found" instead of an error message. Consider destructuring `error` and rendering a distinct error state.</violation>
</file>

<file name="apps/api/src/integration-platform/services/dynamic-manifest-loader.service.ts">

<violation number="1" location="apps/api/src/integration-platform/services/dynamic-manifest-loader.service.ts:73">
P2: `PrismaClientKnownRequestError.code` uses Prisma-specific codes (e.g. `P1001`, `P2002`), never system-level codes like `ECONNREFUSED`. This branch is unreachable. If the intent is to match a connection-refused error from Prisma, use `error.code === 'P1001'` instead.</violation>
</file>

<file name="apps/api/src/admin-feature-flags/admin-feature-flags.controller.ts">

<violation number="1" location="apps/api/src/admin-feature-flags/admin-feature-flags.controller.ts:47">
P2: Missing `forbidNonWhitelisted: true` in `ValidationPipe`. The existing admin controller pattern (see `admin-organizations.controller.ts`) includes this option to reject requests with unknown properties rather than silently stripping them.</violation>
</file>

<file name="apps/api/src/timelines/dto/create-template.dto.ts">

<violation number="1" location="apps/api/src/timelines/dto/create-template.dto.ts:23">
P2: Use `@IsInt()` instead of `@IsNumber()` — `cycleNumber` should only accept integers. `@IsNumber()` allows floats like `1.5`, which would break database unique-key lookups and violate the documented semantics ("1 = initial, 2+ = renewal").</violation>
</file>

<file name="apps/api/src/timelines/admin-timeline-templates.controller.ts">

<violation number="1" location="apps/api/src/timelines/admin-timeline-templates.controller.ts:10">
P1: `UseInterceptors` is imported but never used, and `AdminAuditLogInterceptor` is not applied. The sibling `AdminOrgTimelinesController` applies `@UseInterceptors(AdminAuditLogInterceptor)` at the class level — this controller likely needs it too, so admin mutations on templates are audit-logged.</violation>
</file>

<file name="apps/app/src/app/(app)/[orgId]/admin/timeline-templates/[templateId]/components/TemplateMetadataForm.tsx">

<violation number="1" location="apps/app/src/app/(app)/[orgId]/admin/timeline-templates/[templateId]/components/TemplateMetadataForm.tsx:85">
P2: After a successful save, call `reset(values)` to update `defaultValues` and reset `isDirty` to `false`. Without this, the Save button stays enabled after saving, misleading the user into thinking there are still unsaved changes.</violation>
</file>

<file name="apps/api/src/evidence-forms/evidence-forms.service.ts">

<violation number="1" location="apps/api/src/evidence-forms/evidence-forms.service.ts:419">
P2: `.catch(() => {})` silently swallows errors from `checkAutoCompletePhases`. The internal `runPhaseAdvancement` call has no `catch` block, so its exceptions propagate here and are discarded without logging. Use `.catch((err) => logger.warn(...))` or similar to preserve observability—this pattern is already used inside the helper for `reconcileAutoPhasesForOrganization`.</violation>
</file>

<file name="apps/api/src/admin-feature-flags/admin-feature-flags.service.ts">

<violation number="1" location="apps/api/src/admin-feature-flags/admin-feature-flags.service.ts:127">
P2: Wrap `groupIdentify` + `flush()` in a try/catch with logging, consistent with how `listForOrganization` handles its PostHog calls. A network failure here will bubble up as a generic 500 instead of a meaningful error.</violation>
</file>

<file name="apps/api/src/timelines/timelines-slack.helper.ts">

<violation number="1" location="apps/api/src/timelines/timelines-slack.helper.ts:17">
P2: `fetch` does not reject on HTTP error status codes (4xx/5xx). Check `response.ok` so that Slack errors (malformed payload, rate-limiting, invalid webhook) are logged instead of silently swallowed.</violation>

<violation number="2" location="apps/api/src/timelines/timelines-slack.helper.ts:54">
P2: Floating promise: `sendSlack()` is async but its return value is neither awaited nor returned. This makes the notification entirely fire-and-forget with no way for callers to sequence or test completion. Return the promise (or `await` it in an `async` function) so consumers can choose whether to await.</violation>
</file>

<file name="apps/app/src/app/(app)/[orgId]/admin/organizations/[adminOrgId]/components/TimelineActivateForm.tsx">

<violation number="1" location="apps/app/src/app/(app)/[orgId]/admin/organizations/[adminOrgId]/components/TimelineActivateForm.tsx:30">
P2: `new Date(startDate).toISOString()` can throw a `RangeError` if the date string is invalid, which skips `setLoading(false)` and permanently locks the button in a loading state. Wrap the async body in `try/finally` to guarantee the loading state is reset.</violation>
</file>

<file name="apps/api/src/frameworks/frameworks.service.ts">

<violation number="1" location="apps/api/src/frameworks/frameworks.service.ts:196">
P2: Triggering `checkAutoCompletePhases` on every `getScores` read request introduces heavy, unthrottled write operations on a read path. Each call runs 5+ DB queries and potentially completes phases. Concurrent dashboard loads for the same org will run this pipeline in parallel, causing redundant DB load and potential race conditions when two runs try to complete the same phase simultaneously.

Consider moving this to an event-driven trigger (e.g., after task status changes) or adding a per-org debounce/lock to prevent redundant parallel executions.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.}

[cubic-dev-ai]

P1: UseInterceptors is imported but never used, and AdminAuditLogInterceptor is not applied. The sibling AdminOrgTimelinesController applies @UseInterceptors(AdminAuditLogInterceptor) at the class level — this controller likely needs it too, so admin mutations on templates are audit-logged.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/timelines/admin-timeline-templates.controller.ts, line 10:

<comment>`UseInterceptors` is imported but never used, and `AdminAuditLogInterceptor` is not applied. The sibling `AdminOrgTimelinesController` applies `@UseInterceptors(AdminAuditLogInterceptor)` at the class level — this controller likely needs it too, so admin mutations on templates are audit-logged.</comment>

<file context>
@@ -0,0 +1,113 @@
+  Param,
+  Body,
+  UseGuards,
+  UseInterceptors,
+  UsePipes,
+  ValidationPipe,
</file context>

[cubic-dev-ai]

P2: A whitespace-only unlockReason (e.g. " ") passes @IsNotEmpty() because class-validator doesn't trim. Add a @Transform to trim, matching the pattern used in UploadAttachmentDto.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/timelines/dto/unlock-timeline.dto.ts, line 12:

<comment>A whitespace-only `unlockReason` (e.g. `"   "`) passes `@IsNotEmpty()` because class-validator doesn't trim. Add a `@Transform` to trim, matching the pattern used in `UploadAttachmentDto`.</comment>

<file context>
@@ -0,0 +1,14 @@
+  @IsString()
+  @IsNotEmpty()
+  @MaxLength(2000)
+  unlockReason: string;
+}
+
</file context>

[cubic-dev-ai]

P2: Silent .catch(() => {}) swallows errors without logging. The same module's PeopleService logs these failures with this.logger.warn(...) in all 4 of its checkAutoCompletePhases calls. Follow the same pattern here to maintain debuggability.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/people/people-invite.service.ts, line 108:

<comment>Silent `.catch(() => {})` swallows errors without logging. The same module's `PeopleService` logs these failures with `this.logger.warn(...)` in all 4 of its `checkAutoCompletePhases` calls. Follow the same pattern here to maintain debuggability.</comment>

<file context>
@@ -95,6 +99,15 @@ export class PeopleInviteService {
+      checkAutoCompletePhases({
+        organizationId,
+        timelinesService: this.timelinesService,
+      }).catch(() => {});
+    }
+
</file context>

Suggested change

	}).catch(() => {});
	}).catch((err) => {
	this.logger.warn('timeline auto-complete check failed', err);
	});

[cubic-dev-ai]

P2: API error from useAdminTimelineTemplate is not destructured or handled. If the fetch fails (network error, server error), the user sees "Template not found" instead of an error message. Consider destructuring error and rendering a distinct error state.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/admin/timeline-templates/[templateId]/components/TemplateEditorPage.tsx, line 26:

<comment>API `error` from `useAdminTimelineTemplate` is not destructured or handled. If the fetch fails (network error, server error), the user sees "Template not found" instead of an error message. Consider destructuring `error` and rendering a distinct error state.</comment>

<file context>
@@ -0,0 +1,116 @@
+  orgId,
+  templateId,
+}: TemplateEditorPageProps) {
+  const { template, isLoading, mutate } = useAdminTimelineTemplate(templateId);
+
+  const phasesForBar =
</file context>

[cubic-dev-ai]

P2: PrismaClientKnownRequestError.code uses Prisma-specific codes (e.g. P1001, P2002), never system-level codes like ECONNREFUSED. This branch is unreachable. If the intent is to match a connection-refused error from Prisma, use error.code === 'P1001' instead.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/integration-platform/services/dynamic-manifest-loader.service.ts, line 73:

<comment>`PrismaClientKnownRequestError.code` uses Prisma-specific codes (e.g. `P1001`, `P2002`), never system-level codes like `ECONNREFUSED`. This branch is unreachable. If the intent is to match a connection-refused error from Prisma, use `error.code === 'P1001'` instead.</comment>

<file context>
@@ -68,8 +68,17 @@ export class DynamicManifestLoaderService
     }
+    if (
+      error instanceof Prisma.PrismaClientKnownRequestError &&
+      error.code === 'ECONNREFUSED'
+    ) {
+      return true;
</file context>

Suggested change

	error.code === 'ECONNREFUSED'
	error.code === 'P1001'

[cubic-dev-ai]

P2: Wrap groupIdentify + flush() in a try/catch with logging, consistent with how listForOrganization handles its PostHog calls. A network failure here will bubble up as a generic 500 instead of a meaningful error.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/admin-feature-flags/admin-feature-flags.service.ts, line 127:

<comment>Wrap `groupIdentify` + `flush()` in a try/catch with logging, consistent with how `listForOrganization` handles its PostHog calls. A network failure here will bubble up as a generic 500 instead of a meaningful error.</comment>

<file context>
@@ -0,0 +1,139 @@
+      throw new BadRequestException('PostHog is not configured on this environment');
+    }
+
+    client.groupIdentify({
+      groupType: 'organization',
+      groupKey: orgId,
</file context>

[cubic-dev-ai]

P2: fetch does not reject on HTTP error status codes (4xx/5xx). Check response.ok so that Slack errors (malformed payload, rate-limiting, invalid webhook) are logged instead of silently swallowed.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/timelines/timelines-slack.helper.ts, line 17:

<comment>`fetch` does not reject on HTTP error status codes (4xx/5xx). Check `response.ok` so that Slack errors (malformed payload, rate-limiting, invalid webhook) are logged instead of silently swallowed.</comment>

<file context>
@@ -0,0 +1,125 @@
+  const webhookUrl = process.env.SLACK_CX_WEBHOOK_URL;
+  if (!webhookUrl) return;
+  try {
+    await fetch(webhookUrl, {
+      method: 'POST',
+      headers: { 'Content-Type': 'application/json' },
</file context>

[cubic-dev-ai]

P2: Floating promise: sendSlack() is async but its return value is neither awaited nor returned. This makes the notification entirely fire-and-forget with no way for callers to sequence or test completion. Return the promise (or await it in an async function) so consumers can choose whether to await.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/timelines/timelines-slack.helper.ts, line 54:

<comment>Floating promise: `sendSlack()` is async but its return value is neither awaited nor returned. This makes the notification entirely fire-and-forget with no way for callers to sequence or test completion. Return the promise (or `await` it in an `async` function) so consumers can choose whether to await.</comment>

<file context>
@@ -0,0 +1,125 @@
+  phaseName: string;
+}) {
+  const link = adminTimelineUrl(orgId);
+  sendSlack(
+    [
+      section(`:bell:  *Ready for Review*`),
</file context>

[cubic-dev-ai]

P2: new Date(startDate).toISOString() can throw a RangeError if the date string is invalid, which skips setLoading(false) and permanently locks the button in a loading state. Wrap the async body in try/finally to guarantee the loading state is reset.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/admin/organizations/[adminOrgId]/components/TimelineActivateForm.tsx, line 30:

<comment>`new Date(startDate).toISOString()` can throw a `RangeError` if the date string is invalid, which skips `setLoading(false)` and permanently locks the button in a loading state. Wrap the async body in `try/finally` to guarantee the loading state is reset.</comment>

<file context>
@@ -0,0 +1,65 @@
+      return;
+    }
+
+    setLoading(true);
+    const res = await api.post(
+      `/v1/admin/organizations/${orgId}/timelines/${timelineId}/activate`,
</file context>

[cubic-dev-ai]

P2: Triggering checkAutoCompletePhases on every getScores read request introduces heavy, unthrottled write operations on a read path. Each call runs 5+ DB queries and potentially completes phases. Concurrent dashboard loads for the same org will run this pipeline in parallel, causing redundant DB load and potential race conditions when two runs try to complete the same phase simultaneously.

Consider moving this to an event-driven trigger (e.g., after task status changes) or adding a per-org debounce/lock to prevent redundant parallel executions.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/frameworks/frameworks.service.ts, line 196:

<comment>Triggering `checkAutoCompletePhases` on every `getScores` read request introduces heavy, unthrottled write operations on a read path. Each call runs 5+ DB queries and potentially completes phases. Concurrent dashboard loads for the same org will run this pipeline in parallel, causing redundant DB load and potential race conditions when two runs try to complete the same phase simultaneously.

Consider moving this to an event-driven trigger (e.g., after task status changes) or adding a per-org debounce/lock to prevent redundant parallel executions.</comment>

<file context>
@@ -182,6 +191,15 @@ export class FrameworksService {
     ]);
+
+    // Fire-and-forget: auto-complete timeline phases when tasks hit 100%
+    checkAutoCompletePhases({
+      organizationId,
+      timelinesService: this.timelinesService,
</file context>