-
Notifications
You must be signed in to change notification settings - Fork 3.1k
v0.2.3: fix #592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
v0.2.3: fix #592
Conversation
…g issues (#587) * fix(variable-resolution): don't inject stringified json, use var refs * fix lint * remove unused var --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@vikhyaths-air.lan>
* fix(knowledge-base-selector): should trigger sockets event for persistence * fix subblock value updates for non useSubblockValue components * fix lint --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@Vikhyaths-MacBook-Air.local>
…dge + block (#582) * auto connect race condition * fix lint * Update apps/sim/hooks/use-collaborative-workflow.ts Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * Update apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * fix lint --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@Vikhyaths-Air.attlocal.net> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@Vikhyaths-MacBook-Air.local>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
✅ No security or compliance issues detected. Reviewed everything up to 1a78dd5. Security Overview
Detected Code Changes
Reply to this PR with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR fixes race conditions and synchronization issues in collaborative workflow features, focusing on autoconnect edges and subblock selectors.
- Enhanced validation with new
AutoConnectEdgeSchemainapps/sim/socket-server/validation/schemas.tsfor atomic block+edge operations - Refactored subblock selectors (document, file, knowledge-base, project) to use
useCollaborativeWorkflowhooks instead of direct store mutations - Added
insertAutoConnectEdgehelper inapps/sim/socket-server/database/operations.tsto handle edge creation synchronously with blocks - Implemented atomic block addition with autoconnect edges in
apps/sim/hooks/use-collaborative-workflow.ts - Cleaned up verbose logging in
apps/sim/app/api/function/execute/route.ts
11 files reviewed, 9 comments
Edit PR Review Bot Settings | Greptile
| if (!isPreview) { | ||
| setValue(blockId, subBlock.id, document.id) | ||
| collaborativeSetSubblockValue(blockId, subBlock.id, document.id) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: redundant isPreview check - the parent if statement on line 170 already guards against preview mode
| if (!isPreview) { | |
| setValue(blockId, subBlock.id, document.id) | |
| collaborativeSetSubblockValue(blockId, subBlock.id, document.id) | |
| } | |
| collaborativeSetSubblockValue(blockId, subBlock.id, document.id) |
| collaborativeSetSubblockValue(blockId, subBlockId, newFiles) | ||
| useWorkflowStore.getState().triggerUpdate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider awaiting the collaborative update to ensure synchronization completes before triggering workflow update
| tx: any, | ||
| workflowId: string, | ||
| autoConnectEdge: any, | ||
| logger: any | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: avoid using any types for database transaction, edge data, and logger - specify proper types
| setStoreValue(null) | ||
| useSubBlockStore.getState().setValue(blockId, subBlockId, null) | ||
| collaborativeSetSubblockValue(blockId, subBlockId, null) | ||
| useWorkflowStore.getState().triggerUpdate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Early UI state clear could cause inconsistency if file deletion fails. Consider moving this after successful deletion
| // Handle auto-connect edge if present | ||
| await insertAutoConnectEdge(tx, workflowId, payload.autoConnectEdge, logger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: code duplication with 'add' case - consider extracting error handling for auto-connect edge
| if (!isPreview) { | ||
| setValue(blockId, subBlock.id, knowledgeBase.id) | ||
| // Use collaborative update for both local store and persistence | ||
| collaborativeSetSubblockValue(blockId, subBlock.id, knowledgeBase.id) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: redundant isPreview check since it's already checked at the start of the function
| if (!isPreview) { | |
| setValue(blockId, subBlock.id, knowledgeBase.id) | |
| // Use collaborative update for both local store and persistence | |
| collaborativeSetSubblockValue(blockId, subBlock.id, knowledgeBase.id) | |
| } | |
| // Use collaborative update for both local store and persistence | |
| collaborativeSetSubblockValue(blockId, subBlock.id, knowledgeBase.id) |
| } | ||
|
|
||
| const { type } = event.detail | ||
| console.log('🛠️ Adding block from toolbar:', type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: consider using an established logger instead of console.log
| console.log('🛠️ Adding block from toolbar:', type) | |
| logger.info('Adding block from toolbar:', { type }) |
| type: 'workflowEdge', | ||
| }) | ||
| } | ||
| console.log('✅ Auto-connect edge created:', autoConnectEdge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: remove debug log in production
| if (subBlock.id === 'teamId') { | ||
| setValue(blockId, 'teamId', projectId) | ||
| setValue(blockId, 'projectId', '') | ||
| collaborativeSetSubblockValue(blockId, 'teamId', projectId) | ||
| collaborativeSetSubblockValue(blockId, 'projectId', '') | ||
| } else if (subBlock.id === 'projectId') { | ||
| setValue(blockId, 'projectId', projectId) | ||
| collaborativeSetSubblockValue(blockId, 'projectId', projectId) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider extracting Linear-specific project/team ID handling into a separate function for better maintainability
* fix(variable resolution): use variable references to not have escaping issues (#587) * fix(variable-resolution): don't inject stringified json, use var refs * fix lint * remove unused var --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@vikhyaths-air.lan> * fix(subblock updates): special selectors persistence (#591) * fix(knowledge-base-selector): should trigger sockets event for persistence * fix subblock value updates for non useSubblockValue components * fix lint --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@Vikhyaths-MacBook-Air.local> * fix(race-cond): for auto-connect rare race condition between adding edge + block (#582) * auto connect race condition * fix lint * Update apps/sim/hooks/use-collaborative-workflow.ts Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * Update apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * fix lint --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@Vikhyaths-Air.attlocal.net> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@Vikhyaths-MacBook-Air.local> --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@vikhyaths-air.lan> Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@Vikhyaths-MacBook-Air.local> Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@Vikhyaths-Air.attlocal.net> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
* fix(variable resolution): use variable references to not have escaping issues (simstudioai#587) * fix(variable-resolution): don't inject stringified json, use var refs * fix lint * remove unused var --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@vikhyaths-air.lan> * fix(subblock updates): special selectors persistence (simstudioai#591) * fix(knowledge-base-selector): should trigger sockets event for persistence * fix subblock value updates for non useSubblockValue components * fix lint --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@Vikhyaths-MacBook-Air.local> * fix(race-cond): for auto-connect rare race condition between adding edge + block (simstudioai#582) * auto connect race condition * fix lint * Update apps/sim/hooks/use-collaborative-workflow.ts Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * Update apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * fix lint --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@Vikhyaths-Air.attlocal.net> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@Vikhyaths-MacBook-Air.local> --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@vikhyaths-air.lan> Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@Vikhyaths-MacBook-Air.local> Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@Vikhyaths-Air.attlocal.net> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Description
Type of change
Checklist:
bun run test)Security Considerations: