refactor: migrate server actions to ActionResult<T> (#102)#112
refactor: migrate server actions to ActionResult<T> (#102)#112ryota-murakami merged 3 commits intomainfrom
Conversation
…on (#102) Convert client-consumed server actions from throw-based to ActionResult<T> pattern: - board.ts: createBoard, updateBoardPositions - project-info.ts: all 6 wrappers (getProjectInfo, upsertProjectInfo, etc.) - maintenance-project-info.ts: all 6 wrappers - repo-cards.ts: deleteRepoCard type annotation Update all callers to check result.success instead of try/catch.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughMigrates many server actions to an ActionResult discriminated union, updates callers to check result.success and use result.data/error, removes some direct Sentry captures (and adds centralized Sentry in new wrappers), and adjusts tests and UI flows to the new response envelope and navigation/toast behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Component (Create/Board/Kanban)
participant Action as Server Action (withAuthResult)
participant DB
participant Sentry
rect rgba(92,123,255,0.5)
User->>UI: click create/update/comment action
UI->>Action: call action(...) (await)
end
rect rgba(16,185,129,0.5)
Action->>DB: perform DB ops / validation
DB-->>Action: success / failure
end
alt DB success
Action-->>UI: { success: true, data: ... }
UI->>UI: apply success flow (navigate/toast/update state)
else DB failure
Action->>Sentry: capture exception / context
Action-->>UI: { success: false, error: "..." }
UI->>UI: revert optimistic update / show error toast
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #112 +/- ##
==========================================
+ Coverage 73.29% 73.43% +0.13%
==========================================
Files 137 137
Lines 4116 4115 -1
Branches 1065 1073 +8
==========================================
+ Hits 3017 3022 +5
+ Misses 1078 1072 -6
Partials 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/actions/maintenance-project-info.ts`:
- Around line 54-72: The wrapper functions like getMaintenanceProjectInfo are
duplicating Sentry reporting because getProjectInfoCore (in
shared-project-info.ts) already calls Sentry.captureException; to centralize
reporting at the ActionResult/wrapper layer (preferred), remove
Sentry.captureException calls from the core helpers (e.g., getProjectInfoCore
and the other core functions referenced by the six wrapper functions in this
file) so the core just throws errors, and keep the existing
Sentry.captureException and ActionResult handling in the wrappers
(getMaintenanceProjectInfo and the other five functions) to ensure a single
capture point.
🧹 Nitpick comments (6)
src/lib/actions/board.ts (1)
568-590:updateBoardPositions— looks good.One minor observation: Line 583 maps the original
updatesarray (pre-validation) into the RPC call rather thanvalidation.data. SincesafeParsecan strip/transform values, usingvalidation.datawould be more defensive.Proposed fix
- return withAuthResult(async (supabase) => { - // Atomic batch update via PostgreSQL RPC — all updates succeed or all fail - // Note: RLS policies on board table still enforce user ownership - const { error } = await supabase.rpc('batch_update_board_positions', { - p_updates: updates.map(({ id, position }) => ({ id, position })), - }) + return withAuthResult(async (supabase) => { + const validatedUpdates = validation.data + // Atomic batch update via PostgreSQL RPC — all updates succeed or all fail + // Note: RLS policies on board table still enforce user ownership + const { error } = await supabase.rpc('batch_update_board_positions', { + p_updates: validatedUpdates.map(({ id, position }) => ({ id, position })), + })src/app/boards/new/CreateBoardForm.tsx (1)
40-51: Variableresultshadows the outerresultfrom line 31.Both the Zod
safeParseresult (line 31) and thecreateBoardreturn (line 40) are namedresult. This compiles fine since the outer one isn't needed insidestartTransition, but it's a readability trip hazard. Consider renaming one (e.g.,createResultorboardResult).src/hooks/board/useNoteModal.ts (1)
119-130:saveignores theActionResultfromupsertProjectInfo.
upsertProjectInfonow returnsActionResult<void>, but the result is discarded. If the save fails, the user gets no feedback. Consider checkingresult.successand surfacing an error (e.g., via toast).Proposed fix
const save = useCallback( async (note: string, links: ProjectLink[]) => { if (!cardId) return - await upsertProjectInfo(cardId, { + const result = await upsertProjectInfo(cardId, { note, comment: '', links, }) + if (!result.success) { + // Surface error to user — consider toast or callback + console.error('Failed to save project info:', result.error) + } }, [cardId], )src/components/Board/KanbanBoard.tsx (2)
299-302: Empty failure branch — optimistic update is never rolled back and user gets no feedback.The old code had
try/catchwithSentry.captureException. Now the server-side Sentry capture still happens (inside the wrapper), but the client silently keeps stale optimistic state on failure. The// Could implement rollback here if neededcomment acknowledges the gap.For consistency with
BoardGrid.handleDragEnd(which reverts state + shows a toast on failure), consider at least notifying the user:Proposed fix
const result = await updateComment(cardId, newComment) if (!result.success) { - // Could implement rollback here if needed + // Rollback optimistic update + setComments((prev) => ({ + ...prev, + [cardId]: { + comment: prev[cardId]?.comment ?? '', + color: prev[cardId]?.color ?? 'primary', + }, + })) + toast.error('Failed to update comment') }
325-325:handleCommentColorChangeandhandleCommentDeletesilently discard theActionResult.Both apply optimistic updates but never check the result, so failures are invisible to the user. This is inconsistent with
handleCommentChange(which at least capturesresult) and withBoardGrid(which reverts + toasts).Proposed fix for handleCommentColorChange
// Persist to database - await updateCommentColor(cardId, color) + const result = await updateCommentColor(cardId, color) + if (!result.success) { + toast.error('Failed to update comment color') + }Proposed fix for handleCommentDelete
// Persist to database - await deleteComment(cardId) + const result = await deleteComment(cardId) + if (!result.success) { + toast.error('Failed to delete comment') + }Also applies to: 347-347
src/lib/actions/maintenance-project-info.ts (1)
54-237: Consider a generic helper to eliminate the repeated try/catch boilerplate.All 6 functions follow an identical pattern:
try { await coreFunction(...); return { success, data } } catch { Sentry; return { success: false, error } }. A small utility could DRY this up:async function wrapAction<T>( fn: () => Promise<T>, context: string, extra?: Record<string, unknown>, ): Promise<ActionResult<T>> { try { const data = await fn() return { success: true, data } } catch (error) { // Only if you keep Sentry here (see other comment) Sentry.captureException(error, { extra: { context, ...extra } }) return { success: false, error: error instanceof Error ? error.message : `Failed: ${context}`, } } }Then each wrapper becomes a one-liner. Not blocking — the current code is clear and correct.
- Remove Sentry.captureException from shared-project-info.ts core functions - Centralize error reporting at ActionResult wrapper layer only - Fix callers to unwrap ActionResult before using data - Make getCommentsCore throw on error (consistent with other core functions)
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
🧪 E2E Coverage Report (Sharded: 12 parallel jobs)
📊 Full report available in workflow artifacts |
Apply ActionResult<T> unwrapping to extracted hooks instead of inline code. getCommentsForMaintenanceItems and getMaintenanceProjectInfo now return ActionResult — callers check .success before accessing .data.
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
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 (2)
src/app/maintenance/hooks/useMaintenanceNoteModal.ts (1)
82-97:⚠️ Potential issue | 🟠 MajorCheck
upsertMaintenanceProjectInforesult before closing the modal — save failures are silently swallowed.
upsertMaintenanceProjectInforeturnsActionResult<void>, but line 86 discards the result. The modal closes and state resets unconditionally (lines 91–94), leaving the user with no feedback if the save failed. This is inconsistent with howgetMaintenanceProjectInfois handled inopenNoteModal(which checksresult.success).Suggested fix
async (note: string, links: ProjectLink[]) => { if (!selectedRepoForNote) return const currentComment = comments[selectedRepoForNote.id]?.comment || '' - await upsertMaintenanceProjectInfo(selectedRepoForNote.id, { + const result = await upsertMaintenanceProjectInfo(selectedRepoForNote.id, { note, comment: currentComment, links, }) + if (!result.success) { + // surface the error (toast, console, etc.) instead of silently closing + console.error(result.error) + return + } setNoteModalOpen(false) setSelectedRepoForNote(null) setCurrentNote('') setCurrentLinks([]) },src/lib/actions/shared-project-info.ts (1)
38-50:⚠️ Potential issue | 🟡 MinorRemove stale JSDoc reference to Sentry for the
labelfield.The
labelparameter is not used for Sentry error context (which is not present in this module). Currently, thelabelfield is configuration-only and unused throughout the core functions. Update the JSDoc to clarify its actual purpose, or remove it if it's truly unused:Suggested update
/** * Foreign key configuration for project info operations * * `@param` column - The FK column name in the projectinfo table - * `@param` label - Human-readable label for Sentry error context + * `@param` label - Human-readable label for context and loggingIf
labelis not intended to be used, consider removing the field entirely to avoid future confusion.
🤖 Fix all issues with AI agents
In `@src/app/maintenance/hooks/useMaintenanceComments.ts`:
- Around line 47-49: The fetch in useMaintenanceComments silently ignores
failures; update the getCommentsForMaintenanceItems.then callback to handle the
!result.success case by logging the error and notifying the user: when
result.success is false call console.error(result.error) (or a logger) and
trigger a user-visible notification (e.g., a toast or set an error state)
instead of doing nothing; keep the current setComments(result.data) path
unchanged for success so the logic around setComments remains intact.
🧹 Nitpick comments (1)
src/app/maintenance/hooks/useMaintenanceComments.ts (1)
63-81: Optimistic updates are never rolled back on server failure.
updateMaintenanceComment,updateMaintenanceCommentColor, anddeleteMaintenanceCommentnow returnActionResult, but the result is discarded in all three handlers (lines 75, 102, 115). If the server call fails, the UI keeps the optimistic state with no rollback or user notification.Consider checking the result and reverting state on failure, e.g.:
Sketch for handleCommentSave
async (repoId: string, newComment: string, options: CommentSaveOptions) => { + const prev = comments setComments((prev) => ({ ...prev, [repoId]: { comment: newComment, color: prev[repoId]?.color ?? 'neutral', }, })) - await updateMaintenanceComment(repoId, newComment) + const result = await updateMaintenanceComment(repoId, newComment) + if (!result.success) { + setComments(prev) + // optionally toast result.error + } if (options.closeOnSave) { setEditingCommentId(null) } },Same pattern applies to
handleColorChange(line 102) andhandleCommentDelete(line 115).
ryota-murakami
deleted the
refactor/issue-102-action-result-migration
branch
2 participants
Summary
ActionResult<T>discriminated union patternboard.ts: ConvertcreateBoard,updateBoardPositionsto usewithAuthResult<T>project-info.ts/maintenance-project-info.ts: All 12 thin wrappers now returnActionResult<T>with try/catch around Core callsrepo-cards.ts:deleteRepoCardtyped asActionResult<void>CreateBoardForm,BoardGrid,KanbanBoard,useNoteModal,board-data,MaintenanceClient) to checkresult.successinstead of try/catchTest plan
pnpm typecheckpassespnpm lintpassespnpm testpasses (1203 tests, including updateduseNoteModal.test.ts)pnpm e2ein CI (12 shards)Closes #102
Summary by CodeRabbit
Bug Fixes
Refactor