shell: Make it all reacty #21012

mvollmer · 2024-09-16T13:24:15Z

Notes to reviewers:

This is bug-compatible with the old shell. Let's concentrate on the code structure here and not on fixing bugs.
This is by far not all the cleanups that can be done, it's just the first (big) step to untangle the mess. Let's avoid scope creep and concentrate on making this here solid.
Start with shell.jsx and state.jsx.
frames.jsx is a bit ugly, but should be well isolated from the rest. Also check shell: Use the "load" event instead of polling to catch ready frames #21061 for a followup that improves it significantly.
The rest should be straighforward. :-)

Ideas for followups:

The "jump" operation has a quirk re omitting the hash that we could remove. A omitted hash either means "empty hash in the final URL" or "same hash as previously", depending on whether the jump operation actually changes which frame is current or not. This can be avoided by changing all callers of "jump" to pass "/" as the hash when they want to have a empty one. I think it's mostly tests that exploit the quirk.
Remove the hacks and botches from the old React components, which is hopefully now possible.
Replace HostModalState with useDialogs.
Stop polling frames for readiness (which is a ill-defined thing anyway), and just keep them hidden until "onload" has fired. That also fixes the dark-mode flicker on Firefox. shell: Use the "load" event instead of polling to catch ready frames #21061
Add current_manifest to ShellState and use that in TopNav to get at the parent field. (TopNav uses the full path to look up the manifest itself, but should really only use the first component.) shell: Take docs from the correct manifest item #21072
Convert util.jsx into TypeScript, maybe clean it up / split it up at the same time.

pkg/newshell/base_index.js

pkg/newshell/hosts_dialog.jsx

pkg/newshell/shell.jsx

pkg/shell/shell.jsx

garrett · 2024-09-16T14:47:57Z

The host switcher mockups are so old, they used PF3 and had the dashboard:

(And the dashboard was where we edited machines prior.)

For PF4, if we're doing a 1:1 port, we should consider doing popovers and having a modal show up when you add or edit without dismissing the popover. (It will look different from these mockups for sure.)

pkg/shell/shell.jsx

jelly · 2024-09-18T13:47:07Z

test/verify/check-pages

@@ -71,6 +71,8 @@ class TestPages(testlib.MachineCase):
        m = self.machine
        b = self.browser

+        m.upload(["/home/mvo/work/cockpit/dist/shell"], "/usr/share/cockpit")


pkg/shell/shell.jsx

jelly · 2024-09-18T14:07:33Z

