Skip to content

fix(flows): clear prompt-bar build seed after dispatch so reopening the copilot can't re-save the flow#4628

Merged
senamakel merged 11 commits into
tinyhumansai:mainfrom
YellowSnnowmann:fix/copilot-build-seed-remount-4597
Jul 9, 2026
Merged

fix(flows): clear prompt-bar build seed after dispatch so reopening the copilot can't re-save the flow#4628
senamakel merged 11 commits into
tinyhumansai:mainfrom
YellowSnnowmann:fix/copilot-build-seed-remount-4597

Conversation

@YellowSnnowmann

@YellowSnnowmann YellowSnnowmann commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fixes Reopening the copilot re-sends the prompt-bar build seed and overwrites the flow #4597: closing and reopening the flow-canvas copilot re-sent the prompt-bar mode:'build' turn, which could re-run the builder and overwrite the just-saved flow.
  • WorkflowCopilotPanel now reports the build seed consumed via a new optional onBuildSeedConsumed callback, fired once right after the build turn is dispatched.
  • FlowCanvasPage strips copilotBuild from the ephemeral route state on consumption (preserving other state fields, e.g. a repair seed), so a remount has no seed left to re-fire.
  • The in-mount buildSentRef guard is retained as defense-in-depth.

Problem

WorkflowCopilotPanel's buildSentRef guard is per-mount, but the route seed location.state.copilotBuild persists for the lifetime of the route. Closing and reopening the copilot fully remounts the panel, resets the flag to false, and re-dispatches the same build turn against the still-present seed. Build mode is persist-capable, so this is a real overwrite hazard for the saved flow. Flagged P2 by chatgpt-codex-connector on #4578; related to #4596.

Solution

Lift an onBuildSeedConsumed callback up to FlowCanvasPage (the component that owns useLocation/useNavigate), threaded page → FlowEditorWorkflowCopilotPanel. The panel calls it once, right after dispatching the build turn. The page clears the seed with navigate(location.pathname, { replace: true, state: next }), where next is a shallow copy of location.state with only copilotBuild removed. After clearing, the buildSeed memo recomputes to null, so the panel's build effect early-returns on remount and cannot re-fire. No module-level singleton introduced.

Submission Checklist

  • Tests added or updated (happy path + failure/edge case) — panel reports consumption exactly once; remount after the seed is cleared sends no second build turn; host strips copilotBuild from route state on consumption.
  • Diff coverage ≥ 80% — changed lines covered (only a defensive early-return guard on clearBuildSeed is uncovered).
  • Coverage matrix updated — N/A: behaviour-only bugfix, no feature row change.
  • All affected feature IDs listed under ## RelatedN/A: behaviour-only bugfix.
  • No new external network dependencies introduced.
  • Manual smoke checklist updated — N/A: does not touch release-cut surfaces.
  • Linked issue closed via Closes #4597.

Impact

Desktop/web flows canvas only. Prevents an accidental overwrite of a saved flow when the copilot is closed and reopened after a prompt-bar instant-create. No performance, security, or migration implications.

Related


AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: fix/copilot-build-seed-remount-4597

Summary by CodeRabbit

  • Bug Fixes

    • Improved copilot auto-start build behavior so the build-seed is marked consumed only after the dispatch reports dispatched.
    • Correctly handles skipped and failure outcomes to avoid duplicate or repeated build actions, including after reopen/remount.
    • Propagates build-seed consumption up to clear only the temporary copilotBuild/build-seed state while preserving other location state.
  • New Features

    • Added an optional onBuildSeedConsumed callback to support host-controlled build-seed clearing.
  • Tests

    • Expanded coverage for dispatch result semantics (dispatched/skipped/failed), callback timing, and remount non-retriggering.

…he copilot can't re-save the flow

