feat(projects): kanban / swimlane board views#265
Merged
Conversation
📝 WalkthroughWalkthroughAdds a Projects board feature: kanban/lanes UI with drag‑and‑drop, filtering/grouping, task modal and covers, SSE-backed live updates, backend event broker and SSE route, extended projects API (comments/relationships/patch), types, CSS tokens, and comprehensive unit/e2e tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Browser (User)
participant UI as ProjectBoard
participant DnD as dndAction
participant API as projectsApi
participant Broker as EventBroker
participant Other as Other Browser
User->>UI: drag task card to "Claimed"
UI->>DnD: compute actions (task, source, target, groupBy, laneKey)
DnD-->>UI: return API call descriptors or blocked reason
UI->>API: execute API calls (update/claim/release/close)
API->>Broker: publish(task.* event)
API-->>UI: respond success
Broker->>Other: SSE event delivered
Other->>Other: applyEvent -> update UI
sequenceDiagram
participant Browser as UI
participant Data as useBoardData
participant Live as useBoardLive
participant API as projectsApi
Browser->>Data: mount(projectId)
Data->>API: list(open, claimed, closed)
API-->>Data: task lists
Data->>Data: dedupe & set tasks
Browser->>Live: mount(projectId, onEvent)
Live->>API: subscribeEvents(projectId, onEvent)
Note over API,Live: SSE later: task.claimed arrives
Live->>Data: applyEvent(event)
Data->>Browser: UI re-renders with updated task
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
…ift, just-claimed pulse
…le, DnD dispatch, persistence
…=<id> modal routing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
desktop/src/apps/ProjectsApp/board/__tests__/ProjectBoard.test.tsx-22-26 (1)
22-26:⚠️ Potential issue | 🟡 Minor“Toggles” test only asserts one-way switch.
The test title claims bidirectional toggle behavior, but it verifies only the Kanban selection state.
🧪 Suggested patch
- it("toggles between Lanes and Kanban modes", async () => { + it("toggles between Lanes and Kanban modes", async () => { render(<ProjectBoard projectId="p1" currentUserId="u1" />); - fireEvent.click(await screen.findByRole("tab", { name: /Kanban/ })); - expect(screen.getByRole("tab", { name: /Kanban/ })).toHaveAttribute("aria-selected", "true"); + const kanban = await screen.findByRole("tab", { name: /Kanban/ }); + const lanes = screen.getByRole("tab", { name: /Lanes/ }); + + fireEvent.click(kanban); + expect(kanban).toHaveAttribute("aria-selected", "true"); + + fireEvent.click(lanes); + expect(lanes).toHaveAttribute("aria-selected", "true"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/__tests__/ProjectBoard.test.tsx` around lines 22 - 26, The test "toggles between Lanes and Kanban modes" currently only verifies selecting the Kanban tab; update it to assert the full two-way toggle by (1) clicking the "Kanban" tab (using screen.findByRole / fireEvent.click) and asserting "Kanban" has aria-selected="true" and "Lanes" has aria-selected="false", then (2) clicking the "Lanes" tab and asserting "Lanes" has aria-selected="true" and "Kanban" has aria-selected="false" so the ProjectBoard component's tab switching is validated in both directions.desktop/src/apps/ProjectsApp/board/boardFiltering.ts-15-16 (1)
15-16:⚠️ Potential issue | 🟡 Minor
hasAttachmentsis exposed but currently does nothing.
applyFiltersintentionally passes throughhasAttachments, while the UI allows users to toggle it. That creates a misleading no-op filter state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/boardFiltering.ts` around lines 15 - 16, The hasAttachments filter is a no-op right now; update the task-filtering logic in applyFilters (in boardFiltering.ts) to actually respect the hasAttachments parameter: when hasAttachments is true return only tasks where t.attachments && t.attachments.length > 0, and when hasAttachments is false return only tasks where !t.attachments || t.attachments.length === 0; make this check alongside the other filter predicates so undefined attachment arrays are handled safely and the UI toggle no longer becomes a misleading passthrough.desktop/src/apps/ProjectsApp/board/TaskModal.tsx-72-75 (1)
72-75:⚠️ Potential issue | 🟡 MinorGuard against stale
onUpdatedresponses overwriting current task view.
setTask(t)unconditionally replaces the active modal task. If an older async save resolves after navigating to another task, it can switch the modal back unexpectedly.Proposed fix
onUpdated={(t) => { - setTask(t); + setTask(prev => (prev?.id === t.id ? t : prev)); setAllTasks(prev => prev.map(x => x.id === t.id ? t : x)); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/TaskModal.tsx` around lines 72 - 75, The onUpdated handler currently calls setTask(t) unconditionally and can overwrite the modal if a stale async save resolves; change the handler to guard before updating the modal task by checking the current task in state (use the functional form like setTask(prev => prev && prev.id === t.id ? t : prev)) so only the open modal is replaced, and still update the list with setAllTasks(prev => prev.map(x => x.id === t.id ? t : x)); keep the same onUpdated arrow but add this identity check to prevent stale responses from switching the modal.desktop/src/apps/ProjectsApp/board/TaskCard.module.css-7-12 (1)
7-12:⚠️ Potential issue | 🟡 MinorRename keyframe to kebab-case to satisfy stylelint.
The animation/keyframe identifier violates the configured
keyframes-name-pattern.Proposed fix
-.justClaimed { animation: justClaimed 1.4s ease-out 1; border-color: rgba(245,158,11,.4); } -@keyframes justClaimed { +.justClaimed { animation: just-claimed 1.4s ease-out 1; border-color: rgba(245,158,11,.4); } +@keyframes just-claimed {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/TaskCard.module.css` around lines 7 - 12, The keyframes identifier justClaimed violates the project's keyframes-name-pattern; rename the animation/keyframe to kebab-case (e.g., just-claimed) and update the .justClaimed rule to reference the new name (change animation: justClaimed → animation: just-claimed and `@keyframes` justClaimed → `@keyframes` just-claimed), then update any code that uses the CSS module class (e.g., references to styles.justClaimed or className={styles.justClaimed}) to use the same module classname if you also rename the export, or keep the JS/TS classname and only change the keyframe name to avoid breaking imports.tests/projects/test_events_route.py-85-85 (1)
85-85:⚠️ Potential issue | 🟡 MinorRename ambiguous loop variable names to satisfy lint and readability.
Lines 85 and 104 use
l, which triggers Ruff E741.Suggested fix
- data_lines = [l for l in lines if l.startswith("data:")] + data_lines = [line for line in lines if line.startswith("data:")] ... - heartbeat_lines = [l for l in lines if l.startswith(":")] + heartbeat_lines = [line for line in lines if line.startswith(":")]Also applies to: 104-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/projects/test_events_route.py` at line 85, The list comprehension and subsequent loop use the ambiguous single-letter variable name `l`, which triggers Ruff E741; update both uses (the `data_lines = [l for l in lines if l.startswith("data:")]` comprehension and the later loop at the other occurrence) to a clearer identifier like `line` or `raw_line` (e.g., `data_lines = [line for line in lines if line.startswith("data:")]` and similarly rename the loop variable) to satisfy the linter and improve readability, keeping all behavior unchanged.desktop/src/apps/ProjectsApp/board/BoardToolbar.tsx-65-65 (1)
65-65:⚠️ Potential issue | 🟡 MinorLive status text is misleading when disconnected.
Line 65 always displays
● Liveeven whenp.liveis false.Suggested fix
- <span className={`${styles.pill} ${p.live ? styles.live : styles.dead}`}>● Live</span> + <span className={`${styles.pill} ${p.live ? styles.live : styles.dead}`}> + {p.live ? "● Live" : "● Offline"} + </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/BoardToolbar.tsx` at line 65, The span currently always displays "● Live" even when the prop p.live is false; update the render in BoardToolbar (the span using styles.pill with styles.live / styles.dead and the p.live prop) to render text conditionally—e.g., show "● Live" when p.live is true and "● Disconnected" (or "● Offline"/"● Dead" as preferred) when p.live is false—so the displayed label matches the applied style (styles.live vs styles.dead).desktop/src/apps/ProjectsApp/board/ProjectBoard.tsx-52-57 (1)
52-57:⚠️ Potential issue | 🟡 MinorOlder claim timers can clear a newer highlight early.
Each
task.claimedevent starts a freshsetTimeout, but the previous timeout is never cancelled. If a second claim arrives before the first 1.5s window ends, the first timer will still fire and clearjustClaimedfor the newer task too. Store the timer id in a ref and clear it before scheduling the next one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/ProjectBoard.tsx` around lines 52 - 57, The task.claimed handler in ProjectBoard uses setTimeout to clear justClaimed but never cancels previous timers, so an older timeout can clear a newer highlight; fix this by keeping the timer id in a ref (e.g., claimTimeoutRef) and call clearTimeout(claimTimeoutRef.current) before creating a new setTimeout, then store the new id in claimTimeoutRef.current and clear it when unmounting (useEffect cleanup) to avoid leaks; update the code around the e.kind === "task.claimed" branch and the setJustClaimed usage accordingly.
🧹 Nitpick comments (11)
desktop/src/apps/ProjectsApp/board/BoardColumn.module.css (1)
7-7: Consider adding a placeholder comment inside the empty block to satisfy the linter.The static analysis tool flagged this empty block. While the inline comment explains the intent, adding content inside the block (or disabling the rule inline) would silence the warning.
💡 Suggested fix
-.ready, .claimed, .closed {} /* status modifiers reserved for future glow */ +.ready, .claimed, .closed { /* stylelint-disable-next-line block-no-empty */ } /* status modifiers reserved for future glow */Or alternatively, add a harmless declaration:
-.ready, .claimed, .closed {} /* status modifiers reserved for future glow */ +.ready, .claimed, .closed { /* reserved for future glow effects */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/BoardColumn.module.css` at line 7, The empty CSS rule for the status modifiers (.ready, .claimed, .closed) triggers the linter; open the block in BoardColumn.module.css and add a harmless placeholder (for example a single-line comment or a no-op declaration) inside the .ready, .claimed, .closed {} rule to satisfy the linter while preserving the intent that these modifiers are reserved for future styles.tinyagentos/routes/projects.py (1)
488-500: Consider version compatibility before switching toTimeoutError.While
asyncio.TimeoutErroris deprecated in Python 3.11+, this project supports Python 3.10+ (requires-python = ">=3.10"inpyproject.toml). In Python 3.10 and earlier,asyncio.TimeoutErroris a distinct exception class separate from the built-inTimeoutError. Using onlyTimeoutErrorwould not catch timeouts fromasyncio.wait_for()in Python 3.10.If targeting Python 3.11+, the change is safe. For Python 3.10 compatibility, either keep
_asyncio.TimeoutErroror catch both exceptions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/routes/projects.py` around lines 488 - 500, The event_stream coroutine currently catches _asyncio.TimeoutError only, which is required for Python 3.10 but would be redundant in 3.11+; update the exception handling in the try/except around await _asyncio.wait_for(queue.get(), timeout=15.0) inside event_stream to be compatible across supported versions by catching both _asyncio.TimeoutError and the built-in TimeoutError (e.g., except (_asyncio.TimeoutError, TimeoutError):) or by keeping _asyncio.TimeoutError and adding TimeoutError as a second clause so timeouts from wait_for(queue.get()) are handled on Python 3.10 and 3.11+. Ensure the finally block still calls await broker.unsubscribe(project_id, queue).tests/projects/test_routes_update_parent.py (1)
54-61: Assert the setup PATCHes succeed before the cycle assertion.Line 54 and Line 58 currently ignore setup responses. Add
200asserts so this test only passes when the chain is truly established.Proposed test hardening
- await client.patch( + r1 = await client.patch( f"/api/projects/{pid}/tasks/{a['id']}", json={"parent_task_id": b["id"]}, ) - await client.patch( + assert r1.status_code == 200 + + r2 = await client.patch( f"/api/projects/{pid}/tasks/{b['id']}", json={"parent_task_id": c["id"]}, ) + assert r2.status_code == 200🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/projects/test_routes_update_parent.py` around lines 54 - 61, The two setup PATCH requests that establish the parent chain (the calls to client.patch for f"/api/projects/{pid}/tasks/{a['id']}" and f"/api/projects/{pid}/tasks/{b['id']}") currently ignore their responses; update the test to capture each response and assert response.status_code == 200 for both before proceeding to the cycle detection/assertion so the test only continues when the parent relationships are actually created.desktop/src/apps/ProjectsApp/board/__tests__/useBoardLive.test.tsx (1)
26-30: Add a resubscribe test whenprojectIdchanges.A rerender test (
p1→p2) would lock down cleanup of the old subscription and creation of the new one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/__tests__/useBoardLive.test.tsx` around lines 26 - 30, Add a rerender test for useBoardLive that verifies cleanup and resubscription when the projectId changes: mock projectsApi.subscribeEvents to return a jest/vi unsubscribe function you can spy on, renderHook(() => useBoardLive("p1", () => {})) then rerender with projectId "p2", and assert that subscribeEvents was called for both "p1" and "p2" and that the unsubscribe returned from the first subscribeEvents call was invoked (ensuring the old subscription was cleaned up) while a new subscription was created for the new projectId.desktop/src/lib/__tests__/projects-tasks.test.ts (1)
50-58: Add an assertion for JSON message forwarding insubscribeEventstests.This currently validates construction/cleanup only. Please also invoke
onmessageand assert the callback receives parsedProjectEvent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/lib/__tests__/projects-tasks.test.ts` around lines 50 - 58, Update the test for projectsApi.subscribeEvents to also exercise JSON message forwarding: when you create the eventSourceMock return value (the object with close and onmessage), after calling projectsApi.subscribeEvents("p1", callback) set that mock object's onmessage to a function or directly invoke onmessage with a MessageEvent whose data is a JSON string representing a ProjectEvent, then assert the provided callback was called with the parsed ProjectEvent object; reference the subscribeEvents function and the projectsApi.subscribeEvents call in the test so you locate where to invoke the onmessage handler and add the assertion.desktop/src/lib/projects.ts (1)
146-150: Use a dedicatedUpdateTaskPatchtype instead ofPartial<ProjectTask>.The frontend
ProjectTasktype includes server-managed fields (id,project_id,status,claimed_by,closed_at,closed_by, etc.), and usingPartial<ProjectTask>for the patch parameter allows client code to mistakenly pass these fields. While the backend correctly rejects them via theUpdateTaskInmodel, the frontend type should accurately reflect the contract—onlytitle,body,priority,labels,assignee_id, andparent_task_idare patchable fields. Define a dedicatedUpdateTaskPatchtype to avoid confusion and improve type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/lib/projects.ts` around lines 146 - 150, Define a new UpdateTaskPatch type listing only the allowed patchable fields (title, body, priority, labels, assignee_id, parent_task_id) and replace the current patch parameter type in the update function signature from Partial<ProjectTask> to UpdateTaskPatch; update the update method declaration (update: (pid: string, tid: string, patch: Partial<ProjectTask>) => ...) to use UpdateTaskPatch and ensure any callers are adjusted to the narrower type so client code cannot pass server-managed fields like id, project_id, status, claimed_by, closed_at, or closed_by.desktop/src/apps/ProjectsApp/board/TaskCardCover.module.css (1)
6-7: Respect reduced-motion preference for cursor blink.The infinite blink animation should be disabled for users who request reduced motion.
♿ Suggested patch
.cursor { display: inline-block; width: 6px; height: 11px; background: var(--board-accent-violet); animation: blink 1.1s steps(2) infinite; } `@keyframes` blink { 0%, 50% { opacity: 1; } 50.01%, 100% { opacity: 0; } } +@media (prefers-reduced-motion: reduce) { + .cursor { animation: none; } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/TaskCardCover.module.css` around lines 6 - 7, The cursor blink animation (.cursor and `@keyframes` blink) should be disabled for users who prefer reduced motion: add a prefers-reduced-motion media query that targets .cursor and sets animation: none (or animation-duration: 0s) so the blink is not played, leaving the keyframes intact for other users.desktop/src/apps/ProjectsApp/board/modal/SubTasks.tsx (1)
13-14: Make the sub-task checkbox explicitly non-interactive.These controls are read-only display, but still appear actionable. Prefer
disabled(or a non-form visual indicator) to avoid interaction ambiguity.Suggested change
- <input type="checkbox" checked={c.status === "closed"} readOnly aria-label={c.title} /> + <input type="checkbox" checked={c.status === "closed"} disabled aria-label={c.title} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/modal/SubTasks.tsx` around lines 13 - 14, The checkbox in SubTasks.tsx is currently readOnly but still appears actionable; change the input element for sub-tasks (the checkbox using checked={c.status === "closed"} and aria-label={c.title}) to be explicitly non-interactive by adding the disabled attribute (or replace it with a non-form visual indicator) so users cannot attempt interaction and screen readers still get proper state via aria attributes.desktop/src/apps/ProjectsApp/board/TaskModal.module.css (1)
19-20: Add visible keyboard focus styles for custom button roles.
[role="button"]currently has hover-only affordance. Add:focus-visibleparity so keyboard navigation has a clear focus indicator.Suggested CSS tweak
.layout > aside [role="button"] { cursor: pointer; } .layout > aside [role="button"]:hover { color: `#fff`; } +.layout > aside [role="button"]:focus-visible { + outline: 2px solid rgba(167,139,250,.75); + outline-offset: 2px; + color: `#fff`; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/TaskModal.module.css` around lines 19 - 20, The CSS selector ".layout > aside [role=\"button\"]" only defines a hover state; add a matching ":focus-visible" rule so keyboard users see the same visible focus affordance as hover. Update the stylesheet by adding a ".layout > aside [role=\"button\"]:focus-visible" rule (and optionally ".layout > aside [role=\"button\"]:focus") that mirrors the hover styles (e.g., color and any outline/box-shadow) to ensure parity for keyboard navigation and assistive tech.desktop/src/apps/ProjectsApp/board/BoardFilters.tsx (1)
22-25: Add expanded-state semantics on the filter trigger.Expose popover state on the
+ Filterbutton (aria-expanded,aria-haspopup) so assistive tech can understand toggle behavior.Proposed fix
- <button type="button" className={styles.pill} onClick={() => setOpen(o => !o)}>+ Filter</button> + <button + type="button" + className={styles.pill} + aria-expanded={open} + aria-haspopup="dialog" + onClick={() => setOpen(o => !o)} + > + + Filter + </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/BoardFilters.tsx` around lines 22 - 25, The filter toggle button lacks accessible expanded-state semantics; update the button (the element rendering "+ Filter" that calls setOpen and reads open) to include aria-expanded set to the current open state and aria-haspopup="dialog", and add an aria-controls pointing at the popover's id; also give the popover div (role="dialog", the element rendered when open) a stable id (e.g., "filters-popover") so aria-controls can reference it, ensuring assistive tech can understand the toggle behavior.desktop/src/apps/ProjectsApp/board/boardGrouping.ts (1)
66-69: Optional: de-duplicate per-task labels before bucketing.If a task contains repeated labels, it will be duplicated in the same lane.
Suggested fix
- for (const lbl of t.labels) { + for (const lbl of new Set(t.labels)) { if (!buckets.has(lbl)) buckets.set(lbl, []); buckets.get(lbl)!.push(t); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/boardGrouping.ts` around lines 66 - 69, Tasks are being added to the same bucket multiple times when a task's t.labels contains duplicates; before iterating labels (the loop using "for (const lbl of t.labels)") deduplicate the labels for that task (e.g., convert t.labels to a Set or otherwise unique array) so when you call buckets.set/get and push t into buckets the task is only added once per distinct label; update the loop to iterate over the unique labels for each task while still using the existing buckets map and push logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@desktop/src/apps/ProjectsApp/board/boardDnd.ts`:
- Around line 69-73: The label branch currently only appends targetLbl and
clears all labels for "__unlabeled__", causing incorrect membership and loss of
metadata; update the logic in the "label" case to use the source lane label
(e.g., the original lane key / sourceLabel passed into the DnD handler) and
produce a labels array that removes sourceLabel and conditionally adds
targetLbl: if sourceLabel === targetLbl return null; if targetLbl is non-null
set labels = [...task.labels.filter(l => l !== sourceLabel), targetLbl]
(avoiding duplicates); if targetLbl is null set labels = task.labels.filter(l =>
l !== sourceLabel) so you only remove the moved label and preserve other
metadata labels like cover:*. Ensure you reference laneKey/targetLbl,
task.labels and the source lane identifier when implementing this change.
In `@desktop/src/apps/ProjectsApp/board/modal/Activity.tsx`:
- Around line 24-28: The submit handler (submit) currently posts a comment using
projectsApi.tasks.addComment with author_id: currentUserId even when
currentUserId may be empty; add a guard in submit to return early (or show/throw
an error) if currentUserId is falsy and do not call addComment, and also ensure
any UI submit button is disabled when currentUserId is empty so setComments is
only called after a successful addComment with a valid author_id.
- Around line 15-20: The async calls in the useEffect and the comment submit
path are unguarded and can cause unhandled rejections; wrap the await
projectsApi.tasks.listComments(...) inside a try/catch in the useEffect
(preserving the cancelled flag behavior and only calling setComments on success)
and likewise wrap the submit POST call (e.g., the handler that calls
projectsApi.tasks.createComment or similar) in try/catch to handle errors,
update UI state (spinner/error state) consistently, and log or surface the error
to the user; ensure you do not swallow cancellations and keep the cancelled flag
check before calling setComments or other state updates.
In `@desktop/src/apps/ProjectsApp/board/modal/MetadataPane.tsx`:
- Line 32: The onSave handler currently passes Number(v) directly to save which
can produce NaN; before calling save in MetadataPane's onSave, validate and
parse v (e.g., use parseInt/Number and check Number.isFinite or !Number.isNaN)
and only include priority in the payload when it is a valid number—otherwise
omit the priority field or send null/default; update the onSave wrapper around
save to perform this guard so save({ priority: ... }) never receives NaN.
- Line 64: The draft state (const [draft, setDraft] = useState(value)) can
become stale when the prop value changes; update the component to sync draft
with incoming value by adding an effect that sets setDraft(value) whenever value
(or the specific task prop) changes, ensuring editors reopened after parent
updates reflect the latest value while preserving local edits when appropriate.
- Around line 73-76: The Enter key handler and the blur handler both call
onSave, causing duplicate saves; update the handlers in MetadataPane.tsx to
suppress the blur save when Enter initiated the save: add a useRef boolean
(e.g., suppressBlurSaveRef), set suppressBlurSaveRef.current = true in the
onKeyDown Enter branch after calling onSave(draft), and in onBlur first check if
(suppressBlurSaveRef.current) { suppressBlurSaveRef.current = false; return; }
otherwise call onSave(draft); keep references to onSave, onCancel, setDraft,
draft and value so you reset state correctly (setDraft(value) and onCancel())
when Escape is pressed.
In `@desktop/src/apps/ProjectsApp/board/modal/Relationships.tsx`:
- Around line 9-15: The async IIFE calling projectsApi.tasks.listRelationships
in the effect can reject and cause unhandled rejections; wrap the Promise.all
call and subsequent setRels([...from, ...to]) in a try/catch, log or report the
error, and provide a safe fallback (e.g., setRels([]) or leave rels unchanged)
while still guarding with the cancelled flag before calling setRels; update the
IIFE around projectsApi.tasks.listRelationships, the catch should handle both
"from" and "to" failures and prevent unhandled promise rejections.
In `@desktop/src/apps/ProjectsApp/board/ProjectBoard.tsx`:
- Around line 93-105: The current rollback in ProjectBoard.tsx restores the
entire snapshot via setTasks(snapshot) when any awaited call in result.calls
fails, which can clobber concurrent SSE/live updates; instead on error either
(A) trigger a safe refetch of the current task list from the server (call the
existing tasks-fetching routine used by this component) and setTasks to that
fresh data, or (B) perform a targeted rollback of only the moved task by
computing the task's original index/state from the saved snapshot and updating
just that task in state (use the same task id from result.calls to locate it)
rather than replacing the whole tasks array; update the catch block around the
projectsApi.tasks.* calls to implement one of these approaches and remove
setTasks(snapshot) full-array restores.
- Around line 35-42: The persisted prefs read in the useEffect
(localStorage.getItem(PERSIST_KEY(projectId))) must be validated before
restoring: instead of blindly calling setViewMode and setGroupBy with values
from JSON, check that the parsed p.viewMode is a valid ViewMode and p.groupBy is
a valid GroupBy (e.g., compare against Object.values(ViewMode) and
Object.values(GroupBy) or your allowed arrays) and only call
setViewMode(p.viewMode) / setGroupBy(p.groupBy) when those checks pass;
otherwise ignore the stale/unknown value or fall back to defaults so the
downstream lookup (the code that uses groupBy at fn(filtered)) cannot receive
undefined and throw.
In `@desktop/src/apps/ProjectsApp/board/TaskCardCover.tsx`:
- Around line 24-56: TaskCardCover is rendering empty shells when kind is "code"
or "terminal" because callers only pass kind (no payload in data); either
prevent inferCoverKind from returning payload-dependent kinds or add fallbacks
in TaskCardCover: update inferCoverKind to only return "code" or "terminal" when
the task contains the necessary preview payload (e.g., attachments/metadata)
instead of purely from labels, or modify TaskCardCover to detect missing data
prop (when kind === "code" or "terminal" and data is falsy or lacks
snippet/lines) and fall back to a safe cover ("screenshot", "gradient", or
"none"); refer to inferCoverKind and TaskCardCover and the component props kind
and data to locate where to implement the change.
In `@desktop/src/apps/ProjectsApp/board/TaskModal.tsx`:
- Around line 24-33: The useEffect that calls projectsApi.tasks.list(projectId)
can reject and leave the modal loading; wrap the async call in try/catch and
handle failures: inside the IIFE catch the error, check the cancelled flag
before mutating state, call setAllTasks([]) or setTask(null) and set an
error/loading state (or call the existing error handler) so the modal doesn't
hang, and ensure the cleanup (cancelled) is respected; update the effect around
useEffect, the anonymous async IIFE, and the places where setAllTasks/setTask
are called to handle the error path.
- Around line 38-42: The global onKey handler in TaskModal.tsx (function onKey
using KeyboardEvent and calling onClose/onNext/onPrev) triggers navigation even
when the user is typing; update onKey to ignore events whose target is an
editable element by checking that e.target is an HTMLElement and returning early
if it's an input, textarea, select, or any element with isContentEditable true
(or matches a contenteditable selector). Keep the existing Escape/Arrow handling
for non-editable contexts and ensure onClose/onNext/onPrev behavior is unchanged
when the event is not from an editable field.
In `@desktop/src/apps/ProjectsApp/board/useBoardData.ts`:
- Around line 13-29: Wrap the initial IIFE that calls
projectsApi.tasks.list(...) in a try/catch/finally so any thrown error doesn't
leave loading stuck; specifically, around the Promise.all call in the anonymous
async function, catch errors from projectsApi.tasks.list and in the catch set an
appropriate fallback (e.g., setTasks([]) or leave existing tasks) and log or
surface the error, and ensure setLoading(false) is always called in a finally
block; reference the existing variables/functions: the anonymous async IIFE,
projectsApi.tasks.list, setTasks, setLoading, and cancelled to avoid updating
state after cancellation.
In `@desktop/src/apps/ProjectsApp/board/useBoardLive.ts`:
- Around line 16-20: The current useBoardLive sets connected via
setConnected(true) optimistically before the EventSource actually opens and
never flips on mid-stream failures; either rename the state to reflect
subscription (e.g., subscribed) and keep the optimistic setConnected(true)
behavior, or wire setConnected to the EventSource lifecycle: remove the
immediate setConnected(true), attach handlers on the EventSource instance
(onopen => setConnected(true), onerror/onclose => setConnected(false)), and
ensure cleanup (off()/close()) still sets active=false and unsubscribes; update
usages of setConnected/connected accordingly (or rename to
setSubscribed/subscribed) and preserve existing active/off logic.
In `@desktop/src/apps/ProjectsApp/ProjectWorkspace.tsx`:
- Around line 29-39: The auth logic needs an explicit "resolved" flag and the
TaskModal should only mount when there's a valid user id; add a new state like
authResolved (boolean) in ProjectWorkspace, set it to true in both the fetch
success path (after checking u?.id and conditionally calling
setCurrentUserId(u.id)) and in the catch/finally path so failures don't leave
the UI stuck loading, and change the TaskModal render condition to require
authResolved && currentUserId (or Boolean(currentUserId)) so the modal never
mounts with an empty/invalid user id; update any logic referencing currentUserId
(e.g., setCurrentUserId, TaskModal props) accordingly.
In `@desktop/src/lib/projects.ts`:
- Around line 180-186: subscribeEvents currently only exposes onmessage so
callers can’t detect SSE open/error/close; change its signature to add an
optional onState callback (e.g. subscribeEvents(projectId: string, onEvent: (ev:
ProjectEvent)=>void, onState?: (state: 'open'|'error'|'closed', err?:
any)=>void): () => void) and wire EventSource.onopen to call onState('open'),
EventSource.onerror to call onState('error', event) and the returned cleanup
function to call onState('closed') before closing; keep onState optional for
backwards compatibility and ensure JSON parse try/catch remains.
In `@tests/e2e/test_projects_board.py`:
- Line 83: The tests use hardcoded project slugs (calls to
_create_project_and_tasks) which cause collisions across persistent or parallel
runs; update the test calls (the _create_project_and_tasks invocations at the
shown locations and the other affected calls) to generate and pass a unique slug
per run (e.g., append a timestamp/UUID or use pytest-provided unique
identifiers) or change _create_project_and_tasks to accept an optional slug and
auto-generate a unique one when none is provided so each test run creates
non-colliding project slugs.
- Line 88: The locator is passing a lambda to page.get_by_role(name=...), which
is invalid in Playwright Python; replace the callable with a string or regex
Pattern (e.g., use re.compile or a literal "Closed") in the get_by_role call so
expect(page.get_by_role(...)).to_be_visible() uses a supported name filter; also
add an import for re if you choose a regex. Ensure you update the get_by_role
invocation (the call currently at expect(page.get_by_role("region", name=lambda
n: "Closed" in (n or ""))).to_be_visible()) to use the Pattern[str] or string
instead.
In `@tests/projects/test_events_route.py`:
- Around line 100-102: The test is using _collect_sse_lines to read a single SSE
line with timeout=16.0 which is too tight for a 15s heartbeat; increase the wait
margin (e.g., set timeout to around 20.0 or higher) in the call within
tests/projects/test_events_route.py so _collect_sse_lines(app,
f"/api/projects/{pid}/events", cookies, n_lines=1, timeout=...) has a larger
timeout to avoid CI jitter-related flakes.
- Around line 64-65: The except block in the task cleanup currently swallows all
exceptions via "except (asyncio.CancelledError, Exception): pass"; change this
so only asyncio.CancelledError is suppressed and any other Exception is
re-raised (or allowed to propagate) so real failures in the ASGI app surface in
tests—i.e., replace the combined tuple with "except asyncio.CancelledError:
pass" and remove the blanket Exception catch (or add "except Exception: raise"
if you need explicit handling) in the task cleanup path in
tests/projects/test_events_route.py.
---
Minor comments:
In `@desktop/src/apps/ProjectsApp/board/__tests__/ProjectBoard.test.tsx`:
- Around line 22-26: The test "toggles between Lanes and Kanban modes" currently
only verifies selecting the Kanban tab; update it to assert the full two-way
toggle by (1) clicking the "Kanban" tab (using screen.findByRole /
fireEvent.click) and asserting "Kanban" has aria-selected="true" and "Lanes" has
aria-selected="false", then (2) clicking the "Lanes" tab and asserting "Lanes"
has aria-selected="true" and "Kanban" has aria-selected="false" so the
ProjectBoard component's tab switching is validated in both directions.
In `@desktop/src/apps/ProjectsApp/board/boardFiltering.ts`:
- Around line 15-16: The hasAttachments filter is a no-op right now; update the
task-filtering logic in applyFilters (in boardFiltering.ts) to actually respect
the hasAttachments parameter: when hasAttachments is true return only tasks
where t.attachments && t.attachments.length > 0, and when hasAttachments is
false return only tasks where !t.attachments || t.attachments.length === 0; make
this check alongside the other filter predicates so undefined attachment arrays
are handled safely and the UI toggle no longer becomes a misleading passthrough.
In `@desktop/src/apps/ProjectsApp/board/BoardToolbar.tsx`:
- Line 65: The span currently always displays "● Live" even when the prop p.live
is false; update the render in BoardToolbar (the span using styles.pill with
styles.live / styles.dead and the p.live prop) to render text
conditionally—e.g., show "● Live" when p.live is true and "● Disconnected" (or
"● Offline"/"● Dead" as preferred) when p.live is false—so the displayed label
matches the applied style (styles.live vs styles.dead).
In `@desktop/src/apps/ProjectsApp/board/ProjectBoard.tsx`:
- Around line 52-57: The task.claimed handler in ProjectBoard uses setTimeout to
clear justClaimed but never cancels previous timers, so an older timeout can
clear a newer highlight; fix this by keeping the timer id in a ref (e.g.,
claimTimeoutRef) and call clearTimeout(claimTimeoutRef.current) before creating
a new setTimeout, then store the new id in claimTimeoutRef.current and clear it
when unmounting (useEffect cleanup) to avoid leaks; update the code around the
e.kind === "task.claimed" branch and the setJustClaimed usage accordingly.
In `@desktop/src/apps/ProjectsApp/board/TaskCard.module.css`:
- Around line 7-12: The keyframes identifier justClaimed violates the project's
keyframes-name-pattern; rename the animation/keyframe to kebab-case (e.g.,
just-claimed) and update the .justClaimed rule to reference the new name (change
animation: justClaimed → animation: just-claimed and `@keyframes` justClaimed →
`@keyframes` just-claimed), then update any code that uses the CSS module class
(e.g., references to styles.justClaimed or className={styles.justClaimed}) to
use the same module classname if you also rename the export, or keep the JS/TS
classname and only change the keyframe name to avoid breaking imports.
In `@desktop/src/apps/ProjectsApp/board/TaskModal.tsx`:
- Around line 72-75: The onUpdated handler currently calls setTask(t)
unconditionally and can overwrite the modal if a stale async save resolves;
change the handler to guard before updating the modal task by checking the
current task in state (use the functional form like setTask(prev => prev &&
prev.id === t.id ? t : prev)) so only the open modal is replaced, and still
update the list with setAllTasks(prev => prev.map(x => x.id === t.id ? t : x));
keep the same onUpdated arrow but add this identity check to prevent stale
responses from switching the modal.
In `@tests/projects/test_events_route.py`:
- Line 85: The list comprehension and subsequent loop use the ambiguous
single-letter variable name `l`, which triggers Ruff E741; update both uses (the
`data_lines = [l for l in lines if l.startswith("data:")]` comprehension and the
later loop at the other occurrence) to a clearer identifier like `line` or
`raw_line` (e.g., `data_lines = [line for line in lines if
line.startswith("data:")]` and similarly rename the loop variable) to satisfy
the linter and improve readability, keeping all behavior unchanged.
---
Nitpick comments:
In `@desktop/src/apps/ProjectsApp/board/__tests__/useBoardLive.test.tsx`:
- Around line 26-30: Add a rerender test for useBoardLive that verifies cleanup
and resubscription when the projectId changes: mock projectsApi.subscribeEvents
to return a jest/vi unsubscribe function you can spy on, renderHook(() =>
useBoardLive("p1", () => {})) then rerender with projectId "p2", and assert that
subscribeEvents was called for both "p1" and "p2" and that the unsubscribe
returned from the first subscribeEvents call was invoked (ensuring the old
subscription was cleaned up) while a new subscription was created for the new
projectId.
In `@desktop/src/apps/ProjectsApp/board/BoardColumn.module.css`:
- Line 7: The empty CSS rule for the status modifiers (.ready, .claimed,
.closed) triggers the linter; open the block in BoardColumn.module.css and add a
harmless placeholder (for example a single-line comment or a no-op declaration)
inside the .ready, .claimed, .closed {} rule to satisfy the linter while
preserving the intent that these modifiers are reserved for future styles.
In `@desktop/src/apps/ProjectsApp/board/BoardFilters.tsx`:
- Around line 22-25: The filter toggle button lacks accessible expanded-state
semantics; update the button (the element rendering "+ Filter" that calls
setOpen and reads open) to include aria-expanded set to the current open state
and aria-haspopup="dialog", and add an aria-controls pointing at the popover's
id; also give the popover div (role="dialog", the element rendered when open) a
stable id (e.g., "filters-popover") so aria-controls can reference it, ensuring
assistive tech can understand the toggle behavior.
In `@desktop/src/apps/ProjectsApp/board/boardGrouping.ts`:
- Around line 66-69: Tasks are being added to the same bucket multiple times
when a task's t.labels contains duplicates; before iterating labels (the loop
using "for (const lbl of t.labels)") deduplicate the labels for that task (e.g.,
convert t.labels to a Set or otherwise unique array) so when you call
buckets.set/get and push t into buckets the task is only added once per distinct
label; update the loop to iterate over the unique labels for each task while
still using the existing buckets map and push logic.
In `@desktop/src/apps/ProjectsApp/board/modal/SubTasks.tsx`:
- Around line 13-14: The checkbox in SubTasks.tsx is currently readOnly but
still appears actionable; change the input element for sub-tasks (the checkbox
using checked={c.status === "closed"} and aria-label={c.title}) to be explicitly
non-interactive by adding the disabled attribute (or replace it with a non-form
visual indicator) so users cannot attempt interaction and screen readers still
get proper state via aria attributes.
In `@desktop/src/apps/ProjectsApp/board/TaskCardCover.module.css`:
- Around line 6-7: The cursor blink animation (.cursor and `@keyframes` blink)
should be disabled for users who prefer reduced motion: add a
prefers-reduced-motion media query that targets .cursor and sets animation: none
(or animation-duration: 0s) so the blink is not played, leaving the keyframes
intact for other users.
In `@desktop/src/apps/ProjectsApp/board/TaskModal.module.css`:
- Around line 19-20: The CSS selector ".layout > aside [role=\"button\"]" only
defines a hover state; add a matching ":focus-visible" rule so keyboard users
see the same visible focus affordance as hover. Update the stylesheet by adding
a ".layout > aside [role=\"button\"]:focus-visible" rule (and optionally
".layout > aside [role=\"button\"]:focus") that mirrors the hover styles (e.g.,
color and any outline/box-shadow) to ensure parity for keyboard navigation and
assistive tech.
In `@desktop/src/lib/__tests__/projects-tasks.test.ts`:
- Around line 50-58: Update the test for projectsApi.subscribeEvents to also
exercise JSON message forwarding: when you create the eventSourceMock return
value (the object with close and onmessage), after calling
projectsApi.subscribeEvents("p1", callback) set that mock object's onmessage to
a function or directly invoke onmessage with a MessageEvent whose data is a JSON
string representing a ProjectEvent, then assert the provided callback was called
with the parsed ProjectEvent object; reference the subscribeEvents function and
the projectsApi.subscribeEvents call in the test so you locate where to invoke
the onmessage handler and add the assertion.
In `@desktop/src/lib/projects.ts`:
- Around line 146-150: Define a new UpdateTaskPatch type listing only the
allowed patchable fields (title, body, priority, labels, assignee_id,
parent_task_id) and replace the current patch parameter type in the update
function signature from Partial<ProjectTask> to UpdateTaskPatch; update the
update method declaration (update: (pid: string, tid: string, patch:
Partial<ProjectTask>) => ...) to use UpdateTaskPatch and ensure any callers are
adjusted to the narrower type so client code cannot pass server-managed fields
like id, project_id, status, claimed_by, closed_at, or closed_by.
In `@tests/projects/test_routes_update_parent.py`:
- Around line 54-61: The two setup PATCH requests that establish the parent
chain (the calls to client.patch for f"/api/projects/{pid}/tasks/{a['id']}" and
f"/api/projects/{pid}/tasks/{b['id']}") currently ignore their responses; update
the test to capture each response and assert response.status_code == 200 for
both before proceeding to the cycle detection/assertion so the test only
continues when the parent relationships are actually created.
In `@tinyagentos/routes/projects.py`:
- Around line 488-500: The event_stream coroutine currently catches
_asyncio.TimeoutError only, which is required for Python 3.10 but would be
redundant in 3.11+; update the exception handling in the try/except around await
_asyncio.wait_for(queue.get(), timeout=15.0) inside event_stream to be
compatible across supported versions by catching both _asyncio.TimeoutError and
the built-in TimeoutError (e.g., except (_asyncio.TimeoutError, TimeoutError):)
or by keeping _asyncio.TimeoutError and adding TimeoutError as a second clause
so timeouts from wait_for(queue.get()) are handled on Python 3.10 and 3.11+.
Ensure the finally block still calls await broker.unsubscribe(project_id,
queue).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d15a19d-04f5-4780-a189-5b828e070288
📒 Files selected for processing (53)
.gitignoredesktop/src/apps/ProjectsApp/ProjectWorkspace.tsxdesktop/src/apps/ProjectsApp/board/BoardColumn.module.cssdesktop/src/apps/ProjectsApp/board/BoardColumn.tsxdesktop/src/apps/ProjectsApp/board/BoardFilters.tsxdesktop/src/apps/ProjectsApp/board/BoardLane.module.cssdesktop/src/apps/ProjectsApp/board/BoardLane.tsxdesktop/src/apps/ProjectsApp/board/BoardToolbar.module.cssdesktop/src/apps/ProjectsApp/board/BoardToolbar.tsxdesktop/src/apps/ProjectsApp/board/ProjectBoard.module.cssdesktop/src/apps/ProjectsApp/board/ProjectBoard.tsxdesktop/src/apps/ProjectsApp/board/TaskCard.module.cssdesktop/src/apps/ProjectsApp/board/TaskCard.tsxdesktop/src/apps/ProjectsApp/board/TaskCardCover.module.cssdesktop/src/apps/ProjectsApp/board/TaskCardCover.tsxdesktop/src/apps/ProjectsApp/board/TaskModal.module.cssdesktop/src/apps/ProjectsApp/board/TaskModal.tsxdesktop/src/apps/ProjectsApp/board/__tests__/BoardColumn.test.tsxdesktop/src/apps/ProjectsApp/board/__tests__/BoardLane.test.tsxdesktop/src/apps/ProjectsApp/board/__tests__/BoardToolbar.test.tsxdesktop/src/apps/ProjectsApp/board/__tests__/ProjectBoard.test.tsxdesktop/src/apps/ProjectsApp/board/__tests__/TaskCard.test.tsxdesktop/src/apps/ProjectsApp/board/__tests__/TaskCardCover.test.tsxdesktop/src/apps/ProjectsApp/board/__tests__/TaskModal.test.tsxdesktop/src/apps/ProjectsApp/board/__tests__/boardDnd.test.tsdesktop/src/apps/ProjectsApp/board/__tests__/boardFiltering.test.tsdesktop/src/apps/ProjectsApp/board/__tests__/boardGrouping.test.tsdesktop/src/apps/ProjectsApp/board/__tests__/useBoardData.test.tsxdesktop/src/apps/ProjectsApp/board/__tests__/useBoardLive.test.tsxdesktop/src/apps/ProjectsApp/board/boardDnd.tsdesktop/src/apps/ProjectsApp/board/boardFiltering.tsdesktop/src/apps/ProjectsApp/board/boardGrouping.tsdesktop/src/apps/ProjectsApp/board/modal/Activity.tsxdesktop/src/apps/ProjectsApp/board/modal/Hero.tsxdesktop/src/apps/ProjectsApp/board/modal/MetadataPane.tsxdesktop/src/apps/ProjectsApp/board/modal/Relationships.tsxdesktop/src/apps/ProjectsApp/board/modal/SubTasks.tsxdesktop/src/apps/ProjectsApp/board/types.tsdesktop/src/apps/ProjectsApp/board/useBoardData.tsdesktop/src/apps/ProjectsApp/board/useBoardLive.tsdesktop/src/lib/__tests__/projects-tasks.test.tsdesktop/src/lib/projects.tsdesktop/src/theme/tokens.cssdesktop/src/types/css-modules.d.tstests/e2e/test_projects_board.pytests/projects/test_event_broker.pytests/projects/test_event_broker_integration.pytests/projects/test_events_route.pytests/projects/test_routes_update_parent.pytinyagentos/app.pytinyagentos/projects/events.pytinyagentos/projects/task_store.pytinyagentos/routes/projects.py
Comment on lines
+69
to
+73
| case "label": { | ||
| const targetLbl = laneKey === "__unlabeled__" ? null : laneKey; | ||
| if (targetLbl && task.labels.includes(targetLbl)) return null; | ||
| const next = targetLbl ? [...task.labels, targetLbl] : []; | ||
| return { labels: next }; |
There was a problem hiding this comment.
Label-lane drops are mutating membership incorrectly.
This branch appends the target label but never removes the source one, so dragging a card from one label lane to another is an add, not a move. On the next regroup the card can still match the old lane. Dropping into __unlabeled__ is also too destructive here because it clears all labels, including metadata labels like cover:* that other board code relies on. This needs source-lane context or a narrower patch strategy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/src/apps/ProjectsApp/board/boardDnd.ts` around lines 69 - 73, The
label branch currently only appends targetLbl and clears all labels for
"__unlabeled__", causing incorrect membership and loss of metadata; update the
logic in the "label" case to use the source lane label (e.g., the original lane
key / sourceLabel passed into the DnD handler) and produce a labels array that
removes sourceLabel and conditionally adds targetLbl: if sourceLabel ===
targetLbl return null; if targetLbl is non-null set labels =
[...task.labels.filter(l => l !== sourceLabel), targetLbl] (avoiding
duplicates); if targetLbl is null set labels = task.labels.filter(l => l !==
sourceLabel) so you only remove the moved label and preserve other metadata
labels like cover:*. Ensure you reference laneKey/targetLbl, task.labels and the
source lane identifier when implementing this change.
Comment on lines
+15
to
+20
| useEffect(() => { | ||
| let cancelled = false; | ||
| (async () => { | ||
| const items = await projectsApi.tasks.listComments(projectId, taskId); | ||
| if (!cancelled) setComments(items); | ||
| })(); |
There was a problem hiding this comment.
Guard async API calls with try/catch to prevent unhandled rejections.
Both load and submit paths should handle request failures explicitly and keep UI state consistent.
Proposed minimal error-safe pattern
useEffect(() => {
let cancelled = false;
(async () => {
- const items = await projectsApi.tasks.listComments(projectId, taskId);
- if (!cancelled) setComments(items);
+ try {
+ const items = await projectsApi.tasks.listComments(projectId, taskId);
+ if (!cancelled) setComments(items);
+ } catch {
+ if (!cancelled) setComments([]);
+ }
})();
return () => { cancelled = true; };
}, [projectId, taskId]);
const submit = async () => {
const body = draft.trim();
if (!body) return;
- const c = await projectsApi.tasks.addComment(projectId, taskId, { body, author_id: currentUserId });
- setComments(prev => [...prev, c]);
- setDraft("");
+ try {
+ const c = await projectsApi.tasks.addComment(projectId, taskId, { body, author_id: currentUserId });
+ setComments(prev => [...prev, c]);
+ setDraft("");
+ } catch {
+ // TODO: surface error toast/inline state
+ }
};Also applies to: 24-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/src/apps/ProjectsApp/board/modal/Activity.tsx` around lines 15 - 20,
The async calls in the useEffect and the comment submit path are unguarded and
can cause unhandled rejections; wrap the await
projectsApi.tasks.listComments(...) inside a try/catch in the useEffect
(preserving the cancelled flag behavior and only calling setComments on success)
and likewise wrap the submit POST call (e.g., the handler that calls
projectsApi.tasks.createComment or similar) in try/catch to handle errors,
update UI state (spinner/error state) consistently, and log or surface the error
to the user; ensure you do not swallow cancellations and keep the cancelled flag
check before calling setComments or other state updates.
Comment on lines
+180
to
+186
| subscribeEvents(projectId: string, onEvent: (ev: ProjectEvent) => void): () => void { | ||
| const es = new EventSource(`/api/projects/${projectId}/events`); | ||
| es.onmessage = (e) => { | ||
| try { onEvent(JSON.parse(e.data) as ProjectEvent); } catch { /* heartbeat / malformed — skip */ } | ||
| }; | ||
| return () => es.close(); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the actual file
cat -n desktop/src/lib/projects.ts | head -200 | tail -40Repository: jaylfc/tinyagentos
Length of output: 1869
🏁 Script executed:
# Check if the method signature and usage patterns exist elsewhere
rg "subscribeEvents" --type ts --type js -B 2 -A 5Repository: jaylfc/tinyagentos
Length of output: 4986
🏁 Script executed:
# Look for EventSource usage patterns in the codebase
rg "EventSource" --type ts --type js -B 2 -A 5Repository: jaylfc/tinyagentos
Length of output: 50374
🏁 Script executed:
# Check if there are any error handling patterns for EventSource in tests or docs
rg "onopen|onerror" --type ts --type js -B 2 -A 2Repository: jaylfc/tinyagentos
Length of output: 50375
Add onState callback to expose SSE connection state.
subscribeEvents only forwards onmessage. Without onopen/onerror hooks, downstream callers cannot distinguish a successful connection from a dropped or failed stream. Current usage in useBoardLive.ts calls setConnected(true) immediately without awaiting actual connection, creating a false positive if the EventSource fails to open.
Proposed API extension
- subscribeEvents(projectId: string, onEvent: (ev: ProjectEvent) => void): () => void {
+ subscribeEvents(
+ projectId: string,
+ onEvent: (ev: ProjectEvent) => void,
+ onState?: (state: "open" | "error" | "closed") => void,
+ ): () => void {
const es = new EventSource(`/api/projects/${projectId}/events`);
+ es.onopen = () => onState?.("open");
+ es.onerror = () => onState?.("error");
es.onmessage = (e) => {
try { onEvent(JSON.parse(e.data) as ProjectEvent); } catch { /* heartbeat / malformed — skip */ }
};
- return () => es.close();
+ return () => { es.close(); onState?.("closed"); };
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| subscribeEvents(projectId: string, onEvent: (ev: ProjectEvent) => void): () => void { | |
| const es = new EventSource(`/api/projects/${projectId}/events`); | |
| es.onmessage = (e) => { | |
| try { onEvent(JSON.parse(e.data) as ProjectEvent); } catch { /* heartbeat / malformed — skip */ } | |
| }; | |
| return () => es.close(); | |
| }, | |
| subscribeEvents( | |
| projectId: string, | |
| onEvent: (ev: ProjectEvent) => void, | |
| onState?: (state: "open" | "error" | "closed") => void, | |
| ): () => void { | |
| const es = new EventSource(`/api/projects/${projectId}/events`); | |
| es.onopen = () => onState?.("open"); | |
| es.onerror = () => onState?.("error"); | |
| es.onmessage = (e) => { | |
| try { onEvent(JSON.parse(e.data) as ProjectEvent); } catch { /* heartbeat / malformed — skip */ } | |
| }; | |
| return () => { es.close(); onState?.("closed"); }; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/src/lib/projects.ts` around lines 180 - 186, subscribeEvents
currently only exposes onmessage so callers can’t detect SSE open/error/close;
change its signature to add an optional onState callback (e.g.
subscribeEvents(projectId: string, onEvent: (ev: ProjectEvent)=>void, onState?:
(state: 'open'|'error'|'closed', err?: any)=>void): () => void) and wire
EventSource.onopen to call onState('open'), EventSource.onerror to call
onState('error', event) and the returned cleanup function to call
onState('closed') before closing; keep onState optional for backwards
compatibility and ensure JSON parse try/catch remains.
Comment on lines
+64
to
+65
| except (asyncio.CancelledError, Exception): | ||
| pass |
There was a problem hiding this comment.
Do not swallow non-cancellation exceptions in the task cleanup path.
Line 64 currently catches Exception and ignores it, which can mask real failures in the ASGI app and produce false-positive test passes.
Suggested fix
- except (asyncio.CancelledError, Exception):
- pass
+ except asyncio.CancelledError:
+ pass📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| except (asyncio.CancelledError, Exception): | |
| pass | |
| except asyncio.CancelledError: | |
| pass |
🧰 Tools
🪛 Ruff (0.15.11)
[error] 64-65: try-except-pass detected, consider logging the exception
(S110)
[warning] 64-64: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/projects/test_events_route.py` around lines 64 - 65, The except block
in the task cleanup currently swallows all exceptions via "except
(asyncio.CancelledError, Exception): pass"; change this so only
asyncio.CancelledError is suppressed and any other Exception is re-raised (or
allowed to propagate) so real failures in the ASGI app surface in tests—i.e.,
replace the combined tuple with "except asyncio.CancelledError: pass" and remove
the blanket Exception catch (or add "except Exception: raise" if you need
explicit handling) in the task cleanup path in
tests/projects/test_events_route.py.
Comment on lines
+100
to
+102
| lines = await _collect_sse_lines( | ||
| app, f"/api/projects/{pid}/events", cookies, n_lines=1, timeout=16.0 | ||
| ) |
There was a problem hiding this comment.
Increase heartbeat wait margin to reduce test flakiness.
Line 101 uses timeout=16.0 for a 15s heartbeat interval; this is very tight under CI jitter.
Suggested fix
- lines = await _collect_sse_lines(
- app, f"/api/projects/{pid}/events", cookies, n_lines=1, timeout=16.0
- )
+ lines = await _collect_sse_lines(
+ app, f"/api/projects/{pid}/events", cookies, n_lines=1, timeout=20.0
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| lines = await _collect_sse_lines( | |
| app, f"/api/projects/{pid}/events", cookies, n_lines=1, timeout=16.0 | |
| ) | |
| lines = await _collect_sse_lines( | |
| app, f"/api/projects/{pid}/events", cookies, n_lines=1, timeout=20.0 | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/projects/test_events_route.py` around lines 100 - 102, The test is
using _collect_sse_lines to read a single SSE line with timeout=16.0 which is
too tight for a 15s heartbeat; increase the wait margin (e.g., set timeout to
around 20.0 or higher) in the call within tests/projects/test_events_route.py so
_collect_sse_lines(app, f"/api/projects/{pid}/events", cookies, n_lines=1,
timeout=...) has a larger timeout to avoid CI jitter-related flakes.
- boardDnd: label-lane drops replace category labels and preserve cover:* markers (was append-only, leaked source label) - MetadataPane: clamp priority via Number.isFinite + range; sync draft on value change; guard duplicate Enter+blur saves - TaskModal: skip arrow-key task nav while focus is in form fields - ProjectBoard: validate viewMode/groupBy from localStorage before use - ProjectWorkspace: separate auth-resolved from has-user; gate TaskModal on a real currentUserId; show "Sign in required" when auth fails - Activity: block submit and disable button when currentUserId is empty - e2e: replace lambda name= with re.compile (Playwright requires str or Pattern); slugs now uuid-suffixed to allow parallel runs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
desktop/src/apps/ProjectsApp/board/ProjectBoard.tsx (2)
53-62: Minor:setTimeoutlacks cleanup on unmount.If the component unmounts before the 1500ms timeout fires,
setJustClaimed(null)will attempt to update unmounted component state. Consider clearing the timeout in a cleanup function or using a ref to track mount status.Example with cleanup
+ const claimTimeoutRef = useRef<number | null>(null); + const { connected } = useBoardLive(projectId, (e) => { if (e.kind === "task.claimed") { const id = String((e.payload as { id?: string }).id ?? ""); if (id) { setJustClaimed(id); - setTimeout(() => setJustClaimed(null), 1500); + if (claimTimeoutRef.current) clearTimeout(claimTimeoutRef.current); + claimTimeoutRef.current = window.setTimeout(() => setJustClaimed(null), 1500); } } applyEvent(e); }); + + useEffect(() => () => { + if (claimTimeoutRef.current) clearTimeout(claimTimeoutRef.current); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/ProjectBoard.tsx` around lines 53 - 62, The setTimeout started when handling "task.claimed" (inside the useBoardLive callback) can fire after unmount and update state via setJustClaimed; fix by tracking the timer id (e.g., use a useRef like claimTimerRef) and before creating a new timer clear any existing one, then clearTimeout(claimTimerRef.current) in a component cleanup (or in the return of a useEffect) to avoid updating unmounted state; update the handler around setJustClaimed and ensure applyEvent(e) remains called as before.
111-112: Announcement uses stale task data.After the API calls complete,
tasksstate hasn't yet been updated by SSE events.tasks.find(t => t.id === taskId)returns the pre-move task data. The title should still be correct, but if you wanted to announce the new status, consider usingcolumnStatusdirectly (which is already used).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/ProjectBoard.tsx` around lines 111 - 112, The announcement currently reads the task from tasks.find(t => t.id === taskId) which can be stale; instead derive the title from the task being moved (capture the task object before mutating state or accept title as an argument) and build the message using the known columnStatus directly, then call setAnnouncement with that constructed string (replace the tasks.find usage with the captured/method parameter title and columnStatus in the setAnnouncement call).desktop/src/apps/ProjectsApp/board/boardDnd.ts (1)
70-74: Consider explicit handling for unexpected priority lane keys.If
laneKeyis not"p0","p1", or"p2", it silently defaults to priority 3. This is safe but could mask bugs if lane keys are dynamically generated or misconfigured.case "priority": { - const p = laneKey === "p0" ? 0 : laneKey === "p1" ? 1 : laneKey === "p2" ? 2 : 3; + const map: Record<string, number> = { p0: 0, p1: 1, p2: 2, p3: 3 }; + const p = map[laneKey]; + if (p === undefined) return null; // Unknown lane, no change if (task.priority === p) return null; return { priority: p }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/boardDnd.ts` around lines 70 - 74, The priority branch in the case "priority" block computes p from laneKey with a fallback to 3 which can mask bad lane keys; update the logic in that block (the case "priority" clause that defines p using laneKey and compares to task.priority) to explicitly handle unexpected laneKey values — e.g., use a switch or map for "p0"/"p1"/"p2", and in the default branch either log a warning/error (or throw) and return null instead of silently assigning priority 3 so misconfigured lane keys are surfaced.desktop/src/apps/ProjectsApp/board/TaskModal.tsx (1)
28-28: Type cast bypasses safety check forcancelledstatus.
ProjectTask.statusincludes"cancelled"butTask.statusonly allows"open" | "claimed" | "closed". Theas unknown as Task[]cast hides this mismatch - if a cancelled task is fetched, downstream code expecting only three statuses may behave unexpectedly.Consider aligning
TaskwithProjectTaskor filtering out cancelled tasks after fetch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/board/TaskModal.tsx` at line 28, The code casts the API result to Task[] bypassing the fact ProjectTask may include "cancelled" while Task only allows "open"|"claimed"|"closed", so either reconcile types or filter out/handle cancelled items: modify the projectsApi.tasks.list usage (the variable all) to either fetch as ProjectTask[] and map/convert to Task[] (filtering or remapping any status === "cancelled") or update the Task type to include "cancelled" if that status is valid across the app; ensure any downstream usage of Task (e.g., wherever Task.status is read) gets only the expected statuses by performing the filter/transform after projectsApi.tasks.list rather than using as unknown as Task[].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@desktop/src/apps/ProjectsApp/board/ProjectBoard.tsx`:
- Around line 151-171: The code uses lanes!.map(...) in ProjectBoard which will
NPE when viewMode !== "lanes" (e.g., "timeline") because lanes can be null;
update the render logic to guard access to lanes by checking viewMode ===
"lanes" (or lanes != null) before mapping and render the appropriate timeline UI
otherwise; specifically adjust the conditional that renders the <div
className={styles.lanes}> so it only maps lanes when lanes is non-null and use
BoardLane, counts, renderCard and dispatchDnd only within that guarded branch.
In `@desktop/src/apps/ProjectsApp/ProjectWorkspace.tsx`:
- Around line 35-38: The fetch handling in the ProjectWorkspace auth check uses
u?.id but the API returns { user: { ... } }, so currentUserId never gets set;
update the response handling around the fetch(...).then((r) => (r.ok ? r.json()
: null)).then((u) => { ... }) to read u?.user?.id (and guard against null u)
when calling setCurrentUserId, i.e. replace the u?.id access with u?.user?.id
and keep the existing cancelled and setAuthResolved logic intact so failed/null
responses still setAuthResolved(true).
---
Nitpick comments:
In `@desktop/src/apps/ProjectsApp/board/boardDnd.ts`:
- Around line 70-74: The priority branch in the case "priority" block computes p
from laneKey with a fallback to 3 which can mask bad lane keys; update the logic
in that block (the case "priority" clause that defines p using laneKey and
compares to task.priority) to explicitly handle unexpected laneKey values —
e.g., use a switch or map for "p0"/"p1"/"p2", and in the default branch either
log a warning/error (or throw) and return null instead of silently assigning
priority 3 so misconfigured lane keys are surfaced.
In `@desktop/src/apps/ProjectsApp/board/ProjectBoard.tsx`:
- Around line 53-62: The setTimeout started when handling "task.claimed" (inside
the useBoardLive callback) can fire after unmount and update state via
setJustClaimed; fix by tracking the timer id (e.g., use a useRef like
claimTimerRef) and before creating a new timer clear any existing one, then
clearTimeout(claimTimerRef.current) in a component cleanup (or in the return of
a useEffect) to avoid updating unmounted state; update the handler around
setJustClaimed and ensure applyEvent(e) remains called as before.
- Around line 111-112: The announcement currently reads the task from
tasks.find(t => t.id === taskId) which can be stale; instead derive the title
from the task being moved (capture the task object before mutating state or
accept title as an argument) and build the message using the known columnStatus
directly, then call setAnnouncement with that constructed string (replace the
tasks.find usage with the captured/method parameter title and columnStatus in
the setAnnouncement call).
In `@desktop/src/apps/ProjectsApp/board/TaskModal.tsx`:
- Line 28: The code casts the API result to Task[] bypassing the fact
ProjectTask may include "cancelled" while Task only allows
"open"|"claimed"|"closed", so either reconcile types or filter out/handle
cancelled items: modify the projectsApi.tasks.list usage (the variable all) to
either fetch as ProjectTask[] and map/convert to Task[] (filtering or remapping
any status === "cancelled") or update the Task type to include "cancelled" if
that status is valid across the app; ensure any downstream usage of Task (e.g.,
wherever Task.status is read) gets only the expected statuses by performing the
filter/transform after projectsApi.tasks.list rather than using as unknown as
Task[].
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e621a5fe-aa0f-44a5-b020-a9d9d7c6e350
📒 Files selected for processing (7)
desktop/src/apps/ProjectsApp/ProjectWorkspace.tsxdesktop/src/apps/ProjectsApp/board/ProjectBoard.tsxdesktop/src/apps/ProjectsApp/board/TaskModal.tsxdesktop/src/apps/ProjectsApp/board/boardDnd.tsdesktop/src/apps/ProjectsApp/board/modal/Activity.tsxdesktop/src/apps/ProjectsApp/board/modal/MetadataPane.tsxtests/e2e/test_projects_board.py
🚧 Files skipped from review as they are similar to previous changes (3)
- desktop/src/apps/ProjectsApp/board/modal/Activity.tsx
- desktop/src/apps/ProjectsApp/board/modal/MetadataPane.tsx
- tests/e2e/test_projects_board.py
…om persisted views
- /api/auth/me returns {"user": {...}}; ProjectWorkspace was reading
u.id directly so currentUserId was always null and the board showed
"Sign in required" forever
- Persisted viewMode allowed "timeline", which is reserved in the toolbar
but unimplemented — restoring it on reload triggered lanes!.map(...) on
null. Restrict storage to lanes|kanban
Python 3.10 consistently runs ~14-15min in the CI matrix, frequently hitting the 15min cap and causing flaky cancellations even on master (see run 24937069102: test (3.10) cancelled at 15m16s while 3.11/3.12/ 3.13 all passed at 12-14min). 25min gives proper headroom without masking real regressions.
jaylfc
changed the title
4 tasks
This was referenced Apr 29, 2026
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
1 participant
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
Pass B item #1 from the Projects roadmap: rich kanban / swimlane board views inside a single project workspace. Foundation (PRs #260, #264) already on master.
This is a draft — only the backend live-event spine is in place so far (5 of 25 tasks). Pushed early so CI + CodeRabbit can run on partial work.
Progress
ProjectEventBrokerpub/sub primitive (projects/events.py)task_storemutations (post-commit, conditional onrowcount)PATCH /tasksacceptsparent_task_idwith self/cross-project/indirect-cycle guardsGET /api/projects/{id}/eventswith 15s heartbeatNo schema changes — three-state lifecycle (
open|claimed|closed) and existing relationship table are sufficient.Test plan
tests/projects/test_event_broker.py— 5 unit tests (replay, fan-out, isolation, unsubscribe, replay-cap)tests/projects/test_event_broker_integration.py— 4 integration tests throughtask_storetests/projects/test_routes_update_parent.py— 4 tests (accept, self-cycle, cross-project, indirect cycle)tests/projects/test_events_route.py— 2 tests (drives ASGI directly; httpxASGITransportbuffers full body and deadlocks on infinite SSE streams, so a custom_collect_sse_lines()helper is used)Notes for reviewers
feat(projects-board):/refactor(projects-board):events.pyand tracked in spec §4 (multi-worker out of scope)ASGITransportdeviation in Task 4 tests is intentional — seetests/projects/test_events_route.pydocstringSummary by CodeRabbit
New Features
Bug Fixes
Tests