pkg/shell/hosts.jsx

        hosts_sel.appendChild(this.el);
    }

    componentWillUnmount() {
+        const hosts_sel = document.getElementById("nav-hosts");


Seems like an easy split off PR?

Yeah, but it's only necessary here because '#nav-hosts' doesn't exist yet (in this PR only) when the file is loaded. Also, I hope we can get rid of this DOM manipulation completely in this PR. Eventually.

jelly · 2024-09-19T13:20:34Z

pkg/shell/hosts.jsx

@@ -307,7 +306,6 @@ export class CockpitHosts extends React.Component {
                                item_render={render}
                                sorting={(a, b) => true}
                                filtering={this.filterHosts}
-                                current={label}


So it seems CockpitNav does not use current at all, can we then split this removal off?

No, that was a mistake unfortunately. It uses current to notice when navigation has happened, and then resets the search field.

This might be a good example of hard to understand magic that we want to get rid of. There might be a signal on the ShellState that announces navigation, or we might have a navigation_counter, or something more obvious.

pkg/shell/shell.jsx

pkg/shell/state.jsx

pkg/shell/shell.jsx

pkg/shell/state.jsx

mvollmer · 2024-10-02T13:28:10Z

Green, except for the revdeps. Those should go green with #21067.

pkg/shell/shell.jsx

pkg/shell/state.jsx

martinpitt

Many thanks for starting to untangle this mess! It's a lot easier to follow now.

I don't want to block this much -- I have two comments which I'd consider a blocker (bringing back the "yaya ssh dangerous" warning and the ensure_frame() comment spelling), but both of them should be trivial. The rest is just thinking aloud or follow-up material, and thus optional.

pkg/shell/util.jsx

pkg/shell/active-pages-modal.jsx

pkg/shell/frames.jsx

pkg/shell/hosts_dialog.jsx

martinpitt · 2024-10-09T12:10:47Z

pkg/shell/idle.jsx

+            .catch(e => {
+                if (e.message.indexOf("GetUInt not available") === -1)
+                    console.warn(e.message);
+            });


Follow-up: This was introduced so long ago that we can just count on it now.

pkg/shell/state.jsx

martinpitt

Cheers! 🚀 from my POV, let's do the rest in all these follow-ups.

And rewrite much of the state handling to make it easier to understand. Specifically, the old Index and MachineIndex classes and their complicated interactions have been replaced with a hopefully much more straightforward ShellState class. However, the existing React components such as CockpitHosts and TopNav have not been significantly touched. The API for launching the HostModal dialogs has been changed to make it more suitable for a later rewrite with Dialogs.run() ala pkg/lib/cockpit-connect-ssh.

mvollmer · 2024-10-10T08:53:47Z

Cheers! 🚀 from my POV, let's do the rest in all these follow-ups.

Thanks for the review!

I have decided to include #21074 here, after all. It only affects frames.jsx.

martinpitt

/me reacts with 👍

cockpituous · 2024-10-10T09:56:05Z

pkg/shell/active-pages-modal.jsx

+                    frame,
+                    active,
+                    selected: active,
+                    displayName: frame.host === "localhost" ? "/" + frame.path : frame.host + ":/" + frame.path,


This added line is not executed by any test.

cockpituous · 2024-10-10T09:56:05Z

pkg/shell/failures.jsx

+    if (machine.restarting) {
+        title = _("The machine is rebooting");
+        message = "";
+    } else if (connecting) {


These 4 added lines are not executed by any test.

cockpituous · 2024-10-10T09:56:05Z

pkg/shell/frames.jsx

+            if (frame.ready && iframe.getAttribute("data-ready") == null)
+                iframe.setAttribute("data-ready", "1");
+            else if (!frame.ready && iframe.getAttribute("data-ready"))
+                iframe.removeAttribute("data-ready");


This added line is not executed by any test.

cockpituous · 2024-10-10T09:56:05Z

pkg/shell/frames.jsx

+            if (frame.loaded && iframe.getAttribute("data-loaded") == null)
+                iframe.setAttribute("data-loaded", "1");
+            else if (!frame.loaded && iframe.getAttribute("data-loaded"))
+                iframe.removeAttribute("data-loaded");


This added line is not executed by any test.

cockpituous · 2024-10-10T09:56:05Z

pkg/shell/frames.jsx

+                     style === "auto") ||
+                    style === "dark") {


These 2 added lines are not executed by any test.

cockpituous · 2024-10-10T09:56:09Z

pkg/shell/util.jsx

+    self.search = function(prop, value) {
+        for (const x in self.items) {
+            if (self.items[x][prop] === value)
+                return self.items[x];


These 4 added lines are not executed by any test.

cockpituous · 2024-10-10T09:56:09Z

pkg/shell/util.jsx

+function component_checksum(machine, path) {
+    const parts = path.split("/");
+    const pkg = parts[0];
+    if (machine.manifests && machine.manifests[pkg] && machine.manifests[pkg][".checksum"])
+        return "$" + machine.manifests[pkg][".checksum"];


These 5 added lines are not executed by any test.

cockpituous · 2024-10-10T09:56:09Z

pkg/shell/util.jsx

+export function compute_frame_url(machine, path) {
+    let base, checksum;
+    if (machine.manifests && machine.manifests[".checksum"])
+        checksum = "$" + machine.manifests[".checksum"];


This added line is not executed by any test.

cockpituous · 2024-10-10T09:56:09Z

pkg/shell/util.jsx

+    if (checksum && checksum == component_checksum(machine, path)) {
+        if (machine.connection_string === "localhost")
+            base = "..";


These 3 added lines are not executed by any test.

cockpituous · 2024-10-10T09:56:09Z

pkg/shell/util.jsx

+        if (machine.connection_string === "localhost")
+            base = "..";
+        else
+            base = "../../" + checksum;


This added line is not executed by any test.

github-advanced-security bot found potential problems Sep 16, 2024

View reviewed changes

pkg/newshell/base_index.js Fixed Show fixed Hide fixed

pkg/newshell/base_index.js Fixed Show fixed Hide fixed

pkg/newshell/hosts_dialog.jsx Fixed Show fixed Hide fixed

pkg/newshell/hosts_dialog.jsx Fixed Show fixed Hide fixed

pkg/newshell/shell.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the shell-never-go-full-react branch from ef20c0f to c06e5df Compare September 16, 2024 13:31

github-advanced-security bot found potential problems Sep 16, 2024

View reviewed changes

pkg/shell/shell.jsx Fixed Show fixed Hide fixed

mvollmer added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Sep 17, 2024

mvollmer force-pushed the shell-never-go-full-react branch from c06e5df to 40bd4da Compare September 17, 2024 10:40

github-advanced-security bot found potential problems Sep 17, 2024

View reviewed changes

pkg/shell/shell.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the shell-never-go-full-react branch 3 times, most recently from f845f71 to 08db4e9 Compare September 18, 2024 12:56

jelly reviewed Sep 18, 2024

View reviewed changes

mvollmer force-pushed the shell-never-go-full-react branch 2 times, most recently from fd4891c to 566ddbc Compare September 19, 2024 11:33

github-advanced-security bot found potential problems Sep 19, 2024

View reviewed changes

pkg/shell/shell.jsx Fixed Show fixed Hide fixed

jelly reviewed Sep 19, 2024

View reviewed changes

mvollmer force-pushed the shell-never-go-full-react branch 2 times, most recently from b5f4113 to 39ba1c5 Compare September 20, 2024 08:24

github-advanced-security bot found potential problems Sep 20, 2024

View reviewed changes

pkg/shell/shell.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the shell-never-go-full-react branch from 39ba1c5 to 7bb403f Compare September 20, 2024 12:53

github-advanced-security bot found potential problems Sep 20, 2024

View reviewed changes

pkg/shell/shell.jsx Fixed Show fixed Hide fixed

pkg/shell/state.jsx Fixed Show fixed Hide fixed

pkg/shell/state.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the shell-never-go-full-react branch from 3c98dcc to 7be28f6 Compare September 23, 2024 09:56

github-advanced-security bot found potential problems Sep 23, 2024

View reviewed changes

pkg/shell/shell.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the shell-never-go-full-react branch from 8ec067c to 13770f8 Compare September 23, 2024 12:16

github-advanced-security bot found potential problems Sep 23, 2024

View reviewed changes

pkg/shell/state.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the shell-never-go-full-react branch 2 times, most recently from 2d3f8aa to 7819c2a Compare September 23, 2024 14:59

github-advanced-security bot found potential problems Sep 23, 2024

View reviewed changes

pkg/shell/state.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the shell-never-go-full-react branch 3 times, most recently from 551ed93 to f13e171 Compare September 24, 2024 10:21

This was referenced Oct 2, 2024

test: Don't explicitly go to x0.cockpit.lan in TestIPA #21064

Merged

test: Prepare for Shell rewrite #21067

Merged

mvollmer force-pushed the shell-never-go-full-react branch from a2d9927 to 8d6214e Compare October 3, 2024 07:03

mvollmer added the blocked Don't land until something else happens first (see task list) label Oct 3, 2024

mvollmer marked this pull request as ready for review October 3, 2024 07:26

mvollmer force-pushed the shell-never-go-full-react branch from 8d6214e to d4b3297 Compare October 3, 2024 08:39

mvollmer mentioned this pull request Oct 3, 2024

shell: Remove workaround for Chrome crash #21074

Closed

1 task

mvollmer force-pushed the shell-never-go-full-react branch from d4b3297 to 69289fb Compare October 7, 2024 14:53

github-advanced-security bot found potential problems Oct 7, 2024

View reviewed changes

pkg/shell/shell.jsx Fixed Show fixed Hide fixed

mvollmer mentioned this pull request Oct 7, 2024

Shell: Extra warnings when connecting to remote hosts #20826

Open

mvollmer force-pushed the shell-never-go-full-react branch 3 times, most recently from 8894552 to 17bd0ad Compare October 8, 2024 07:31

mvollmer mentioned this pull request Oct 8, 2024

shell: Take docs from the correct manifest item #21072

Open

mvollmer removed the blocked Don't land until something else happens first (see task list) label Oct 8, 2024

mvollmer requested review from jelly and martinpitt October 8, 2024 09:37

mvollmer commented Oct 9, 2024

View reviewed changes

pkg/shell/state.jsx Outdated Show resolved Hide resolved

martinpitt requested changes Oct 9, 2024

View reviewed changes

mvollmer force-pushed the shell-never-go-full-react branch from 17bd0ad to b9296b2 Compare October 10, 2024 08:06

martinpitt previously approved these changes Oct 10, 2024

View reviewed changes

mvollmer dismissed martinpitt’s stale review via cd23324 October 10, 2024 08:52

mvollmer force-pushed the shell-never-go-full-react branch from b9296b2 to cd23324 Compare October 10, 2024 08:52

mvollmer requested a review from martinpitt October 10, 2024 08:57

martinpitt approved these changes Oct 10, 2024

View reviewed changes

cockpituous reviewed Oct 10, 2024

View reviewed changes

mvollmer merged commit 7cd6428 into cockpit-project:main Oct 10, 2024
86 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shell: Make it all reacty #21012

shell: Make it all reacty #21012

mvollmer commented Sep 16, 2024 •

edited

Loading

garrett commented Sep 16, 2024 •

edited

Loading

jelly Sep 18, 2024

jelly Sep 18, 2024

mvollmer Sep 20, 2024

jelly Sep 19, 2024

mvollmer Sep 20, 2024

mvollmer commented Oct 2, 2024

martinpitt left a comment

martinpitt Oct 9, 2024

martinpitt left a comment

mvollmer commented Oct 10, 2024

martinpitt left a comment

cockpituous Oct 10, 2024

cockpituous Oct 10, 2024

cockpituous Oct 10, 2024

cockpituous Oct 10, 2024

cockpituous Oct 10, 2024

cockpituous Oct 10, 2024

cockpituous Oct 10, 2024

cockpituous Oct 10, 2024

cockpituous Oct 10, 2024

cockpituous Oct 10, 2024

shell: Make it all reacty #21012

shell: Make it all reacty #21012

Conversation

mvollmer commented Sep 16, 2024 • edited Loading

garrett commented Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvollmer commented Oct 2, 2024

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

mvollmer commented Oct 10, 2024

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvollmer commented Sep 16, 2024 •

edited

Loading

garrett commented Sep 16, 2024 •

edited

Loading