surface ctx.getHistory() -> { size, stepCount }#206
Open
ianmacartney wants to merge 7 commits intomainfrom
Open
surface ctx.getHistory() -> { size, stepCount }#206ianmacartney wants to merge 7 commits intomainfrom
ianmacartney wants to merge 7 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds runtime history introspection: StepExecutor now tracks incremental Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow as Workflow Handler
participant Ctx as WorkflowCtx
participant Channel as BaseChannel
participant Executor as StepExecutor
Workflow->>Ctx: ctx.meta.getHistory()
Ctx->>Executor: getHistory()
Executor-->>Ctx: { size, stepCount }
Ctx-->>Workflow: { size, stepCount }
Workflow->>Ctx: runMutation / runAction
Ctx->>Channel: send StepRequest
Channel->>Executor: deliver JournalEntry(s)
Executor->>Executor: increment journalSize & stepCount (per entry)
Executor-->>Ctx: updated history available via getHistory()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
This was referenced Feb 19, 2026
Merged
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
commit: |
Closed
ianmacartney
force-pushed
the
ian/resume
branch
2 times, most recently
from
acee365 to
b7fdb37
Compare
ianmacartney
force-pushed
the
ian/get-history
branch
from
9aab249 to
eed620f
Compare
ianmacartney
force-pushed
the
ian/get-history
branch
from
eed620f to
df08ea2
Compare
ianmacartney
force-pushed
the
graphite-base/206
branch
from
b7fdb37 to
28c53f9
Compare
ianmacartney
force-pushed
the
ian/get-history
branch
from
df08ea2 to
3a5d0a0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/step.ts (1)
226-235:⚠️ Potential issue | 🟠 MajorDouble-counting issue for inline-completed steps.
When
startStepsreturns entries that complete inline (i.e.,entry.step.runResultis set), the counters are incremented here at lines 227-228. However, the caller at lines 82-85 then callscompleteMessagefor each of these entries, which increments the counters again at lines 119-120. This results in double-counting bothstepCountandjournalSizefor inline-completed steps.🐛 Proposed fix: skip counting in startSteps for inline-completed entries
const entries = (await this.ctx.runMutation( this.component.journal.startSteps, { workflowId: this.workflowId, generationNumber: this.generationNumber, steps, workpoolOptions: this.workpoolOptions, }, )) as JournalEntry[]; for (const entry of entries) { - this.stepCount++; - this.journalSize += getConvexSize(entry); + // Only count entries that won't go through completeMessage + // (i.e., entries that are still in progress) + if (entry.step.inProgress) { + this.stepCount++; + this.journalSize += getConvexSize(entry); + } if (this.journalSize > MAX_JOURNAL_SIZE) { throw new Error( journalSizeError(this.journalSize, this.workflowId) + ` The failing step was ${entry.step.name} (${entry._id})`, ); } }Alternatively, you could move the size check before the conditional increment, but you'd need to compute the size regardless of whether you're tracking it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/step.ts` around lines 226 - 235, The loop in startSteps that increments this.stepCount and this.journalSize double-counts entries that completed inline because completeMessage also increments those counters; update the loop in startSteps to skip the counter increments for entries where entry.step.runResult is already set (i.e., treat inline-completed entries as already-counted) so only non-inline-started entries update this.stepCount/this.journalSize, leaving completeMessage unchanged.
🧹 Nitpick comments (1)
README.md (1)
777-791: Consider using consistent naming for the workflow context.Throughout this README, the workflow handler uses
stepas the context parameter name (e.g., line 50:handler: async (step, args)), but this tip example usesctx. For consistency with the rest of the documentation, consider renamingctxtostep:📝 Suggested diff for consistency
while (true) { - const result = await ctx.runAction( + const result = await step.runAction( internal.example.pollForResult, { id }, { runAfter: 1000 }, // Poll every second ); if (result !== null) { return result; } - const { stepCount, size } = ctx.getHistory(); + const { stepCount, size } = step.getHistory(); if (stepCount > 100 || size > 2_000_000) { return null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 777 - 791, Rename the workflow context variable from ctx to step in this example to match the rest of the README: change the parameter name and all usages so internal.example.pollForResult is invoked via step.runAction(...) and the history check uses step.getHistory(); ensure you update the loop variables and any references (result, id, stepCount, size) to use the renamed context symbol consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/client/step.ts`:
- Around line 226-235: The loop in startSteps that increments this.stepCount and
this.journalSize double-counts entries that completed inline because
completeMessage also increments those counters; update the loop in startSteps to
skip the counter increments for entries where entry.step.runResult is already
set (i.e., treat inline-completed entries as already-counted) so only
non-inline-started entries update this.stepCount/this.journalSize, leaving
completeMessage unchanged.
---
Nitpick comments:
In `@README.md`:
- Around line 777-791: Rename the workflow context variable from ctx to step in
this example to match the rest of the README: change the parameter name and all
usages so internal.example.pollForResult is invoked via step.runAction(...) and
the history check uses step.getHistory(); ensure you update the loop variables
and any references (result, id, stepCount, size) to use the renamed context
symbol consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34df3b59-5cdc-4b9f-96c7-ee761b1cb340
⛔ Files ignored due to path filters (1)
example/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (6)
README.mdexample/convex/historyExample.tsexample/convex/nestedWorkflow.tssrc/client/step.tssrc/client/workflowContext.tssrc/client/workflowMutation.ts
ianmacartney
force-pushed
the
ian/get-history
branch
from
3a5d0a0 to
68ab18d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/step.ts (1)
98-106: ImprovegetHistory()JSDoc to document the stepCount/size asymmetry.The current JSDoc for
getHistory()inworkflowContext.ts(lines 130-132) doesn't explain whystepCountincludes pending steps in the buffer whilesizereflects only finished steps. The in-code comment (lines 100-102) documents this well, but it should also be reflected in the public API documentation:/** * Get the current history of the workflow execution. * * `@returns` The size in bytes and number of steps processed so far. * Note: size reflects only finished steps, while stepCount includes pending steps in the buffer. */ getHistory: () => { size: number; stepCount: number };Alternatively, consider making both metrics consistent (both include or exclude pending steps) to avoid surprises for callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/step.ts` around lines 98 - 106, Update the public JSDoc for getHistory in workflowContext.ts to document the asymmetry between size and stepCount: state that size reflects only finished steps (journalSize) while stepCount includes pending buffered steps (this.receiver.bufferSize added to this.stepCount); modify the comment above getHistory() to match the in-code note in src/client/step.ts and clearly describe which metric includes pending steps so callers aren't surprised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/workflowContext.ts`:
- Around line 128-132: The JSDoc for the getHistory accessor is missing the
opening /** delimiter which breaks parsing; update the comment above getHistory
(the JSDoc block that currently starts with "*" before getHistory) to start with
"/**" so it becomes a valid JSDoc comment for the getHistory: () => { size:
number; stepCount: number } declaration.
---
Nitpick comments:
In `@src/client/step.ts`:
- Around line 98-106: Update the public JSDoc for getHistory in
workflowContext.ts to document the asymmetry between size and stepCount: state
that size reflects only finished steps (journalSize) while stepCount includes
pending buffered steps (this.receiver.bufferSize added to this.stepCount);
modify the comment above getHistory() to match the in-code note in
src/client/step.ts and clearly describe which metric includes pending steps so
callers aren't surprised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2180424-47b7-448d-a2ad-4870022b0753
⛔ Files ignored due to path filters (1)
example/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (6)
README.mdexample/convex/historyExample.tsexample/convex/nestedWorkflow.tssrc/client/step.tssrc/client/workflowContext.tssrc/client/workflowMutation.ts
✅ Files skipped from review due to trivial changes (1)
- example/convex/nestedWorkflow.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- example/convex/historyExample.ts
ianmacartney
force-pushed
the
ian/get-history
branch
2 times, most recently
from
0372be7 to
2f7161a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
example/convex/historyExample.ts (1)
9-28: Optional: reduce repeated history snapshot logging boilerplate.The repeated
getHistory()+console.logpattern can be factored into a tiny helper to keep the example tighter.♻️ Suggested cleanup
export const historyWorkflow = workflow.define({ args: {}, handler: async (ctx) => { - const h0 = ctx.getHistory(); - console.log(`Before any steps: ${h0.stepCount} steps, ${h0.size} bytes`); + const logHistory = (label: string) => { + const h = ctx.getHistory(); + console.log(`${label}: ${h.stepCount} steps, ${h.size} bytes`); + return h; + }; + const h0 = logHistory("Before any steps"); @@ - const h1 = ctx.getHistory(); - console.log(`After step 1: ${h1.stepCount} steps, ${h1.size} bytes`); + const h1 = logHistory("After step 1"); @@ - const h2 = ctx.getHistory(); - console.log(`After step 2: ${h2.stepCount} steps, ${h2.size} bytes`); + const h2 = logHistory("After step 2"); @@ - const h3 = ctx.getHistory(); - console.log(`After step 3: ${h3.stepCount} steps, ${h3.size} bytes`); + const h3 = logHistory("After step 3");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/historyExample.ts` around lines 9 - 28, Factor the repeated ctx.getHistory() + console.log boilerplate into a small helper function (e.g., logHistory or printHistory) and call it before/after each step; locate occurrences around getHistory() and the runMutation/runAction calls (references: ctx.getHistory(), ctx.runMutation(internal.historyExample.smallStep), ctx.runAction(internal.historyExample.mediumStep), variables h0,h1,h2,h3) and replace the four manual snapshots with calls to the helper that accepts a label like "Before any steps" / "After step 1" etc., returning or discarding the History object as needed.src/client/stepContext.test.ts (1)
100-103: Refactor repeated history stub + add one direct passthrough assertion.Using a shared stub will reduce duplication, and adding a focused assertion on
ctx.getHistory()will explicitly cover the new API wiring.♻️ Suggested test cleanup and coverage bump
+const zeroHistory = () => ({ size: 0, stepCount: 0 }); + describe("StepExecutor + WorkflowCtx integration", () => { + it("exposes getHistory from injected callback", () => { + const channel = new BaseChannel<StepRequest>(0); + const ctx = createWorkflowCtx("wf-history" as any, channel, () => ({ + size: 12, + stepCount: 3, + })); + expect(ctx.getHistory()).toEqual({ size: 12, stepCount: 3 }); + }); + it("resolves a successful step", async () => { const channel = new BaseChannel<StepRequest>(0); - const ctx = createWorkflowCtx("wf-1" as any, channel, () => ({ - size: 0, - stepCount: 0, - })); + const ctx = createWorkflowCtx("wf-1" as any, channel, zeroHistory);(Apply
zeroHistoryreplacement to the remaining test cases as well.)Also applies to: 120-123, 141-144, 162-165, 198-201, 238-241, 275-278, 313-316, 339-342, 359-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/stepContext.test.ts` around lines 100 - 103, Introduce a shared history stub (e.g., zeroHistory) and use it when constructing the test contexts instead of repeating inline { size: 0, stepCount: 0 } in createWorkflowCtx calls; update the instantiation at the shown diff (createWorkflowCtx("wf-1", channel, () => ({ size: 0, stepCount: 0 }))) to pass the shared zeroHistory object via the factory, then add a direct assertion that ctx.getHistory() === zeroHistory to verify passthrough wiring; apply the same replacement to the other repeated occurrences (lines referenced in the review) so all tests use the shared stub and include the explicit ctx.getHistory() equality check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@example/convex/historyExample.ts`:
- Around line 9-28: Factor the repeated ctx.getHistory() + console.log
boilerplate into a small helper function (e.g., logHistory or printHistory) and
call it before/after each step; locate occurrences around getHistory() and the
runMutation/runAction calls (references: ctx.getHistory(),
ctx.runMutation(internal.historyExample.smallStep),
ctx.runAction(internal.historyExample.mediumStep), variables h0,h1,h2,h3) and
replace the four manual snapshots with calls to the helper that accepts a label
like "Before any steps" / "After step 1" etc., returning or discarding the
History object as needed.
In `@src/client/stepContext.test.ts`:
- Around line 100-103: Introduce a shared history stub (e.g., zeroHistory) and
use it when constructing the test contexts instead of repeating inline { size:
0, stepCount: 0 } in createWorkflowCtx calls; update the instantiation at the
shown diff (createWorkflowCtx("wf-1", channel, () => ({ size: 0, stepCount: 0
}))) to pass the shared zeroHistory object via the factory, then add a direct
assertion that ctx.getHistory() === zeroHistory to verify passthrough wiring;
apply the same replacement to the other repeated occurrences (lines referenced
in the review) so all tests use the shared stub and include the explicit
ctx.getHistory() equality check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6a67fdc-7ec2-4f20-9ba0-7152941ed071
⛔ Files ignored due to path filters (1)
example/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (9)
.github/workflows/test.ymlREADME.mdexample/convex/README.mdexample/convex/historyExample.tsexample/convex/nestedWorkflow.tssrc/client/step.tssrc/client/stepContext.test.tssrc/client/workflowContext.tssrc/client/workflowMutation.ts
✅ Files skipped from review due to trivial changes (3)
- .github/workflows/test.yml
- example/convex/README.md
- example/convex/nestedWorkflow.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/client/workflowMutation.ts
- src/client/workflowContext.ts
- README.md
- src/client/step.ts
ianmacartney
force-pushed
the
ian/get-history
branch
from
2f7161a to
2674a10
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/convex/historyExample.ts`:
- Around line 9-28: The calls to ctx.getHistory() are using the wrong API;
replace each ctx.getHistory() with ctx.meta.getHistory() so the history is
accessed from the meta object. Update the four occurrences after the initial h0,
h1, h2, and h3 captures (surrounding the ctx.runMutation calls that use
internal.historyExample.smallStep and the ctx.runAction call that uses
internal.historyExample.mediumStep) to call ctx.meta.getHistory() instead,
leaving the surrounding runMutation/runAction invocations and log statements
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06f0a18e-6c5d-4586-b1bb-4c77a637b598
⛔ Files ignored due to path filters (1)
example/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (9)
.github/workflows/test.ymlREADME.mdexample/convex/README.mdexample/convex/historyExample.tsexample/convex/nestedWorkflow.tssrc/client/step.tssrc/client/stepContext.test.tssrc/client/workflowContext.tssrc/client/workflowMutation.ts
✅ Files skipped from review due to trivial changes (4)
- .github/workflows/test.yml
- example/convex/README.md
- example/convex/nestedWorkflow.ts
- src/client/stepContext.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- src/client/workflowContext.ts
- src/client/step.ts
ianmacartney
force-pushed
the
ian/get-history
branch
from
2674a10 to
1045ae5
Compare
ianmacartney
force-pushed
the
ian/get-history
branch
from
1045ae5 to
3f18038
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds workflow history tracking with a new
ctx.getHistory()method to monitor workflow execution metrics.Fixes #178
What changed?
StepExecutorto track workflow execution metrics:journalEntrySizewithjournalSizeand addedstepCountto track both total size and number of stepsgetHistory()method that returns these metricsgetHistory()method to theWorkflowCtxtypecreateWorkflowCtxfunction to accept and expose the history getterHow to test?
ctx.getHistory()sizeandstepCountpropertiesWhy make this change?
This change provides developers with visibility into workflow execution metrics, allowing them to monitor the size and complexity of their workflows. This is particularly useful for debugging performance issues and preventing workflows from hitting size limits unexpectedly. The incremental tracking approach is also more efficient than calculating the entire journal size upfront.
Summary by CodeRabbit
New Features
Documentation
Examples
Tests
Chore