Skip to content

Commit d8753ca

Browse files
Marfuenclaude
andcommitted
fix(timeline): address remaining AI review P2 feedback
API: - extract COMPLETION_TYPES to shared timeline-constants, narrow DTO completionType from string to CompletionType union - trigger checkAutoCompletePhases on both done and not_relevant task statuses (AUTO_TASKS counts both as finished) - drop unnecessary checkAutoCompletePhases after task creation — a todo task can't advance any AUTO phase App: - extract COMPLETION_OPTIONS constant to timeline-templates/constants and share across PhaseRow, TemplateEditor, template-actions - remove orderIndex from form state (controlled/uncontrolled hybrid); compute from array position at submit time in template-actions - convert TemplateMetadataForm's framework fetch from raw useEffect to SWR with proper error handling - delete duplicate admin layout under timeline-templates (parent admin layout already guards the route) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent c4b5b91 commit d8753ca

File tree

11 files changed

+79
-130
lines changed

11 files changed

+79
-130
lines changed

apps/api/src/tasks/tasks.service.ts

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -410,14 +410,19 @@ export class TasksService {
410410
);
411411
});
412412

413-
// Check timeline auto-completion when tasks are marked done
414-
if (status === TaskStatus.done) {
413+
// Check timeline auto-completion when tasks reach a "finished" state —
414+
// AUTO_TASKS counts both done and not_relevant as complete, so both
415+
// statuses can unblock a phase.
416+
if (
417+
status === TaskStatus.done ||
418+
status === TaskStatus.not_relevant
419+
) {
415420
checkAutoCompletePhases({
416421
organizationId,
417422
timelinesService: this.timelinesService,
418423
}).catch((err) => {
419-
this.logger.warn('timeline auto-complete check failed', err);
420-
});
424+
this.logger.warn('timeline auto-complete check failed', err);
425+
});
421426
}
422427

423428
return { updatedCount: result.count };
@@ -714,14 +719,18 @@ export class TasksService {
714719
console.error('Failed to send status change notifications:', error);
715720
});
716721

717-
// Check timeline auto-completion when task is marked done
718-
if (updateData.status === TaskStatus.done) {
722+
// Check timeline auto-completion when task reaches a "finished"
723+
// state (AUTO_TASKS counts done and not_relevant as complete).
724+
if (
725+
updateData.status === TaskStatus.done ||
726+
updateData.status === TaskStatus.not_relevant
727+
) {
719728
checkAutoCompletePhases({
720729
organizationId,
721730
timelinesService: this.timelinesService,
722731
}).catch((err) => {
723-
this.logger.warn('timeline auto-complete check failed', err);
724-
});
732+
this.logger.warn('timeline auto-complete check failed', err);
733+
});
725734
}
726735
}
727736

@@ -853,13 +862,9 @@ export class TasksService {
853862
},
854863
});
855864

856-
// Check timeline auto-completion after task creation (total task count changed)
857-
checkAutoCompletePhases({
858-
organizationId,
859-
timelinesService: this.timelinesService,
860-
}).catch((err) => {
861-
this.logger.warn('timeline auto-complete check failed', err);
862-
});
865+
// Intentionally no checkAutoCompletePhases here: new tasks default to
866+
// todo, which can't advance an AUTO_TASKS phase and has no bearing on
867+
// AUTO_POLICIES / AUTO_PEOPLE / AUTO_FINDINGS criteria.
863868

