Skip to content
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

[Assistant views] Remove 'assistant-search' & '{conversationId}' views #8109

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

philipperolet
Copy link
Contributor

@philipperolet philipperolet commented Oct 19, 2024

Description

  • Removes 'assistant-search' view, which was actually exactly the same as "list". The comment indicating inactive global agents were returned was erroneous

  • Removes '{conversationId}' view, since:

    • it was actually only used in AssistantSuggestion, not anywhere else;
    • the view didn't actually work, it was ~equivalent to list;
    • the use case it was supposed to enable, viewing a shared conversation with somebody else's personal assistant, is already somewhat working: user can view conversation but not mention assistant or see its details which is kind of fine.

We could aim in a follow-up PR to reinstate the conversation view cleanly; that would require more work and is IMO low value and not a priority.

Risks

Low, since it's just a view change. Tested locally

Regarding the conversationId removal specifically:

  • handled all explicit calls with conversationId
  • looked for agentsGetView with potential implicit agentsGetView: { conversationId }: there are
    • the internal api asisstant/agent_configurations, which is only called via swr's useAgentConfigurations
      => checked it does not use conversationId (since it also takes agentsGetView as param)
    • the v1 api DustApi.getAgentsConfiguration call => here also, conversationId not used

Deploy

front

Copy link

github-actions bot commented Oct 19, 2024

Warnings
⚠️

Files in **/api/v1/ have been modified and the PR has the documentation-ack label.
Don't forget to run npm run docs and use the Deploy OpenAPI Docs Github action to update https://docs.dust.tt/reference.

Generated by 🚫 dangerJS against 6b14597

Description
---
- Removes 'assistant-search' view, which was actually exactly the same
as "list". The comment indicating inactive global agents were returned
was erroneous

- Removes '{conversationId}' view, since:
 - it was actually only used in AssistantSuggestion, not anywhere else;
 - the view didn't actually work, it was ~equivalent to list;
 - the use case it was supposed to enable, viewing a shared
 conversation with somebody else's personal assistant, is already
somewhat working: user can view conversation but not mention assistant
or see its details which is kind of fine.

We could aim in a follow-up PR to reinstate the conversation view
cleanly; that would require more work and is IMO not a priority.

Risks
---
Low, since it's just a view change. Tested locally. Regarding conversationId:
- handled all explicit calls
- looked for agentsGetView with potential implicit `agentsGetView: { conversationId }`: there are
  - the internal api `asisstant/agent_configurations`, which is only called via swr's `useAgentConfigurations`
	 => checked it does not use conversationId (since it also takes agentsGetView as param)
  - the v1 api DustApi.getAgentsConfiguration call => here also, conversationId not used

Deploy
---
front
@philipperolet philipperolet added the documentation-ack Used to acknowledge that documentation is up-to-date with this PR label Oct 21, 2024
@philipperolet philipperolet requested a review from spolu October 21, 2024 20:24
Copy link
Contributor

@spolu spolu left a comment

Choose a reason for hiding this comment

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

LGTM

A lint run related with cacheWithRedis is visible here. Since you touched it recently maybe we can add a comment to disable it or use unknown?

@philipperolet
Copy link
Contributor Author

LGTM

A lint run related with cacheWithRedis is visible here. Since you touched it recently maybe we can add a comment to disable it or use unknown?

Done

@philipperolet philipperolet merged commit 84b3cae into main Oct 22, 2024
7 checks passed
@philipperolet philipperolet deleted the clean-views branch October 22, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-ack Used to acknowledge that documentation is up-to-date with this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants