[ui] Update Create Optimized Prompt screen UI feedback#1043
Closed
sfierro wants to merge 4 commits into
Closed
Conversation
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughAdds a filesystem entry file and enhances two UI components: InfoTooltip gains a wrapper rendering mode and improved newline/spacing handling; the prompt optimization job creation page receives numerous UI and interaction refinements for run configurations and evaluator selection. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Summary of ChangesHello @sfierro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant new 'Prompt Optimization' capability, enabling automated prompt refinement through a dedicated workflow. It consolidates related AI-driven features under a new 'Optimize' navigation entry, enhancing user experience by providing clearer guidance and streamlined access to advanced functionalities. The changes span across backend API definitions, datamodels, and extensive frontend UI updates to support the new feature and improve existing components like model browsing and run configuration management. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request effectively updates the UI for the "Create Optimized Prompt" screen based on user feedback, improving clarity and usability. The changes to subtitles, tooltips, and warning messages are well-implemented. I've provided a few suggestions to enhance security and maintainability by addressing a missing rel attribute on a link, replacing an inline style with a Tailwind class, and recommending a file be added to .gitignore.
- Add wrapper mode to InfoTooltip for custom content - Remove gray styling from unselected rows - Enable row selection by clicking anywhere in the row - Replace "View Eval" button with TableButton dropdown - Replace inline error displays with tooltip on "Not Ready" badge - Fix eval_configs link to include spec_id and eval_id - Add proper error messages with links to relevant pages Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📊 Coverage ReportOverall Coverage: 91% Diff: origin/sfierro/optimize-feature...HEADNo lines with coverage information in this diff.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/web_ui/src/routes/(app)/prompt_optimization/[project_id]/[task_id]/create_prompt_optimization_job/+page.svelte (2)
997-1032: Rowon:clickand checkboxon:changeboth fire on checkbox click — two redundant reactive updates.When a checkbox is clicked, the
<tr>'son:clickfires (usingis_selectedto toggle) and then the checkbox'son:changefires (usinge.currentTarget.checkedto set). The final state is correct in all cases, butselected_eval_idsis reassigned twice per checkbox click. Addingon:click|stopPropagationto the checkbox<td>eliminates the redundant update and makes the intent clearer.♻️ Proposed refactor
- <td> + <td on:click|stopPropagation> <input type="checkbox"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompt_optimization/[project_id]/[task_id]/create_prompt_optimization_job/+page.svelte around lines 997 - 1032, The table row and the checkbox both update selected_eval_ids when the checkbox is clicked, causing two reactive updates; stop the row's on:click from firing when interacting with the checkbox by adding Svelte event modifier stopPropagation to the checkbox container (e.g., add on:click|stopPropagation on the <td> that wraps the <input> or directly on the <input>), so clicks on the checkbox only trigger the checkbox's on:change handler and not the <tr> on:click; keep existing logic that checks evalItem.id and validation_status and only modify event propagation.
1139-1156:{#ifeval_url}guard is always true; consider guarding onevalItem.idinstead.
eval_urlis constructed as a template literal withspec_id = "legacy"(always set), so it's always a non-empty truthy string even whenevalItem.idisundefined(it would produce.../legacy/undefined). The meaningful guard isevalItem.id:♻️ Proposed fix
- {`#if` eval_url} + {`#if` evalItem.id}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompt_optimization/[project_id]/[task_id]/create_prompt_optimization_job/+page.svelte around lines 1139 - 1156, The current {`#if` eval_url} check is always truthy because eval_url is built with a static spec_id and can contain "undefined"; change the conditional to check the real identifier (e.g., {`#if` evalItem?.id} or {`#if` evalItem && evalItem.id}) so the dropdown (around TableButton and the <ul> anchor) only renders when evalItem.id exists, and ensure any construction of eval_url happens only when evalItem.id is present (compute eval_url lazily or inside that guarded block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude:
- Line 1: Remove the accidentally committed local Claude config file named
".claude" from the repo and prevent it from being re-added: delete the file from
the repository history/working tree (e.g., git rm or remove and commit) and add
an entry for ".claude" to .gitignore so the local path /Users/samfierro/.claude
is not tracked in future commits; include a short commit message like "Remove
local Claude config and add to .gitignore" and ensure the removal is pushed.
In `@app/web_ui/src/lib/ui/info_tooltip.svelte`:
- Around line 118-131: The wrapper-mode trigger div incorrectly has
role="tooltip" and is not keyboard-focusable; remove the role attribute from the
trigger (leave role="tooltip" on the floating popup element), make the trigger
focusable by adding tabindex="0" to the div bound to triggerElement, and wire an
accessible relationship by adding aria-describedby referencing the tooltip
element's id (assign an id to the popup div and set aria-describedby on the
trigger once IDs are available); keep existing handlers showTooltip/hideTooltip
and focus/blur logic.
In
`@app/web_ui/src/routes/`(app)/prompt_optimization/[project_id]/[task_id]/create_prompt_optimization_job/+page.svelte:
- Around line 1089-1121: The tooltip can render "undefined" because
tagFromFilterId("") returns undefined so train_tag is falsy and dataset_add_link
becomes undefined; update the logic around tagFromFilterId, train_tag,
dataset_add_link, tooltip_parts and combined_tooltip so the training-set message
is only added when train_tag is truthy: compute train_tag =
tagFromFilterId(evalItem.train_set_filter_id || "") and dataset_add_link only if
train_tag; in tooltip_parts push the training message only when train_error
indicates missing training data AND train_tag is defined (use a conditional that
references train_tag), and when inserting the tag or link interpolate only the
confirmed train_tag/dataset_add_link values to avoid rendering 'undefined'.
---
Nitpick comments:
In
`@app/web_ui/src/routes/`(app)/prompt_optimization/[project_id]/[task_id]/create_prompt_optimization_job/+page.svelte:
- Around line 997-1032: The table row and the checkbox both update
selected_eval_ids when the checkbox is clicked, causing two reactive updates;
stop the row's on:click from firing when interacting with the checkbox by adding
Svelte event modifier stopPropagation to the checkbox container (e.g., add
on:click|stopPropagation on the <td> that wraps the <input> or directly on the
<input>), so clicks on the checkbox only trigger the checkbox's on:change
handler and not the <tr> on:click; keep existing logic that checks evalItem.id
and validation_status and only modify event propagation.
- Around line 1139-1156: The current {`#if` eval_url} check is always truthy
because eval_url is built with a static spec_id and can contain "undefined";
change the conditional to check the real identifier (e.g., {`#if` evalItem?.id} or
{`#if` evalItem && evalItem.id}) so the dropdown (around TableButton and the <ul>
anchor) only renders when evalItem.id exists, and ensure any construction of
eval_url happens only when evalItem.id is present (compute eval_url lazily or
inside that guarded block).
| @@ -0,0 +1 @@ | |||
| /Users/samfierro/.claude No newline at end of file | |||
Contributor
There was a problem hiding this comment.
Remove accidentally committed local Claude config file.
This file contains /Users/samfierro/.claude — a local developer machine path that appears to be Claude AI's local settings/memory file. It has no place in the repository and leaks filesystem layout.
Add it to .gitignore and remove it from the tree.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude at line 1, Remove the accidentally committed local Claude config
file named ".claude" from the repo and prevent it from being re-added: delete
the file from the repository history/working tree (e.g., git rm or remove and
commit) and add an entry for ".claude" to .gitignore so the local path
/Users/samfierro/.claude is not tracked in future commits; include a short
commit message like "Remove local Claude config and add to .gitignore" and
ensure the removal is pushed.
Comment on lines
+118
to
+131
| {#if wrapper} | ||
| <!-- Wrapper mode: wraps custom content --> | ||
| <div | ||
| bind:this={triggerElement} | ||
| on:mouseenter={showTooltip} | ||
| on:mouseleave={hideTooltip} | ||
| on:focus={showTooltip} | ||
| on:blur={hideTooltip} | ||
| class="inline-block" | ||
| aria-label="Tooltip" | ||
| role="tooltip" | ||
| > | ||
| <slot /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Incorrect role="tooltip" on the wrapper trigger div; keyboard focus inaccessible.
Two issues with the wrapper-mode <div>:
-
Wrong ARIA role —
role="tooltip"is reserved for the popup element (the floating<div>at line 165 already has it). The trigger wrapper must not carry this role; it typically usesaria-describedbypointing to the tooltip element'sid. -
Not keyboard-focusable —
on:focus/on:blurare wired up, but a<div>withouttabindex="0"is never focused via keyboard, so the tooltip is unreachable for keyboard-only users.
♿ Suggested fix
<div
bind:this={triggerElement}
on:mouseenter={showTooltip}
on:mouseleave={hideTooltip}
on:focus={showTooltip}
on:blur={hideTooltip}
class="inline-block"
- aria-label="Tooltip"
- role="tooltip"
+ tabindex="0"
>Give the tooltip <div> an id and add aria-describedby on the trigger as a follow-up when IDs are wired in.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {#if wrapper} | |
| <!-- Wrapper mode: wraps custom content --> | |
| <div | |
| bind:this={triggerElement} | |
| on:mouseenter={showTooltip} | |
| on:mouseleave={hideTooltip} | |
| on:focus={showTooltip} | |
| on:blur={hideTooltip} | |
| class="inline-block" | |
| aria-label="Tooltip" | |
| role="tooltip" | |
| > | |
| <slot /> | |
| </div> | |
| {`#if` wrapper} | |
| <!-- Wrapper mode: wraps custom content --> | |
| <div | |
| bind:this={triggerElement} | |
| on:mouseenter={showTooltip} | |
| on:mouseleave={hideTooltip} | |
| on:focus={showTooltip} | |
| on:blur={hideTooltip} | |
| class="inline-block" | |
| tabindex="0" | |
| > | |
| <slot /> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/web_ui/src/lib/ui/info_tooltip.svelte` around lines 118 - 131, The
wrapper-mode trigger div incorrectly has role="tooltip" and is not
keyboard-focusable; remove the role attribute from the trigger (leave
role="tooltip" on the floating popup element), make the trigger focusable by
adding tabindex="0" to the div bound to triggerElement, and wire an accessible
relationship by adding aria-describedby referencing the tooltip element's id
(assign an id to the popup div and set aria-describedby on the trigger once IDs
are available); keep existing handlers showTooltip/hideTooltip and focus/blur
logic.
Comment on lines
+1089
to
+1121
| {@const eval_configs_link = `/specs/${project_id}/${task_id}/${spec_id}/${evalItem.id}/eval_configs`} | ||
| {@const train_tag = tagFromFilterId( | ||
| evalItem.train_set_filter_id || "", | ||
| )} | ||
| {@const dataset_add_link = | ||
| train_tag && | ||
| `/dataset/${project_id}/${task_id}/add_data?tags=${train_tag}`} | ||
| {@const tooltip_parts = [ | ||
| "**Eval Not Ready**\n\nFix the following issues and click Refresh to update this eval's status.", | ||
| judge_error === "No judge configured" | ||
| ? "**No judge configured** — This eval doesn't have a default judge. To fix, [set default judge](" + | ||
| eval_configs_link + | ||
| ") for this eval." | ||
| : null, | ||
| judge_error === "Model not supported" | ||
| ? "**Model not supported** — This eval's default judge model is not supported. Kiln Prompt Optimization only supports OpenRouter, OpenAI, Gemini, and Anthropic providers. To fix, [change your default judge](" + | ||
| eval_configs_link + | ||
| ") for this eval." | ||
| : null, | ||
| train_error === "Training set required" || | ||
| train_error === "Training set is empty" | ||
| ? "**Training set is empty** — This eval doesn't have any training data. To fix, [add samples to your dataset](" + | ||
| dataset_add_link + | ||
| ") with the tag " + | ||
| `'${train_tag}'` + | ||
| "." | ||
| : null, | ||
| other_error | ||
| ? "**Error**\n\n" + other_error | ||
| : null, | ||
| ].filter((x) => x !== null)} | ||
| {@const combined_tooltip = | ||
| tooltip_parts.join("\n\n")} |
Contributor
There was a problem hiding this comment.
undefined rendered in tooltip text when eval has no train_set_filter_id.
When train_error === "Training set required", the eval has no train_set_filter_id, so:
tagFromFilterId("")→undefineddataset_add_link = undefined && ...→undefined
The tooltip string then becomes:
...To fix, [add samples to your dataset](undefined) with the tag 'undefined'.
This produces a broken link and nonsensical tag name in the tooltip. Fix by guarding the link/tag on train_tag being defined:
🐛 Proposed fix
- train_error === "Training set required" || train_error === "Training set is empty"
- ? "**Training set is empty** — This eval doesn't have any training data. To fix, [add samples to your dataset](" +
- dataset_add_link +
- ") with the tag " +
- `'${train_tag}'` +
- "."
- : null,
+ train_error === "Training set required"
+ ? "**No training set configured** — This eval doesn't have a training set. Please configure one in the eval settings."
+ : train_error === "Training set is empty" && train_tag && dataset_add_link
+ ? "**Training set is empty** — This eval doesn't have any training data. To fix, [add samples to your dataset](" +
+ dataset_add_link +
+ ") with the tag " +
+ `'${train_tag}'` +
+ "."
+ : train_error
+ ? "**Training set issue** — " + train_error
+ : null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/web_ui/src/routes/`(app)/prompt_optimization/[project_id]/[task_id]/create_prompt_optimization_job/+page.svelte
around lines 1089 - 1121, The tooltip can render "undefined" because
tagFromFilterId("") returns undefined so train_tag is falsy and dataset_add_link
becomes undefined; update the logic around tagFromFilterId, train_tag,
dataset_add_link, tooltip_parts and combined_tooltip so the training-set message
is only added when train_tag is truthy: compute train_tag =
tagFromFilterId(evalItem.train_set_filter_id || "") and dataset_add_link only if
train_tag; in tooltip_parts push the training message only when train_error
indicates missing training data AND train_tag is defined (use a conditional that
references train_tag), and when inserting the tag or link interpolate only the
confirmed train_tag/dataset_add_link values to avoid rendering 'undefined'.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes KIL-416
Description
This PR updates the "Create Optimized Prompt" screen UI based on feedback:
Changes:
Subtitle: Fixed grammar and updated text from "Create a prompt optimized for your evals using training data." to "Automatically optimize your prompt, maximizing its quality using evals."
Step 1 - Select Run Configuration:
Prompt Section:
Step 2 - Select Optimization Evals:
Removed Warning: Removed "Some evaluators are not selected. Your prompt will not be optimized for these evaluators." warning
No Evaluators Selected: Changed from red text to nicer Warning UI (red icon, grey text)
Eval Row Behavior:
Summary by CodeRabbit
New Features
Improvements