appsmithorg / appsmith

Platform to build admin panels, internal tools, and dashboards. Integrates with 25+ databases and any API.
https://www.appsmith.com
Apache License 2.0
34.64k stars 3.74k forks source link

[Bug]: Client WS version check fails when using NGINX. New version pop-up won't go away using latest version #33169

Open migruiz4 opened 6 months ago

migruiz4 commented 6 months ago

Is there an existing issue for this?

Description

There is an issue in the latest version of Appsmith 1.23 where the "Hey! There is a new version of Appsmith available. Please consider refreshing your window." pop up message won't go away, even if both client and RTS server are using the same version.

image

Thanks to the information provided here, I have been able to dig into this issue, so here is what I found:

The RTS server version is v1.23.0: image

Investigating into the client source code, I found out that the version is stored into the browser storage, and in case of mismatch, the toast message is displayed. Here is what I found in my storage: image

VERSION_UPDATE_STATE:"{"currentVersion":"{{env \"APPSMITH_VERSION_ID\"}}","upgradeVersion":"v1.23.0","timesShown":1,"event":"UPDATE_REQUESTED"}"

This took me to this PR (https://github.com/appsmithorg/appsmith/pull/31325) merged two weeks ago, and released in v1.22.0, which I think could be the root cause of the issue.

In the same PR, it appears that NGINX has been deprecated in favor of Caddy. Is it a bug or does it mean that support for NGINX is discontinued?

Steps To Reproduce

Public Sample App

No response

Environment

Release

Severity

Medium (Frustrating UX)

Issue video log

No response

Version

Self Hosted - 1.23.0

Nikhil-Nandagopal commented 6 months ago

@migruiz4 are you using our docker image or building appsmith from source?

migruiz4 commented 6 months ago

Hi @Nikhil-Nandagopal,

I'm building from sources and using NGINX as web server as mentioned in the description.

I'm aware the docker image uses Caddy, and therefore I haven't been able to reproduce the issue using it.

I was wondering if this change was intentional and from now on only Caddy is supported, as NGINX worked fine until Appsmith version 1.21

migruiz4 commented 6 months ago

Hi @Nikhil-Nandagopal,

While taking a look again at https://github.com/appsmithorg/appsmith/pull/31325 I saw it introduced a typo here, where instead of config.startsWith("}") it should be config.endsWith("}")): https://github.com/appsmithorg/appsmith/blob/5a6f09552285d86d470f44c5479e3d012704e6b0/app/client/public/index.html#L25

I patched that line, rebuilt the client and the toast message is gone, so maybe that was the root cause of the issue.

sharat87 commented 6 months ago

@migruiz4, just getting to this. Looks like you've found the problem in the code. I'd be happy to merge a PR with a fix to that line if you're interested. Thanks!

To answer your questions,

  1. We no longer use NGINX in the final Appsmith Docker image. It has been replaced with Caddy several versions ago. Primary reason this was done was for better support for automatic TLS and TLS certificate management.
  2. In dev-time, unfortunately, we're still using NGINX, with the start-https.sh script. The start-caddy.sh script is still a bit buggy and I'm waiting on a separate fix that another team is working on, to get the polish I need. After that, the start-caddy.sh should also start working.

The reason we haven't found this yet is likely because not many people use the start-caddy.sh script today, and that double startsWith logic never applies in the Docker image's environment.

Let me know if there's further questions. Cheers!

Nikhil-Nandagopal commented 5 months ago

Bumping this issue up to prod because it should have made it's way there by now if it was not fixed