The copilot's per-mount `buildSentRef` guard resets on remount while the route
seed `location.state.copilotBuild` persists, so closing and reopening the
copilot re-fired the same `mode:'build'` turn and could re-run the builder and
overwrite the saved flow. The panel now reports the seed consumed via a new
`onBuildSeedConsumed` callback, and FlowCanvasPage strips `copilotBuild` from
the route state (preserving other fields) once dispatched, so a remount has no
seed left to re-fire. The in-mount `buildSentRef` guard is retained as
defense-in-depth.

Closes tinyhumansai#4597
@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

WorkflowCopilotPanel now reports seeded build dispatch outcomes, FlowCanvasPage clears copilotBuild after consumption, and locale translation entries are reformatted without changing their text.

Changes

Build seed consumption fix

Layer / File(s) Summary
Send outcome and panel consumption
app/src/hooks/useWorkflowBuilderChat.ts, app/src/components/flows/WorkflowCopilotPanel.tsx, app/src/components/flows/WorkflowCopilotPanel.test.tsx
send() now returns structured outcomes for dispatched, skipped, and failed turns; WorkflowCopilotPanel calls onBuildSeedConsumed only for dispatched build seeds and resets its guard on skipped sends; tests cover dispatch timing, skipped sends, failures, remounts, and clarifying-question follow-ups.
Route state clearing and wiring
app/src/pages/FlowCanvasPage.tsx, app/src/pages/__tests__/FlowCanvasPage.test.tsx
FlowEditor accepts onBuildSeedConsumed, FlowCanvasPage removes copilotBuild from location.state with a replace navigation when consumption is reported, and the page test verifies buildSeed becomes null afterward.

Locale string formatting

Layer / File(s) Summary
Wrapped translation entries
app/src/lib/i18n/de.ts, app/src/lib/i18n/es.ts, app/src/lib/i18n/fr.ts, app/src/lib/i18n/it.ts, app/src/lib/i18n/pl.ts, app/src/lib/i18n/pt.ts, app/src/lib/i18n/ru.ts
Locale entries are reformatted across several translation files without changing the underlying message text.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • tinyhumansai/openhuman#4578: Sets up the same copilot build-seed flow and structured send semantics that this PR extends with consumption handling.
  • tinyhumansai/openhuman#4630: Touches the same WorkflowCopilotPanel/useWorkflowBuilderChat send-result path and related follow-up behavior.

Suggested labels: bug

Poem

A seed was sent, then cleared away,
The bunny flow no longer repeats its play.
One hop, one turn, no doubled track,
Then route state blinks and never looks back.
🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The reformat-only edits in multiple i18n files are unrelated to the build-seed fix. Move the translation formatting-only changes into a separate PR or revert them from this fix.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title is concise and accurately summarizes the main flow-canvas build-seed fix.
Linked Issues check ✅ Passed The route seed is cleared after dispatch and remount behavior is covered by tests, matching #4597.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

Comment @coderabbitai help to get the list of available commands.

@YellowSnnowmann YellowSnnowmann marked this pull request as ready for review July 7, 2026 12:10
@YellowSnnowmann YellowSnnowmann requested a review from a team July 7, 2026 12:10
@coderabbitai coderabbitai Bot added the bug label Jul 7, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cdfbcdfa5c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread app/src/components/flows/WorkflowCopilotPanel.tsx Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
app/src/pages/__tests__/FlowCanvasPage.test.tsx (1)

331-356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage that clearBuildSeed preserves unrelated route-state fields.

This test only exercises a location.state containing copilotBuild. The PR objective explicitly calls out preserving other fields (e.g. a copilotRepair seed) when stripping copilotBuild — that specific behavior (the { ...state } spread minus copilotBuild) isn't asserted anywhere, so a regression that clears the whole state object instead of just the one key wouldn't be caught.

