8000 Fix: stale resource in details panel after update from apis/store by ixrock · Pull Request #7187 · lensapp/lens · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix: stale resource in details panel after update from apis/store #7187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Fix: stale resource in details panel after update from apis/store #7187

wants to merge 6 commits into from

Conversation

ixrock
Copy link
Contributor
@ixrock ixrock commented Feb 17, 2023 •
edited
Loading
Screen.Recording.2023-02-17.at.16.24.33.mov

@ixrock
PoC: Stale resource in details panel after update from apis/store, fix …
998a9e8 
…#7186

Updated currently for: Replication Controllers & Namespaces.

Signed-off-by: Roman <ixrock@gmail.com>
@ixrock ixrock added the bug Something isn't working label Feb 17, 2023
@ixrock ixrock added this to the 6.4.0 milestone Feb 17, 2023
@ixrock ixrock requested a review from a team as a code owner February 17, 2023 14:32
@ixrock ixrock self-assigned this Feb 17, 2023
@Nokel81
Copy link
Collaborator
Nokel81 commented Feb 17, 2023

Another solution would be something like asyncSubscribableComputed which @Iku-turso says is already mostly created for computedChannel. That way we don't have to fix each details panel individually and the contract of currentKubeObjectForDetailsViewInjectable stays the same.

@ixrock
Copy link
Contributor Author
ixrock commented Feb 17, 2023 •
edited
Loading

Another solution would be something like asyncSubscribableComputed which @Iku-turso says is already mostly created for computedChannel. That way we don't have to fix each details panel individually and the contract of currentKubeObjectForDetailsViewInjectable stays the same.

This is more complex solution/more abstraction which is not good.
All what we need is just get rid of currentKubeObjectInDetailsInjectable (replace to currentKubeObjectForDetailsViewInjectable2 and rename)

@Nokel81
Copy link
Collaborator
Nokel81 commented Feb 17, 2023

You solution doesn't work though. If a KubeObject is opened in details without that list view being navigated to then the bug presented still happens.

@ixrock
Copy link
Contributor Author
ixrock commented Feb 18, 2023

You solution doesn't work though. If a KubeObject is opened in details without that list view being navigated to then the bug presented still happens.

didn't get what you mean.. can you record some demo? or exact set of instructions how to represent the issue..

@Nokel81
Copy link
Collaborator
Nokel81 commented Feb 19, 2023

Sure. I think the following will show the error.

  1. Go to Pods
  2. Click on a pod that is controlled by a deployment
  3. Click on the link to the deployment
  4. Update a label on that deployment
  5. See that the details panel doesn't update.

aleksfront
aleksfront reviewed Feb 20, 2023
View reviewed changes
Copy link
Contributor
@aleksfront aleksfront left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

< 8000 button type="submit" data-view-component="true" class="Button--secondary Button--medium Button d-inline-block"> Hide comment

LGTM once tests passed.

Another solution would be something like asyncSubscribableComputed which @Iku-turso says is already mostly created for computedChannel.

Current solution in this PR is enough for now in my opinion for the 6.4.0. Described one probably will be ready for upcoming versions.

@Nokel81 Followed your example by changing labels on a deployment and it works:

labelling.a.deployment.mov

@ixrock
clean up: get rid of old implementation of `currentKubeObjectInDetail…
5a88444 
…sInjectable`

Signed-off-by: Roman <ixrock@gmail.com>
@ixrock
Copy link
Contributor Author
ixrock commented Feb 20, 2023 •
edited
Loading

@aleksfront for some reasons I've been able to reproduce the bug Sebastian was described above..

@Nokel81 is this the case? If so it's not updated after sequential navigations within opened detail-view, right?
Anyway I think could be even fixed in follow-up PR since now opening details panel works as expected and active object is up-to-date with it's store.

Screen.Recording.2023-02-20.at.13.56.45.mov

@ixrock
Copy link
Contributor Author
ixrock commented Feb 21, 2023

@lensapp/lens-maintainers any ideas how to fix tests?

@ixrock ixrock force-pushed the fix/resource_details_panel_stale_data branch from 8fb890e to 5a88444 Compare February 21, 2023 12:50
@Nokel81
Copy link
Collaborator
Nokel81 commented Feb 21, 2023

Marking as 6.5.0 since this is a longer standing bug and this won't be blocking that release.

@Nokel81 Nokel81 modified the milestones: 6.4.0, 6.5.0 Feb 21, 2023
@Nokel81
Copy link
Collaborator
Nokel81 commented Feb 21, 2023

https://github.com/mobxjs/mobx-utils#fromresource is what I think we should use for this case.

The URL seems to be something like: http://127.0.0.1:52267/e54971f316520ea4fa8d54555edc3054/api/v1/nodes?fieldSelector=metadata.name%3Dlens-desktop-kube&resourceVersion=0&watch=true

@ixrock
Merge remote-tracking branch 'origin/master' into fix/resource_detail…
86f6f39 
…s_panel_stale_data
@ixrock
switch to asyncComputed
c739d7e 
Signed-off-by: Roman <ixrock@gmail.com>
@ixrock
update snapshots with a new command npm run test:update-snapshots f…
7b862b9 
…rom `packages/core`

Signed-off-by: Roman <ixrock@gmail.com>
ixrock added a commit that referenced this pull request Feb 23, 2023
@ixrock
alternative to #7187
883ad8d 
Signed-off-by: Roman <ixrock@gmail.com>
@ixrock ixrock mentioned this pull request Feb 23, 2023
Merged
@ixrock ixrock closed this Feb 23, 2023
@ixrock ixrock deleted the fix/resource_details_panel_stale_data branch February 23, 2023 11:43
Nokel81 pushed a commit that referenced this pull request Mar 13, 2023
@ixrock
Keep up-to-date resource in the details view (Drawer) (#7224)
79a4eb3 
* alternative to #7187

Signed-off-by: Roman <ixrock@gmail.com>

* update snapshots with `jest src -u` from `packages/core`

Signed-off-by: Roman <ixrock@gmail.com>

* skipping some tests cause i have no idea how to fix those and what is wrong

Signed-off-by: Roman <ixrock@gmail.com>

* fix tests

Signed-off-by: Roman <ixrock@gmail.com>

---------

Signed-off-by: Roman <ixrock@gmail.com>
gabriel-mirantis pushed a commit that referenced this pull request Mar 21, 2023
@ixrock @gabriel-mirantis
Keep up-to-date resource in the details view (Drawer) (#7224)
c4f8f11 
* alternative to #7187

Signed-off-by: Roman <ixrock@gmail.com>

* update snapshots with `jest src -u` from `packages/core`

Signed-off-by: Roman <ixrock@gmail.com>

* skipping some tests cause i have no idea how to fix those and what is wrong

Signed-off-by: Roman <ixrock@gmail.com>

* fix tests

Signed-off-by: Roman <ixrock@gmail.com>

---------

Signed-off-by: Roman <ixrock@gmail.com>
Signed-off-by: Gabriel <gaccettola@mirantis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jansav jansav Awaiting requested review from jansav

@Nokel81 Nokel81 Awaiting requested review from Nokel81

@aleksfront aleksfront Awaiting requested review from aleksfront

@Iku-turso Iku-turso Awaiting requested review from Iku-turso

Assignees

@ixrock ixrock

Labels
bug Something isn't working
Projects
None yet
Milestone
6.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ixrock @Nokel81 @aleksfront
0