Improve workspace fetch with member param #182
Conversation
…improvement # Conflicts: # packages/agent-toolkit/src/core/tools/platform-api-tools/list-workspace-tool/list-workspace-tool.test.ts # packages/agent-toolkit/src/core/tools/platform-api-tools/list-workspace-tool/list-workspace-tool.ts # packages/agent-toolkit/src/monday-graphql/schema.graphql
shaneer
requested review from
RomKadria and
damian-rakus
and removed request for
RomKadria
packages/agent-toolkit/src/core/tools/platform-api-tools/base-monday-api-tool.ts
Outdated
Show resolved
Hide resolved
...s/agent-toolkit/src/core/tools/platform-api-tools/list-workspace-tool/list-workspace-tool.ts
Outdated
Show resolved
Hide resolved
...s/agent-toolkit/src/core/tools/platform-api-tools/list-workspace-tool/list-workspace-tool.ts
Outdated
Show resolved
Hide resolved
...s/agent-toolkit/src/core/tools/platform-api-tools/list-workspace-tool/list-workspace-tool.ts
Outdated
Show resolved
Hide resolved
| let workspaces = filterNullWorkspaces(res); | ||
| let usedMemberOnly = true; | ||
|
|
||
| // If searching with a term and no matches found in member workspaces, try with all workspaces |
There was a problem hiding this comment.
Hey Shanee, can we include the comment that we don't allow "LLM filtering" at this point as it could cause the MCP to never search for all workspaces?
Cause initially I was surprised why we don't return those workspaces if the length is smaller than DEFAULT_WORKSPACE_LIMIT and comment here would be helpful for other devs
…orkspaces if no member workspaes are found
There was a problem hiding this comment.
tests should sit in the same folder as the array util itselfs, so util > array-util (folder)-> logic + test
There was a problem hiding this comment.
not the current practice in the util file
There was a problem hiding this comment.
unfortunate, this is bad practice as its hard to see logic and test because of that
| .default(DEFAULT_WORKSPACE_LIMIT) | ||
| .describe(`The number of workspaces to return. Default and maximum allowed is ${DEFAULT_WORKSPACE_LIMIT}`), | ||
| page: z.number().min(1).default(1).describe('The page number to return. Default is 1.'), | ||
| .describe(`Number of workspaces to return. Default and max allowed is ${DEFAULT_WORKSPACE_LIMIT}`), |
There was a problem hiding this comment.
in general, why would we want the max to be the default, gives no insentive to use something else xD
| }); | ||
|
|
||
| // First, try to get workspaces where the user is a member (more relevant results) | ||
| let res = await this.mondayApi.request<ListWorkspacesQueryResponse>( |
There was a problem hiding this comment.
its funny to me we are trying t create search logic here 😂
I wonder how good does it work, we dont have metric on the misses which is unfortunate here
if we have a lot of misses we just make doulbe api queries...
There was a problem hiding this comment.
Once the MCP is internal we can add logs for things like this :)
For now I guess our only option would be thru BB events
We can examine in BB or in your tracing tool how many times one user calls this tools multiple times right after the other
There was a problem hiding this comment.
its not about one tool one after the other, as this is two gql calls in the same tool call. we can trace it today I think anyway and check it but yeah looks...
https://monday.monday.com/boards/8222767873/views/175916623/pulses/10906328723
TLDR;
Using the new membershipKind enum to fetch first only the workspaces that I am a member of and then falling back to all if not found
Additional Small Changes