Skip to content

Conversation

@devin-ai-integration
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Nov 24, 2025

Summary

Fixes a bug where bulkRecordHook would not complete commits when records were modified and trackChanges was enabled. This caused the commit:completed event to never fire, blocking workflows that depend on commit completion.

Root Cause: The plugin only called completeCommit() in three cases:

  • No records to process
  • No modified records after processing
  • On error

But in the "happy path" where records ARE modified, it called event.update(modifiedRecords) without subsequently calling completeCommit(), leaving commits incomplete indefinitely.

The Fix: Added await completeCommit(event, debug) after successfully updating modified records (line 133).

Please explain how to summarize this PR for the Changelog:

Fixed bug where commits were never completed when bulkRecordHook modified records with trackChanges enabled, preventing commit:completed events from firing.

Tell code reviewer how and what to test:

Automated Tests:

  • All existing tests pass (34 tests in record-hook plugin)
  • Lint checks pass

Manual Testing Needed:

  1. Create a workbook with settings.trackChanges = true
  2. Register a bulkRecordHook that modifies at least one field
  3. Trigger a commit (e.g., insert/update records)
  4. Verify:
    • The commit is marked as completed via GET /v1/commits?sheetId=...
    • The commit:completed event is emitted
    • Custom actions or workflows gated on commit completion are unblocked

Key Review Points:

  • ⚠️ Multiple hooks: If multiple bulkRecordHooks are registered on the same sheet, each will call completeCommit(). The Platform backend guards against duplicate completions, but this may generate "Commit already completed" log messages.
  • ⚠️ No new test coverage: This fix doesn't include a new test specifically for the "modified records + trackChanges" scenario. Consider adding one.
  • ⚠️ Customer validation needed: The original issue mentioned "bulkRecordHooks running over each other" which could also involve frontend issues (multiple listener registrations). This fix addresses the plugin bug but may not fully resolve the customer's issue if they have frontend problems.

Related

…hanges is enabled

When bulkRecordHook modifies records and calls event.update(), it was not
calling completeCommit() afterward. This caused commits to never be marked
as completed when trackChanges was enabled, preventing commit:completed
events from firing.

This fix adds a call to completeCommit() after successfully updating
modified records, ensuring that commits are properly completed and
commit:completed events are emitted.

Fixes issue where bulkRecordHooks running over each other prevented
final commit:completed event from firing.

Co-Authored-By: [email protected] <[email protected]>
@devin-ai-integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@promptless
Copy link

promptless bot commented Nov 24, 2025

📝 Documentation updates detected!

Updated existing suggestion: Add changelog entry for record-hook commit:completed fix

@devin-ai-integration
Copy link
Contributor Author

Closing due to inactivity for more than 7 days. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant