Unsaved changes indicator

[doc-han]

Description

This PR shows a red dot indicator on the save button when there are unsaved changes in a workflow.

Closes #3682

Validation steps

1. Open a workflow
2. Make some changes to a job/edge
3. Do you see a red dot showing that some unsaved changes exist? we expect a yes

If you're seeing a red dot right upon entering a workflow, it means there already exist unsaved changes(between what's in the DB and what's on ydoc).

Additional notes for the reviewer

1. (Is there anything else the reviewer should know or look out for?)

AI Usage

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

• Code generation (copilot but not intellisense)
• Learning or fact checking
• Strategy / design
• Optimisation / refactoring
• Translation / spellchecking / doc gen
• Other
• I have not used AI

You can read more details in our Responsible AI Policy

Pre-submission checklist

• I have performed a self-review of my 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 Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.30%. Comparing base (cde09c5) to head (79826e3).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4289   +/-   ##
=======================================
  Coverage   89.30%   89.30%           
=======================================
  Files         425      425           
  Lines       19899    19900    +1     
=======================================
+ Hits        17770    17771    +1     
  Misses       2129     2129           

☔ View full report in Codecov by Sentry.
📢 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.

[taylordowns2000]

noting project_credential_id from our call! ping when ready to test again please :-)

[taylordowns2000]

works great @doc-han . i've requested a review from @stuartc since he worked on this also before the break.

two things (not blocking for me):

1. see comment on the hard-coded expected structure. is there another way?
2. check out the issue (and comment) here about warning on unsaved changes... i think it would be really nice to finally tell users "hey, you're about to navigate away with unsaved stuff" and I wonder if this new function allows us to do that?

[lmac-1]

Most of my review comments are low priority / nice-to-haves and shouldn't block merging.

But I noticed that "Allow console.log() usage" (workflow.enable_job_logs) and "Max concurrency" (workflow.concurrency) don't trigger the unsaved changes dot. This should be addressed before merging.

[lmac-1]

What is this comment for? What should it be fixed to, and when? It would be better if we don't use any() here

[doc-han]

will remove the comment. the value held by this field is currently not used or supposed to be used by anything. it's stale data holding the current state of saved workflow. used as base for comparison. I think there's no need need to strictly validate it.

[lmac-1]

Btw I jumped straight into review comments but also wanted to say, this PR is great work 🤩. Love it!

[stuartc]

@doc-han you mean it's not used by anything 'outside'. But it is used for comparison, thats a good enough reason to have typing on it no?

Like what is the shape of the value? Everything else has a type.

[stuartc]

This returns a new object every call, it looks like we will get lots of rerenders/calls to transformWorkflow.

Please verify this.

We can avoid recomputing and rendering with something like this:

export function useUnsavedChanges() {
  const { workflow } = useSessionContext();

  // Individual selectors - stable references
  const jobs = useWorkflowState(state => state.jobs);
  const triggers = useWorkflowState(state => state.triggers);
  const edges = useWorkflowState(state => state.edges);
  const positions = useWorkflowState(state => state.positions);
  const name = useWorkflowState(state => state.workflow?.name);

  // Memoize comparison
  const hasChanges = useMemo(() => {
    if (!workflow) return false;
    const storeWorkflow = { jobs, triggers, edges, positions: positions || {}, name };
    return isDiffWorkflow(transformWorkflow(workflow), transformWorkflow(storeWorkflow));
  }, [workflow, jobs, triggers, edges, positions, name]);

  return { hasChanges };
}

[stuartc]

Why isn't there a notify call here? All the other ones call notify afterwards.

I'm guessing it works because it's called right after 'setLatestSnapshotLockVersion' and it just happens to have it's effects in the state? But this seems like it could be a quiet issue waiting?

[stuartc]

@doc-han you mean it's not used by anything 'outside'. But it is used for comparison, thats a good enough reason to have typing on it no?

Like what is the shape of the value? Everything else has a type.

[stuartc]

I'm wondering why broadcast_from! was changed to broadcast!. The main reason we use broadcast specifically is to do force us handle all the server changes atomically in the callback we get on save.

So the 'saver' gets both the success and the new baseline workflow at the same time. We have the workflow in memory and ready to be able to render it out in the reply, and by broadcasting to
ourselves we send a second message leaving the saver (depending on latency) with a period of time where the workflow was saved but the baseline isn't updated.

It also splits responsibility between the feedback of "Saved successfully" and then later a listener does the real work.

I noted that save_and_sync doesn't have this change, meaning GitHub saves wouldn't update the baseline. Are we missing a test here?

workflow_channel.ex - save_workflow handler

{:reply, {:ok, %{
  saved_at: workflow.updated_at,
  lock_version: workflow.lock_version,
  workflow: workflow # ← Add this
}}, socket}

And change back to broadcast_from!

broadcast_from!(socket, "workflow_saved", %{
  latest_snapshot_lock_version: workflow.lock_version,
  workflow: workflow
})

Then on the client, the save callback should call
setBaseWorkflow(response.workflow) directly, rather than relying o n the
broadcast listener.

This also means save_and_sync needs the same treatment - include workflow in
response and use the callback to update baseline.

An observation about the tests for the WorkflowChannel (not from your changes), is that we have essentially zero tests for broadcast events. Which would have caught the inconsistency with save_and_sync.