prompt activation frontend #12626
prompt activation frontend #12626
Conversation
emilia-friedberg
requested review from
eadams17,
brendanshean and
anathomical
| @@ -61,7 +61,7 @@ export const Checkbox = ({ label, mode=LIGHT, onClick, state, selected, id, }: c | |||
| return ( | |||
| <div className="checkbox-container"> | |||
| {renderCheckbox()} | |||
| <label className={labelClass} htmlFor={checkboxId}>{label}</label> | |||
| {label ? <label className={labelClass} htmlFor={checkboxId}>{label}</label> : null} | |||
There was a problem hiding this comment.
This is purely stylistic, and I don't even know if it's better, but thoughts on using
{label && <label className={labelClass} htmlFor={checkboxId}>{label}</label>}instead of a ternary? That may not actually do exactly the same thing, but I feel like I've seen us use that pattern in a couple of places, and I personally find it slightly more readable. But, again, it's just a style thing, so not a big deal either way.
| expect(screen.getByText(/Prompt Name/i)).toBeInTheDocument(); | ||
| expect(screen.getByText(/Dataset 1.0/i)).toBeInTheDocument(); |
There was a problem hiding this comment.
Are these values hard-coded into the template, or are they based on some of the data defined in activated_llm_prompts? If they're hard-coded, leaving this as-is makes sense, but if they're based on values in the input object, I think it would be clearer to have them referenced by variable name. Such as
expect(screen.getByText(/${activatedPromptsResponse. activated_llm_prompts[0]. trial.llm_prompt.name}/i)).toBeInTheDocument();Or maybe something more readable, but something that makes it clear that the input should impact the output.
| @@ -84,6 +84,12 @@ def active_llm_prompt | |||
| .first | |||
| end | |||
|
|
|||
| def active_activated_llm_prompt | |||
| activated_llm_prompts | |||
| .merge(Evidence::ActivatedLLMPrompt.active) | |||
There was a problem hiding this comment.
I like the use of merge here to apply a scope.
| end | ||
|
|
||
| it 'includes associated trial data' do | ||
| trial = create(:evidence_research_gen_ai_trial, llm_prompt:) |
There was a problem hiding this comment.
This is purely stylistic, so take or leave it. I prefer to use let or let! whenever a create is called, but you can't call let inside an it block, so I'd structure this with a context wrapper.
context 'with associated trial data' do
let!(:trial) { create(:evidence_research_gen_ai_trial, llm_prompt:) }
it 'includes associated trial data' do
... # same code
end
end|
|
||
| it 'includes associated trial data' do | ||
| trial = create(:evidence_research_gen_ai_trial, llm_prompt:) | ||
| activated_llm_prompt.update!(llm_prompt:) |
There was a problem hiding this comment.
Does this doe anything? The create call on line 24 looks like it already sets llm_prompt to the same value as this.
| end | ||
|
|
||
| it 'sets other prompts to inactive if active is true' do | ||
| existing_prompt = create(:evidence_activated_llm_prompt, llm_prompt:, prompt:, active: true) |
There was a problem hiding this comment.
Again, take or leave style thing, but I like create calls in let which means you'd need a context wrapper.
| end | ||
| end | ||
|
|
||
| def prompt |
There was a problem hiding this comment.
I would make this method private since I don't think we need it exposed outside the controller.. In general, I try to make the only public methods in a controller correspond to specified routes.
| attribute: 'llmPromptName', | ||
| width: '72px', | ||
| headerClassName: 'center-content', | ||
| rowSectionClassName: 'center-content allow-wrap', |
There was a problem hiding this comment.
Just an idea... I think at some point down the road, we should probably update Backpack to include these different class instances so we have them all documented somewhere
WHAT
Build out the frontend and controller endpoints to make it possible to activate a prompt.
WHY
This is what will allow the curriculum team to pick which LLM Prompt actually gets used to serve feedback.
HOW
Pretty straightforward React and Rails code! We also changed the styling of some other components to make room for the new columns.
Screenshots
Notion Card Links
https://www.notion.so/quill/Activate-A-Winning-Prompt-146d42e6f941806dabc3e98fae6fadf2?pvs=4
What have you done to QA this feature?
Tested locally and on staging; Jamie will also be QAing on staging.