Skip to content

fix(kanban-simple): wrap JSON.parse calls with safe fallback #568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
data-bot-coasys wants to merge 1 commit into
base: dev
Choose a base branch
from
+37 −13
Open

fix(kanban-simple): wrap JSON.parse calls with safe fallback #568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 22 additions & 9 deletions views/kanban-view-simple/src/components/Board/Board.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,19 @@ type BoardProps = {
getProfile: (did: string) => Promise<Profile>;
};

/** Safely parse a JSON string, returning the fallback value on failure.
* This handles cases where the value is a literal:// URI string or a
* signed expression envelope that cannot be parsed as a JSON array. */
function safeJsonParse<T>(value: string | undefined | null, fallback: T): T {
if (!value) return fallback;
try {
return JSON.parse(value);
} catch {
console.warn('Failed to parse JSON value, using fallback:', value?.slice(0, 80));
return fallback;
}
}
Comment on lines +21 to +29

@coderabbitai coderabbitai Bot Mar 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

git ls-files | grep -E "Board\.tsx|kanban-view" | head -20

Repository: coasys/flux

Length of output: 1144

🏁 Script executed:

wc -l views/kanban-view-simple/src/components/Board/Board.tsx

Repository: coasys/flux

Length of output: 114

🏁 Script executed:

cat -n views/kanban-view-simple/src/components/Board/Board.tsx | head -40

Repository: coasys/flux

Length of output: 2066

🏁 Script executed:

cat -n views/kanban-view-simple/src/components/Board/Board.tsx | sed -n '115,130p;145,160p;158,172p;210,225p;238,255p;282,295p;313,325p;340,355p'

Repository: coasys/flux

Length of output: 6141

🏁 Script executed:

cat -n views/kanban-view-simple/src/components/Board/Board.tsx | sed -n '200,260p'

Repository: coasys/flux

Length of output: 3205

🏁 Script executed:

cat -n views/kanban-view-simple/src/components/Board/Board.tsx | sed -n '275,310p'

Repository: coasys/flux

Length of output: 1803

Add runtime type validation to safeJsonParse<T> before returning parsed values

The function catches JSON parse errors but cannot prevent valid-but-wrong JSON (e.g., objects, numbers, or non-string arrays) from breaking array operations at call sites. This is especially critical in drag-and-drop reorder logic (lines 122, 153, 166, 218, 246, 251, 289, 320, 348) where .splice(), .filter(), or .map() are called immediately on results without type guards. Add an Array.isArray() check before returning, or implement shape validation for string[] expectations.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@views/kanban-view-simple/src/components/Board/Board.tsx` around lines 21 -
29, safeJsonParse<T> currently returns parsed JSON without validating its shape,
which lets valid-but-incorrect values (objects, numbers, etc.) reach callers
that immediately call .splice/.filter/.map in the board drag-and-drop logic;
update safeJsonParse to perform runtime shape checks before returning: for the
kanban use case add an overload or optional validator parameter (or a boolean
branch) that verifies Array.isArray(parsed) and that each item is a string
(e.g., typeof item === 'string') when callers expect string[]; if validation
fails, log a warning and return the provided fallback. Ensure callers (the
reorder/drag-and-drop functions that call .splice/.filter/.map) keep using
safeJsonParse<string[]>(...) so the validated type is enforced at runtime.


export type ColumnWithTasks = TaskColumn & { tasks: Task[] };

export default function Board({ perspective, channelId, agent, getProfile }: BoardProps) {
Expand Down Expand Up @@ -106,7 +119,7 @@ export default function Board({ perspective, channelId, agent, getProfile }: Boa
setUpdating(false);
return;
}
const newOrderedColumnIds = [...JSON.parse(currentBoard.orderedColumnIds), newColumn.id];
const newOrderedColumnIds = [...safeJsonParse<string[]>(currentBoard.orderedColumnIds, []), newColumn.id];
currentBoard.orderedColumnIds = JSON.stringify(newOrderedColumnIds);
await currentBoard.save();

Expand Down Expand Up @@ -137,7 +150,7 @@ export default function Board({ perspective, channelId, agent, getProfile }: Boa
setUpdating(false);
return;
}
const orderedColumnIds = JSON.parse(currentBoard.orderedColumnIds);
const orderedColumnIds = safeJsonParse<string[]>(currentBoard.orderedColumnIds, []);
const newOrderedColumnIds = orderedColumnIds.filter((id: string) => id !== columnId);

// Update the UI
Expand All @@ -150,7 +163,7 @@ export default function Board({ perspective, channelId, agent, getProfile }: Boa

// Delete the columns tasks and their links to the perspective
const column = columns.find((col) => col.id === columnId);
const taskIds = column.orderedTaskIds ? JSON.parse(column.orderedTaskIds) : [];
const taskIds = safeJsonParse<string[]>(column.orderedTaskIds, []);
const columnTasks = await Task.findAll(perspective, { where: { id: taskIds } });
Comment on lines 165 to 167

@coderabbitai coderabbitai Bot Mar 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

deleteColumn can crash when column lookup misses

columns.find(...) can return undefined, but column.orderedTaskIds and column.delete() are called unguarded. Add a guard before dereference/delete.

Also applies to: 185-185

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@views/kanban-view-simple/src/components/Board/Board.tsx` around lines 165 -
167, The code calls columns.find(...) and immediately dereferences
column.orderedTaskIds and column.delete(), which can crash if the find returns
undefined; update the deleteColumn logic to check the result of
columns.find(...) (the variable column) and bail out early (return or throw a
controlled error) if it's undefined, then use column.orderedTaskIds only after
that guard (you can still call safeJsonParse on a fallback value if preferred)
and only invoke column.delete() inside the guarded block; apply the same
presence check to the other occurrence that references column (around the
Task.findAll / column.delete usage).

