chore: update .gitignore and improve error handling in workflow compo… #54
Conversation
…nents - Added tsconfig.tsbuildinfo/ to .gitignore to exclude TypeScript build info files. - Commented out unused route handler in userRoutes for clarity. - Enhanced error handling in WorkflowCanvas component to catch and set errors during node updates. - Updated ConfigModal to use Redux for user ID retrieval instead of direct action reference. - Cleaned up formatting in PlaceholderNode component for consistency.
📝 WalkthroughWalkthroughThis pull request introduces several updates across configuration and application code: adds TypeScript build info to gitignore, disables the workflow update route, refactors Redux state access in a modal component, applies formatting cleanup, and adds error handling with position tracking to workflow node changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Pull request overview
This pull request performs maintenance tasks including TypeScript configuration cleanup, error handling improvements, Redux integration fixes, and code formatting standardization.
Changes:
- Added TypeScript build info exclusion to .gitignore
- Wrapped node position update logic in try-catch block for better error handling
- Replaced direct action reference with Redux selector hook for user ID retrieval
- Cleaned up formatting and commented out unused route handler
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| .gitignore | Added tsconfig.tsbuildinfo/ to exclude TypeScript build artifacts |
| apps/http-backend/src/routes/userRoutes/userRoutes.ts | Commented out empty route handler for code clarity |
| apps/web/app/workflows/[id]/page.tsx | Added try-catch error handling to handleNodesChange function and minor formatting fix |
| apps/web/app/workflows/[id]/components/ConfigModal.tsx | Replaced incorrect userAction usage with useAppSelector hook for Redux state access |
| apps/web/app/workflows/[id]/components/nodes/PlaceholderNode.tsx | Standardized code formatting (quotes, spacing) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { getNodeConfig } from "@/app/lib/nodeConfigs"; | ||
| import { useState } from "react"; | ||
| import { HOOKS_URL } from "@repo/common/zod"; | ||
| import { userAction } from "@/store/slices/userSlice"; |
There was a problem hiding this comment.
The import of userAction from "@/store/slices/userSlice" is no longer used after switching to useAppSelector. This unused import should be removed to keep the code clean.
| import { userAction } from "@/store/slices/userSlice"; |
| if (!isOpen || !selectedNode) return null; | ||
| const userId =userAction.setUserId as unknown as string; | ||
| const userId = useAppSelector((state) => state.user.userId) as unknown as string; | ||
| console.log("we are getting this userId from ConfigModal" , userId) |
There was a problem hiding this comment.
Inconsistent spacing around the comma in the console.log statement. There should be a space after the comma for standard formatting consistency.
| console.log("we are getting this userId from ConfigModal" , userId) | |
| console.log("we are getting this userId from ConfigModal", userId); |
| api.triggers.update({ | ||
| TriggerId: change.id, | ||
| Config: { | ||
| ...(typeof changedNode.data.config === "object" && | ||
| changedNode.data.config !== null | ||
| ? changedNode.data.config | ||
| : {}), | ||
| position: change.position, | ||
| }, | ||
| }); | ||
| } else { | ||
| // Otherwise, update in node table | ||
| api.nodes.update({ | ||
| NodeId: change.id, | ||
| position: change.position, | ||
| }, | ||
| }); | ||
| } else { | ||
| // Otherwise, update in node table | ||
| api.nodes.update({ | ||
| NodeId: change.id, | ||
| position: change.position, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The API calls inside handleNodesChange are not awaited. These are asynchronous operations that may fail, but without await, the try-catch block won't properly catch errors from these API calls. The errors would become unhandled promise rejections. Either add await to these API calls or handle the promises properly with .catch().
| }); | ||
| }); | ||
| } catch (error: any) { | ||
| setError(error); |
There was a problem hiding this comment.
The error variable is typed as string (line 49) but is being set to an error object. This will cause type mismatches. Either change the error state type to Error | null or convert the error to a string using error.message.
| setError(error); | |
| setError(error instanceof Error ? error.message : String(error)); |
| nodes.find((n) => n.id === nodeId)?.data.nodeType === "trigger"; | ||
| if (isTrigger) { | ||
| await api.triggers.update({ TriggerId: nodeId, Config: config }); | ||
| await api.triggers.update({ TriggerId: nodeId, Config: config}); |
There was a problem hiding this comment.
Inconsistent spacing before the closing parenthesis. There should be a space before the closing brace for consistency with line 435 and standard formatting conventions.
| await api.triggers.update({ TriggerId: nodeId, Config: config}); | |
| await api.triggers.update({ TriggerId: nodeId, Config: config }); |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/app/workflows/[id]/page.tsx (2)
49-49: Typo in initial state and type mismatch with setError calls.The initial value
"'"appears to be a typo (should be""). Additionally,setError(error)calls throughout the file pass Error objects, but the state expects astring. Extract the error message instead.Proposed fix
- const [error, setError] = useState<string>("'"); + const [error, setError] = useState<string>("");And update all
setError(error)calls to:setError(error?.message ?? "An error occurred");
428-436: Type mismatch:onSavecallback signature doesn't matchConfigModalProps.The
ConfigModalPropsinterface definesonSave: (config: any, userId: string) => Promise<void>, but this implementation treats the first parameter asnodeIdand second asconfig. TheConfigModalcomponent callsonSave({ HOOKS_URL }, userId)(line 30), which passes{ HOOKS_URL }as the first argument. This object will be received asnodeIdin the callback, causing line 431 to fail when attempting to accessnodeId.data.nodeType.
🤖 Fix all issues with AI agents
In @.gitignore:
- Line 8: The .gitignore entry uses a directory-style pattern
"tsconfig.tsbuildinfo/" but tsconfig.tsbuildinfo is a file; replace it with a
file/glob pattern such as "**/tsconfig*.tsbuildinfo" or "tsconfig.tsbuildinfo"
(no trailing slash) to match build-info files, and remove the redundant
duplicate entries for tsconfig.tsbuildinfo found elsewhere in the file so only
the consolidated glob remains.
In `@apps/web/app/workflows/`[id]/components/ConfigModal.tsx:
- Around line 23-25: The hook call useAppSelector must be moved before the
conditional early return so hooks run unconditionally: call const userId =
useAppSelector(...) (and any other hooks) at the top of the ConfigModal
component (before checking isOpen or selectedNode) and then keep the if (!isOpen
|| !selectedNode) return null; afterwards; also remove the unused userAction
import referenced on line 6 to avoid dead code.
In `@apps/web/app/workflows/`[id]/page.tsx:
- Around line 61-90: The loop over changes uses forEach and calls async
functions api.triggers.update and api.nodes.update without awaiting them, so the
surrounding try/catch cannot catch rejections; replace the forEach with an
async-aware loop (e.g., for...of) or collect promises and await Promise.all, and
ensure each call to api.triggers.update or api.nodes.update is awaited so errors
propagate to the try/catch and setError(error) will be invoked; locate this
logic around the variables changes, nodes, changedNode and update the block that
currently calls api.triggers.update(...) and api.nodes.update(...) to await the
returned promises.
🧹 Nitpick comments (1)
apps/web/app/workflows/[id]/components/ConfigModal.tsx (1)
24-24: Avoid double type assertion (as unknown as string).This pattern bypasses TypeScript's type safety. If
state.user.userIdisn't typed asstring, consider fixing the Redux state type definition in your user slice, or handle the case where userId might be undefined/null.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignoreapps/http-backend/src/routes/userRoutes/userRoutes.tsapps/web/app/workflows/[id]/components/ConfigModal.tsxapps/web/app/workflows/[id]/components/nodes/PlaceholderNode.tsxapps/web/app/workflows/[id]/page.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/app/workflows/[id]/page.tsx (1)
apps/web/app/lib/api.ts (1)
api(7-71)
apps/web/app/workflows/[id]/components/ConfigModal.tsx (1)
apps/web/app/hooks/redux.ts (1)
useAppSelector(7-7)
🪛 Biome (2.1.2)
apps/web/app/workflows/[id]/components/ConfigModal.tsx
[error] 24-24: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (3)
apps/http-backend/src/routes/userRoutes/userRoutes.ts (1)
437-439: LGTM!Commenting out the unimplemented route handler is fine for cleanup. Consider removing the commented code entirely if there are no plans to implement it soon.
apps/web/app/workflows/[id]/components/nodes/PlaceholderNode.tsx (1)
1-18: LGTM!Formatting adjustments only; no functional changes.
apps/web/app/workflows/[id]/page.tsx (1)
433-433: Minor formatting change acknowledged.The spacing adjustment is fine. The larger concern about the
onSavecallback signature mismatch is addressed in the previous comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| .pnp.js | ||
| /.history | ||
|
|
||
| tsconfig.tsbuildinfo/ |
There was a problem hiding this comment.
Incorrect pattern: tsconfig.tsbuildinfo is a file, not a directory.
The trailing slash causes git to only match directories named tsconfig.tsbuildinfo/. TypeScript build info files are files, not directories. Consider using a glob pattern to ignore all such files and consolidate the duplicate entries below (lines 43, 50, 51, 52).
Suggested fix
-tsconfig.tsbuildinfo/
+*.tsbuildinfoIf you use the glob pattern, you can also remove the redundant specific entries at lines 43, 50, 51, and 52.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tsconfig.tsbuildinfo/ | |
| *.tsbuildinfo |
🤖 Prompt for AI Agents
In @.gitignore at line 8, The .gitignore entry uses a directory-style pattern
"tsconfig.tsbuildinfo/" but tsconfig.tsbuildinfo is a file; replace it with a
file/glob pattern such as "**/tsconfig*.tsbuildinfo" or "tsconfig.tsbuildinfo"
(no trailing slash) to match build-info files, and remove the redundant
duplicate entries for tsconfig.tsbuildinfo found elsewhere in the file so only
the consolidated glob remains.
| if (!isOpen || !selectedNode) return null; | ||
| const userId =userAction.setUserId as unknown as string; | ||
| const userId = useAppSelector((state) => state.user.userId) as unknown as string; | ||
| console.log("we are getting this userId from ConfigModal" , userId) |
There was a problem hiding this comment.
Critical: Hook called after conditional return violates Rules of Hooks.
useAppSelector on line 24 is called after the early return on line 23. React hooks must be called unconditionally and in the same order on every render. This will cause runtime errors or unpredictable behavior.
Move the hook call before the early return, and consider removing the unused userAction import on line 6.
Proposed fix
export default function ConfigModal({
isOpen,
selectedNode,
onClose,
onSave,
}: ConfigModalProps) {
const [loading, setLoading] = useState(false);
+ const userId = useAppSelector((state) => state.user.userId) as string;
if (!isOpen || !selectedNode) return null;
- const userId = useAppSelector((state) => state.user.userId) as unknown as string;
console.log("we are getting this userId from ConfigModal" , userId)Also remove the unused import:
-import { userAction } from "@/store/slices/userSlice";🧰 Tools
🪛 Biome (2.1.2)
[error] 24-24: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In `@apps/web/app/workflows/`[id]/components/ConfigModal.tsx around lines 23 - 25,
The hook call useAppSelector must be moved before the conditional early return
so hooks run unconditionally: call const userId = useAppSelector(...) (and any
other hooks) at the top of the ConfigModal component (before checking isOpen or
selectedNode) and then keep the if (!isOpen || !selectedNode) return null;
afterwards; also remove the unused userAction import referenced on line 6 to
avoid dead code.
| try { | ||
| changes.forEach((change) => { | ||
| if (change.type === "position" && change.position) { | ||
| // Find the node in local state to check its type | ||
| const changedNode = nodes.find((n) => n.id === change.id); | ||
|
|
||
| if (changedNode?.data?.nodeType === "trigger") { | ||
| // If it's a trigger node, update via trigger API | ||
| api.triggers.update({ | ||
| TriggerId: change.id, | ||
| Config: { | ||
| ...(typeof changedNode.data.config === "object" && | ||
| changedNode.data.config !== null | ||
| ? changedNode.data.config | ||
| : {}), | ||
| position: change.position, | ||
| }, | ||
| }); | ||
| } else { | ||
| // Otherwise, update in node table | ||
| api.nodes.update({ | ||
| NodeId: change.id, | ||
| position: change.position, | ||
| }, | ||
| }); | ||
| } else { | ||
| // Otherwise, update in node table | ||
| api.nodes.update({ | ||
| NodeId: change.id, | ||
| position: change.position, | ||
| }); | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
| } catch (error: any) { | ||
| setError(error); | ||
| } |
There was a problem hiding this comment.
Async API calls not awaited—try/catch won't catch errors.
The api.triggers.update and api.nodes.update calls return Promises but are not awaited. The synchronous try/catch block won't catch any errors from these async operations. Additionally, forEach doesn't handle async callbacks properly.
Proposed fix using for...of with await
const handleNodesChange = (changes: NodeChange[]) => {
onNodesChange(changes); // Update UI first
- try {
- changes.forEach((change) => {
+ changes.forEach(async (change) => {
+ try {
if (change.type === "position" && change.position) {
// Find the node in local state to check its type
const changedNode = nodes.find((n) => n.id === change.id);
if (changedNode?.data?.nodeType === "trigger") {
// If it's a trigger node, update via trigger API
- api.triggers.update({
+ await api.triggers.update({
TriggerId: change.id,
Config: {
...(typeof changedNode.data.config === "object" &&
changedNode.data.config !== null
? changedNode.data.config
: {}),
position: change.position,
},
});
} else {
// Otherwise, update in node table
- api.nodes.update({
+ await api.nodes.update({
NodeId: change.id,
position: change.position,
});
}
}
- });
- } catch (error: any) {
- setError(error);
- }
+ } catch (error: any) {
+ setError(error.message ?? "Failed to update node position");
+ }
+ });
};🤖 Prompt for AI Agents
In `@apps/web/app/workflows/`[id]/page.tsx around lines 61 - 90, The loop over
changes uses forEach and calls async functions api.triggers.update and
api.nodes.update without awaiting them, so the surrounding try/catch cannot
catch rejections; replace the forEach with an async-aware loop (e.g., for...of)
or collect promises and await Promise.all, and ensure each call to
api.triggers.update or api.nodes.update is awaited so errors propagate to the
try/catch and setError(error) will be invoked; locate this logic around the
variables changes, nodes, changedNode and update the block that currently calls
api.triggers.update(...) and api.nodes.update(...) to await the returned
promises.
…nents
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.