✅ Suggested additional assertion
   it('clears the build seed from route state once the copilot reports it consumed (`#4597`)', async () => {
     render(
       <MemoryRouter
         initialEntries={[
-          { pathname: '/flows/test-id', state: { copilotBuild: { description: 'digest it' } } },
+          {
+            pathname: '/flows/test-id',
+            state: {
+              copilotBuild: { description: 'digest it' },
+              copilotRepair: { runId: 'run-1' },
+            },
+          },
         ]}>

Then, after triggering onBuildSeedConsumed, assert the sibling field survives (e.g. via a route-state spy or by re-navigating to a repair-seeded route and checking it still resolves).

Based on PR objectives: "The fix should either clear the route seed after the first send or move the consumed-state tracking above the panel so it survives remounts," with preservation of other state fields as an explicit requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/src/pages/__tests__/FlowCanvasPage.test.tsx` around lines 331 - 356, Add
a regression assertion in FlowCanvasPage.test.tsx around the existing
onBuildSeedConsumed flow to verify clearBuildSeed removes only copilotBuild and
preserves sibling location.state fields such as copilotRepair. Update the test
setup for FlowCanvasPage and copilotPanelProps so the route state includes an
extra unrelated seed, then after invoking onBuildSeedConsumed confirm the
remaining field is still present instead of the entire state being cleared. This
should specifically exercise the clearBuildSeed behavior that spreads the
existing state and deletes only the consumed key.
app/src/pages/FlowCanvasPage.tsx (2)

575-575: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Local state shadows the outer [state, setState] (LoadState).

const state = location.state; inside clearBuildSeed shadows the component-level state (the LoadState union driving loading/notFound/error/ready rendering). Both are legitimately named state for good reason, but the shadowing in a file where state is central to the render logic is an easy source of confusion for future edits.

♻️ Proposed rename
-  const clearBuildSeed = useCallback(() => {
-    const state = location.state;
-    if (!state || typeof state !== 'object' || !('copilotBuild' in state)) return;
-    const next = { ...(state as Record<string, unknown>) };
+  const clearBuildSeed = useCallback(() => {
+    const routeState = location.state;
+    if (!routeState || typeof routeState !== 'object' || !('copilotBuild' in routeState)) return;
+    const next = { ...(routeState as Record<string, unknown>) };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/src/pages/FlowCanvasPage.tsx` at line 575, The local `state` variable
inside `clearBuildSeed` shadows the component-level `state` from `[state,
setState]`, which makes the render logic harder to follow. Rename the local
`state` to something specific like `locationState` or `navState` and update the
nearby `clearBuildSeed` logic to use that name, keeping the outer `state`
reserved for the `LoadState` rendering flow.

579-579: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Log line omits correlation fields used elsewhere in this file.

Every other log(...) call in this file includes a correlation field (e.g. id=%s, flow id=%s), but log('build seed consumed — clearing route state') doesn't identify which flow/route this happened for, making it harder to trace during debugging of exactly the kind of remount bug this PR fixes.

📝 Proposed fix
-    log('build seed consumed — clearing route state');
+    log('build seed consumed — clearing route state: id=%s', id);

As per coding guidelines: "On new or changed flows, add verbose debug logging with stable prefixes, correlation fields, and no secrets or full PII; incomplete changes are those lacking logging."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/src/pages/FlowCanvasPage.tsx` at line 579, The log statement in
FlowCanvasPage’s route-state cleanup path is missing the same correlation fields
used by the other log calls in this file. Update the `log('build seed consumed —
clearing route state')` call to include the relevant flow/route identifiers
already available in `FlowCanvasPage` (for example the flow id and any route
id/seed context) so the message can be traced to a specific remount. Keep the
existing stable debug prefix and match the surrounding logging style used
elsewhere in `FlowCanvasPage`.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/src/pages/FlowCanvasPage.tsx`:
- Around line 574-581: The clearBuildSeed callback in FlowCanvasPage is dropping
the current query string and fragment by navigating with only location.pathname.
Update the navigate call to preserve the full current location while replacing
state, using the existing location object in clearBuildSeed so search and hash
remain intact when copilotBuild is removed.

---

Nitpick comments:
In `@app/src/pages/__tests__/FlowCanvasPage.test.tsx`:
- Around line 331-356: Add a regression assertion in FlowCanvasPage.test.tsx
around the existing onBuildSeedConsumed flow to verify clearBuildSeed removes
only copilotBuild and preserves sibling location.state fields such as
copilotRepair. Update the test setup for FlowCanvasPage and copilotPanelProps so
the route state includes an extra unrelated seed, then after invoking
onBuildSeedConsumed confirm the remaining field is still present instead of the
entire state being cleared. This should specifically exercise the clearBuildSeed
behavior that spreads the existing state and deletes only the consumed key.

In `@app/src/pages/FlowCanvasPage.tsx`:
- Line 575: The local `state` variable inside `clearBuildSeed` shadows the
component-level `state` from `[state, setState]`, which makes the render logic
harder to follow. Rename the local `state` to something specific like
`locationState` or `navState` and update the nearby `clearBuildSeed` logic to
use that name, keeping the outer `state` reserved for the `LoadState` rendering
flow.
- Line 579: The log statement in FlowCanvasPage’s route-state cleanup path is
missing the same correlation fields used by the other log calls in this file.
Update the `log('build seed consumed — clearing route state')` call to include
the relevant flow/route identifiers already available in `FlowCanvasPage` (for
example the flow id and any route id/seed context) so the message can be traced
to a specific remount. Keep the existing stable debug prefix and match the
surrounding logging style used elsewhere in `FlowCanvasPage`.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e09a5915-35a5-4d33-b083-2ce5644c707a

📥 Commits

Reviewing files that changed from the base of the PR and between 2ce5e60 and cdfbcdf.

📒 Files selected for processing (4)
  • app/src/components/flows/WorkflowCopilotPanel.test.tsx
  • app/src/components/flows/WorkflowCopilotPanel.tsx
  • app/src/pages/FlowCanvasPage.tsx
  • app/src/pages/__tests__/FlowCanvasPage.test.tsx

Comment thread app/src/pages/FlowCanvasPage.tsx Outdated
…ually dispatches

Addresses review feedback on tinyhumansai#4597: the build effect cleared the route seed
unconditionally after calling send(), but send() no-ops when the socket isn't
connected — so an offline open discarded the prompt and the blank flow could
never auto-build. send() now reports whether it dispatched; the effect consumes
the seed only on a real dispatch and releases the in-mount guard on a no-op so
it retries once the socket reconnects (send() changes identity on socketStatus).
Addresses CodeRabbit on tinyhumansai#4628: clearBuildSeed navigated to a bare pathname,
which drops the current search and hash. Navigate with a location object so
they're preserved while only the copilotBuild route state is stripped.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/src/components/flows/WorkflowCopilotPanel.test.tsx`:
- Around line 277-299: The test in WorkflowCopilotPanel.test.tsx is only
re-checking that hookState.send was called, so waitFor resolves immediately and
does not prove the mocked send() promise has settled. Update the test around
WorkflowCopilotPanel and onBuildSeedConsumed to wait on the actual async
completion of send() (for example by awaiting the mocked promise resolution or
another condition that only becomes true after the send path finishes), then
assert that onBuildSeedConsumed was still not called.

In `@app/src/hooks/useWorkflowBuilderChat.ts`:
- Around line 82-88: `send()` currently returns the same `false` for retryable
no-ops and real dispatch failures, which causes the build-seed effect to treat
errors as safe retries and resend the turn repeatedly. Update
`useWorkflowBuilderChat` so `send` distinguishes offline/busy no-ops from actual
errors, and adjust the build-seed effect that uses `buildSentRef` to only clear
that ref for retryable no-op cases. Keep the logic centered around `send` and
the effect that re-arms on `send` identity changes so failed dispatches do not
trigger duplicate sends.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9bc4cf1a-17e4-44d0-b106-4d3078eea63f

📥 Commits

Reviewing files that changed from the base of the PR and between cdfbcdf and b5a25d4.

📒 Files selected for processing (3)
  • app/src/components/flows/WorkflowCopilotPanel.test.tsx
  • app/src/components/flows/WorkflowCopilotPanel.tsx
  • app/src/hooks/useWorkflowBuilderChat.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/src/components/flows/WorkflowCopilotPanel.tsx

Comment thread app/src/components/flows/WorkflowCopilotPanel.test.tsx
Comment thread app/src/hooks/useWorkflowBuilderChat.ts Outdated
…der send()

Addresses CodeRabbit on tinyhumansai#4628: send() returned `false` for both retryable
no-ops (offline/busy) and real dispatch errors, so the build-seed effect would
re-arm on a thrown failure and resend the turn repeatedly. send() now resolves a
WorkflowBuilderSendOutcome ('dispatched' | 'skipped' | 'failed'); the effect
consumes the seed on 'dispatched', retries only on 'skipped', and leaves the
guard set on 'failed' so a dispatch error can't loop into duplicate turns. Also
flush the mocked send() promise in the no-op test so it truly awaits resolution,
and add a 'failed' test asserting no consume and no resend.
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 7, 2026
Reconcile the two changes to useWorkflowBuilderChat.send():
- This branch (tinyhumansai#4628) returned an outcome (dispatched/skipped/failed) so a
  seeded auto-send retries only on skipped, never on a real dispatch error.
- upstream tinyhumansai#4630 returned { proposed } so a looping caller knows whether the
  turn is still unresolved (clarifying-question carry-forward).

These are orthogonal signals, so send() now returns
WorkflowBuilderSendResult { outcome, proposed } and both callers read the
field they need. Kept upstream's THREAD_NOT_FOUND cleanup in the catch path.
Merged both sides' panel tests.
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 8, 2026

@sanil-23 sanil-23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the build-seed consumption fix — the logic is sound and the edge cases (offline retry via send identity change, StrictMode double-invoke, search/hash preservation, the failed dispatch-error loop) are handled thoughtfully. One test-coverage gap worth addressing before merge, plus a few nits carried over from earlier review rounds. Nothing blocking.

Nice touches: buildSentRef set synchronously before the async send; navigating with a location object to preserve search/hash; clearBuildSeed is idempotent and doesn't remount FlowEditor (keyed by flow.id) so the copilot thread survives the state strip; treating a backend-returned {error} as dispatched (the turn was sent) is correct.

Comment thread app/src/hooks/useWorkflowBuilderChat.ts Outdated
);
setThreadId(null);
}
return 'failed';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major (test coverage): The new send() outcome contract is the heart of this fix, but it isn't asserted at its source — useWorkflowBuilderChat.test.ts wasn't updated and its harness hardcodes the socket as connected (selectSocketStatus: () => 'connected').

So there's no test proving send() returns 'failed' (not 'skipped') when buildWorkflow throws — which was the entire point of commit c0349785 (a failed misreported as skipped re-arms the effect and loops duplicate turns). The panel tests only verify the panel's reaction to a mocked hook, so both sides of the contract are mocked and a regression here would pass all tests. (Diff-coverage passes because these return lines execute; coverage measures execution, not assertion.)

Suggest adding to useWorkflowBuilderChat.test.ts:

it("returns 'failed' (not 'skipped') when the dispatch throws", async () => {
  buildWorkflow.mockRejectedValueOnce(new Error('rpc boom'));
  const { result } = renderHook(() => useWorkflowBuilderChat());
  let outcome: string | undefined;
  await act(async () => { outcome = await result.current.send({ /* … */ }); });
  expect(outcome).toBe('failed');
  expect(result.current.error).toBe('rpc boom');
});

Plus a 'dispatched'-on-success case and a 'skipped' case with a non-connected socket (the socket mock needs to be made configurable).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — added in 6af3bd5d3. useWorkflowBuilderChat.test.ts now has a send() outcome contract block asserting the contract at its source, not through the mocked hook:

  • dispatched on a successful run
  • failed (not skipped) when buildWorkflow rejects — and error is surfaced
  • skipped with no dispatch when the socket is not connected

To exercise the disconnected path the socket-status mock is now configurable (hoisted socketStatus.current, reset to 'connected' in beforeEach). This closes the gap where both sides of the contract were mocked and a failedskipped regression could pass every test.

}
// `failed`: the dispatch was attempted but errored (surfaced via
// `error`). Leave the guard set so we don't auto-resend and duplicate the
// turn; the user can retry manually.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: The comment says on failed the guard stays set "so we don't auto-resend … the user can retry manually." That holds within the mount, but since the route seed is deliberately not cleared on failure, closing + reopening the copilot remounts the panel (buildSentReffalse) and the still-present buildSeed will auto-fire again — i.e. a reopen is an automatic retry, which contradicts "manual retry only."

Not a data-loss hazard (a failed turn persisted nothing), so likely fine/desirable — but worth confirming it's intended rather than an oversight.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed intended — and you're right the comment was imprecise. Clarified in 6af3bd5d3: the guard is per-mount (it only prevents an auto-resend within a mount). A deliberate close + reopen is a fresh mount and will re-fire the build — that is an intentional retry, not an oversight. It's safe because a failed mode:'build' turn persists nothing (#4596), so there is no partial state to reconcile. No logic change, comment only.

Comment thread app/src/pages/FlowCanvasPage.tsx Outdated
// Preserve any other state fields (e.g. a repair seed) — only drop
// `copilotBuild`.
const clearBuildSeed = useCallback(() => {
const state = location.state;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: const state = location.state shadows the component-level state from [state, setState] (LoadState), which drives the whole render switch in this file. Harmless, but rename the local (e.g. routeState) for clarity.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6af3bd5d3 — renamed the local to routeState (with a short comment noting it avoids shadowing the component-level LoadState state).

Comment thread app/src/pages/FlowCanvasPage.tsx Outdated
if (!state || typeof state !== 'object' || !('copilotBuild' in state)) return;
const next = { ...(state as Record<string, unknown>) };
delete next.copilotBuild;
log('build seed consumed — clearing route state');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick (project logging guideline): Every other log(...) in this file carries a correlation field (id=%s), per CLAUDE.md's logging rules. id is in scope from useParams:

-    log('build seed consumed — clearing route state');
+    log('build seed consumed — clearing route state: id=%s', id);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6af3bd5d3: log('build seed consumed — clearing route state: id=%s', id), and id added to the callback dep array.

expect(copilotPanelProps.current?.flowId).toBe('test-id');
});

it('clears the build seed from route state once the copilot reports it consumed (#4597)', async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick (test gap): The PR's explicit design claim is that clearBuildSeed preserves other state fields (const next = { ...state }; delete next.copilotBuild;), but this test only seeds { copilotBuild } — a regression that nukes the whole state object wouldn't be caught. Add a sibling field (e.g. copilotRepair) to the route state and assert it survives after onBuildSeedConsumed() (e.g. via copilotPanelProps.current?.repairSeed).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 6af3bd5d3. The consume test now seeds a sibling copilotRepair alongside copilotBuild and asserts repairSeed survives both before and after onBuildSeedConsumed() — proving only copilotBuild is stripped, not the whole state object.

…ed review nits

Assert the `send()` outcome contract (`dispatched` | `skipped` | `failed`) at
its source in useWorkflowBuilderChat: make the socket-status mock configurable
and add tests proving a thrown dispatch reports `failed` (not the retryable
`skipped`), a success reports `dispatched`, and a disconnected socket reports
`skipped` without dispatching. The panel tests mock this hook, so a `failed`
misreported as `skipped` — which would re-arm the seeded build effect and loop
duplicate turns — would otherwise pass every test.

Also:
- FlowCanvasPage: rename the `clearBuildSeed` local from `state` to `routeState`
  so it no longer shadows the component-level LoadState `state`; add the `id=%s`
  correlation field to its log line per the logging guideline.
- WorkflowCopilotPanel: clarify the `failed`-outcome comment — the guard is
  per-mount, so a deliberate close+reopen is an intentional retry (safe, since a
  failed `mode:'build'` turn persists nothing).
- FlowCanvasPage test: seed a sibling `copilotRepair` alongside `copilotBuild`
  and assert it survives consumption, proving only `copilotBuild` is stripped.
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 8, 2026
…prettier

Merging upstream/main pulled in the orchestration i18n keys (tinyhumansai#4693), whose
long translated values in de/es/fr/it/pl/pt/ru exceed the 100-col printWidth
and tripped `prettier --check .` in the Frontend Checks gate. Reformatted via
`prettier --write` (canonical output — pure line-wrapping, no content change).
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 8, 2026
@YellowSnnowmann

Copy link
Copy Markdown
Contributor Author

Rebased onto latest main and addressed the review round (commits 6af3bd5d3 + c4326dcd8):

  • Review comments — replied inline on all five threads (send() outcome-contract tests, the failed/reopen clarification, the routeState rename, the id=%s log field, and the sibling-state test assertion).
  • CI (Frontend Checks / prettier) — the red prettier check was not from this PR's change: merging main pulled in the orchestration i18n keys (feat(orchestration): promote TinyPlace Orchestration to its own tab + teach the orchestrator to manage a fleet #4693), whose long de/es/fr/it/pl/pt/ru values exceed the 100-col printWidth. Fixed with prettier --write (pure line-wrapping, no content change) in c4326dcd8. Full prettier --check ., tsc, eslint, i18n:check (parity clean), and the affected suites are green locally.

@senamakel senamakel self-assigned this Jul 8, 2026
# Conflicts:
#	app/src/lib/i18n/fr.ts
#	app/src/lib/i18n/ru.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f3a515bedd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +215 to +220
if (outcome === 'dispatched') {
// Clear the ephemeral route seed only once the turn actually
// dispatched, so closing and reopening the panel (which remounts it
// and resets `buildSentRef`) can't re-fire the same build turn
// (issue #4597).
onBuildSeedConsumed?.();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Consume the build seed before awaiting completion

When the auto-build request is slow or still running, this .then does not execute until send() resolves, and send() only returns outcome: 'dispatched' after awaiting the full buildWorkflow RPC. If the user closes and reopens the copilot during that in-flight window, the panel remounts with buildSentRef reset while location.state.copilotBuild is still present, so it can dispatch the same build seed a second time. Fresh evidence is that the new callback is wired to the post-RPC result rather than to the moment the turn is accepted/started.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aeac072111

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// dispatched, so closing and reopening the panel (which remounts it
// and resets `buildSentRef`) can't re-fire the same build turn
// (issue #4597).
onBuildSeedConsumed?.();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Guard seed clearing after leaving the canvas

When the user navigates away from /flows/:id while the auto-build RPC is still running, this .then still fires after WorkflowCopilotPanel/FlowCanvasPage have unmounted and invokes the old clearBuildSeed callback, whose navigate({ pathname: location.pathname, ... }, { replace: true }) is closed over the old flow URL. That can unexpectedly pull the user back to the canvas once the slow build completes; either consume the seed before awaiting the long RPC or guard the callback so it only navigates while this route is still current.

Useful? React with 👍 / 👎.

@senamakel senamakel merged commit 27b6217 into tinyhumansai:main Jul 9, 2026
15 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in Team Openhuman Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Reopening the copilot re-sends the prompt-bar build seed and overwrites the flow

3 participants