-
Notifications
You must be signed in to change notification settings - Fork 100
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.
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
fix: Reduce redundant network calls in rill developer #4883
fix: Reduce redundant network calls in rill developer #4883
Conversation
/review |
Code Review Agent Run Status
Code Review Overview
>>See detailed code suggestions<< See other commands you can run High-level FeedbackEnsure consistent use of resource management methods across all components to maintain efficiency. Review the changes for potential impacts on existing functionalities and compatibility with cloud environments. Consider adding more comprehensive tests to cover new changes and ensure that private constructors do not hinder unit testing. |
}); | ||
} | ||
|
||
public async waitForResource(name: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: The method 'waitForResource' uses a hard-coded comparison string "RECONCILE_STATUS_IDLE", which could limit the flexibility of the ResourceWatcher class.
Fix: Consider making the status string a parameter of the 'waitForResource' method or part of the class configuration to increase flexibility and reusability of the 'ResourceWatcher' class.
Code Suggestion:
- return waitUntil(() => this.statuses.get(name) === "RECONCILE_STATUS_IDLE");
+ return waitUntil(() => this.statuses.get(name) === this.idleStatus); // this.idleStatus should be configurable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesnt make sense to make this configurable.
@@ -1,13 +1,13 @@ | |||
<script lang="ts"> | |||
import { fileArtifacts } from "@rilldata/web-common/features/entity-management/file-artifacts"; | |||
import { createWatchFilesClient } from "@rilldata/web-common/features/entity-management/watch-files-client"; | |||
import { createWatchResourceClient } from "@rilldata/web-common/features/entity-management/watch-resources-client"; | |||
import { WatchFilesClient } from "@rilldata/web-common/features/entity-management/WatchFilesClient"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Import statements for 'WatchFilesClient' and 'WatchResourcesClient' use incorrect paths or naming conventions.
Fix: Correct the import statements to match the actual file names and paths in the project structure.
Code Suggestion:
- import { WatchFilesClient } from "@rilldata/web-common/features/entity-management/WatchFilesClient";
- import { WatchResourcesClient } from "@rilldata/web-common/features/entity-management/WatchResourcesClient";
+ import { WatchFilesClient } from "@rilldata/web-common/features/entity-management/watch-files-client";
+ import { WatchResourcesClient } from "@rilldata/web-common/features/entity-management/watch-resources-client";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be incorrect, did it not catch the file name rename? The file name should match the class name with camel case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some great cleanup in here!
public static New() { | ||
const watcher = new WatchFilesClient(); | ||
return watcher.client; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unconventional in Javascript. Can we just use the constructor to instantiate a new instance?
private invalidateAllFiles() { | ||
// TODO: reset project parser errors | ||
const instanceId = get(runtime).instanceId; | ||
return queryClient.resetQueries({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we move from resetQueries
to refetchQueries
everywhere (but if that's risky, OK to keep out-of-scope of this PR). resetQueries
typically leads to flashing.
const instanceId = get(runtime).instanceId; | ||
const isNew = !this.seenFiles.has(res.path); | ||
|
||
console.log(res.path, isNew); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover console.log
public static New() { | ||
const watcher = new WatchResourcesClient(); | ||
return watcher.client; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this seems unconventional in Javascript
export function refreshResource( | ||
queryClient: QueryClient, | ||
instanceId: string, | ||
res: V1Resource, | ||
) { | ||
return queryClient.resetQueries( | ||
return queryClient.setQueryData( | ||
getRuntimeServiceGetResourceQueryKey(instanceId, { | ||
"name.name": res.meta?.name?.name, | ||
"name.kind": res.meta?.name?.kind, | ||
}), | ||
{ | ||
resource: res, | ||
}, | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this function is just a wrapper around setQueryData
, can we remove the wrapper and use setQueryData
directly? Else it requires a reader to mentally map "refreshResource" -> setQueryData
, which seems unnecessary.
} | ||
|
||
public async waitForResource(name: string) { | ||
return waitUntil(() => this.statuses.get(name) === "RECONCILE_STATUS_IDLE"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our e2e tests should rely on user-visible indicators, not on internal state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I am iterating on this. That part is still TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I am not sure if any user-visible indicators was reliable here. That is why we had waitForResource
earlier.
* Fetches all resources and filters them client side. | ||
* This is to improve network requests since we need the full list all the time as well. | ||
*/ | ||
export function useClientFilteredResources( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
return watcher.client; | ||
} | ||
|
||
private handleWatchResourceResponse(res: V1WatchResourcesResponse) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this code co-location!
Hey sorry this is not ready for review. Hence the bunch of console logs. Sorry if i gave the wrong impression. Also the AI seems wrong most of the time. It feels like spam most of the time. |
6468a84
to
26d5cea
Compare
We have multiple redundant network calls in rill developer.
kind
in addition to the ListResource with no params. We can filter from the full list instead. (Dashboard list is pending since it used in cloud where we might not always make a call to list resources)This also attempts to move any metadata needed for these watch clients to those files instead of the global store FileArtifacts.