Conversation
…omium Replace the old webkit-only 8px scrollbar block with a unified approach: - @supports (-moz-appearance: none) guard so Firefox-only scrollbar-width/color rules don't trigger Chrome 121+ standard scrollbar API - Unified 6px webkit pseudo-element rules for .overflow-y-auto, .overflow-x-auto, .overflow-auto and .scrollbar-thin (no arrows, transparent track) - Remove hide-scrollbar from Notes/Checklist overview and Sidebar so scrollbars are visible in both browsers - Add tailwind-scrollbar plugin classes to KanbanCardDetail panes and Kanban board Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…change Upstream added a bare <div> wrapper around Modal children in default size mode, breaking the flex height chain. Target it via [&>div:last-child] to give it flex-1 + flex-col so the inner panes get a defined height and can scroll again. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…efox - Remove tailwind-scrollbar plugin classes from both scroll panes; the plugin sets scrollbar-width:thin unconditionally which triggers Chrome 121+ standard scrollbar API (auto-hide overlay) making scrollbars invisible in Edge - Change lg:overflow-y-auto to overflow-y-auto so globals.css webkit rules (.overflow-y-auto::-webkit-scrollbar) apply, giving a always-visible 6px bar - Add lg:[&>div:last-child]:overflow-hidden to prevent the Modal wrapper div from showing its own scrollbar on desktop, which overlapped in Firefox Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tailwind arbitrary variants with pseudo-selectors ([&>div:last-child]) do not compile reliably. Replace with a dedicated CSS class: .jotty-modal-content.jotty-kanban-detail-modal > div:last-child targets the bare wrapper div upstream injected, making it flex-1 flex-col so the inner layout and per-pane overflow-y-auto work again. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The upstream Modal wrapper div breaks the flex chain for default size. size=fullscreen gives the children wrapper flex-1 overflow-auto by design. Inner div uses min-h-full + lg:overflow-hidden so panes scroll independently on desktop while the wrapper scrolls as one unit on mobile. Removes the fragile CSS child selector approach. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
scrollbar-thin and related plugin classes set scrollbar-width on Chrome 121+, which causes it to ignore the ::-webkit-scrollbar rules in globals.css and fall back to native auto-hide scrollbars. The global webkit rules already cover .overflow-y-auto and .overflow-auto, so the plugin classes are counterproductive on those elements.
The Firefox-only @supports block listed overflow-y-auto and overflow-auto but missed overflow-x-auto, leaving horizontally scrolling containers with the default OS scrollbar in Firefox. The corresponding webkit rules already include overflow-x-auto.
Subitem drag-and-drop always inserted the dragged item before the drop target instead of at the indicated position, so it landed at the top of the parent's children. The before/after check in handleDragEnd matched the drop-zone id prefix "drop-after::", but subitem drop indicators use the prefix "drop-after-child::". The check never matched for subitems, so position always fell back to "before". Top-level reorder was unaffected because its indicators do use "drop-after::". Match the prefix "drop-after" so both top-level and subitem indicators resolve correctly, consistent with the isDropIndicator check that already uses that prefix. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(checklist): respect drop position when reordering subitems
feat(kanban): add "Archive all" action to auto-complete columns
fix(scrollbar): consistent scrollbar styling across Firefox and Chromium browsers (Windows)
Added portuguese translation file
The client-side parseChecklistContent generated fallback item IDs as
`${id}-${currentItemIndex}` and reset currentItemIndex to 0 in each
recursive call for nested children. Group headers and their nested
items therefore shared the same numeric range, producing duplicate IDs
across siblings and groups. Clicking one item toggled every other item
with the same ID.
The server-side parser (parseMarkdown in checklist-utils.ts) already
avoided this by using a monotonic counter combined with level
(`${id}-${level}-${counter}`). Align the client parser with the same
scheme so a file dropped into the importer is assigned the same IDs
that the server would produce.
Also add a post-parse dedup pass so already-imported files with broken
duplicate IDs heal themselves on the next save: the first occurrence
keeps its ID, every later collision is reassigned a fresh unique ID.
Stored metadata IDs are still honoured.
Tests cover a 4-group, 5-children-each fresh parse (all IDs unique),
preservation of stored item-metadata IDs, and dedup of pre-broken
imported content.
fix: ensure unique item IDs when importing checklists (#501)
(feat) add Kanban status actions to card detail and overflow menu
Add proxyClientMaxBodySize to next.config.mjs to fix bodySizeLimit bug
Feature/neural network
Feature/kanban dnd refactoring
Owner
Author
|
@coderabbitai do your thing |
|
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates Kanban drag-and-drop, detail editing, calendar rendering, and bulk archive flows; adds a profile connections graph with data builders and filters; expands checklist/task API item shapes and routes; introduces remember-me, navigation-guard, and modal behavior changes; and updates translations, configs, and tests across the app. Sequence Diagram(s)sequenceDiagram
participant User
participant KanbanCard
participant DndProvider
participant useKanbanDnd
participant dropItem
participant ChecklistFile
User->>KanbanCard: start drag
KanbanCard->>DndProvider: begin(session)
DndProvider->>useKanbanDnd: handleDrop(DropResult)
useKanbanDnd->>dropItem: FormData(uuid,itemId,targetStatus,targetIndex)
dropItem->>ChecklistFile: write updated checklist markdown
dropItem->>useKanbanDnd: updated checklist result
useKanbanDnd->>DndProvider: settle drag state
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/(loggedInRoutes)/settings/connections/page.tsx (1)
19-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winArchived items still leak back into the graph data.
Lines 19-23 only strip archived IDs out of
linkIndex.LinksTabstill receives the fullnotesandchecklistsarrays, andbuildConnectionGraphData()builds both nodes and tag links from those arrays, so archived content can reappear throughshowOrphansor any shared tag. Filter those collections with the same archived set before passing them down, or move the archived-item check into the graph builder so the contract lives in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`(loggedInRoutes)/settings/connections/page.tsx around lines 19 - 23, The LinksTab is still receiving unfiltered notes and checklists so archived items leak back into the graph; update the return to pass filtered notes and checklists (using the same archivedItems set used by filterArchivedLinkIndex) — e.g. create filteredNotes = notes.filter(n => !archivedItemsSet.has(n.id)) and filteredChecklists = checklists.filter(c => !archivedItemsSet.has(c.id)) (or move the archived check into buildConnectionGraphData so it rejects archived nodes/tags there), then pass filteredNotes and filteredChecklists into LinksTab instead of the original notes/checklists.app/_components/FeatureComponents/Kanban/Kanban.tsx (1)
275-302:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't archive items hidden by active filters.
This column renders
itemsafter search/priority/assignee filtering, but the bulk action still archivescolumnItems. With any filter active, "archive all" persists changes to cards the user cannot see.🛡️ Safer wiring
const columnItems = getItemsByStatus(column.status); const items = _filterItems(columnItems); @@ - archivableCount={columnItems.length} + archivableCount={items.length} onArchiveAll={ permissions?.canEdit - ? () => handleArchiveAll(columnItems) + ? () => handleArchiveAll(items) : undefined }If the intended behavior is "archive the full status column regardless of filters", then disable or hide this action whenever filters are active instead of silently acting on hidden rows.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_components/FeatureComponents/Kanban/Kanban.tsx` around lines 275 - 302, The "Archive all" handler is acting on unfiltered columnItems while the UI shows filtered items (items), causing hidden cards to be archived; change the onArchiveAll wiring on the KanbanColumn so it either (A) archives only the currently-visible filtered items by calling handleArchiveAll(items) instead of handleArchiveAll(columnItems), or (B) disables/hides the archive-all action when filters are active by detecting a filter state (e.g., compare items.length !== columnItems.length or use an existing filter flag) and passing undefined for onArchiveAll when filtered (permissions?.canEdit ? () => handleArchiveAll(items) : undefined OR permissions?.canEdit && !filtersActive ? () => handleArchiveAll(columnItems) : undefined). Ensure use of the existing symbols: columnItems, items, _filterItems, handleArchiveAll, and permissions?.canEdit.app/_hooks/kanban/useKanbanItem.tsx (1)
57-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't resume the timer across time spent with the page closed.
This restore path only persists
startTime, then resumes by computingDate.now() - storedStart. After a reload or browser close, the timer keeps accruing the offline gap as active work time, so the next saved entry is inflated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_hooks/kanban/useKanbanItem.tsx` around lines 57 - 70, The restore logic in useEffect (TIMER_STORAGE_KEY, JSON.parse(stored)) resumes timers using storedStart which counts offline time; instead change the restore path so that when stored.isRunning is true you do NOT compute Date.now() - storedStart: call setStartTime(new Date()), setIsRunning(true) and setCurrentTime(0) (i.e., start fresh from now) so offline time is not treated as active work; keep the existing try/catch and handling for non-running state unchanged.
🟠 Major comments (23)
app/_components/FeatureComponents/Profile/Parts/ConnectionsGraph/graph-data.ts-200-220 (1)
200-220:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftShared tags currently expand into a full clique.
Lines 200-220 create one edge for every pair of items that shares a tag. At the current 600-node ceiling, one popular tag can still emit ~180k links, which will make both
buildConnectionGraphData()and the force-graph render stall badly on the client. Consider capping per-tag fan-out or representing tags as intermediary nodes instead of pairwise edges.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_components/FeatureComponents/Profile/Parts/ConnectionsGraph/graph-data.ts` around lines 200 - 220, This code emits a full clique for each tag (in nodesByTag loop) which explodes links; fix by replacing the pairwise link creation in this block with either (A) a tag-as-intermediary strategy: create a synthetic node for the tag (add a node object for the tag and a single set of links from that tag-node to each member, e.g. set link.type = "tag-node" and mark directed=false) and update tagConnections for members accordingly, and ensure buildConnectionGraphData()/any node-assembly logic includes these tag nodes; or (B) a capped-fanout strategy: limit taggedNodeIds before pairing (e.g. sample or take first MAX_PER_TAG) and then emit pairwise links only for that reduced set; implement one of these options inside the nodesByTag.forEach block (referencing nodesByTag, linkMap, tagConnections) and update buildConnectionGraphData to handle the new tag-node type if you choose the intermediary approach.app/_translations/fr.json-287-287 (1)
287-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncomplete translation: "rememberMe" is in English instead of French.
The value "Remember me" should be translated to French (e.g., "Se souvenir de moi" or "Rester connecté"). French-speaking users will see English text on the login screen.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_translations/fr.json` at line 287, The "rememberMe" translation key currently has an English value; update the JSON value for the "rememberMe" key to a proper French translation (for example "Se souvenir de moi" or "Rester connecté") so the login UI displays French text for that label.app/_translations/it.json-287-287 (1)
287-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncomplete translation: "rememberMe" is in English instead of Italian.
The value "Remember me" should be translated to Italian (e.g., "Ricordami" or "Resta connesso"). Italian-speaking users will see English text on the login screen.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_translations/it.json` at line 287, The translation for the "rememberMe" key is still in English; update the value for the "rememberMe" JSON key to the correct Italian translation (for example "Ricordami" or "Resta connesso") so the login UI shows Italian text for that label.app/_translations/fr.json-696-713 (1)
696-713:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncomplete translations for connections graph UI across French and Italian locales.
Both translation files contain the same 17 untranslated English strings for the connections graph feature. This systematic gap suggests the graph feature was implemented without completing the localization work, resulting in a degraded user experience for non-English speakers.
app/_translations/fr.json#L696-L713: Translate all graph-related keys to French (e.g., "totalConnections" → "Total", "inboundConnections" → "Entrant", "outboundConnections" → "Sortant", "orphans" → "Orphelins", "labels" → "Étiquettes", "arrows" → "Flèches", "nodeSize" → "Taille des nœuds", "linkWidth" → "Largeur des liens", "linkDistance" → "Distance des liens", "tagLinks" → "Liens de tags", etc.).app/_translations/it.json#L696-L713: Translate all graph-related keys to Italian (e.g., "totalConnections" → "Totale", "inboundConnections" → "In entrata", "outboundConnections" → "In uscita", "orphans" → "Orfani", "labels" → "Etichette", "arrows" → "Frecce", "nodeSize" → "Dimensione nodi", "linkWidth" → "Larghezza collegamenti", "linkDistance" → "Distanza collegamenti", "tagLinks" → "Collegamenti tag", etc.).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_translations/fr.json` around lines 696 - 713, The French and Italian translation files both contain untranslated English graph UI strings; update app/_translations/fr.json (lines 696-713) by translating each key (e.g., "totalConnections","inboundConnections","outboundConnections","orphans","labels","arrows","nodeSize","linkWidth","linkDistance","tagLinks","visibleGraphSummary","graphTruncated","updated","openItem","selectItem","inspectNodeHint","graphSearchPlaceholder") into idiomatic French (examples: "Total","Entrant","Sortant","Orphelins","Étiquettes","Flèches","Taille des nœuds","Largeur des liens","Distance des liens","Liens de tags", etc.); and update app/_translations/it.json (lines 696-713) by translating the same keys into Italian (examples: "Totale","In entrata","In uscita","Orfani","Etichette","Frecce","Dimensione nodi","Larghezza collegamenti","Distanza collegamenti","Collegamenti tag", etc.), ensuring placeholders like {items},{links},{visible},{omitted} and UI hints remain intact and grammatically correct.app/_components/GlobalComponents/Auth/LoginForm.tsx-248-253 (1)
248-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden the external releases link against opener-based tabnabbing.
The
target="_blank"anchor should includerel="noopener noreferrer"so the opened tab cannot accesswindow.opener.Proposed fix
<a target="_blank" + rel="noopener noreferrer" href={`https://github.com/fccview/jotty/releases/tag/${appVersion}`} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_components/GlobalComponents/Auth/LoginForm.tsx` around lines 248 - 253, The external releases anchor in LoginForm.tsx (the <a> element rendering the releases link inside the LoginForm component) uses target="_blank" without rel attributes; update that anchor to add rel="noopener noreferrer" so the new tab cannot access window.opener (preventing tabnabbing) while keeping the existing href and target values unchanged.app/_providers/NavigationGuardProvider.tsx-63-72 (1)
63-72:⚠️ Potential issue | 🟠 MajorAdd
e.returnValueto make thebeforeunloadprompt reliableFile: app/_providers/NavigationGuardProvider.tsx
Lines: 63-72useEffect(() => { const onBeforeUnload = (e: BeforeUnloadEvent) => { if (hasUnsavedChanges) { e.preventDefault(); } }; window.addEventListener("beforeunload", onBeforeUnload); return () => window.removeEventListener("beforeunload", onBeforeUnload); }, [hasUnsavedChanges]);
beforeunloadmay not trigger consistently withpreventDefault()alone. Sete.returnValuewhen blocking unload to ensure cross-browser behavior.Proposed fix
useEffect(() => { const onBeforeUnload = (e: BeforeUnloadEvent) => { if (hasUnsavedChanges) { e.preventDefault(); + e.returnValue = ""; } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_providers/NavigationGuardProvider.tsx` around lines 63 - 72, The beforeunload handler (onBeforeUnload used inside the useEffect) currently calls e.preventDefault() when hasUnsavedChanges is true but doesn't set e.returnValue, which can make the prompt unreliable across browsers; update the handler so that when hasUnsavedChanges is true you both call e.preventDefault() and set e.returnValue = '' (or a short string) on the BeforeUnloadEvent before returning, ensuring consistent cross-browser blocking behavior while keeping the existing window.addEventListener/removeEventListener logic intact.app/_components/FeatureComponents/Notes/Parts/TipTap/CustomExtensions/ExcalidrawExtension.tsx-109-115 (1)
109-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent close-path lock when autosave fails.
Line 111 starts an async save without handling rejection in the close path. If save/export fails, close can become non-functional and trap the modal open.
Suggested patch
- const handleClose = () => { - if (excalidrawAPI) { - handleSave(); - } else { - setIsEditing(false); - } - }; + const handleClose = async () => { + if (!excalidrawAPI) { + setIsEditing(false); + return; + } + + try { + await handleSave(); + } catch { + setIsEditing(false); + } + }; ... - onClose={handleClose} + onClose={() => { + void handleClose(); + }}Also applies to: 140-140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_components/FeatureComponents/Notes/Parts/TipTap/CustomExtensions/ExcalidrawExtension.tsx` around lines 109 - 115, handleClose currently triggers the async handleSave when excalidrawAPI exists without handling rejection, which can lock the close path; make handleClose robust by ensuring it always proceeds to close regardless of save success: either make handleClose async and await handleSave() inside a try/catch/finally that calls setIsEditing(false) in the finally block, or call handleSave().catch(() => {/* optional log */}).finally(() => setIsEditing(false)); apply the same fix to the other similar close-handler occurrence in this file (the second handleClose-like close path).app/_utils/dnd/rect-registry.ts-108-117 (1)
108-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude the list node itself in
scrollables()traversal.Line 111 starts at
el.parentElement, so a scrollable drop-list element is never added. That can block drag auto-scroll within long lists.Proposed fix
- lists.forEach(({ el }) => { - let node = el.parentElement; + lists.forEach(({ el }) => { + let node: HTMLElement | null = el; while (node) { if (_scrolls(node)) found.add(node); node = node.parentElement; } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_utils/dnd/rect-registry.ts` around lines 108 - 117, The scrollables() traversal in rect-registry.ts currently starts from el.parentElement so scrollable list elements themselves are skipped; within the scrollables() function (which iterates lists and uses _scrolls(node)), start traversal from the list node itself (let node = el) instead of el.parentElement so that _scrolls(el) can detect and add the list element to the found set (leave the rest of the while loop logic unchanged).app/_components/FeatureComponents/Kanban/CalendarView.tsx-155-172 (1)
155-172:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the calendar bars keyboard-accessible.
The new event bars are clickable
divs, so keyboard users can't focus or activate them. Opening a task from calendar view becomes mouse-only.♿ Suggested fix
- <div + <button + type="button" key={`${segment.event.id}-${segment.colStart}-${segment.lane}`} onClick={() => _openItem(segment.event.itemId)} + aria-label={segment.event.title} + title={segment.event.title} className={cn( - "pointer-events-auto h-4 text-[10px] font-medium leading-4 px-1.5 truncate cursor-pointer hover:brightness-95 dark:hover:brightness-110 transition-[filter,opacity]", + "pointer-events-auto h-4 text-left text-[10px] font-medium leading-4 px-1.5 truncate cursor-pointer hover:brightness-95 dark:hover:brightness-110 transition-[filter,opacity] appearance-none border-0", !barStyle.backgroundColor && "bg-muted/80 text-muted-foreground", segment.continuesPrev ? "rounded-l-none ml-0" : "rounded-l-jotty ml-0.5", segment.continuesNext ? "rounded-r-none mr-0" : "rounded-r-jotty mr-0.5", segment.event.completed && "line-through opacity-70", )} style={{ ...barStyle, gridColumn: `${segment.colStart + 1} / span ${segment.colSpan}`, gridRow: segment.lane + 1, }} > {!segment.continuesPrev ? segment.event.title : "\u00a0"} - </div> + </button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_components/FeatureComponents/Kanban/CalendarView.tsx` around lines 155 - 172, The event bars are clickable divs (the element created in CalendarView that uses onClick={() => _openItem(segment.event.itemId)}) and must be keyboard-accessible: change the div to remain a div but add tabIndex={0}, role="button", a descriptive aria-label (e.g., using segment.event.title), and implement an onKeyDown handler that calls _openItem(segment.event.itemId) when Enter or Space is pressed; keep the existing onClick and visual classes unchanged so mouse behavior is preserved and screen readers receive the label.public/api/paths/checklists.yaml-201-251 (1)
201-251:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMerge
patchanddeleteunder a single/checklists/{listId}/items/{itemIndex}path item.This path key is declared twice. YAML/OpenAPI parsers typically keep only the last mapping entry, so one operation can disappear from the generated spec.
#!/bin/bash rg -n '^/checklists/\{listId\}/items/\{itemIndex\}:' public/api/paths/checklists.yamlExpected result after the fix: exactly one match, with both
patch:anddelete:nested beneath that single path key.Also applies to: 390-390
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/api/paths/checklists.yaml` around lines 201 - 251, The YAML defines the same path key '/checklists/{listId}/items/{itemIndex}' twice so one operation is lost; locate both occurrences and merge them into a single path mapping that contains both the patch and delete operations (keep the existing patch: block shown and add the delete: block under the same path key, or vice versa), removing the duplicate top-level path entry so the file contains exactly one '/checklists/{listId}/items/{itemIndex}:' mapping with both patch and delete nested beneath it.app/_utils/item-status-utils.ts-50-77 (1)
50-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate auto-complete through all ancestors, not just the immediate parent.
completeParent()only evaluates the first parent returned by_findParent(). In a deeper tree, completing a grandchild can auto-complete its direct parent but never re-check the grandparent, so nested completion stops one level too early.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_utils/item-status-utils.ts` around lines 50 - 77, completeParent currently only checks the immediate parent returned by _findParent and stops; change it so auto-complete is propagated up the ancestor chain: after finding the parent (using _findParent) and confirming all its children are completed and an autoCompleteStatus exists, update that parent via updateItem as you do now, then loop/recursively set childId = parent.id and repeat the same logic (find its parent, check siblings' completion, apply update) until no parent or no autoCompleteStatus or not all siblings completed. Ensure you preserve and append history/lastModified fields on each ancestor update and avoid infinite loops by stopping when parent is null or no change is needed.app/api/checklists/[listId]/items/reorder/route.ts-97-106 (1)
97-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPublish reorder writes through the same invalidation path as the other checklist mutations.
This endpoint persists the markdown file and returns success, but it never revalidates checklist pages or broadcasts the change. API-driven reorders will stay stale in other sessions until something else refreshes the list.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/checklists/`[listId]/items/reorder/route.ts around lines 97 - 106, The reorder endpoint currently persists the markdown via stampOrder, serverWriteFile and listToMarkdown but does not trigger the same revalidation/broadcast used by other checklist mutations; after creating updatedList and writing the file, call the centralized invalidation/publish helper used by other checklist mutations (e.g., publishChecklistMutation or whatever function other endpoints call to revalidate pages and broadcast updates) with the updatedList (or list.id/owner/category) so the change revalidates checklist pages and is broadcast to other sessions before returning NextResponse.json({ success: true }); ensure you import or reference the exact helper used by the other mutation handlers to avoid duplicating logic.app/_server/actions/checklist-item/status.ts-74-90 (1)
74-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalid
timeEntriesshould fail the action instead of being silently ignored.If
JSON.parse(timeEntriesStr)throws, this branch logs and returns the original item, but the action still rewrites the checklist, bumpsupdatedAt, broadcasts an update, and returns{ success: true }. Callers will think the time entry was saved when it was actually dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_server/actions/checklist-item/status.ts` around lines 74 - 90, The code silently swallows JSON.parse errors when building updatedItems (using timeEntriesStr, updateItem, statusItems, itemId), allowing the action to proceed as if the time entries were saved; change the catch to fail the action instead: validate/parse timeEntriesStr before computing updatedItems (or rethrow inside the updateItem callback) and when parsing fails throw a descriptive Error (including the original exception) so the calling action aborts, does not compute updatedItems, does not update updatedAt, does not broadcast, and returns an error response rather than { success: true }.app/_utils/item-status-utils.ts-63-75 (1)
63-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake parent auto-complete idempotent before appending history.
This always writes a new history entry once all children are complete, even if the parent is already completed with the same auto-complete status. Because
app/_server/actions/checklist-item/status.tsnow calls this after any child update, pure time-entry edits on completed children will keep duplicating the parent's completion history.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_utils/item-status-utils.ts` around lines 63 - 75, The current logic in the update callback always appends a completion history entry when all children are complete; change it to be idempotent by first checking the existing parent state (p.completed and p.status) and only perform the update and append a history entry if the parent is not already completed with the same autoCompleteStatus.id. Concretely, inside the updateItem callback (the function that receives p), if p.completed === true && p.status === autoCompleteStatus.id then return p unchanged (or return p with no new history/modified fields); otherwise set completed: true, status: autoCompleteStatus.id, lastModifiedBy: username, lastModifiedAt: now and append the new history entry as currently implemented.app/api/search/route.ts-37-43 (1)
37-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChecklist search skips shared lists because it only scans the caller's directory.
Checklist files are stored under
data/checklists/<owner>in the server write paths, but this route only searchesdata/checklists/<user.username>. After the shared-checklists API change, items from lists shared by other users will never be returned here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/search/route.ts` around lines 37 - 43, The checklist search only scans the caller's own directory (checklistsDir) so it misses lists shared by other users; change the rawChecklists logic to build a list of checklist owner directories (the caller's data/checklists/<user.username> plus every owner dir returned by the shared-checklists API), then run grepSearchContent against each directory and concatenate the results. Concretely: replace the single checklistsDir used in the Promise.all with code that calls whatever shared-checklist lookup you have (e.g., a function that returns shared owners for user.username), map those owner names into path.join(process.cwd(), "data", CHECKLISTS_FOLDER, owner), call grepSearchContent(...) for each directory (handling errors with .catch(() => [])), and flatten the array into rawChecklists so downstream code that expects grepSearchContent results continues to work (update the rawChecklists construction where NOTES_DIR, CHECKLISTS_FOLDER and grepSearchContent are referenced).app/_server/actions/file/index.ts-91-97 (1)
91-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a unique temp file per write; the shared
.tmppath still races under concurrent requests.
finalPath + ".tmp"is the same file for every writer targeting that JSON path. Two overlapping writes can clobber each other's temp content or make onerename()fail withENOENT, which reintroduces lost-update behavior for hot files like session data.Suggested fix
- const tmpPath = finalPath + ".tmp"; + const tmpPath = `${finalPath}.${process.pid}.${Date.now()}.${Math.random() + .toString(36) + .slice(2)}.tmp`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_server/actions/file/index.ts` around lines 91 - 97, The current write uses a shared tmpPath (tmpPath = finalPath + ".tmp") which races under concurrent writers; change the temp-file strategy in the block that creates tmpPath/uses fs.writeFile/fs.rename so each writer uses a unique temp name (e.g., append process.pid + timestamp + random/UUID or use a per-write crypto-random suffix), write to that unique tmpPath, then atomic-rename it to finalPath; also ensure you unlink the unique tmpPath on errors (or handle rename ENOENT appropriately) so leftover temps are cleaned up and concurrent writes no longer clobber each other (references: variables finalPath, tmpPath and the fs.writeFile/fs.rename operations).app/_server/actions/checklist-item/drop.ts-25-31 (1)
25-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
targetStatusagainst allowed board statuses before applying the move.Currently any string can be persisted to
item.status, which can create values not present inlist.statuses.Suggested fix
+import { DEFAULT_KANBAN_STATUSES } from "`@/app/_consts/kanban`"; @@ if (!list.items.some((item) => item.id === itemId)) { return { success: false, error: "Item not found" }; } + + const allowedStatuses = + list.statuses && list.statuses.length > 0 + ? list.statuses + : DEFAULT_KANBAN_STATUSES; + if (!allowedStatuses.some((status) => status.id === targetStatus)) { + return { success: false, error: "Invalid targetStatus" }; + } const now = new Date().toISOString();Also applies to: 53-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_server/actions/checklist-item/drop.ts` around lines 25 - 31, Validate that targetStatus is one of the allowed statuses before accepting the input: instead of only checking presence of uuid/itemId/targetStatus/targetIndex, fetch or reference the corresponding list.statuses (the board's allowed statuses) and ensure targetStatus is included; if not, return success: false with a clear error. Update both validation sites (the existing check around uuid/itemId/targetStatus/targetIndex and the similar check at the later block noted by the reviewer) so they both reject unknown statuses rather than allowing arbitrary strings to be persisted to item.status.app/_server/actions/checklist-item/drop.ts-86-93 (1)
86-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not return a failed action after persistence just because broadcast fails.
At Lines 86-91, a websocket failure bubbles to the outer
catch, returning"Failed to drop item"even though data is already saved. That desynchronizes client rollback behavior from persisted state.Suggested fix
- await broadcast({ - type: "checklist", - action: "updated", - entityId: list.id, - username, - }); + try { + await broadcast({ + type: "checklist", + action: "updated", + entityId: list.id, + username, + }); + } catch (error) { + console.warn("Broadcast failed after successful dropItem save:", error); + } return { success: true, data: updatedList };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_server/actions/checklist-item/drop.ts` around lines 86 - 93, The broadcast websocket call is allowed to fail and currently bubbles up to the outer catch causing a false "Failed to drop item" after persistence; wrap the broadcast(...) call in its own try/catch (or move the return { success: true, data: updatedList } to before broadcasting) so persistence success is always returned; on broadcast failure, log/report the error but do not rethrow it, ensuring the function returns the persisted updatedList even if broadcast({ type: "checklist", action: "updated", entityId: list.id, username }) fails.app/_hooks/kanban/useKanbanDnd.ts-74-89 (1)
74-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle thrown
dropItemerrors so optimistic state is always reverted.At Line 74,
dropItem(formData)is awaited withouttry/catch. If it throws, Lines 82-88 never run, leaving the optimistic state stuck.Suggested fix
- const response = await dropItem(formData); - if (response.success && response.data) { - setChecklist(response.data); - onUpdate(response.data); - return; - } - - console.error("dropItem failed:", response.error); - setChecklist(snapshot); - onUpdate(snapshot); - showToast({ - type: "error", - title: t("common.error"), - message: t("kanban.moveFailed"), - }); + try { + const response = await dropItem(formData); + if (response.success && response.data) { + setChecklist(response.data); + onUpdate(response.data); + return; + } + throw new Error(response.error || "dropItem failed"); + } catch (error) { + console.error("dropItem failed:", error); + setChecklist(snapshot); + onUpdate(snapshot); + showToast({ + type: "error", + title: t("common.error"), + message: t("kanban.moveFailed"), + }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_hooks/kanban/useKanbanDnd.ts` around lines 74 - 89, The await call to dropItem(formData) in the drop handler may throw and leaves the optimistic state; wrap the await in a try/catch so thrown errors also revert state: in the try keep the existing behavior (await dropItem, if success setChecklist(response.data), onUpdate(response.data) and return; otherwise fall through to the existing failure block), and in the catch setChecklist(snapshot), call onUpdate(snapshot), log the caught error (console.error) and call showToast with the same error message so the optimistic state is always reverted on exceptions.app/_utils/kanban/board-utils.ts-38-41 (1)
38-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp negative
visIndexbefore anchor lookup.At Line 38,
-1 < visibleItems.lengthis true, so Line 39 can produceundefinedand Line 40 throws onanchor.id.Suggested fix
export const visToColIndex = ( visibleItems: Item[], columnItems: Item[], visIndex: number, ): number => { - if (visIndex < visibleItems.length) { - const anchor = visibleItems[visIndex]; + const safeVisIndex = Math.max(0, visIndex); + if (safeVisIndex < visibleItems.length) { + const anchor = visibleItems[safeVisIndex]; const anchorIdx = columnItems.findIndex((item) => item.id === anchor.id); return anchorIdx === -1 ? columnItems.length : anchorIdx; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_utils/kanban/board-utils.ts` around lines 38 - 41, The guard that uses visIndex must also prevent negative indices: change the condition around the anchor lookup so visIndex is clamped or checked to be >= 0 before accessing visibleItems (e.g., ensure visIndex >= 0 && visIndex < visibleItems.length or set visIndex = Math.max(0, visIndex) before using it); then proceed to compute const anchor = visibleItems[visIndex], find anchorIdx in columnItems, and return anchorIdx === -1 ? columnItems.length : anchorIdx (variables: visIndex, visibleItems, anchor, anchorIdx, columnItems).app/_utils/checklist-utils.ts-107-109 (1)
107-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winServer-side parsing still preserves duplicate stored item IDs.
The new generator only helps when
metadata.idis missing. If an existing checklist file already contains duplicatemetadata.idvalues,parseMarkdown()still returns duplicateItem.ids even thoughapp/_utils/client-parser-utils.tsnow repairs them. All of the mutate-by-id paths (updateItem, delete, reorder) assume IDs are unique, so after a reload the wrong item can be mutated.Suggested fix
const { items } = buildNestedItems(itemLines, 0, 0, 0); + + const dedupeItemIds = (itemList: Item[], seen: Set<string>): void => { + itemList.forEach((item) => { + if (seen.has(item.id)) { + item.id = generateItemId(0); + } + seen.add(item.id); + if (item.children?.length) { + dedupeItemIds(item.children, seen); + } + }); + }; + + dedupeItemIds(items, new Set<string>()); let statuses = undefined;Also applies to: 230-230, 273-273
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_utils/checklist-utils.ts` around lines 107 - 109, parseMarkdown currently preserves duplicate metadata.id values so server-side parsed Item.id can collide and break mutate-by-id paths (updateItem, delete, reorder); modify parseMarkdown to detect duplicate IDs while building items and replace any repeated metadata.id with a generated id (reuse generateItemId) so every returned Item.id is unique, mirroring the repair logic in app/_utils/client-parser-utils.ts; ensure the deduplication runs during parsing (before returning items) and that generateItemId is available in that scope or passed in, so downstream code can safely assume unique Item.id values.app/api/checklists/[listId]/items/[itemIndex]/route.ts-19-27 (1)
19-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-string
text/descriptionvalues before appending toFormData.This route only checks presence. If a client sends
{ "text": {} }or{ "description": [] },FormData.append()will stringify it andupdateItem()will persist[object Object]or a comma-joined array into the markdown file. Since this is a public API endpoint, the payload needs a type check before it reaches storage.Suggested fix
const body = await request.json(); const { text, description } = body; + + if ( + (text !== undefined && typeof text !== "string") || + (description !== undefined && typeof description !== "string") + ) { + return NextResponse.json( + { error: "'text' and 'description' must be strings" }, + { status: 400 }, + ); + } if (!text && description === undefined) { return NextResponse.json( { error: "Provide at least 'text' or 'description' to update" }, { status: 400 }, ); } @@ - if (text) formData.append("text", text); - if (description !== undefined) formData.append("description", description ?? ""); + if (typeof text === "string" && text.length > 0) { + formData.append("text", text); + } + if (typeof description === "string") { + formData.append("description", description); + }Also applies to: 61-62
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/checklists/`[listId]/items/[itemIndex]/route.ts around lines 19 - 27, Validate that incoming text and description are strings before appending to FormData or calling updateItem: if text is provided and typeof text !== "string" or description is provided and typeof description !== "string", return NextResponse.json with a 400 error (e.g. "text and description must be strings when provided"). Apply this same check at the other update site that also appends to FormData (the later block that handles partial updates) so non-string values are rejected consistently before any FormData.append or updateItem(...) calls.app/_components/FeatureComponents/Kanban/KanbanCardDetail.tsx-343-369 (1)
343-369:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep date-only fields as date-only, and don’t wipe
startDatewhen clearingtargetDate.These handlers currently round-trip a
DatePickervalue throughnew Date(value).toISOString()and also clearstartDatein the!valuebranch. For a US user, selecting2026-06-13can re-render as2026-06-12after_toLocalDateValue()converts the stored UTC midnight back to local time, and removing only the target date will silently delete the saved start date as well.Suggested fix
const handleStartDateChange = async (value: string) => { setStartDateInput(value); - const iso = value ? new Date(value).toISOString() : ""; const targetKey = _dateKey(targetDateInput); if (value && targetKey && value > targetKey) { setTargetDateInput(value); - await _saveField({ startDate: iso, targetDate: iso }); + await _saveField({ startDate: value, targetDate: value }); return; } - await _saveField({ startDate: iso }); + await _saveField({ startDate: value }); }; const handleTargetDateChange = async (value: string) => { setTargetDateInput(value); if (!value) { - setStartDateInput(""); - await _saveField({ targetDate: "", startDate: "" }); + await _saveField({ targetDate: "" }); return; } - const iso = new Date(value).toISOString(); const startKey = _dateKey(startDateInput); if (startKey && startKey > value) { setStartDateInput(value); - await _saveField({ targetDate: iso, startDate: iso }); + await _saveField({ targetDate: value, startDate: value }); return; } - await _saveField({ targetDate: iso }); + await _saveField({ targetDate: value }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_components/FeatureComponents/Kanban/KanbanCardDetail.tsx` around lines 343 - 369, The handlers handleStartDateChange and handleTargetDateChange currently convert the date-picker value to an ISO UTC datetime and also clear startDate when targetDate is cleared; change them to keep and persist date-only strings (use the raw YYYY-MM-DD value from the picker instead of new Date(...).toISOString()) and do not wipe startDate when targetDate is cleared. Concretely: in handleStartDateChange and handleTargetDateChange use the incoming value (not new Date(...).toISOString()) when calling _saveField, use _dateKey(startDateInput)/_dateKey(targetDateInput) only for comparisons, and in the !value branch of handleTargetDateChange only set targetDate to "" (do not set startDate to ""). Ensure comparisons that enforce start<=target still update both fields when needed.
🧹 Nitpick comments (3)
tests/server-actions/auth.test.ts (1)
346-351: ⚡ Quick winTighten the remember-me assertion to avoid false-positive test passes.
expect.any(Boolean)verifies only type, not behavior. This should assertfalsefor this no-rememberMe input path, with a separate test assertingtruewhenrememberMeis submitted.Proposed fix for this test
expect(mockCreateSession).toHaveBeenCalledWith( expect.any(String), 'alice', - 'ldap', - expect.any(Boolean) + 'ldap', + false )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server-actions/auth.test.ts` around lines 346 - 351, The test currently uses expect.any(Boolean) for the remember-me flag which is too loose; update the assertion on mockCreateSession to expect(false) for this no-rememberMe input path (i.e., change expect.any(Boolean) to false in the expect(mockCreateSession).toHaveBeenCalledWith(...) call) and add a separate test that submits rememberMe and asserts expect(mockCreateSession).toHaveBeenCalledWith(..., true) to cover the positive case.app/_components/FeatureComponents/Kanban/KanbanCard.tsx (1)
98-102: ⚡ Quick winInclude the timer-stop callback in the effect dependency list.
Line 102 only depends on
isLifted; includestopTimerOnDragto avoid stale closure risk.Suggested patch
+ const { stopTimerOnDrag } = kanbanItemHook; + useEffect(() => { if (isLifted) { - kanbanItemHook.stopTimerOnDrag(); + stopTimerOnDrag(); } - }, [isLifted]); + }, [isLifted, stopTimerOnDrag]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_components/FeatureComponents/Kanban/KanbanCard.tsx` around lines 98 - 102, The effect in KanbanCard's useEffect currently only depends on isLifted which can capture a stale closure of kanbanItemHook.stopTimerOnDrag; update the dependency array to include kanbanItemHook.stopTimerOnDrag (or the stopTimerOnDrag function reference) so the effect re-runs when that callback changes, i.e., change the dependency list from [isLifted] to [isLifted, kanbanItemHook.stopTimerOnDrag] (or destructure stopTimerOnDrag and use [isLifted, stopTimerOnDrag]) to avoid stale closures.tests/server-actions/drop-item.test.ts (1)
154-227: ⚡ Quick winAdd regression tests for
invalid targetStatusandbroadcast failure after write.Current suite does not assert these two contract paths, which are high-risk for state consistency.
Suggested test additions
+ it("rejects an invalid targetStatus", async () => { + const result = await dropItem( + createFormData({ + uuid: BOARD_UUID, + itemId: "task-1", + targetStatus: "not-a-real-status", + targetIndex: "0", + }), + ); + + expect(result.success).toBe(false); + expect(mockServerWriteFile).not.toHaveBeenCalled(); + expect(mockBroadcast).not.toHaveBeenCalled(); + }); + + it("still returns success when broadcast fails after write", async () => { + mockBroadcast.mockRejectedValue(new Error("ws down")); + + const result = await dropItem( + createFormData({ + uuid: BOARD_UUID, + itemId: "task-1", + targetStatus: "todo", + targetIndex: "0", + }), + ); + + expect(result.success).toBe(true); + expect(mockServerWriteFile).toHaveBeenCalledTimes(1); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server-actions/drop-item.test.ts` around lines 154 - 227, Add two tests to the dropItem test suite: (1) "rejects invalid targetStatus" — call dropItem(createFormData({... targetStatus: "invalid", ...})) and assert result.success is false, result.error indicates invalid status (or at least non-success), and that mockServerWriteFile and mockBroadcast were not called; (2) "handles broadcast failure after write" — arrange mockServerWriteFile to resolve and mockBroadcast to throw/reject, call dropItem with a valid payload, assert the final result reports failure (result.success false and an appropriate error), assert mockServerWriteFile was called (write occurred) and mockBroadcast was called (and failed), ensuring the test documents the contract that writes are not rolled back on broadcast failure. Reference the existing helpers and mocks: dropItem, createFormData, mockServerWriteFile, and mockBroadcast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a620a354-9b49-44d6-b3c1-49efcab1c7e9
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (91)
app/(loggedInRoutes)/settings/connections/page.tsxapp/_components/FeatureComponents/Home/Parts/ChecklistHome.tsxapp/_components/FeatureComponents/Home/Parts/NotesHome.tsxapp/_components/FeatureComponents/Kanban/ArchivedItemsModal.tsxapp/_components/FeatureComponents/Kanban/CalendarView.tsxapp/_components/FeatureComponents/Kanban/Kanban.tsxapp/_components/FeatureComponents/Kanban/KanbanCard.tsxapp/_components/FeatureComponents/Kanban/KanbanCardDetail.tsxapp/_components/FeatureComponents/Kanban/KanbanCardDetailProperties.tsxapp/_components/FeatureComponents/Kanban/KanbanColumn.tsxapp/_components/FeatureComponents/Kanban/KanbanItemContent.tsxapp/_components/FeatureComponents/Notes/Parts/MermaidRenderer.tsxapp/_components/FeatureComponents/Notes/Parts/TipTap/CustomExtensions/ExcalidrawExtension.tsxapp/_components/FeatureComponents/Notes/Parts/TipTap/CustomExtensions/MermaidExtension.tsxapp/_components/FeatureComponents/Notes/Parts/TipTap/CustomExtensions/SlashCommands.tsxapp/_components/FeatureComponents/Profile/Parts/ConnectionsGraph/ConnectionsGraph.tsxapp/_components/FeatureComponents/Profile/Parts/ConnectionsGraph/graph-data.tsapp/_components/FeatureComponents/Profile/Parts/LinksTab.tsxapp/_components/FeatureComponents/Profile/Parts/UserPreferencesTab.tsxapp/_components/GlobalComponents/Auth/LoginForm.tsxapp/_components/GlobalComponents/Dropdowns/Dropdown.tsxapp/_components/GlobalComponents/Modals/Modal.tsxapp/_components/GlobalComponents/Sidebar/SidebarWrapper.tsxapp/_consts/dnd.tsapp/_hooks/dnd/DndProvider.tsxapp/_hooks/dnd/index.tsxapp/_hooks/dnd/useDragItem.tsapp/_hooks/dnd/useDropList.tsapp/_hooks/kanban/useCalendarView.tsapp/_hooks/kanban/useKanban.tsapp/_hooks/kanban/useKanbanDnd.tsapp/_hooks/kanban/useKanbanItem.tsxapp/_hooks/useChecklist.tsxapp/_hooks/useNoteEditor.tsxapp/_providers/NavigationGuardProvider.tsxapp/_schemas/user-schemas.tsapp/_server/actions/auth/index.tsapp/_server/actions/checklist-item/crud.tsapp/_server/actions/checklist-item/drop.tsapp/_server/actions/checklist-item/index.tsapp/_server/actions/checklist-item/status.tsapp/_server/actions/file/index.tsapp/_server/actions/note/parsers.tsapp/_server/actions/session/index.tsapp/_server/actions/users/crud.tsapp/_styles/globals.cssapp/_translations/de.jsonapp/_translations/en.jsonapp/_translations/es.jsonapp/_translations/fr.jsonapp/_translations/it.jsonapp/_translations/klingon.jsonapp/_translations/ko.jsonapp/_translations/nl.jsonapp/_translations/pirate.jsonapp/_translations/pl.jsonapp/_translations/pt.jsonapp/_translations/ru.jsonapp/_translations/tr.jsonapp/_translations/zh.jsonapp/_types/checklist.tsapp/_types/dnd.tsapp/_types/index.tsapp/_types/user.tsapp/_utils/checklist-utils.tsapp/_utils/client-parser-utils.tsapp/_utils/dnd/auto-scroll.tsapp/_utils/dnd/dnd-math.tsapp/_utils/dnd/drag-store.tsapp/_utils/dnd/rect-registry.tsapp/_utils/item-status-utils.tsapp/_utils/kanban/board-utils.tsapp/_utils/kanban/calendar-utils.tsapp/_utils/kanban/index.tsxapp/api/checklists/[listId]/items/[itemIndex]/route.tsapp/api/checklists/[listId]/items/reorder/route.tsapp/api/checklists/route.tsapp/api/docs/route.tsapp/api/search/route.tsnext.config.mjspackage.jsonpatches/body-size-limit_20260427.jspublic/api/paths/checklists.yamlpublic/api/paths/search.yamltests/server-actions/auth.test.tstests/server-actions/drop-item.test.tstests/utils/board-utils.test.tstests/utils/client-parser-utils.test.tstests/utils/connections-graph-data.test.tstests/utils/dnd-math.test.tstests/utils/item-status-utils.test.ts
💤 Files with no reviewable changes (1)
- patches/body-size-limit_20260427.js
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/_utils/checklist-utils.ts (1)
229-241:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
itemMetadatacan overwrite the resolvedid.After setting fallback IDs (Lines 230 and 273), spreading
...itemMetadata(Lines 240 and 279) can restore invalid/duplicateidvalues. This parser also lacks a post-parse dedupe pass, so duplicates can survive into runtime state.Suggested fix pattern
- item = { - id: itemMetadata.id || generateItemId(parentLevel), + const { id: metadataId, ...restMetadata } = itemMetadata; + const resolvedId = + typeof metadataId === "string" && metadataId.trim() + ? metadataId + : generateItemId(parentLevel); + item = { + id: resolvedId, text: itemText, completed, order: currentItemIndex, status, timeEntries, estimatedTime, startDate, targetDate, description, - ...itemMetadata, + ...restMetadata, ...(recurrence ? { recurrence } : {}), ... - item = { - id: itemMetadata.id || generateItemId(parentLevel), + const { id: metadataId, ...restMetadata } = itemMetadata; + const resolvedId = + typeof metadataId === "string" && metadataId.trim() + ? metadataId + : generateItemId(parentLevel); + item = { + id: resolvedId, text: itemText, completed, order: currentItemIndex, description, ...metadata, - ...itemMetadata, + ...restMetadata, ...(recurrence && { recurrence }), };Also applies to: 272-280
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_utils/checklist-utils.ts` around lines 229 - 241, The constructed item object currently spreads ...itemMetadata after assigning id (using generateItemId(parentLevel)), which allows an invalid/duplicate id from itemMetadata to overwrite the resolved id; change the construction so the resolved id cannot be overwritten (either spread itemMetadata first and then set id: itemMetadata.id || generateItemId(parentLevel), or remove/id-sanitize itemMetadata.id before spreading), and add a post-parse dedupe pass in the parser that scans generated items for duplicate ids and regenerates or reassigns unique ids (use the same generateItemId logic) to ensure no duplicates survive into runtime state; apply the same change to the other symmetric site where items are built.
♻️ Duplicate comments (2)
app/api/checklists/[listId]/items/reorder/route.ts (2)
54-59:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSelf-drop corrupts the item tree (still unresolved).
When
activeItemId === overItemId, the code removes the item at line 86 then attempts to re-insert it relative to itself. InisDropIntomode, the item gets pushed into its own (now-orphaned) children; otherwise,findIndexreturns-1after removal, causing incorrect insertion.Add an early return before the splice:
if (!activeInfo || !overInfo) { return NextResponse.json({ error: "Item not found" }, { status: 404 }); } + + if (activeItemId === overItemId) { + return NextResponse.json({ success: true }); + } activeInfo.siblings.splice(activeInfo.index, 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/checklists/`[listId]/items/reorder/route.ts around lines 54 - 59, Add an early no-op return when the dragged item ID equals the target ID to avoid removing and re-inserting the same node: check if activeItemId === overItemId immediately after validating inputs (before the splice/removal logic that currently mutates the tree) and return early (e.g., a 200/204 success or JSON indicating no change) so that isDropInto and subsequent findIndex logic never run on a now-orphaned node.
68-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing edit permission check on shared lists (still unresolved).
This route authenticates the caller via
withApiAuthbut does not verifyPermissionTypes.EDITbefore writing. On shared checklists, any authenticated user with API access can reorder items, bypassing the permission model enforced by other checklist mutation endpoints likestatus.tsandcrud.ts.const list = await getListById(params.listId, user.username); if (!list) { return NextResponse.json({ error: "List not found" }, { status: 404 }); } + + const { checkUserPermission } = await import("`@/app/_server/actions/sharing`"); + const canEdit = await checkUserPermission( + list.uuid || list.id, + list.category || "Uncategorized", + "checklist", + user.username, + "edit", + ); + if (!canEdit) { + return NextResponse.json({ error: "Permission denied" }, { status: 403 }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/checklists/`[listId]/items/reorder/route.ts around lines 68 - 71, After fetching the list with getListById in the reorder route, enforce the same edit-permission check used by other checklist mutation endpoints: verify the authenticated user has PermissionTypes.EDIT on that list (mirror the check logic used in status.ts/crud.ts, e.g., call the project's helper/utility that checks list permissions or inspect list.permissions for PermissionTypes.EDIT). If the user lacks edit permission, return a 403 response (e.g., NextResponse.json({ error: "Forbidden" }, { status: 403 })) and do not perform the reorder; add this check immediately after the existing list existence check and before any write operations. Ensure you import or reference PermissionTypes and the same permission-check helper used elsewhere so behavior is consistent.
🟡 Minor comments (15)
app/_translations/de.json-285-285 (1)
285-285:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTranslate
auth.rememberMein the non-English locales.
This new login label is still hard-coded in English, so German and Spanish users will see an untranslated string.
app/_translations/de.json#L285-L285: replace"Remember me"with a German translation.app/_translations/es.json#L287-L287: replace"Remember me"with a Spanish translation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_translations/de.json` at line 285, Replace the hard-coded English login label for auth.rememberMe in both locale files: in app/_translations/de.json (lines 285-285) change the value "Remember me" to the German translation "Angemeldet bleiben"; in app/_translations/es.json (lines 287-287) change the value "Remember me" to the Spanish translation "Recuérdame".app/_translations/de.json-695-715 (1)
695-715:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFinish localizing the new profile graph labels.
Several newly added graph strings are still in English in both locales, and the German file also has a typo invalidationErrorTitle(Validerungsfehler).
app/_translations/de.json#L695-L715: translate the remaining graph labels (visibleGraphSummary,graphTruncated,totalConnections, etc.) and fix the typo.app/_translations/es.json#L695-L715: translate the remaining graph labels instead of shipping English fallbacks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_translations/de.json` around lines 695 - 715, In app/_translations/de.json (lines 695-715) fix the typo by changing validationErrorTitle value to "Validierungsfehler" and provide German translations for the remaining keys visibleGraphSummary, graphTruncated, totalConnections, inboundConnections, outboundConnections, updated, openItem, selectItem, inspectNodeHint, graphSearchPlaceholder, orphans, labels, arrows, nodeSize, linkWidth, linkDistance, repelForce, tagLinks, linkedItems, uuid; in app/_translations/es.json (lines 695-715) replace the English fallbacks by adding Spanish translations for the same keys (visibleGraphSummary, graphTruncated, totalConnections, inboundConnections, outboundConnections, updated, openItem, selectItem, inspectNodeHint, graphSearchPlaceholder, orphans, labels, arrows, nodeSize, linkWidth, linkDistance, repelForce, tagLinks, linkedItems, uuid) so both locale files contain localized strings and no English remains.app/_translations/fr.json-696-713 (1)
696-713:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNew locale keys were added without full localization, causing mixed-language UI.
app/_translations/fr.json#L696-L713: translate new profile graph labels/messages to French.app/_translations/fr.json#L287-L287: translateauth.rememberMeto French.app/_translations/it.json#L696-L713: translate new profile graph labels/messages to Italian.app/_translations/it.json#L287-L287: translateauth.rememberMeto Italian.app/_translations/klingon.json#L721-L739: align new profile graph labels/messages with the locale’s translation strategy.app/_translations/klingon.json#L312-L312: localizeauth.rememberMeconsistently with the rest of the locale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_translations/fr.json` around lines 696 - 713, Update the untranslated/new locale keys for each affected file: for app/_translations/fr.json (lines 696-713) provide French translations for the keys visibleGraphSummary, graphTruncated, totalConnections, inboundConnections, outboundConnections, updated, openItem, selectItem, inspectNodeHint, graphSearchPlaceholder, orphans, labels, arrows, nodeSize, linkWidth, linkDistance, repelForce, tagLinks; for app/_translations/fr.json (line 287) translate auth.rememberMe into French; for app/_translations/it.json (lines 696-713) provide Italian translations for the same profile graph keys; for app/_translations/it.json (line 287) translate auth.rememberMe into Italian; for app/_translations/klingon.json (lines 721-739) add/adjust the profile graph keys to follow the locale’s existing translation style and conventions (use the same key names as above); for app/_translations/klingon.json (line 312) localize auth.rememberMe consistently with the rest of the Klingon locale. Ensure each translation preserves the placeholder tokens ({items}, {links}, {visible}, {omitted}) exactly and keep key names unchanged.app/_translations/ko.json-312-312 (1)
312-312:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTranslate newly added Korean locale strings that are still in English.
Line 312 and Lines 721-738 are currently English in
ko.json, which will produce mixed-language UI in login and the connections graph panel.Also applies to: 721-738
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_translations/ko.json` at line 312, Translate the remaining English strings in the Korean locale file so the UI is consistent: replace the "rememberMe" value with its Korean translation and locate and translate the other English entries used by the login screen and the connections graph panel (the remaining English strings in ko.json) to appropriate Korean text; ensure you only change the values (not the keys) so keys like "rememberMe" remain intact and update any punctuation/spacing to match existing translations.app/_translations/pirate.json-312-312 (1)
312-312:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep new Pirate locale entries in the same voice as the rest of the file.
Line 312 and Lines 721-739 are plain English and break the established Pirate localization style for this locale.
Also applies to: 721-739
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_translations/pirate.json` at line 312, The added translation "rememberMe": "Remember me" is in plain English and breaks the Pirate locale voice—replace its value with a Pirate-styled string (e.g., "Remind me, matey" or similar) and likewise convert the other plain-English entries noted in the review later in this file to match the established Pirate tone; update only the translation values (leave keys like "rememberMe" unchanged) and ensure punctuation/contractions match the existing Pirate phrasing used elsewhere in the file.app/_translations/nl.json-287-287 (1)
287-287:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winComplete Dutch localization for new auth/graph keys.
Line 287 and Lines 696-713 are still English in
nl.json; this causes mixed-language UX in authentication and graph controls.Also applies to: 696-713
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_translations/nl.json` at line 287, The "rememberMe" key currently has an English value and several other auth/graph strings (lines 696–713) are still in English; update the value of "rememberMe" to the Dutch translation "Onthoud mij" and replace every remaining English string in that auth/graph block with their correct Dutch equivalents so the nl.json contains fully Dutch text for the authentication and graph control keys (ensure exact JSON keys are preserved, only change the string values).app/_translations/pt.json-711-729 (1)
711-729:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTranslate the new graph-control labels in
profileto Portuguese.Lines 712-729 are still English (
visibleGraphSummary,graphTruncated,totalConnections,openItem,graphSearchPlaceholder, etc.), causing mixed-language UI in the PT locale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_translations/pt.json` around lines 711 - 729, The PT translation file still contains English strings for several graph-control keys; update the values for the keys visibleGraphSummary, graphTruncated, totalConnections, inboundConnections, outboundConnections, updated, openItem, selectItem, inspectNodeHint, graphSearchPlaceholder, orphans, labels, arrows, nodeSize, linkWidth, linkDistance, repelForce, and tagLinks in app/_translations/pt.json to proper Portuguese equivalents (e.g., "visibleGraphSummary" -> "{items} itens visíveis, {links} ligações visíveis", translate "graphTruncated" to something like "Mostrando os {visible} itens com mais conexões. {omitted} itens com menos conexões foram omitidos.", and localize the other keys similarly) so the PT locale no longer mixes English and Portuguese.app/_translations/ru.json-311-313 (1)
311-313:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize newly added Russian strings to avoid mixed-language UI.
Line 312 (
auth.rememberMe) and Lines 721-738 (profilegraph labels/controls) are still English. Russian users will see mixed-language text in login and connections graph screens.Suggested patch (examples)
- "rememberMe": "Remember me" + "rememberMe": "Запомнить меня" - "visibleGraphSummary": "{items} visible items, {links} visible links", + "visibleGraphSummary": "{items} видимых элементов, {links} видимых связей", - "totalConnections": "Total", + "totalConnections": "Всего", - "openItem": "Open item", + "openItem": "Открыть элемент"Also applies to: 721-738
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_translations/ru.json` around lines 311 - 313, The Russian locale contains untranslated entries causing mixed-language UI: update the "auth.rememberMe" key in app/_translations/ru.json to a proper Russian translation and translate the profile graph labels/controls referenced as the profile.* keys (the labels/controls shown around lines ~721-738) into Russian as well; locate and replace the English strings with their Russian equivalents, preserving the existing JSON keys and punctuation/escaping so only the values change.app/_translations/pl.json-286-287 (1)
286-287:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize newly added Polish strings to avoid mixed-language UI.
Line 287 (
auth.rememberMe) and Lines 696-713 (profilegraph labels) are still in English, while surrounding Polish copy is localized. This will surface mixed-language text in core UI flows.Suggested patch (examples)
- "rememberMe": "Remember me" + "rememberMe": "Zapamiętaj mnie" - "visibleGraphSummary": "{items} visible items, {links} visible links", + "visibleGraphSummary": "{items} widocznych elementów, {links} widocznych połączeń", - "totalConnections": "Total", + "totalConnections": "Łącznie", - "openItem": "Open item", + "openItem": "Otwórz element"Also applies to: 696-713
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_translations/pl.json` around lines 286 - 287, Update the untranslated English strings in the translations JSON: replace the auth.rememberMe key value with a Polish translation (e.g., "Zapamiętaj mnie") and translate the profile graph label strings (the keys under "profile" that appear around lines 696–713) into Polish so the UI is fully localized and consistent with surrounding entries; keep the existing keys (auth.rememberMe and profile.*) unchanged and only modify their string values.app/_translations/pt.json-688-688 (1)
688-688:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix encoded quotes and punctuation in PT user-facing strings.
Line 688 and Line 1021 use literal
", which will render incorrectly in plain text. Line 1022 is missing a closing parenthesis in the shortcut hint.Suggested patch
- "noArchivedItemsFound": "Nenhum item arquivado encontrado correspondente a "{searchQuery}"", + "noArchivedItemsFound": "Nenhum item arquivado encontrado correspondente a \"{searchQuery}\"", - "noEmojisYet": "Ainda não foram criados emojis personalizados. Clique em "Adicionar emoji" para começar.", + "noEmojisYet": "Ainda não foram criados emojis personalizados. Clique em \"Adicionar emoji\" para começar.", - "enterAnyEmoji": "Introduzir qualquer emoji ou usar o seletor de emoji (Windows: Win+.; Mac: Ctrl+Cmd+Space", + "enterAnyEmoji": "Introduzir qualquer emoji ou usar o seletor de emoji (Windows: Win+.; Mac: Ctrl+Cmd+Space)"Also applies to: 1021-1022
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_translations/pt.json` at line 688, The PT translation has HTML-encoded double quotes and a missing parenthesis in a shortcut hint: replace the " sequences with plain double-quote characters in the "noArchivedItemsFound" value and in any other PT strings containing " (search the file for """), and add the missing closing parenthesis to the shortcut hint string referenced in the PT file (fix the shortcut hint entry on the same section so it ends with the closing ")").app/_components/FeatureComponents/Kanban/KanbanCardDetail.tsx-310-326 (1)
310-326:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOptimistic update may leave UI inconsistent on server failure.
setStatusInput(status)is called before the server request. IfupdateItemStatusfails or returnssuccess: false, the localstatusInputstate remains updated while the actual item status is unchanged.Consider reverting the optimistic update on failure:
🛠️ Suggested fix
const handleStatusChange = async (status: string) => { + const previousStatus = statusInput; setStatusInput(status); const formData = new FormData(); formData.append("listId", checklistId); formData.append("itemId", item.id); formData.append("status", status); formData.append("category", category); const result = await updateItemStatus(formData); if (result.success && result.data) { onUpdate(result.data); const updatedItem = _findItemInChecklist(result.data, item.id); if (updatedItem) { setItem(updatedItem); setStatusInput(updatedItem.status || status); } + } else { + setStatusInput(previousStatus); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_components/FeatureComponents/Kanban/KanbanCardDetail.tsx` around lines 310 - 326, The handler handleStatusChange is doing an optimistic update by calling setStatusInput(status) before the network call; capture the previous status (e.g., const prev = statusInput) then setStatusInput(status) optimistically, await updateItemStatus(formData), and if the call fails or returns result.success === false revert with setStatusInput(prev) (and avoid calling onUpdate/setItem on failure); on success continue calling onUpdate(result.data) and update item/state as now done. Also consider surfacing an error (e.g., a toast) when reverting so users know the change did not persist.app/_hooks/useNoteEditor.tsx-170-180 (1)
170-180:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing dependency
notesDefaultModein effect.The effect checks
notesDefaultMode !== "edit"at line 171, butnotesDefaultModeis not included in the dependency array. IfnotesDefaultModechanges (e.g., user preference update), the effect won't re-run.🔧 Suggested fix
- }, [contentIsDirty, title, category, note, isEditing]); + }, [contentIsDirty, title, category, note, isEditing, notesDefaultMode, setProviderUnsaved]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_hooks/useNoteEditor.tsx` around lines 170 - 180, The useEffect in useNoteEditor watches contentIsDirty, title, category, note, and isEditing but omits notesDefaultMode even though the callback checks notesDefaultMode !== "edit"; update the effect's dependency array to include notesDefaultMode so the effect will re-run when the default mode changes, ensuring setHasUnsavedChanges(...) and setProviderUnsaved(...) are recalculated correctly; keep the existing logic in the effect (using title, category, note, isEditing, contentIsDirty) and only modify the dependency list to add notesDefaultMode.app/_components/FeatureComponents/Kanban/KanbanColumn.tsx-144-152 (1)
144-152:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClose the archive-all confirmation modal after a successful archive.
The confirm handler never resets
showArchiveAllModal, so the modal can remain open after a successful archive action.Suggested fix
const handleArchiveAll = async () => { if (!onArchiveAll || archivableCount === 0) return; setIsArchiving(true); try { await onArchiveAll(); + setShowArchiveAllModal(false); } finally { setIsArchiving(false); } };Also applies to: 244-247
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_components/FeatureComponents/Kanban/KanbanColumn.tsx` around lines 144 - 152, The archive-all confirmation modal isn't being closed after a successful archive; update the handler(s) to reset showArchiveAllModal only on success. In the handleArchiveAll function (and the equivalent handler at the other site), move or add setShowArchiveAllModal(false) into the try block immediately after the awaited onArchiveAll() call so the modal closes on success, and keep setIsArchiving(false) in the finally block to always clear the loading state.app/_components/GlobalComponents/Auth/LoginForm.tsx-199-213 (1)
199-213:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the remember-me text click target consistent with disabled state.
The text click handler still toggles state while the control is disabled (loading/SSO/lockout), which breaks expected disabled behavior.
Suggested fix
<span - onClick={() => setRememberMe(!rememberMe)} + onClick={() => { + if (isLoading || isSsoLoading || countdown > 0) return; + setRememberMe((prev) => !prev); + }} className="text-sm lg:text-xs text-muted-foreground cursor-pointer select-none" > {t("rememberMe")} </span>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_components/GlobalComponents/Auth/LoginForm.tsx` around lines 199 - 213, The clickable label for the "remember me" toggle currently calls setRememberMe even when the control is disabled (checked via isLoading, isSsoLoading, countdown), so change the span's onClick to respect the same disabled condition used by the Toggle: guard the handler so it only calls setRememberMe(!rememberMe) when isLoading === false && isSsoLoading === false && countdown === 0 (or remove the handler when those conditions are true); also update the span's className to remove cursor-pointer / use a non-interactive style when disabled so visual affordance matches behavior. Reference: the Toggle component/props, rememberMe state, setRememberMe, isLoading, isSsoLoading, countdown and the span element in LoginForm.tsx.app/_server/actions/users/crud.ts-207-207 (1)
207-207:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep user default preference initialization consistent across creation flows.
Line 207 sets
hideMobileStatusDropdown: "disable"increateUser, butregisterinapp/_server/actions/auth/index.tsstill creates users without this field. This makes defaults depend on how the account was created.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_server/actions/users/crud.ts` at line 207, The createUser flow sets the user preference hideMobileStatusDropdown: "disable" but the register flow in register() when creating a new user omits this field, causing inconsistent defaults; update the register user-creation logic to include hideMobileStatusDropdown: "disable" (or better, refactor both createUser and register to use a shared defaultPreferences object) so newly registered users get the same hideMobileStatusDropdown default as users created via createUser.
🧹 Nitpick comments (7)
app/_components/FeatureComponents/Kanban/KanbanCard.tsx (1)
98-102: 💤 Low valueMissing dependency in useEffect.
The
useEffectreferenceskanbanItemHook.stopTimerOnDragbut doesn't include it in the dependency array. While this likely works becausestopTimerOnDragis stable, ESLint's exhaustive-deps rule will warn. Consider adding the dependency or using a ref to silence the warning explicitly.useEffect(() => { if (isLifted) { kanbanItemHook.stopTimerOnDrag(); } - }, [isLifted]); + }, [isLifted, kanbanItemHook.stopTimerOnDrag]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_components/FeatureComponents/Kanban/KanbanCard.tsx` around lines 98 - 102, The useEffect in KanbanCard.tsx that calls kanbanItemHook.stopTimerOnDrag when isLifted changes is missing kanbanItemHook.stopTimerOnDrag in its dependency array; update the effect to include kanbanItemHook.stopTimerOnDrag (or, if you prefer to keep a stable reference, create a ref or memoized callback in the hook and reference that) so the dependency array lists both isLifted and kanbanItemHook.stopTimerOnDrag and ESLint exhaustive-deps warnings are resolved.app/_components/FeatureComponents/Kanban/Kanban.tsx (1)
154-208: ⚖️ Poor tradeoffSequential archiving may be slow for large columns.
handleArchiveAllawaits eacharchiveItemcall sequentially. For columns with many items, this could take significant time and leave the UI in an intermediate state.Consider batching or parallel execution with
Promise.all(if the server supports it), or at minimum show a loading indicator during the operation.app/_utils/kanban/calendar-utils.ts (1)
39-40: 💤 Low valueMinor: Redundant normalization in return object.
Both
startDateandendDateassignments perform the same comparison. SincestartDateis already normalized to be<= endDateon line 39, line 40's comparison is redundant.return { id: item.id, title: item.text, startDate: startDate <= endDate ? startDate : endDate, - endDate: endDate >= startDate ? endDate : startDate, + endDate: startDate <= endDate ? endDate : startDate,This is functionally equivalent but slightly clearer that one condition determines both assignments. However, current code is still correct.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_utils/kanban/calendar-utils.ts` around lines 39 - 40, The return object does redundant comparisons for startDate/endDate; compute the boolean once (e.g., const isStartBefore = startDate <= endDate) and use it to assign both fields so startDate = isStartBefore ? startDate : endDate and endDate = isStartBefore ? endDate : startDate, replacing the two repeated ternaries for clarity.app/_components/FeatureComponents/Notes/Parts/TipTap/CustomExtensions/SlashCommands.tsx (1)
351-359: 💤 Low valueUnbounded results may cause performance or usability issues with large datasets.
Removing the result limit means all matching items are returned. For users with hundreds of notes/checklists, this could cause:
- Slow rendering of the suggestion dropdown
- Overwhelming UI with too many options
Consider adding a reasonable upper limit while keeping the full list for empty queries:
💡 Optional: Add reasonable limit
- if (!query.trim()) return allItems; + if (!query.trim()) return allItems.slice(0, 50); const lowerCaseQuery = query.toLowerCase(); - return allItems.filter( + return allItems.filter( (item) => item.title.toLowerCase().includes(lowerCaseQuery) || item.category?.toLowerCase().includes(lowerCaseQuery) - ); + ).slice(0, 50);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_components/FeatureComponents/Notes/Parts/TipTap/CustomExtensions/SlashCommands.tsx` around lines 351 - 359, The current filtering returns all matching items (using allItems, query and lowerCaseQuery) which can be unbounded; add a reasonable cap (e.g. const MAX_SUGGESTIONS = 50) and apply it after filtering so that the returned array is limited (use .filter(...).slice(0, MAX_SUGGESTIONS)), leaving the early-return for empty queries if you want the full list shown when the query is blank; update any related types/consumers if they assume unlimited results.app/api/search/route.ts (1)
3-3: ⚡ Quick winUnused import
search.The
searchfunction is imported but never used in this file.-import { search } from "`@/app/_server/actions/search`";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/search/route.ts` at line 3, The import of the symbol "search" is unused; either remove the unused import line "import { search } from '`@/app/_server/actions/search`';" from route.ts or, if the endpoint should call that function, replace the unused import by invoking search inside the exported handler (e.g., call search(...) and return its result) and remove any other redundant code accordingly; ensure there are no leftover linter warnings after the change.tests/server-actions/auth.test.ts (1)
346-351: ⚡ Quick win
expect.any(Boolean)is too loose for remember-me regression coverage.This assertion won’t catch incorrect mapping of remember-me state. Add explicit cases (e.g.,
rememberMe=trueandrememberMe=false) and assert exact fourth-argument values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server-actions/auth.test.ts` around lines 346 - 351, The assertion using expect.any(Boolean) is too loose for remember-me regression coverage; update the test(s) that call mockCreateSession so the fourth argument is asserted explicitly for both rememberMe=true and rememberMe=false (either by adding two test cases or parameterizing the test) — replace expect.any(Boolean) with the exact boolean values (true for the remember-me case and false for the non-remember case) when calling expect(mockCreateSession).toHaveBeenCalledWith(...), keeping the other arguments as expect.any(String), 'alice', 'ldap' as before.tests/utils/connections-graph-data.test.ts (1)
244-278: Replace source-string assertions with behavior-based tests for LinksTab/ConnectionsGraph.In
tests/utils/connections-graph-data.test.ts(lines 244-278), these tests read component TSX viareadFileSyncand assert exact substrings (order-*class order,t("profile.unknown"), and internal constants likeMIN_POINTER_RADIUS/nodePointerAreaPaint/enableNodeDrag). RenderLinksTab/ConnectionsGraphand assert observable behavior instead (mobile vs desktop DOM order, rendered label text, and pointer/drag behavior under coarse vs fine pointer).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/connections-graph-data.test.ts` around lines 244 - 278, The tests currently assert literal source substrings; instead render the actual components and assert behavior: replace the file-string checks in the test for LinksTab by rendering the LinksTab component and asserting DOM order (querySelectorAll for the graph and controls and verify graph appears before controls when simulating mobile and after when simulating desktop by toggling window.innerWidth and dispatching a resize or mocking matchMedia), replace the translation key check by rendering LinksTab with your i18n/test translation provider (or a mocked t) and asserting the rendered label text uses the profile.unknown translation, and replace the ConnectionsGraph string checks by rendering ConnectionsGraph and asserting pointer/drag behavior: mock matchMedia('(pointer: coarse)') to simulate coarse vs fine pointers and assert enableNodeDrag effect (e.g., presence/absence of drag handlers or draggable attributes) and verify the pointer hit area size is applied in the DOM (computed radius or attribute reflecting MIN_POINTER_RADIUS / globalScale) rather than searching for MIN_POINTER_RADIUS/nodePointerAreaPaint/globalScale strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ee6ec96d-ea2c-4a83-8bfd-d6cf21662cb5
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (91)
app/(loggedInRoutes)/settings/connections/page.tsxapp/_components/FeatureComponents/Home/Parts/ChecklistHome.tsxapp/_components/FeatureComponents/Home/Parts/NotesHome.tsxapp/_components/FeatureComponents/Kanban/ArchivedItemsModal.tsxapp/_components/FeatureComponents/Kanban/CalendarView.tsxapp/_components/FeatureComponents/Kanban/Kanban.tsxapp/_components/FeatureComponents/Kanban/KanbanCard.tsxapp/_components/FeatureComponents/Kanban/KanbanCardDetail.tsxapp/_components/FeatureComponents/Kanban/KanbanCardDetailProperties.tsxapp/_components/FeatureComponents/Kanban/KanbanColumn.tsxapp/_components/FeatureComponents/Kanban/KanbanItemContent.tsxapp/_components/FeatureComponents/Notes/Parts/MermaidRenderer.tsxapp/_components/FeatureComponents/Notes/Parts/TipTap/CustomExtensions/ExcalidrawExtension.tsxapp/_components/FeatureComponents/Notes/Parts/TipTap/CustomExtensions/MermaidExtension.tsxapp/_components/FeatureComponents/Notes/Parts/TipTap/CustomExtensions/SlashCommands.tsxapp/_components/FeatureComponents/Profile/Parts/ConnectionsGraph/ConnectionsGraph.tsxapp/_components/FeatureComponents/Profile/Parts/ConnectionsGraph/graph-data.tsapp/_components/FeatureComponents/Profile/Parts/LinksTab.tsxapp/_components/FeatureComponents/Profile/Parts/UserPreferencesTab.tsxapp/_components/GlobalComponents/Auth/LoginForm.tsxapp/_components/GlobalComponents/Dropdowns/Dropdown.tsxapp/_components/GlobalComponents/Modals/Modal.tsxapp/_components/GlobalComponents/Sidebar/SidebarWrapper.tsxapp/_consts/dnd.tsapp/_hooks/dnd/DndProvider.tsxapp/_hooks/dnd/index.tsxapp/_hooks/dnd/useDragItem.tsapp/_hooks/dnd/useDropList.tsapp/_hooks/kanban/useCalendarView.tsapp/_hooks/kanban/useKanban.tsapp/_hooks/kanban/useKanbanDnd.tsapp/_hooks/kanban/useKanbanItem.tsxapp/_hooks/useChecklist.tsxapp/_hooks/useNoteEditor.tsxapp/_providers/NavigationGuardProvider.tsxapp/_schemas/user-schemas.tsapp/_server/actions/auth/index.tsapp/_server/actions/checklist-item/crud.tsapp/_server/actions/checklist-item/drop.tsapp/_server/actions/checklist-item/index.tsapp/_server/actions/checklist-item/status.tsapp/_server/actions/file/index.tsapp/_server/actions/note/parsers.tsapp/_server/actions/session/index.tsapp/_server/actions/users/crud.tsapp/_styles/globals.cssapp/_translations/de.jsonapp/_translations/en.jsonapp/_translations/es.jsonapp/_translations/fr.jsonapp/_translations/it.jsonapp/_translations/klingon.jsonapp/_translations/ko.jsonapp/_translations/nl.jsonapp/_translations/pirate.jsonapp/_translations/pl.jsonapp/_translations/pt.jsonapp/_translations/ru.jsonapp/_translations/tr.jsonapp/_translations/zh.jsonapp/_types/checklist.tsapp/_types/dnd.tsapp/_types/index.tsapp/_types/user.tsapp/_utils/checklist-utils.tsapp/_utils/client-parser-utils.tsapp/_utils/dnd/auto-scroll.tsapp/_utils/dnd/dnd-math.tsapp/_utils/dnd/drag-store.tsapp/_utils/dnd/rect-registry.tsapp/_utils/item-status-utils.tsapp/_utils/kanban/board-utils.tsapp/_utils/kanban/calendar-utils.tsapp/_utils/kanban/index.tsxapp/api/checklists/[listId]/items/[itemIndex]/route.tsapp/api/checklists/[listId]/items/reorder/route.tsapp/api/checklists/route.tsapp/api/docs/route.tsapp/api/search/route.tsnext.config.mjspackage.jsonpatches/body-size-limit_20260427.jspublic/api/paths/checklists.yamlpublic/api/paths/search.yamltests/server-actions/auth.test.tstests/server-actions/drop-item.test.tstests/utils/board-utils.test.tstests/utils/client-parser-utils.test.tstests/utils/connections-graph-data.test.tstests/utils/dnd-math.test.tstests/utils/item-status-utils.test.ts
💤 Files with no reviewable changes (1)
- patches/body-size-limit_20260427.js
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/_utils/kanban/board-utils.ts (1)
13-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the first-column fallback consistent with
getColumnItems().
getColumnItems()maps unset or unknown statuses into the lowest-ordered status, butapplyDrop()still compares againstTaskStatus.TODO. On boards whose first status is notTODO, dragging an unset legacy card within that first column will incorrectly persist a status change and can triggercompleteParent()side effects.Proposed fix
const _renumber = (items: Item[]): Item[] => items.map((item, idx) => ({ ...item, order: idx, children: item.children ? _renumber(item.children) : item.children, })); + +const _effectiveStatus = ( + status: string | undefined, + statuses: KanbanStatus[] | undefined, +): string => { + const statusList = statuses || DEFAULT_KANBAN_STATUSES; + const firstStatus = + [...statusList].sort((a, b) => a.order - b.order)[0]?.id || TaskStatus.TODO; + const validIds = new Set(statusList.map((s) => s.id)); + return validIds.has(status || "") ? (status as string) : firstStatus; +}; export const getColumnItems = ( items: Item[], statusId: string, statuses: KanbanStatus[] | undefined, @@ - const isStatusChange = (dragged.status || TaskStatus.TODO) !== targetStatus; + const isStatusChange = + _effectiveStatus(dragged.status, checklist.statuses) !== targetStatus;Also applies to: 64-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_utils/kanban/board-utils.ts` around lines 13 - 31, getColumnItems() treats unknown/unset statuses as the board's lowest-ordered status, but applyDrop() still uses TaskStatus.TODO as the first-column fallback; update applyDrop() to compute the same firstStatus logic as getColumnItems (derive statusList = statuses || DEFAULT_KANBAN_STATUSES, sort by order and take firstStatus = [...statusList].sort((a,b)=>a.order-b.order)[0]?.id || TaskStatus.TODO) and use that firstStatus wherever applyDrop() currently compares against TaskStatus.TODO or decides to map unknown/empty item.status to the first column, so dragging legacy/unset cards in a non-TODO first column will persist the correct status and avoid spurious completeParent() side effects.
♻️ Duplicate comments (1)
app/_server/actions/checklist-item/status.ts (1)
51-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate each
timeEntrieselement before persisting it.Lines 51-63 only reject non-arrays. Payloads like
[null]or["x"]still pass, then Lines 91-93 either throw onentry.useror serialize garbage into the markdown file. Please reject any element that is not a non-null object before callingupdateItem.Proposed fix
if (timeEntriesStr) { try { const parsed = JSON.parse(timeEntriesStr); - if (!Array.isArray(parsed)) { - throw new Error("timeEntries must be an array"); + if ( + !Array.isArray(parsed) || + parsed.some( + (entry) => + !entry || typeof entry !== "object" || Array.isArray(entry), + ) + ) { + throw new Error("timeEntries must be an array of objects"); } parsedTimeEntries = parsed; } catch (e) {Also applies to: 88-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_server/actions/checklist-item/status.ts` around lines 51 - 63, The JSON parsing block that sets parsedTimeEntries must validate that every element is a non-null object before accepting the array: after JSON.parse and Array.isArray check, iterate the array and ensure each element is typeof "object" and !== null (and optionally has expected keys like user/start/end if you want stricter checks); if any element fails, log and return the same { success: false, error: "Invalid timeEntries payload" } rather than assigning parsedTimeEntries. Also add the same defensive validation immediately before calling updateItem (the place that reads entry.user) so malformed payloads are rejected even if upstream parsing changed; reference parsedTimeEntries and the updateItem call to locate where to add these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/_components/FeatureComponents/Kanban/CalendarView.tsx`:
- Around line 157-168: The code currently renders every segment as a
keyboard-focusable button using role="button", tabIndex, onClick and onKeyDown
and always calls _openItem, which creates dead tab stops when onItemClick is not
provided; update the segment rendering so the button semantics and handlers are
only applied when onItemClick exists (i.e., conditionally add role="button",
tabIndex, onClick={() => _openItem(...)} and the onKeyDown handler, and keep the
non-interactive markup—a span/div with the same styling but without
cursor-pointer or focusable attributes—when onItemClick is undefined), ensuring
you reference the existing _openItem and onItemClick symbols and preserve visual
styles for the non-interactive case.
---
Outside diff comments:
In `@app/_utils/kanban/board-utils.ts`:
- Around line 13-31: getColumnItems() treats unknown/unset statuses as the
board's lowest-ordered status, but applyDrop() still uses TaskStatus.TODO as the
first-column fallback; update applyDrop() to compute the same firstStatus logic
as getColumnItems (derive statusList = statuses || DEFAULT_KANBAN_STATUSES, sort
by order and take firstStatus =
[...statusList].sort((a,b)=>a.order-b.order)[0]?.id || TaskStatus.TODO) and use
that firstStatus wherever applyDrop() currently compares against TaskStatus.TODO
or decides to map unknown/empty item.status to the first column, so dragging
legacy/unset cards in a non-TODO first column will persist the correct status
and avoid spurious completeParent() side effects.
---
Duplicate comments:
In `@app/_server/actions/checklist-item/status.ts`:
- Around line 51-63: The JSON parsing block that sets parsedTimeEntries must
validate that every element is a non-null object before accepting the array:
after JSON.parse and Array.isArray check, iterate the array and ensure each
element is typeof "object" and !== null (and optionally has expected keys like
user/start/end if you want stricter checks); if any element fails, log and
return the same { success: false, error: "Invalid timeEntries payload" } rather
than assigning parsedTimeEntries. Also add the same defensive validation
immediately before calling updateItem (the place that reads entry.user) so
malformed payloads are rejected even if upstream parsing changed; reference
parsedTimeEntries and the updateItem call to locate where to add these checks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b31e4e4-077a-4658-b9b5-3b741e192eb5
📒 Files selected for processing (29)
app/_components/FeatureComponents/Kanban/CalendarView.tsxapp/_components/FeatureComponents/Kanban/KanbanCardDetail.tsxapp/_components/FeatureComponents/Notes/Parts/TipTap/CustomExtensions/ExcalidrawExtension.tsxapp/_components/FeatureComponents/Profile/Parts/ConnectionsGraph/graph-data.tsapp/_components/GlobalComponents/Auth/LoginForm.tsxapp/_hooks/kanban/useKanbanDnd.tsapp/_providers/NavigationGuardProvider.tsxapp/_server/actions/checklist-item/drop.tsapp/_server/actions/checklist-item/status.tsapp/_server/actions/file/index.tsapp/_translations/de.jsonapp/_translations/es.jsonapp/_translations/fr.jsonapp/_translations/it.jsonapp/_translations/klingon.jsonapp/_translations/ko.jsonapp/_translations/nl.jsonapp/_translations/pirate.jsonapp/_translations/pl.jsonapp/_translations/pt.jsonapp/_translations/ru.jsonapp/_translations/tr.jsonapp/_translations/zh.jsonapp/_utils/dnd/rect-registry.tsapp/_utils/item-status-utils.tsapp/_utils/kanban/board-utils.tsapp/api/checklists/[listId]/items/[itemIndex]/route.tsapp/api/checklists/[listId]/items/reorder/route.tspublic/api/paths/checklists.yaml
✅ Files skipped from review due to trivial changes (9)
- app/_translations/tr.json
- app/_translations/pt.json
- app/_translations/pirate.json
- app/_translations/pl.json
- app/_translations/ko.json
- app/_translations/de.json
- app/_translations/es.json
- app/_translations/it.json
- app/_translations/fr.json
🚧 Files skipped from review as they are similar to previous changes (15)
- app/_providers/NavigationGuardProvider.tsx
- app/_server/actions/file/index.ts
- app/_components/FeatureComponents/Notes/Parts/TipTap/CustomExtensions/ExcalidrawExtension.tsx
- app/_server/actions/checklist-item/drop.ts
- app/_utils/item-status-utils.ts
- app/_components/GlobalComponents/Auth/LoginForm.tsx
- app/_translations/zh.json
- app/_components/FeatureComponents/Kanban/KanbanCardDetail.tsx
- app/api/checklists/[listId]/items/[itemIndex]/route.ts
- app/_components/FeatureComponents/Profile/Parts/ConnectionsGraph/graph-data.ts
- app/_translations/klingon.json
- app/_translations/ru.json
- app/_translations/nl.json
- app/_hooks/kanban/useKanbanDnd.ts
- public/api/paths/checklists.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
121-121: Re-check esbuild 0.28.1 impact fromresolutions
package.jsonpins"resolutions": { "esbuild": "0.28.1" }, andyarn.lockresolvesesbuildto0.28.1only.esbuild0.28.0 includes a breaking change: integrity checks for its fallback binary download path—so validate that installs/builds work in any environment where the fallback download path could be exercised.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 121, package.json currently forces esbuild via "resolutions": { "esbuild": "0.28.1" } which follows a breaking change introduced in 0.28.0 around integrity checks for fallback binary downloads; reproduce and validate installs and builds in environments that may hit the fallback path (offline/behind mirrors, CI runners, and development machines with restricted network) and confirm esbuild binary acquisition succeeds; if failures occur, either (A) change the resolution to a known-compatible upstream version (e.g., bump to a patched release that restores expected behavior) or (B) remove the forced resolution and allow the package manager to select a compatible esbuild, and add CI jobs that run npm/yarn install + build in representative restricted-network scenarios to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@package.json`:
- Line 121: package.json currently forces esbuild via "resolutions": {
"esbuild": "0.28.1" } which follows a breaking change introduced in 0.28.0
around integrity checks for fallback binary downloads; reproduce and validate
installs and builds in environments that may hit the fallback path
(offline/behind mirrors, CI runners, and development machines with restricted
network) and confirm esbuild binary acquisition succeeds; if failures occur,
either (A) change the resolution to a known-compatible upstream version (e.g.,
bump to a patched release that restores expected behavior) or (B) remove the
forced resolution and allow the package manager to select a compatible esbuild,
and add CI jobs that run npm/yarn install + build in representative
restricted-network scenarios to prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78484d16-a5a9-47fd-b41c-f00904283b8a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
package.json
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/api/tasks.test.ts (1)
608-679: ⚡ Quick winAdd missing negative-path tests for the new task-item GET handler.
The new suite covers happy paths and non-task type, but not unauthorized or malformed/out-of-range
itemIndexbranches. Adding those assertions would lock down route behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/api/tasks.test.ts` around lines 608 - 679, The tests currently cover happy paths and non-task types but miss negative branches for authorization and invalid/out-of-range itemIndex; add tests that call GET_TASK_ITEM using createMockRequest and the same params setup but (1) simulate missing/invalid auth (omit auth header or mock auth failure) with mockGetListById resolved to mockTask and assert response.status is 401 and error indicates unauthorized, and (2) add malformed itemIndex cases (e.g., "abc") and out-of-range indices (e.g., "999" or "0.999") with mockGetListById resolved to mockTask and assert response.status is 400 (or the handler's expected error code) and the error message matches the handler's validation output; keep using getResponseJson to inspect body and reuse existing helpers/mocks (mockGetListById, createMockRequest, GET_TASK_ITEM).public/api/paths/tasks.yaml (1)
564-576: ⚡ Quick winDocument the auth failure response for the new task-item GET operation.
This endpoint is authenticated, but the new operation currently omits a
401response in the spec. Adding it keeps client/server contracts aligned.Suggested OpenAPI addition
'400': description: Invalid item index or not a task checklist + '401': + description: Unauthorized '404': description: Task or item not found🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/api/paths/tasks.yaml` around lines 564 - 576, The new GET task-item operation is missing a 401 response; add a '401' response entry to the operation's responses (next to '200', '400', '404') with description "Unauthorized" and include an application/json content block that uses your existing error response schema (e.g. reference '`#/components/schemas/Error`' or 'ErrorResponse'); if no shared error schema exists, return a small object schema with a message string to represent the auth failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/checklists/`[listId]/items/[itemIndex]/route.ts:
- Line 162: The code currently skips appending text when it's an empty string
because it checks `if (text)`; change this to append when `text` is present in
the payload (i.e., not undefined/null) so empty-string updates are sent. Replace
the falsy check around `formData.append("text", text)` with an explicit
existence check such as `if (text !== undefined && text !== null)` (or `if
(typeof text !== "undefined")`) so that `text: ""` is included; this aligns with
the `hasField` logic that treats an empty string as an intended update.
In `@howto/API.md`:
- Around line 396-397: The examples use "todo" but the documented canonical
status set lists "in_progress", "paused", and "completed"; update the examples
so the "status" field uses one of the documented values (e.g., change "status":
"todo" to "status": "in_progress" or "status": "completed"), or alternatively
update the canonical status list to include "todo" if that is intended; ensure
all examples (including the other occurrence noted) consistently match the
chosen canonical set.
---
Nitpick comments:
In `@public/api/paths/tasks.yaml`:
- Around line 564-576: The new GET task-item operation is missing a 401
response; add a '401' response entry to the operation's responses (next to
'200', '400', '404') with description "Unauthorized" and include an
application/json content block that uses your existing error response schema
(e.g. reference '`#/components/schemas/Error`' or 'ErrorResponse'); if no shared
error schema exists, return a small object schema with a message string to
represent the auth failure.
In `@tests/api/tasks.test.ts`:
- Around line 608-679: The tests currently cover happy paths and non-task types
but miss negative branches for authorization and invalid/out-of-range itemIndex;
add tests that call GET_TASK_ITEM using createMockRequest and the same params
setup but (1) simulate missing/invalid auth (omit auth header or mock auth
failure) with mockGetListById resolved to mockTask and assert response.status is
401 and error indicates unauthorized, and (2) add malformed itemIndex cases
(e.g., "abc") and out-of-range indices (e.g., "999" or "0.999") with
mockGetListById resolved to mockTask and assert response.status is 400 (or the
handler's expected error code) and the error message matches the handler's
validation output; keep using getResponseJson to inspect body and reuse existing
helpers/mocks (mockGetListById, createMockRequest, GET_TASK_ITEM).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 855ff8b3-26d3-4da9-bcbc-5e38bf79e8ea
📒 Files selected for processing (12)
app/_utils/api-item.tsapp/api/checklists/[listId]/items/[itemIndex]/route.tsapp/api/checklists/route.tsapp/api/tasks/[taskId]/items/[itemIndex]/route.tsapp/api/tasks/[taskId]/route.tsapp/api/tasks/route.tshowto/API.mdpublic/api/components/schemas.yamlpublic/api/paths/checklists.yamlpublic/api/paths/tasks.yamltests/api/items.test.tstests/api/tasks.test.ts
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
4 participants
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.
Changelog
features
bugfixes
@bidirectional linking [BUG]: References to other notes not working in Rich-text editor #52910.x[BUG]: Mermaid Sankey not supported anymore? #528new endpoints
A community member decided to make an android client for Jotty.
It's very much under development and needs polishing but the author seems quite dedicated and I kept an eye on development.
They needed some endpoints for it to work better, so what better moment than this to give them a shoutout!
Check it out here if you are interested, I am not involved in the development, I can't vouch for it, but for what I've seen so far the author (@Darknetzz) is pretty dedicated! Link here: https://github.com/Darknetzz/jotty-android
New endpoints curl exmaples
Summary by CodeRabbit