-
Notifications
You must be signed in to change notification settings - Fork 110
H-5892: Don't pass full persisted entities into/out of Flow steps, pass ids instead #8259
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
base: main
Are you sure you want to change the base?
Conversation
PR SummaryShifts flow IO and logging to use
Written by Cursor Bugbot for commit d4243f4. This will update automatically on new commits. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🤖 Augment PR SummarySummary: This PR reduces Temporal workflow/event history size by stopping Flow steps from passing full persisted entity payloads around, and passing only entity IDs (plus minimal metadata) instead. Changes:
Technical Notes: The UI now uses UUID-based filters with current-time temporal axes to load the latest entity versions, which trades exact “at-the-time” entity snapshots for smaller workflow payloads and improved Temporal scalability. 🤖 Was this summary useful? React with 👍 or 👎 |
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.
apps/hash-frontend/src/pages/@/[shortname]/shared/flow-visualizer/activity-log.tsx
Show resolved
Hide resolved
| const operation = metadataByEntityId.get(entity.entityId); | ||
|
|
||
| if (!operation) { | ||
| throw new Error(`Operation not found for entity ${entity.entityId}`); |
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.
Throwing here can crash the Flow Visualizer UI in transient states (e.g., when Apollo serves previousData that doesn’t correspond to the current persistedEntitiesMetadata, or if some entities fail to load). Consider making this case non-fatal so the UI can still render partial results.
🤖 Was this useful? React with 👍 or 👎
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8259 +/- ##
==========================================
- Coverage 59.26% 59.25% -0.02%
==========================================
Files 1191 1191
Lines 113436 113389 -47
Branches 4982 4977 -5
==========================================
- Hits 67232 67187 -45
+ Misses 45428 45426 -2
Partials 776 776
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ? new HashEntity(output.value.entity) | ||
| : undefined, | ||
| }; | ||
| persistedEntitiesByLocalId[unresolvedEntity.localEntityId] = output.value; |
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.
Property name mismatch when spreading into FailedEntityProposal
Low Severity
When handling a non-OK status from persistEntityAction, the code spreads output.value (a PersistedEntityMetadata) into the FailedEntityProposal object. However, PersistedEntityMetadata has property entityId while FailedEntityProposal expects existingEntityId. The spread adds an entityId property that doesn't match the expected type, and existingEntityId remains undefined.
| recordedAt: new Date().toISOString(), | ||
| stepId: Context.current().info.activityId, | ||
| type: "PersistedEntity", | ||
| type: "PersistedEntityMetadata", |
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.
Action output type mismatch in getFileFromUrl implementation
High Severity
The action definition for getFileFromUrl was updated to expect payloadKind: "PersistedEntityMetadata" for the fileEntity output, but the implementation still returns kind: "Entity" with the full serialized entity. Consumers of this action expecting PersistedEntityMetadata (with entityId and operation fields) will receive an incompatible full entity object instead.
🌟 What is the purpose of this PR?
We're currently creating a large event history in Temporal by passing entities created/updated in the Graph during flows in and out of action steps. This means we hit Temporal limits quicker, both for total event history and for getting workflow data out of the Temporal API.
This PR improves that by switching from passing full persisted entity details around, to instead just passing the entity id, and resolving the full entity where needed.
Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
The slight drawback of this approach is that we no longer have a direct record of the exact state of the entity as persisted by the Flow step, as the entity may have been updated again by the time it is resolved for display or other work.
But we can always inspect the history of the event at the time the Flow step was conducted, and any provenance information remains attached to specific editions (e.g. which Flow id and step produced that edition).
🛡 What tests cover this?