await Promise.all(
columnTasks.map(async (task) => {
Expand Down Expand Up @@ -202,7 +215,7 @@ export default function Board({ perspective, channelId, agent, getProfile }: Boa

// Update the perspective
const currentBoard = (await TaskBoard.findAll(perspective, { parent: { model: Channel, id: channelId } }))[0];
const newOrderedColumnIds = JSON.parse(currentBoard.orderedColumnIds);
const newOrderedColumnIds = safeJsonParse<string[]>(currentBoard.orderedColumnIds, []);
newOrderedColumnIds.splice(source.index, 1);
newOrderedColumnIds.splice(destination.index, 0, draggableId);
currentBoard.orderedColumnIds = JSON.stringify(newOrderedColumnIds);
Expand Down Expand Up @@ -230,12 +243,12 @@ export default function Board({ perspective, channelId, agent, getProfile }: Boa
destinationColumn.tasks.splice(destination.index, 0, movedTask);

// Update orderedTaskIds in source column
const sourceOrderedTaskIds = JSON.parse(sourceColumn.orderedTaskIds);
const sourceOrderedTaskIds = safeJsonParse<string[]>(sourceColumn.orderedTaskIds, []);
sourceOrderedTaskIds.splice(source.index, 1);
sourceColumn.orderedTaskIds = JSON.stringify(sourceOrderedTaskIds);

// Update orderedTaskIds in destination column
const destOrderedTaskIds = JSON.parse(destinationColumn.orderedTaskIds);
const destOrderedTaskIds = safeJsonParse<string[]>(destinationColumn.orderedTaskIds, []);
destOrderedTaskIds.splice(destination.index, 0, draggableId);
destinationColumn.orderedTaskIds = JSON.stringify(destOrderedTaskIds);

Expand Down Expand Up @@ -273,7 +286,7 @@ export default function Board({ perspective, channelId, agent, getProfile }: Boa
}

// Create the new orderedTaskIds
const newOrderedTaskIds = JSON.parse(column.orderedTaskIds);
const newOrderedTaskIds = safeJsonParse<string[]>(column.orderedTaskIds, []);
newOrderedTaskIds.splice(source.index, 1);
newOrderedTaskIds.splice(destination.index, 0, draggableId);

Expand Down Expand Up @@ -304,7 +317,7 @@ export default function Board({ perspective, channelId, agent, getProfile }: Boa
const newColumnsWithTasks = (await Promise.all(
newColumns.map(async (column) => {
// Get tasks and order them by the columns orderedTaskIds property
const taskIds = column.orderedTaskIds ? JSON.parse(column.orderedTaskIds) : [];
const taskIds = safeJsonParse<string[]>(column.orderedTaskIds, []);
const columnTasks = await Task.findAll(perspective, { where: { id: taskIds } });
const taskMap = new Map(columnTasks.map((t) => [t.id, t]));
const orderedTasks = taskIds.map((id) => taskMap.get(id)).filter(Boolean);
Expand Down Expand Up @@ -332,7 +345,7 @@ export default function Board({ perspective, channelId, agent, getProfile }: Boa

// Update columns with tasks when columns or tasks change, unless an update is ongoing
useEffect(() => {
if (board && !updatingRef.current) getColumnsWithTasks(columns, JSON.parse(board.orderedColumnIds));
if (board && !updatingRef.current) getColumnsWithTasks(columns, safeJsonParse<string[]>(board.orderedColumnIds, []));
}, [board, columns, tasks]);

// Update board when boards subscription updates
Expand Down
19 changes: 15 additions & 4 deletions views/kanban-view-simple/src/components/TaskSettings/TaskSettings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@ import { Task, TaskColumn } from '@coasys/flux-api';
import AvatarGroup from '../AvatarGroup';
import { ColumnWithTasks } from '../Board/Board';

/** Safely parse a JSON string, returning the fallback value on failure. */
function safeJsonParse<T>(value: string | undefined | null, fallback: T): T {
if (!value) return fallback;
try {
return JSON.parse(value);
} catch {
console.warn('Failed to parse JSON value, using fallback:', value?.slice(0, 80));
return fallback;
}
Comment on lines +10 to +17

@coderabbitai coderabbitai Bot Mar 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

fd TaskSettings.tsx

Repository: coasys/flux

Length of output: 124

🏁 Script executed:

cat -n views/kanban-view-simple/src/components/TaskSettings/TaskSettings.tsx | head -200

Repository: coasys/flux

Length of output: 8566

🏁 Script executed:

wc -l views/kanban-view-simple/src/components/TaskSettings/TaskSettings.tsx

Repository: coasys/flux

Length of output: 128

Add runtime validation to safeJsonParse<string[]> to prevent runtime crashes

JSON.parse can succeed with non-array payloads (objects, strings, numbers). The call sites on lines 73, 78, 124, and 152 immediately invoke array methods (.filter(), spread operator), which will crash if the parsed value is not an array. The current fallback of [] only activates on parse errors, not when parsing succeeds with invalid data.

Add a type guard parameter to validate the parsed value:

Proposed fix 
-function safeJsonParse<T>(value: string | undefined | null, fallback: T): T {
+function safeJsonParse<T>(
+  value: string | undefined | null,
+  fallback: T,
+  isValid: (parsed: unknown) => parsed is T,
+): T {
   if (!value) return fallback;
   try {
-    return JSON.parse(value);
+    const parsed: unknown = JSON.parse(value);
+    return isValid(parsed) ? parsed : fallback;
   } catch {
     console.warn('Failed to parse JSON value, using fallback:', value?.slice(0, 80));
     return fallback;
   }
 }
+
+const isStringArray = (v: unknown): v is string[] =>
+  Array.isArray(v) && v.every((x) => typeof x === 'string');

Then update call sites to pass the validator:

-const sourceOrderedTaskIds = safeJsonParse<string[]>(source.orderedTaskIds, []).filter((id) => id !== task.id);
+const sourceOrderedTaskIds = safeJsonParse<string[]>(source.orderedTaskIds, [], isStringArray).filter((id) => id !== task.id);
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@views/kanban-view-simple/src/components/TaskSettings/TaskSettings.tsx` around
lines 10 - 17, The safeJsonParse<T> helper should reject parsed non-array values
to avoid runtime crashes when callers expect string[]; modify safeJsonParse to
accept an optional type guard (validator: (v: unknown) => v is T) and after
JSON.parse run the validator and return fallback if it fails. Update all callers
that expect string[] (the safeJsonParse<string[]>() usages in TaskSettings where
the result is immediately spread or .filter() is called) to pass Array.isArray
as the validator (or a stricter element-type guard) so only actual arrays are
returned; keep the existing try/catch fallback behavior for parse errors.

}

type Props = {
perspective: PerspectiveProxy;
channelId: string;
Expand Down Expand Up @@ -59,12 +70,12 @@ export default function TaskSettings({
const destination = columns.find((col) => col.columnName === taskColumn);

// Update orderedTaskIds in source column
const sourceOrderedTaskIds = JSON.parse(source.orderedTaskIds).filter((id) => id !== task.id);
const sourceOrderedTaskIds = safeJsonParse<string[]>(source.orderedTaskIds, []).filter((id) => id !== task.id);
source.orderedTaskIds = JSON.stringify(sourceOrderedTaskIds);
await source.save(batchId);
Comment on lines +73 to 75

@coderabbitai coderabbitai Bot Mar 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard .find(...) results before dereferencing

The code dereferences source, destination, and columnModel immediately after .find(...). If live data changes between UI action and save/delete, this will crash.

Proposed fix (defensive null checks) 
 const source = columns.find((col) => col.columnName === column.columnName);
 const destination = columns.find((col) => col.columnName === taskColumn);
+if (!source || !destination) {
+  console.error('Source or destination column not found');
+  setSaving(false);
+  return;
+}
@@
 const columnModel = columns.find((col) => col.columnName === taskColumn);
+if (!columnModel) {
+  console.error('Column not found when creating task');
+  setSaving(false);
+  return;
+}
@@
 const columnModel = columns.find((col) => col.id === column.id);
+if (!columnModel) {
+  console.error('Column not found when deleting task');
+  setDeleting(false);
+  return;
+}

Also applies to: 78-80, 123-126, 151-154

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@views/kanban-view-simple/src/components/TaskSettings/TaskSettings.tsx` around
lines 73 - 75, The code assumes .find(...) returned objects (e.g., source,
destination, columnModel) and dereferences them immediately (e.g., using
source.orderedTaskIds and source.save(batchId)); add defensive null checks after
each .find call in TaskSettings.tsx and bail out or handle the missing object
(early return, user-facing error, or log and skip) before using properties or
calling methods; specifically guard the variables named source, destination, and
columnModel where they are used around the shown snippets so you never call
.orderedTaskIds, .save(...), or access columnModel properties on undefined.


// Update orderedTaskIds in destination column
const destinationOrderedTaskIds = [...(JSON.parse(destination.orderedTaskIds) || []), task.id];
const destinationOrderedTaskIds = [...safeJsonParse<string[]>(destination.orderedTaskIds, []), task.id];
destination.orderedTaskIds = JSON.stringify(destinationOrderedTaskIds);
await destination.save(batchId);
}
Expand Down Expand Up @@ -110,7 +121,7 @@ export default function TaskSettings({

// Store the task position in the column
const columnModel = columns.find((col) => col.columnName === taskColumn);
const newOrderedTaskIds = [...(JSON.parse(columnModel.orderedTaskIds) || []), newTaskModel.id];
const newOrderedTaskIds = [...safeJsonParse<string[]>(columnModel.orderedTaskIds, []), newTaskModel.id];
columnModel.orderedTaskIds = JSON.stringify(newOrderedTaskIds);
await columnModel.save(batchId);

Expand Down Expand Up @@ -138,7 +149,7 @@ export default function TaskSettings({

// Update orderedTaskIds in column
const columnModel = columns.find((col) => col.id === column.id);
const newOrderedTaskIds = JSON.parse(column.orderedTaskIds).filter((id: string) => id !== task.id);
const newOrderedTaskIds = safeJsonParse<string[]>(column.orderedTaskIds, []).filter((id: string) => id !== task.id);
columnModel.orderedTaskIds = JSON.stringify(newOrderedTaskIds);
await columnModel.save(batchId);

Expand Down
Loading