Skip to content

Show which step an AI edit changed - #5250

Open
hanna-paasivirta wants to merge 6 commits into
mainfrom
ai-diff-change-signals
Open

hanna-paasivirta wants to merge 6 commits into
mainfrom
ai-diff-change-signals

Conversation

@hanna-paasivirta

@hanna-paasivirta hanna-paasivirta commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Description

In user testing, people found it hard to tell where the AI's edits landed. This PR makes the AI's code diffs point clearly to the step they changed.

  • The diff header shows the step name next to its adaptor logo.
  • Click the step name or "Go to step" to open the step. The code the AI added flashes twice, then stays lightly highlighted until you click or type.
  • The highlight finds the code by its text, so it still lands in the right place if the step was edited since.
  • Clicking a removed step gives it a small shake, with a "This step was removed" tooltip.
  • The diff shows one column of line numbers instead of two.
  • Added steps with only placeholder code start folded, with no Copy button.

Closes BLD-99. Follows on from #5036.

go-to-step-walkthrough

Validation steps

  1. Ask the AI assistant to change a step's code.
  2. Click the step name in the diff. The step should open with the new code highlighted, flashing twice.
  3. Click it again. The highlight should flash again.
  4. Click or type in the code. The highlight should go away.
  5. Ask the assistant to remove a step, then click its name. It should shake and not open.

Additional notes for the reviewer

  1. Not yet checked in a browser. Tests pass.

AI Usage

Please disclose whether you've used AI anywhere in this PR (it's cool, we just want to know!):

  • I have used Claude Code
  • I have used another model
  • I have not used AI

You can read more details in our Responsible AI Policy

Pre-submission checklist

  • I have performed an AI review of my code (we recommend using /review with Claude Code)
  • I have implemented and tested all related authorization policies. (e.g., :owner, :admin, :editor, :viewer)
  • I have updated the changelog.
  • I have ticked a box in "AI usage" in this PR

@codecov

codecov Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.6%. Comparing base (aac8674) to head (bf8543f).

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #5250     +/-   ##
=======================================
+ Coverage   91.4%   91.6%   +0.2%     
=======================================
  Files        465     465             
  Lines      23416   23416             
=======================================
+ Hits       21399   21444     +45     
+ Misses      2017    1972     -45     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hanna-paasivirta
hanna-paasivirta marked this pull request as ready for review October 15, 2026 06:08
@github-actions

Copy link
Copy Markdown

Security Review ✅

  • S0 (project scoping): N/A — all changes are React/TypeScript, CSS, docs, and vitest tests under assets/js/collaborative-editor/ and assets/js/monaco/; no new queries or web-layer entrypoints touch project-scoped data.
  • S1 (authorization): N/A — no controller actions, LiveView handle_events, or policy modules are added or modified; the UI operates on data already loaded via the existing authorized channel.
  • S2 (audit trail): N/A — no Ecto.Multi / Repo.insert|update|delete on config resources is added; this PR only renders diff highlights and step pills client-side.

@elias-ba elias-ba 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.

Great work Hanna 👏🏽, this is a nice thing to have, and pointing the diff at the step it changed is the right call. A few comments and questions below, could you please feel free to let me know what you think?

The main thing is the browser pass you mentioned. The highlight has nothing holding it, so the whole effect can come out and the editor tests still pass, which means the browser is the only place anyone will find out whether it works. While you are in there, worth looking at a hunk with both an add and a remove in it, since the single line-number column prints the same number twice with different content and the comment that came out said why that reads badly.

Thanks!

// Found by text, not line number: the code may have been edited since
// the reply. Of several copies, the one nearest where it was wins; a run
// edited away, or only blank lines, highlights nothing.
const ranges = pendingHighlight.added.flatMap(({ line, text }) => {

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.

The search is exact and case-sensitive, so this only tolerates the code moving, not being edited. Change one character inside an added run and it matches nothing, the highlight doesn't appear, and the editor doesn't scroll, with nothing to tell the user why. Your comment above says as much, but the description promises it still lands in the right place if the step was edited since, so one of the two should give.

The other half is that a short run matches everywhere. A one-line change like return state; will have copies all over a job, and nearest-wins picks by line distance, so after the code has shifted the highlight can land on a line the assistant never wrote.

Nothing covers any of this. The whole effect can be deleted and the editor tests stay green, so the browser pass is carrying all of it. Would a test with a stubbed findMatches over a fixture be worth having, for which ranges come out and for a run that's been rewritten?

canOpen
? 'Go to step'
: step.type === 'remove'
? 'This step was removed'

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.

This tells the user something we don't know. Anything not on the canvas gets this, so a step whose apply failed, or one whose reply we reloaded before the workflow finished syncing, is reported as no longer in the workflow and shakes. The panel sits outside the loading gate, so a reopened conversation can render its diffs while the canvas is still empty. Could the wording hold back from the claim when we just can't find it?

action={
<>
<CopyStepButton body={copyBody} />
{!isPlaceholder && <CopyStepButton body={copyBody} />}

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.

Small one. Unfolding a placeholder step shows the code but still no Copy, since this keys off the classification rather than the fold. Could it follow the fold instead?

@github-project-automation github-project-automation Bot moved this from New Issues to In review in Core Oct 19, 2026

This branch has not been deployed

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

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

2 participants