feat: cache clear button #2742
feat: cache clear button #2742
Conversation
Ziinc
force-pushed
the
feat/cache-clear-button
branch
from
cf275df to
2eaa4b7
Compare
Ziinc
force-pushed
the
feat/cache-clear-button
branch
from
a15ee64 to
ba1575d
Compare
Ziinc
force-pushed
the
feat/cache-clear-button
branch
from
4b6a59f to
739c7a9
Compare
SteffenDE
left a comment
SteffenDE
left a comment
There was a problem hiding this comment.
A couple of comments from my side :)
| :enable_auth, | ||
| :labels | ||
| ] | ||
| for pid <- Resolver.list_caches(endpoint) do |
There was a problem hiding this comment.
Just to make sure since the PR title or description does not mention this: you also removed the should_kill_caches? check, so now it will always invalidate.
| <button phx-click="clear-endpoint-cache" phx-value-endpoint-id={@show_endpoint.id} class="tw-text-black tw-p-1 tw-flex tw-gap-1 tw-items-center tw-justify-center tw-border-0 tw-bg-transparent"> | ||
| <i class="inline-block h-3 w-3 fas fa-trash"></i><span> clear cache</span> | ||
| </button> |
There was a problem hiding this comment.
Just as an idea: maybe you add a <.subheader_button> component to prevent copying the classes here? Otherwise you need to remember to update this file if you change how the subheader links render.
| def handle_event( | ||
| "clear-endpoint-cache", | ||
| %{"endpoint-id" => endpoint_id}, | ||
| socket | ||
| ) do | ||
| endpoint = Endpoints.get_endpoint_query(endpoint_id) | ||
| :ok = Endpoints.clear_all_endpoint_caches(endpoint) |
There was a problem hiding this comment.
you need to be careful in case not every user should be able to clear arbitrary endpoints. Since you expect this event to only be invoked from the button, you could skip the phx-value-endpoint-id and directly access the show_endpoint assign here.
Always remember that handle_event params come directly from the client and can never be trusted.
Also, I don't know how long this typically takes, but the fact that you wait for 30 seconds in clear_all_endpoint_caches suggests that it might take a bit. If that's the case I would recommend to do this asynchronously using start_async and show a loading indicator while it is processing. Otherwise the whole LiveView will be unresponsive while it is processing this event.
| TestUtils.retry_assert([sleep: 250], fn -> | ||
| view | ||
| |> element(".subhead button", "clear cache") | ||
| |> render_click() | ||
|
|
||
| assert render(view) =~ "Cache cleared successfully" | ||
| assert render(view) =~ ~r/active caches:.+0/ | ||
| end) |
There was a problem hiding this comment.
I don't think that you need this retry logic. render_click waits for the handle_event to succeed, so you should always see the success message afterwards.
There was a problem hiding this comment.
shifting the render_click out of the retry_assert actually results in the test becoming very flaky, so would need to leave it in for now. it is likely due to the Task.async_await but haven't really figured out why it is slow 🤔
adds a cache clear button to the ui
