chore(ui): add feedback_replace to ui, update listeners w/ feedback_type
#2804
Conversation
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=72873893e98fe38f4508d3b2b65bddab746a255a |
.../src/components/PagePanelComponents/Home/Browse3/pages/wfReactInterface/traceServerClient.ts
Outdated
Show resolved
Hide resolved
gtarpenning
changed the title
feedback_refannotation_ref
| @@ -23,7 +25,13 @@ export class TraceServerClient extends DirectTraceServerClient { | |||
| }> = []; | |||
| private onDeleteListeners: Array<() => void>; | |||
| private onRenameListeners: Array<() => void>; | |||
| private onFeedbackListeners: Record<string, Array<() => void>>; | |||
| // weave_ref -> feedback_ref -> callback | |||
There was a problem hiding this comment.
not sure what this means
There was a problem hiding this comment.
oh, you are talking about the keys of the nested structure
gtarpenning
changed the title
annotation_reffeedback_type
| @@ -94,7 +109,10 @@ export class TraceServerClient extends DirectTraceServerClient { | |||
|
|
|||
| public feedbackCreate(req: FeedbackCreateReq): Promise<FeedbackCreateRes> { | |||
| const res = super.feedbackCreate(req).then(createRes => { | |||
| const listeners = this.onFeedbackListeners[req.weave_ref] ?? []; | |||
| const feedbackTypeResolved = | |||
| req.feedback_type ?? this.FEEDBACK_TYPE_DEFAULT; | |||
There was a problem hiding this comment.
i think feedback type is always required?
There was a problem hiding this comment.
Yeah it is, unfortunately because it always exists this now requires changes to the existing feedback listening stuff, working through that now
| } | ||
| return purgeRes; | ||
| }); | ||
| return res; | ||
| } | ||
|
|
||
| public feedbackReplace(req: FeedbackReplaceReq): Promise<FeedbackReplaceRes> { | ||
| const res = super.feedbackReplace(req).then(replaceRes => { | ||
| const feedbackTypeResolved = |
There was a problem hiding this comment.
same here - always required, no?
| this.onFeedbackListeners[feedbackTypeResolved] = {}; | ||
| } | ||
| if (!(weaveRef in this.onFeedbackListeners[feedbackTypeResolved])) { | ||
| this.onFeedbackListeners[feedbackTypeResolved][weaveRef] = []; |
There was a problem hiding this comment.
your comment says it is going to be // weave_ref -> feedback_type -> callback. i think that comment is better than the reverse
| const listeners = this.onFeedbackListeners[req.weave_ref] ?? []; | ||
| const feedbackTypeResolved = | ||
| req.feedback_type ?? this.FEEDBACK_TYPE_DEFAULT; | ||
| const listeners = |
There was a problem hiding this comment.
for here and in replace, i think what you want to hit all the listeners for weave_ref + feedback_type AND weave_ref + ''.
gtarpenning
force-pushed
the
griffin/feedback-replace-ts
branch
from
04ded0c to
005a165
Compare
Description
WB-21662
Add
feedback_replaceendpoint to the frontend code. Also, update the feedback listener functions to also account forfeedback_types. By default, the functionality should be the same, feedback without afeedback_typein thepayloadshould requery on mutation. However, now when registering a listener, you can pass an optionalfeedback_typewhich will then only refresh when mutations affect feedback with thatfeedback_typein the payload.Testing
How was this PR tested?