feat: sunsetting legacy editor#4908
Conversation
Security Review ✅
|
|
@doc-han I am blocking out some time today to go through this 🙏🏻 |
There was a problem hiding this comment.
Hey @doc-han, looking good! The deleted Elixir modules have zero dangling references, so that's great.
But assets/js/editor - the legacy editor directory - isn't touched at all in this PR, because the collaborative editor still depends on two files in it. We should resolve this now:
- Copy
magic-completion.tsandes5.min.dts.jsinto the collaborative editor and repoint the imports.CollaborativeMonaco.tsxandloadDTS.tscurrently import both files fromassets/js/editor - Extract
DEFAULT_TEXTout ofassets/js/editor/Editor.tsx. This is currently used byusePlaceholders.tsimport that one constant. Pull it into a small shared constants file soEditor.tsxhas n remaining importers either. - Once 1 and 2 are done, delete
assets/js/editorentirely, includinglib/es5.dts.jsandlib/es5.min.dts.js - Delete the dead
.job-editor-panelCSS inassets/css/app.cssas the only component using it was already removed - Add a new CHANGELOG.md entry. It belongs in a "Removed" entry and should mention that we are removing the user-facing setting (
prefer_legacy_editor) and retires public URLs (/w/:id/legacy,/w/new/legacy)
By the way, I still haven't done manual testing, but happy to do some after these points are addressed :)
elias-ba
left a comment
There was a problem hiding this comment.
Hey @doc-han, really clean sunset - I went digging for the scary stuff (dangling refs to the deleted modules, whether the collaborative editor and its AI still stand on their own) and it all holds up nicely 🙌 I'm building on @lmac-1's review rather than repeating it, so I've skipped the changelog + the assets/js/editor cleanup since she's got those covered.
A couple of extra things from me:
-
Deep links - left an inline on the redirect controller. Old
?a=<run>bookmarks won't follow the run anymore. More a question than a blocker. -
A test I think we lost by accident - deleting the legacy
trigger_test.exsalso took the only coverage of the passwordless / SSO webhook-secret reveal inWebhookAuthMethodFormComponent, and that component is still alive (mounted incollaborate.exand project settings). The surviving SSO tests are for reauth/profile, not the webhook reveal. Since it's a secret-disclosure path, could we port those 1-2 tests onto the surviving component ? -
A bit more dead code to sweep while we're in here - on top of Lucy's list:
workflow_live/components.exstill hasworkflow_diagram/1(rendersphx-hook="WorkflowDiagram", a hook that isn't registered anymore), pluscreate_job_panel/1andpanel/1with no callers, andui_metrics.exonly logged events the deleted JS used to emit.editorUrlConversion.tsalso has no prod caller now - though if we reuse it for the deep-link thing above, it earns its keep 🙂
Nothing blocking from my side once Lucy's points land. Thanks for taking this one on, it's a big one 💪
| defp redirect_preserving_query(conn, base_path) do | ||
| target = | ||
| case conn.query_string do | ||
| "" -> base_path | ||
| query -> base_path <> "?" <> query | ||
| end | ||
|
|
||
| conn | ||
| |> redirect(to: target) | ||
| |> halt() | ||
| end |
There was a problem hiding this comment.
Hey @doc-han, nice and tidy PR 🙏 One thing I bumped into here: we forward the raw query string, but the old editor and the collaborative one use different param names. So an old bookmarked run link like /w/:id/legacy?a=<run_id>&m=history redirects to /w/:id?a=<run_id>..., and the collaborative editor only reads run= (see collaborate.ex handle_params), so the run isn't followed - the user just lands on a blank editor. Funny thing is we already have this exact mapping in assets/js/utils/editorUrlConversion.ts (a -> run, s -> job/trigger/edge, m -> panel), it's just client-side. Do you think it's worth mapping at least a -> run here so shared run links keep working ? Not a blocker, more a question.
|
Hey @doc-han, went back for a second, deeper pass now that the shape's settled and a few more things surfaced. Building on what I left above rather than repeating it 🙂
Still nothing blocking beyond that channel guard, and even that's a couple of lines. Thanks again for taking this one on 💪 |
|
@elias-ba on your last review. One crash I'd want to guard before merge On the deep-link point I raised above Small one |
elias-ba
left a comment
There was a problem hiding this comment.
Hey @doc-han, this one was a big lift and it came out really clean 🙌🏽. Thanks for working through all the earlier feedback, the a → run remap, the ported webhook-secret tests, the migration rollback and the dead-code sweep are all sorted. Approving 👏🏽
While I was in here I did one more deep pass just for dead code the sweep might've stepped around, and found three leftovers. None block this and none are bugs, so genuinely your call whether to fold them in while the context is fresh or spin up a quick follow-up:
- An orphaned legacy LiveView,
RunLive.RunViewerLive- its only mount wasWorkflowLive.JobView(deleted here) vialive_render, and the collaborative editor already has its own React run viewer, so the whole module is dead now. This is the one I'd most like to see go, since an orphaned legacy view slightly undercuts the clean sunset. - A dead cluster in
WorkflowLive.Helpers- you removedcollaborative_editor_url/2, but the rest of its legacy URL/param family went out withedit.extoo:build_url/2plus the*_params/param/query_parambuilders, andbroadcast_updated_params. I traced it and they only reference each other now, none of the live functions touch them.subscribe_to_params_updatedies together with the RunViewerLive above. - A few now-orphaned test helpers in
workflow_live_helpers.ex(editor_elementand friends) that lost their callers when the legacy tests went. Lowest priority of the three, and that file's a mix so it wants a per-function look rather than a blanket delete.
Really nice work taking this one on man, thanks again 💪🏽
1. Legacy deep-link params silently dropped — correctness (possibly intentional)
The redirect only remaps Failure scenario: a saved bookmark like Caveat: the controller's own moduledoc says this is intentional, so this is a product call rather than an outright bug. If any fidelity is worth keeping, 2. Default job-body string now duplicated — cleanup
The new shared Failure scenario: two sources of truth. A future edit to the "Job Writing Guide" starter comment updates one creation path (auto-layout placeholders) but not the other (Yjs-backed 3. Dead URL-conversion module left behind — cleanup
After this branch removed Ties into #1: 4. Router comment contradicts the controller — cleanup
The added comment claims "the raw query string is forwarded as-is", but the controller does remap Failure scenario: a maintainer trusting the router comment assumes no param rewriting happens, so when debugging why a legacy Recommendation
|
Description
This PR sunsets the legacy editor
Closes #4402
Validation steps
Manual QA — Legacy editor sunset
Redirects
/projects/:id/w/:wid/legacyredirects to the collaborative editor/projects/:id/w/new/legacyredirects to the new-workflow flowLegacy users
Collaborative editor (regression)
AI Assistant
Additional notes for the reviewer
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)