Feedback inspector in Assistant builder #9680

overmode · 2024-12-31T15:46:23Z

Description

Add a bubble Performance tab to the assistant builder to inspect feedbacks

Screen.Recording.2025-01-08.at.12.45.20.mov

Risk

Break assistant builder

Deploy Plan

Deploy Front

front/lib/resources/agent_message_feedback_resource.ts

overmode · 2025-01-02T14:11:40Z

front/components/assistant_builder/AssistantBuilderPreviewDrawer.tsx

+      if (!config.versionCreatedAt) {
+        return `v${config.version}`;
+      }
+      return new Date(config.versionCreatedAt).toLocaleDateString("en-US", {


I use a date instead of timeAgo because the iterations might be close in time, and you still want to easily distinguish them.
This is consistent with the date given in the prompt versioning Popover

overmode · 2025-01-02T14:35:48Z

front/components/assistant_builder/AssistantBuilder.tsx

@@ -565,6 +569,7 @@ export default function AssistantBuilder({
              rightPanelStatus={rightPanelStatus}
              openRightPanelTab={openRightPanelTab}
              builderState={builderState}
+              agentConfigurationId={agentConfigurationId}


passing Id instead of LightAgentConfigurationType because a larger refactoring is needed (wouldn't be consistent with submitAssistantBuilderForm, SharingButton, InstructionScreen, useSlackChannel, ...)

overmode · 2025-01-02T14:46:42Z

front/components/assistant_builder/AssistantBuilderPreviewDrawer.tsx

-          shouldAnimatePreviewDrawer &&
-            rightPanelStatus.tab === "Preview" &&
-            rightPanelStatus.openedAt != null &&
+        className={cn(


This keeps the same display except with a white background for the feedback tab

overmode · 2025-01-02T14:55:44Z

front/components/assistant_builder/AssistantBuilderPreviewDrawer.tsx

+              )}
+              {agentConfigurationId && (
+                <TabsTrigger
+                  value="Performance"


Analytics will be added to this tab later -> Performance is a sensible name

overmode · 2025-01-02T15:04:13Z

front/components/assistant_builder/AssistantBuilderPreviewDrawer.tsx

+    feedback.conversationId &&
+    feedback.messageId &&
+    feedback.isConversationShared
+      ? `${process.env.NEXT_PUBLIC_DUST_CLIENT_FACING_URL}/w/${owner.sId}/assistant/${feedback.conversationId}#${feedback.messageId}`


We add a messageId anchor, because another PR will make it possible to auto-scroll to the message.

IMHO this should be returned by the endpoint directly since this is very brittle the day we decide to change the endpoint, pretty sure we won't find this occurrence.

You mean getting the link from the endpoint ?
I don't see how it would be easier to spot in the endpoint vs in this file, given that the endpoint is not the conversation one.

given that the endpoint is not the conversation one.
Not sure to get this one 🤔

overmode · 2025-01-02T15:07:50Z

front/components/assistant_builder/PrevNextButtons.tsx

@@ -20,7 +20,7 @@ export function PrevNextButtons({
        <Button
          label="Previous"
          size="md"
-          variant="highlight"
+          variant="primary"


Makes buttons black -> Asked from design team

overmode · 2025-01-02T15:08:32Z

front/lib/api/assistant/feedback.ts

+        | AgentMessageFeedbackType
+        | AgentMessageFeedbackWithMetadataType
+      )[];
+      totalFeedbackCount: number;


The count is needed, because pagination relies on it

overmode · 2025-01-02T15:27:01Z

front/lib/resources/agent_message_feedback_resource.ts

    };

+    // Get the total feedback count, because needed for pagination
+    const totalFeedbackCountPromise = AgentMessageFeedback.count({


This hits the highest number of feedbacks

explain analyze SELECT count(*) AS "count" FROM "agent_message_feedbacks" AS "agent_message_feedback" WHERE "agent_message_feedback"."workspaceId" = 44603 AND "agent_message_feedback"."agentConfigurationId" = 'claude-3-sonnet';

Aggregate (cost=40.55..40.56 rows=1 width=8) (actual time=0.281..0.281 rows=1 loops=1) -> Seq Scan on agent_message_feedbacks agent_message_feedback (cost=0.00..39.62 rows=372 width=0) (actual time=0.012..0.249 rows=628 loops=1) Filter: (("workspaceId" = 44603) AND (("agentConfigurationId")::text = 'claude-3-sonnet'::text)) Rows Removed by Filter: 640 Planning Time: 0.097 ms Execution Time: 0.307 ms

overmode · 2025-01-02T15:29:12Z

front/components/assistant_builder/AssistantBuilderPreviewDrawer.tsx

+          }
+          className="hidden lg:flex"
+        >
+          <TabsList className="inline-flex items-center gap-2 border-b border-separator">


Removed the h-10 because it was producing a vertical scroll bar. The rest is unchanged.

front/components/assistant_builder/AssistantBuilder.tsx

flvndvd · 2025-01-02T16:16:38Z

front/components/assistant_builder/AssistantBuilderPreviewDrawer.tsx

-            rightPanelStatus.openedAt != null &&
+        className={cn(
+          "grow-1 mb-5 h-full overflow-y-auto border-structure-200",
+          {


IMHO it becomes pretty hard to figure the style by looking at this code...

I agree that it's complex, I tried not to touch the design, imho we already had a good bunch of complexity and this syntax is easier to read

front/components/assistant_builder/AssistantBuilderPreviewDrawer.tsx

flvndvd · 2025-01-02T16:24:02Z

front/lib/resources/agent_message_feedback_resource.ts

+      | AgentMessageFeedbackWithMetadataType
+    )[];
+    totalFeedbackCount: number;
+  }> {
    const where: WhereOptions<AgentMessageFeedback> = {
      // IMPORTANT: Necessary for global models who share ids across workspaces.


I thought that global agent could not get feedback?

They used to (at the time I wrote this comment), and an extra layer of security doesn't hurt I guess ?

I guess this layer should happened before hitting our DB since this does not require any DB interaction and can be disregard earlier.

front/lib/resources/agent_message_feedback_resource.ts

flvndvd · 2025-01-02T16:27:07Z

front/lib/resources/agent_message_feedback_resource.ts

@@ -91,35 +92,42 @@ export class AgentMessageFeedbackResource extends BaseResource<AgentMessageFeedb
    });
  }

-  static async fetch({
+  static async getAgentConfigurationFeedbacksByDescVersion({


This goes against the Resource pattern approach. All those functions should actually return a Resource not an object. Then if you need. serialized object you should use a toJSON. Any reason to not follow this pattern?

Yes,
The main obstacle is that you need to do several includes (on AgentMessage, Message and Conversation) which prevents you from only getting a resource back, and associating a toJSON to only a feedback resource.
I got this hint from @Fraggle https://dust4ai.slack.com/archives/C050SM8NSPK/p1734706622967029?thread_ts=1734705178.946699&cid=C050SM8NSPK

Ok did not get this info. OOC why don't we use class variable to have those on the resource like we do for many other resources?

Example with DataSourceViewResource here:

dust/front/lib/resources/data_source_view_resource.ts

Line 73 in 5cb6462

readonly editedByUser?: Attributes<UserModel>;

Agree that the returned object is too complex is that we should try to return resources instead.

Will spend some time with @overmode today to align the pattern.

Please share your conclusions, i'm interested :)

We ended up creating an attribute in the resource class to allow easy access to related objects. My main takeaways are that we should indeed refrain from returning anything else than a resource in a _resource file, that toJSON should be kept simple even if that means adding pose-serialize logic to /lib/api files, and that sequelize is really bad a typing.

front/lib/swr/assistants.ts

tdraier · 2025-01-07T08:02:34Z

front/lib/resources/agent_message_feedback_resource.ts

@@ -91,35 +92,42 @@ export class AgentMessageFeedbackResource extends BaseResource<AgentMessageFeedb
    });
  }

-  static async fetch({
+  static async getAgentConfigurationFeedbacksByDescVersion({


Agree that the returned object is too complex is that we should try to return resources instead.

tdraier · 2025-01-07T08:10:51Z

front/lib/swr/assistants.ts

+  const { data, error, mutate, size, setSize, isLoading, isValidating } =
+    useSWRInfiniteWithDefaults(
+      (pageIndex: number, previousPageData) => {
+        console.log("previousPageData", previousPageData);


flvndvd · 2025-01-07T15:20:16Z

front/components/assistant_builder/AssistantBuilderPreviewDrawerFeedbacks.tsx

+  });
+
+  // Intersection observer to detect when the user has scrolled to the bottom of the list.
+  const bottomRef = useRef<HTMLDivElement>(null);


Any reason to use the existing hook useLastMessageGroupObserver (ok naming is not generic, but the code is!)

I would need to refactor the version to not accept message but a generic entity (because I'm observing over feedbacks, not messages), and accept the id as a parameter (to avoid conflicts).
2nd point is fine but 1st one is painful, you don't want to have a 'any' type even though that's ~the idea.

One more type, we could leverage duck typing to ensure that we accept all entities that have at least id set 🙃.

As seen irl the go-to type is unknown

flvndvd · 2025-01-07T15:21:23Z

front/components/assistant_builder/AssistantBuilderPreviewDrawerFeedbacks.tsx

+              agentConfigurationFeedbacks[index - 1].agentConfigurationVersion;
+          return (
+            <div key={feedback.id} className="animate-fadeIn">
+              {isNewVersion && (


Hard to understand how isNewVersion influences isLatestVersion, can we align naming?

flvndvd · 2025-01-07T15:21:59Z

front/lib/resources/agent_message_feedback_resource.ts

@@ -91,35 +92,42 @@ export class AgentMessageFeedbackResource extends BaseResource<AgentMessageFeedb
    });
  }

-  static async fetch({
+  static async getAgentConfigurationFeedbacksByDescVersion({


flvndvd · 2025-01-07T15:23:24Z

front/lib/swr/assistants.ts

+        // If we have reached the last page and there are no more
+        // messages or the previous page has no messages, return null.
+        if (previousPageData && previousPageData.feedbacks.length < limit) {
+          setHasMore(false);


Why do we expose hasMore? IIRC, returning null here will stop the fetch like it's being done in the message.

it is used to avoid calling setSize for ever, we also have a variation of it in messages https://github.com/dust-tt/dust/blob/lucas/feedbacks-inspector/front/components/assistant/conversation/ConversationViewer.tsx#L203

flvndvd · 2025-01-08T09:26:54Z

front/components/assistant_builder/AssistantBuilderPreviewDrawerFeedbacks.tsx

+    isAgentConfigurationFeedbacksLoading,
+    isValidating,
+    agentConfigurationFeedbacks,
+    hasMore: feedbacksNotExhausted,


nit
Pretty confusing naming, we are loosing the boolean meaning.

flvndvd · 2025-01-08T09:27:30Z

front/components/assistant_builder/AssistantBuilderPreviewDrawerFeedbacks.tsx

+        <AgentConfigurationVersionHeader
+          agentConfiguration={agentConfigurationHistory[0]}
+          agentConfigurationVersion={agentConfigurationHistory[0].version}
+          isLatestVersion={true}


nit

Suggested change

isLatestVersion={true}

isLatestVersion

explicitly setting to true is used widely across the codebase (59 files), I have no easy way to look for the other method to compare, but seems like I'm keeping this one. Cool syntax trick though 👍

…, more loaded at once

…er.tsx Co-authored-by: Flavien David <[email protected]>

overmode changed the title ~~Main code brought in~~ Feedback inspector in Assistant builder Dec 31, 2024

overmode commented Jan 2, 2025

View reviewed changes

front/lib/resources/agent_message_feedback_resource.ts Outdated Show resolved Hide resolved

overmode commented Jan 2, 2025

View reviewed changes

overmode marked this pull request as ready for review January 2, 2025 15:30

overmode requested a review from flvndvd January 2, 2025 15:58

flvndvd reviewed Jan 2, 2025

View reviewed changes

overmode requested a review from flvndvd January 3, 2025 09:58

overmode force-pushed the lucas/feedbacks-inspector branch from 8090417 to 4c61121 Compare January 3, 2025 15:29

tdraier reviewed Jan 7, 2025

View reviewed changes

overmode force-pushed the lucas/feedbacks-inspector branch 2 times, most recently from 3aa78fe to 5cd4c98 Compare January 7, 2025 14:48

flvndvd reviewed Jan 7, 2025

View reviewed changes

flvndvd reviewed Jan 8, 2025

View reviewed changes

overmode requested a review from flvndvd January 8, 2025 10:14

flvndvd approved these changes Jan 8, 2025

View reviewed changes

overmode and others added 7 commits January 8, 2025 13:59

Main code brought in

acc8fbe

Improved design for feedback list. Bug with ordering

c29d504

Specialized fetch function (renamed), small margin added to feedbacks…

6ff05c2

…, more loaded at once

Using pagination inside feedbacks side modal

937ac19

Renamed batch to page, minor improvements

f850b2e

Update front/components/assistant_builder/AssistantBuilderPreviewDraw…

8187de5

…er.tsx Co-authored-by: Flavien David <[email protected]>

Moved feedback code in its own file, added some props, regular cn usage

a4f195f

overmode added 9 commits January 8, 2025 13:59

UseSWRInfinity and IntersectionObserver

858e989

Modified message anchor to be a hash param

6f1dee6

linter

d3c7e34

Now returning resource in static resource function

fa7baf8

Using a query param

74a826d

Renamed variable

117157b

bask to hasMore variable name

ae1c980

useInView hook

4213802

Fixed drawer styleing (and icon)

e693865

overmode force-pushed the lucas/feedbacks-inspector branch from 44edbfe to e693865 Compare January 8, 2025 12:59

overmode merged commit 6f0be19 into main Jan 8, 2025
6 of 7 checks passed

overmode deleted the lucas/feedbacks-inspector branch January 8, 2025 14:07

Feedback inspector in Assistant builder #9680

Feedback inspector in Assistant builder #9680

Conversation

overmode commented Dec 31, 2024 • edited Loading

Description

Risk

Deploy Plan

Choose a reason for hiding this comment

overmode Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overmode Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overmode Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

overmode Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overmode Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overmode commented Dec 31, 2024 •

edited

Loading

overmode Jan 2, 2025 •

edited

Loading

overmode Jan 2, 2025 •

edited

Loading

overmode Jan 2, 2025 •

edited

Loading

overmode Jan 2, 2025 •

edited

Loading

overmode Jan 7, 2025 •

edited

Loading