864869
return {
865870
id: task.id,

apps/api/src/timelines/dto/create-phase-template.dto.ts

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,7 @@ import {
88
IsBoolean,
99
} from 'class-validator';
1010
import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger';
11-
12-
const COMPLETION_TYPES = [
13-
'AUTO_TASKS',
14-
'AUTO_POLICIES',
15-
'AUTO_PEOPLE',
16-
'AUTO_FINDINGS',
17-
'AUTO_UPLOAD',
18-
'MANUAL',
19-
] as const;
11+
import { COMPLETION_TYPES, type CompletionType } from '../timeline-constants';
2012

2113
export class CreatePhaseTemplateDto {
2214
@ApiProperty({ description: 'Phase name', example: 'Gap Assessment' })
@@ -57,7 +49,7 @@ export class CreatePhaseTemplateDto {
5749
})
5850
@IsOptional()
5951
@IsIn(COMPLETION_TYPES)
60-
completionType?: string;
52+
completionType?: CompletionType;
6153

6254
@ApiPropertyOptional({
6355
description:
@@ -103,5 +95,5 @@ export class AddPhaseToInstanceDto {
10395
})
10496
@IsOptional()
10597
@IsIn(COMPLETION_TYPES)
106-
completionType?: string;
98+
completionType?: CompletionType;
10799
}

apps/api/src/timelines/dto/update-phase-template.dto.ts

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,7 @@ import {
77
IsBoolean,
88
} from 'class-validator';
99
import { ApiPropertyOptional } from '@nestjs/swagger';
10-
11-
const COMPLETION_TYPES = [
12-
'AUTO_TASKS',
13-
'AUTO_POLICIES',
14-
'AUTO_PEOPLE',
15-
'AUTO_FINDINGS',
16-
'AUTO_UPLOAD',
17-
'MANUAL',
18-
] as const;
10+
import { COMPLETION_TYPES, type CompletionType } from '../timeline-constants';
1911

2012
export class UpdatePhaseTemplateDto {
2113
@ApiPropertyOptional({ description: 'Phase name' })
@@ -57,7 +49,7 @@ export class UpdatePhaseTemplateDto {
5749
})
5850
@IsOptional()
5951
@IsIn(COMPLETION_TYPES)
60-
completionType?: string;
52+
completionType?: CompletionType;
6153

6254
@ApiPropertyOptional({
6355
description:

apps/api/src/timelines/dto/update-phase.dto.ts

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,7 @@ import {
88
Min,
99
} from 'class-validator';
1010
import { ApiPropertyOptional } from '@nestjs/swagger';
11-
12-
const COMPLETION_TYPES = [
13-
'AUTO_TASKS',
14-
'AUTO_POLICIES',
15-
'AUTO_PEOPLE',
16-
'AUTO_FINDINGS',
17-
'AUTO_UPLOAD',
18-
'MANUAL',
19-
] as const;
11+
import { COMPLETION_TYPES, type CompletionType } from '../timeline-constants';
2012

2113
export class UpdatePhaseDto {
2214
@ApiPropertyOptional({ description: 'Phase name' })
@@ -64,7 +56,7 @@ export class UpdatePhaseDto {
6456
})
6557
@IsOptional()
6658
@IsIn(COMPLETION_TYPES)
67-
completionType?: string;
59+
completionType?: CompletionType;
6860

6961
@ApiPropertyOptional({
7062
description: 'Whether completing this phase should lock the timeline',
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
export const COMPLETION_TYPES = [
2+
'AUTO_TASKS',
3+
'AUTO_POLICIES',
4+
'AUTO_PEOPLE',
5+
'AUTO_FINDINGS',
6+
'AUTO_UPLOAD',
7+
'MANUAL',
8+
] as const;
9+
10+
export type CompletionType = (typeof COMPLETION_TYPES)[number];

apps/app/src/app/(app)/[orgId]/admin/timeline-templates/[templateId]/components/TemplateMetadataForm.tsx

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
'use client';
22

3-
import { useEffect, useState } from 'react';
3+
import { useState } from 'react';
44
import { useForm, Controller } from 'react-hook-form';
55
import { zodResolver } from '@hookform/resolvers/zod';
66
import { z } from 'zod';
77
import { toast } from 'sonner';
8-
import { api } from '@/lib/api-client';
8+
import useSWR from 'swr';
9+
import { api, apiClient } from '@/lib/api-client';
910
import type { AdminTimelineTemplate } from '@/hooks/use-admin-timelines';
1011
import {
1112
Button,
@@ -43,15 +44,18 @@ export function TemplateMetadataForm({
4344
onMutate,
4445
}: TemplateMetadataFormProps) {
4546
const [saving, setSaving] = useState(false);
46-
const [frameworks, setFrameworks] = useState<Framework[]>([]);
4747

48-
useEffect(() => {
49-
api
50-
.get<{ data: Framework[] }>('/v1/frameworks/available')
51-
.then((res) => {
52-
if (res.data?.data) setFrameworks(res.data.data);
53-
});
54-
}, []);
48+
const { data: frameworks = [] } = useSWR(
49+
['/v1/frameworks/available'],
50+
async () => {
51+
const res = await apiClient.get<{ data: Framework[] }>(
52+
'/v1/frameworks/available',
53+
);
54+
if (res.error) throw new Error(res.error);
55+
return res.data?.data ?? [];
56+
},
57+
{ revalidateOnFocus: false },
58+
);
5559

5660
const {
5761
register,

apps/app/src/app/(app)/[orgId]/admin/timeline-templates/components/PhaseRow.tsx

Lines changed: 2 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,7 @@ import { Button, Text } from '@trycompai/design-system';
55
import { TrashCan } from '@trycompai/design-system/icons';
66
import { Input } from '@trycompai/ui/input';
77
import { Label } from '@trycompai/ui/label';
8-
9-
const COMPLETION_OPTIONS = [
10-
{ value: 'MANUAL', label: 'Manual' },
11-
{ value: 'AUTO_TASKS', label: 'Auto (Tasks)' },
12-
{ value: 'AUTO_POLICIES', label: 'Auto (Policies)' },
13-
{ value: 'AUTO_PEOPLE', label: 'Auto (People)' },
14-
{ value: 'AUTO_FINDINGS', label: 'Auto (Findings)' },
15-
{ value: 'AUTO_UPLOAD', label: 'Auto (Upload)' },
16-
] as const;
8+
import { COMPLETION_OPTIONS, type CompletionType } from './constants';
179

1810
interface PhaseFormValues {
1911
name: string;
@@ -23,15 +15,8 @@ interface PhaseFormValues {
2315
id?: string;
2416
name: string;
2517
description?: string;
26-
orderIndex: number;
2718
defaultDurationWeeks: number;
28-
completionType:
29-
| 'AUTO_TASKS'
30-
| 'AUTO_POLICIES'
31-
| 'AUTO_PEOPLE'
32-
| 'AUTO_FINDINGS'
33-
| 'AUTO_UPLOAD'
34-
| 'MANUAL';
19+
completionType: CompletionType;
3520
locksTimelineOnComplete?: boolean;
3621
}[];
3722
}
@@ -112,14 +97,6 @@ export function PhaseRow({ index, register, errors, onRemove }: PhaseRowProps) {
11297
</select>
11398
</div>
11499

115-
<input
116-
type="hidden"
117-
{...register(`phases.${index}.orderIndex`, {
118-
valueAsNumber: true,
119-
})}
120-
value={index}
121-
/>
122-
123100
<div className="col-span-2 flex items-center gap-2">
124101
<input
125102
id={`phases.${index}.locksTimelineOnComplete`}

apps/app/src/app/(app)/[orgId]/admin/timeline-templates/components/TemplateEditor.tsx

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -27,22 +27,19 @@ import {
2727
saveExistingTemplate,
2828
} from './template-actions';
2929

30-
const COMPLETION_TYPES = [
31-
'AUTO_TASKS',
32-
'AUTO_POLICIES',
33-
'AUTO_PEOPLE',
34-
'AUTO_FINDINGS',
35-
'AUTO_UPLOAD',
36-
'MANUAL',
37-
] as const;
38-
3930
const phaseSchema = z.object({
4031
id: z.string().optional(),
4132
name: z.string().min(1, 'Name is required'),
4233
description: z.string().optional(),
43-
orderIndex: z.number().min(0),
4434
defaultDurationWeeks: z.number().min(1, 'Must be at least 1 week'),
45-
completionType: z.enum(COMPLETION_TYPES),
35+
completionType: z.enum([
36+
'AUTO_TASKS',
37+
'AUTO_POLICIES',
38+
'AUTO_PEOPLE',
39+
'AUTO_FINDINGS',
40+
'AUTO_UPLOAD',
41+
'MANUAL',
42+
]),
4643
locksTimelineOnComplete: z.boolean().optional(),
4744
});
4845

@@ -129,7 +126,6 @@ export function TemplateEditor({
129126
append({
130127
name: '',
131128
description: '',
132-
orderIndex: fields.length,
133129
defaultDurationWeeks: 2,
134130
completionType: 'MANUAL',
135131
locksTimelineOnComplete: false,
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
export const COMPLETION_OPTIONS = [
2+
{ value: 'MANUAL', label: 'Manual' },
3+
{ value: 'AUTO_TASKS', label: 'Auto (Tasks)' },
4+
{ value: 'AUTO_POLICIES', label: 'Auto (Policies)' },
5+
{ value: 'AUTO_PEOPLE', label: 'Auto (People)' },
6+
{ value: 'AUTO_FINDINGS', label: 'Auto (Findings)' },
7+
{ value: 'AUTO_UPLOAD', label: 'Auto (Upload)' },
8+
] as const;
9+
10+
export type CompletionType = (typeof COMPLETION_OPTIONS)[number]['value'];

apps/app/src/app/(app)/[orgId]/admin/timeline-templates/components/template-actions.ts

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { toast } from 'sonner';
22
import { api } from '@/lib/api-client';
33
import type { AdminTimelineTemplate } from '@/hooks/use-admin-timelines';
4+
import type { CompletionType } from './constants';
45

56
interface TemplateFormValues {
67
name: string;
@@ -10,15 +11,8 @@ interface TemplateFormValues {
1011
id?: string;
1112
name: string;
1213
description?: string;
13-
orderIndex: number;
1414
defaultDurationWeeks: number;
15-
completionType:
16-
| 'AUTO_TASKS'
17-
| 'AUTO_POLICIES'
18-
| 'AUTO_PEOPLE'
19-
| 'AUTO_FINDINGS'
20-
| 'AUTO_UPLOAD'
21-
| 'MANUAL';
15+
completionType: CompletionType;
2216
locksTimelineOnComplete?: boolean;
2317
}[];
2418
}
@@ -39,7 +33,6 @@ export function getDefaults(
3933
id: p.id,
4034
name: p.name,
4135
description: p.description ?? '',
42-
orderIndex: p.orderIndex,
4336
defaultDurationWeeks: p.defaultDurationWeeks,
4437
completionType: p.completionType ?? 'MANUAL',
4538
locksTimelineOnComplete: p.locksTimelineOnComplete ?? false,
@@ -56,13 +49,13 @@ export async function createNewTemplate(values: TemplateFormValues) {
5649
if (res.error) throw new Error(res.error);
5750

5851
const created = res.data as { id: string };
59-
for (const phase of values.phases) {
52+
for (const [index, phase] of values.phases.entries()) {
6053
const phaseRes = await api.post(
6154
`/v1/admin/timeline-templates/${created.id}/phases`,
6255
{
6356
name: phase.name,
6457
description: phase.description || undefined,
65-
orderIndex: phase.orderIndex,
58+
orderIndex: index,
6659
defaultDurationWeeks: phase.defaultDurationWeeks,
6760
completionType: phase.completionType,
6861
locksTimelineOnComplete: phase.locksTimelineOnComplete ?? false,
@@ -98,15 +91,15 @@ export async function saveExistingTemplate(
9891
}
9992
}
10093

101-
// Create or update phases
102-
for (const phase of values.phases) {
94+
// Create or update phases (orderIndex is the array position)
95+
for (const [index, phase] of values.phases.entries()) {
10396
if (phase.id && existingIds.has(phase.id)) {
10497
const patchRes = await api.patch(
10598
`/v1/admin/timeline-templates/${template.id}/phases/${phase.id}`,
10699
{
107100
name: phase.name,
108101
description: phase.description || undefined,
109-
orderIndex: phase.orderIndex,
102+
orderIndex: index,
110103
defaultDurationWeeks: phase.defaultDurationWeeks,
111104
completionType: phase.completionType,
112105
locksTimelineOnComplete: phase.locksTimelineOnComplete ?? false,
@@ -119,7 +112,7 @@ export async function saveExistingTemplate(
119112
{
120113
name: phase.name,
121114
description: phase.description || undefined,
122-
orderIndex: phase.orderIndex,
115+
orderIndex: index,
123116
defaultDurationWeeks: phase.defaultDurationWeeks,
124117
completionType: phase.completionType,
125118
locksTimelineOnComplete: phase.locksTimelineOnComplete ?? false,

0 commit comments

Comments
 (0)