feat: Add Milestone kanban page#3884
Merged
Merged
Conversation
Signed-off-by: Adriano Lazzaretti <lazzaretti136@gmail.com>
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new
tasksKanbanStatetype was changed toJson, butparseKanbanStatestill assumes a JSON string and the update path callsJSON.stringifybefore sending; this risks double-encoding or runtime errors if the backend starts returning structured JSON—consider standardising on either a string or object representation end-to-end and removing theas any/manualJSON.parse. - In
useTasksForTurboUi, theinputforApi.project_tasks.createis typed asanyeven thoughProjectTasksCreateInputnow supportsstatus; you can tighten this by constructing aProjectTasksCreateInput(e.g. via conditional spread forstatus) to keep the callsite type-safe.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `tasksKanbanState` type was changed to `Json`, but `parseKanbanState` still assumes a JSON string and the update path calls `JSON.stringify` before sending; this risks double-encoding or runtime errors if the backend starts returning structured JSON—consider standardising on either a string or object representation end-to-end and removing the `as any`/manual `JSON.parse`.
- In `useTasksForTurboUi`, the `input` for `Api.project_tasks.create` is typed as `any` even though `ProjectTasksCreateInput` now supports `status`; you can tighten this by constructing a `ProjectTasksCreateInput` (e.g. via conditional spread for `status`) to keep the callsite type-safe.
## Individual Comments
### Comment 1
<location> `app/assets/js/models/tasks/useTasksForTurboUi.tsx:120-122` </location>
<code_context>
try {
- const res = await Api.project_tasks.create({
+ const backendStatus: ProjectTaskStatus | null = Tasks.serializeTaskStatus(task.status ?? null);
+
+ const input: any = {
name: task.title,
assigneeId: task.assignee,
</code_context>
<issue_to_address>
**suggestion:** Avoid `any` for the create-task payload and type it as the proper API input including optional status.
Using `input: any` drops type safety at the API boundary. Since `ProjectTasksCreateInput` now accepts optional `status?: ProjectTaskStatus`, you can keep this strictly typed:
```ts
const backendStatus = Tasks.serializeTaskStatus(task.status ?? null);
const input: ProjectTasksCreateInput = {
name: task.title,
assigneeId: task.assignee,
dueDate: serializeContextualDate(task.dueDate),
milestoneId: task.milestone?.id || null,
projectId,
...(backendStatus ? { status: backendStatus } : {}),
};
const res = await Api.project_tasks.create(input);
```
That way changes to the API contract are caught by TypeScript instead of being masked by `any`.
Suggested implementation:
```typescript
try {
const backendStatus = Tasks.serializeTaskStatus(task.status ?? null);
const input: ProjectTasksCreateInput = {
name: task.title,
assigneeId: task.assignee,
dueDate: serializeContextualDate(task.dueDate),
milestoneId: task.milestone?.id || null,
projectId,
...(backendStatus ? { status: backendStatus } : {}),
};
```
1. Make sure `ProjectTasksCreateInput` is imported in this file from the same API types module where `ProjectTaskStatus` (or other project task types) are defined, for example by extending the existing type import line to include it.
2. If the `Api.project_tasks.create` call currently still uses an inline object literal (not visible in this snippet), change it to `const res = await Api.project_tasks.create(input);` so that the strongly typed `input` is actually used.
</issue_to_address>
### Comment 2
<location> `app/assets/js/pages/MilestoneKanbanPage/index.tsx:63-65` </location>
<code_context>
+ return `v1-MilestoneKanbanPage-${id}`;
+}
+
+function parseKanbanState(
+ raw: string | null | undefined,
+ statuses: ReturnType<typeof Tasks.parseTaskStatusesForTurboUi>,
+): MilestoneKanbanState {
+ let parsed: any = {};
</code_context>
<issue_to_address>
**issue (bug_risk):** Broaden `parseKanbanState` to accept `Json` and avoid relying on `any` casts at the call site.
The function assumes `raw` is a string and calls `JSON.parse(raw)`, but at the call site you pass `milestone.tasksKanbanState as any` where `tasksKanbanState` is `Json | null`. If this ever becomes a structured object, `JSON.parse` will throw and the `as any` will hide that mismatch.
Consider updating the signature to accept the actual type and handle both strings and objects, so the call site can pass `milestone.tasksKanbanState` directly without `as any`, e.g.:
```ts
function parseKanbanState(
raw: unknown,
statuses: ReturnType<typeof Tasks.parseTaskStatusesForTurboUi>,
): MilestoneKanbanState {
let parsed: any = {};
if (typeof raw === "string" && raw.trim()) {
try {
parsed = JSON.parse(raw);
} catch (e) {
console.error("Failed to parse tasksKanbanState", e);
parsed = {};
}
} else if (raw && typeof raw === "object") {
parsed = raw;
}
const state: MilestoneKanbanState = {};
statuses.forEach((status) => {
const key = status.value;
const list = (parsed as any)?.[key];
state[key] = Array.isArray(list) ? list : [];
});
return state;
}
```
</issue_to_address>
### Comment 3
<location> `app/assets/js/pages/MilestoneKanbanPage/index.tsx:118-119` </location>
<code_context>
+ transformResult: transformPerson,
+ });
+
+ const kanbanState = React.useMemo(() => parseKanbanState(milestone.tasksKanbanState as any, statusOptions), [
+ milestone.tasksKanbanState,
+ statusOptions,
+ ]);
</code_context>
<issue_to_address>
**suggestion:** Remove the `as any` cast by aligning the `parseKanbanState` signature with the actual `tasksKanbanState` type.
The `as any` cast erodes type safety and can hide mismatches if `tasksKanbanState`’s shape changes. Once `parseKanbanState` is updated to accept `Json | null` (or `unknown`) and handle both strings and objects, this can become:
```ts
const kanbanState = React.useMemo(
() => parseKanbanState(milestone.tasksKanbanState, statusOptions),
[milestone.tasksKanbanState, statusOptions],
);
```
This keeps types aligned and avoids masking issues with the kanban state format.
Suggested implementation:
```typescript
const kanbanState = React.useMemo(
() => parseKanbanState(milestone.tasksKanbanState, statusOptions),
[milestone.tasksKanbanState, statusOptions],
);
```
To fully align types and make this compile safely, you’ll also need to:
1. Update the `parseKanbanState` function (where it is defined) so its first parameter matches the type of `milestone.tasksKanbanState` (likely `Json | null` or `unknown` rather than a narrower type).
2. Inside `parseKanbanState`, handle both cases where the input is:
- A string (e.g., JSON that needs `JSON.parse`), and
- An already-parsed object (just use it directly).
3. Adjust any other call sites of `parseKanbanState` if they rely on the old parameter type so they pass a value compatible with the new, more general type.
</issue_to_address>
### Comment 4
<location> `app/assets/js/pages/MilestoneKanbanPage/index.tsx:139` </location>
<code_context>
+ taskId: event.taskId,
+ milestoneId: milestone.id,
+ status: backendStatus,
+ milestoneKanbanState: JSON.stringify(event.updatedKanbanState),
+ });
+
</code_context>
<issue_to_address>
**suggestion:** Consider storing `milestoneKanbanState` as structured JSON instead of a JSON string.
You’ve typed `tasksKanbanState` / `ProjectTasksUpdateKanbanInput.milestoneKanbanState` as `Json`, but still stringify and later `JSON.parse` the value. If the backend supports JSON/JSONB, you can pass the structured object directly:
```ts
await Api.project_tasks.updateKanban({
taskId: event.taskId,
milestoneId: milestone.id,
status: backendStatus,
milestoneKanbanState: event.updatedKanbanState,
});
```
and have `tasksKanbanState` returned as the same object. That removes the need for `JSON.parse` in `parseKanbanState` and avoids type drift between string vs object. If persistence must be a string, consider keeping the type as `string` instead of `Json` to match reality.
Suggested implementation:
```typescript
try {
await Api.project_tasks.updateKanban({
taskId: event.taskId,
milestoneId: milestone.id,
status: backendStatus,
milestoneKanbanState: event.updatedKanbanState,
});
import Api from "@/api";
```
In this file and the related codebase, you will also need to:
1. Update any `parseKanbanState` (or similarly named) helper that currently does `JSON.parse(tasksKanbanState)` so it treats `tasksKanbanState`/`milestoneKanbanState` as an already-structured object (i.e. remove `JSON.parse` and adjust types accordingly).
2. Ensure the type for `ProjectTasksUpdateKanbanInput.milestoneKanbanState` (and any corresponding `tasksKanbanState` / `milestoneKanbanState` fields on models or API responses) reflects a structured JSON type rather than a string. If the backend uses JSON/JSONB, keep it as a JSON type and stop converting to string; if the backend must persist it as a string, then instead align the TypeScript types to `string` rather than `Json`.
3. Adjust any other call sites or selectors that currently expect `milestoneKanbanState` as a string (e.g. places where `typeof ... === "string"` or `JSON.parse` / `JSON.stringify` are used) so they work directly with the object.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
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.
No description provided.