Views management refactoring #669

ixrock · 2020-08-10T19:04:41Z

Back to managing views with webview (iframe) in favour of smoothest clusters switching

jakolehm · 2020-08-10T19:12:18Z

src/main/index.ts

@@ -41,8 +40,7 @@ async function main() {
  const updater = new AppUpdater()
  updater.start();

-  registerFileProtocol(appProto, app.getPath("userData"));


Why appProto has been removed?

Because it's never used

Might be a dumb question but how do we display uploaded cluster icons then?

I switched to saving the base64 encoding of them in the user store

It removes the filename issue that we were seeing with some unicode characters, and it removes the security problems when running on dev mode.

I switched to saving the base64 encoding of them in the user store

Ok, do we have a migration for that?

I did not code one up, I should do that then

I did not code one up, I should do that then

Yes we should have a migration. Otherwise Lens will drop all the existing cluster icons on upgrade.

Nokel81

Looks good from a top level

Nokel81 · 2020-08-10T19:59:09Z

src/common/cluster-store.ts

+          const cluster = this.getById(state.id);
+          if (cluster) {
+            cluster.updateModel(state)
+          }


Suggested change

const cluster = this.getById(state.id);

if (cluster) {

cluster.updateModel(state)

}

this.getById(state.id)?.updatedModel(state);

src/common/ipc.ts

Nokel81 · 2020-08-10T20:12:14Z

src/common/ipc.ts

+}
+
+export function createIpcChannel({ autoBind = true, mode = IpcMode.ASYNC, timeout = 0, handle, channel }: IpcChannelInit) {
+  channel = `${mode}:${channel}`


Shouldn't it be an error to given channel === ""?

not critical imo, why not an empty string as a name?

Just seems like a weird that a channel called "sync:" or "async:" could be made. Another possible solution could be

channel = `${mode}:${channel || "default"}`

Nokel81 · 2020-08-10T20:17:50Z

src/common/ipc.ts

+          if (mode === IpcMode.ASYNC) {
+            event.reply(channel, res);
+          }
+          if (mode === IpcMode.SYNC) {
+            event.returnValue = res;
+          }


shouldn't this either be a switch or if ... else if?

doesn't really matter, only if it's a question of code-readability? :)

IMHO if ... else if would be better here.

Nokel81 · 2020-08-10T20:20:06Z

src/common/ipc.ts

+      if (mode === IpcMode.SYNC) {
+        ipcRenderer.sendSync(channel, req)
+      }
+      return new Promise(async (resolve, reject) => {


why are we returning a promise if the mode is sync?

maybe we should, haven't even tested sync mode and it's better to use this "as last resort" as defined in docs:
https://www.electronjs.org/docs/api/ipc-renderer#ipcrenderersendsyncchannel-args

Just thinking that do we need sync mode here if we are only using async messaging?

Well, this IPC helper is intended to be multi-functional, so I would keep it, but have to test if it's really works as expected.

src/main/routes/metrics-route.ts

src/main/window-manager.ts

src/renderer/bootstrap.tsx

src/renderer/components/+apps-releases/release.store.ts

src/renderer/components/cluster-manager/cluster-manager.tsx

nevalla · 2020-08-11T06:03:13Z

~~Cluster settings does not open when clicking from left menu. It's working if opening from system menu, but the layout is broken.~~ (FIXED)

aleksfront · 2020-08-11T07:56:51Z

Cluster settings does not open when clicking from left menu. It's working if opening from system menu, but the layout is broken.

Layout is fixed.

jakolehm · 2020-08-12T06:40:14Z

src/common/user-store.ts

-        .filter(ctx => ctx.cluster)
-        .filter(ctx => !this.seenContexts.has(ctx.name))
-        .forEach(ctx => this.newContexts.add(ctx.name));
+      localContexts.forEach(({ cluster, name }) => {


I think this is matter of taste. I would revert this change as it's not related to views management at all, right?

jakolehm · 2020-08-12T06:42:29Z

src/main/cluster-manager.ts

-    return cluster;
+    logger.debug(`getClusterForRequest(): ${req.headers.host}${req.url}`)
+    const clusterId = req.headers.host.split(".")[0]
+    return this.getCluster(clusterId)


Why logic has been removed? Don't we use 127.0.0.1:<port>/uuid anymore anywhere?

Seems so.. Looks like outdated code from very beginning of lens-app.

I asked because I port/uuid addresses were used when doing requests outside of renderer (wildcard dns to localhost works only within browser scope). Anyway I don't like to see changes that are totally outside of PR scope, it makes review unnecessary complicated.

Ok, reverted back. Someone should check if this is actually works, cause top comment:
// lens-server is connecting to 127.0.0.1:<port>/<uid>
which unclear in terms of what you saying how this might be used outside.