add rule criteria - #472
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
…eatures/add-rule-criteria
…eatures/add-rule-criteria
PR Summary by QodoAdd rule criteria editor and code-script generation action
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. Compile icon not keyboardable
|
| title="Generate code script" | ||
| onkeydown={() => {}} | ||
| onclick={() => compile()} | ||
| ></i> |
There was a problem hiding this comment.
1. Compile icon not keyboardable 🐞 Bug ≡ Correctness
In agent-rule-item.svelte the new compile icon is focusable (tabindex=0) but its onkeydown handler is a no-op, so keyboard users cannot trigger compilation via Enter/Space even though the UI presents it as a link.
Agent Prompt
### Issue description
The new compile affordance is rendered as an `<i>` with `role="link"` + `tabindex="0"`, but `onkeydown={() => {}}` means there is no keyboard activation path.
### Issue Context
This was introduced with the new Criteria/compile UI. The click handler calls `compile()`, but keyboard events never do.
### Fix Focus Areas
- src/routes/page/agent/[agentId]/agent-components/rules/agent-rule-item.svelte[186-199]
Suggested implementation options:
- Prefer a semantic `<button type="button">` wrapping the icon (native keyboard behavior), or
- Implement `onkeydown` to call `compile()` when `e.key === 'Enter'` (and optionally `' '`), with `e.preventDefault()` for Space.
- Add `aria-label="Generate code script"` (tooltip `title` is not a reliable accessible name).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Code script can only be generated by admins, once a trigger is picked and criteria text exists. | ||
| let canCompile = $derived( | ||
| !!rule.trigger_name && !!rule.config?.criteria?.trim() | ||
| ); |
There was a problem hiding this comment.
2. Compile allowed when disabled 🐞 Bug ≡ Correctness
canCompile ignores rule.disabled, so a disabled rule can still show and execute the compile action even though the Criteria textarea is disabled, allowing code generation/overwrite for a rule the UI marks as disabled.
Agent Prompt
### Issue description
The Criteria textarea is disabled when `rule.disabled` is true, but the compile icon’s visibility/enabled state is driven only by `canCompile` which does not include `rule.disabled`. This allows generating code scripts for disabled rules.
### Issue Context
- `canCompile` currently checks `trigger_name` and `criteria.trim()` only.
- The compile icon is rendered when `canCompile` is true.
- The Criteria textarea is disabled via `disabled={rule.disabled}`.
### Fix Focus Areas
- src/routes/page/agent/[agentId]/agent-components/rules/agent-rule-item.svelte[36-40]
- src/routes/page/agent/[agentId]/agent-components/rules/agent-rule-item.svelte[186-213]
Suggested fix:
- Update `canCompile` to include `!rule.disabled`.
- Additionally (defense-in-depth), in the parent compile handler (agent-rule.svelte) refuse to open the confirm modal / call the API when `rule.disabled` is true.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| function generateCodeScript(rule) { | ||
| isLoading = true; | ||
| generateAgentCodeScript(agent.id, { | ||
| options: { |
There was a problem hiding this comment.
3. No in-flight compile guard 🐞 Bug ☼ Reliability
generateCodeScript() sets isLoading but never checks it, so users can trigger multiple confirmed generateAgentCodeScript requests concurrently, causing duplicate work and racing success/error banners/timeouts.
Agent Prompt
### Issue description
The code-generation flow does not enforce mutual exclusion:
- `generateCodeScript()` sets `isLoading = true`, but does not early-return if already loading.
- The UI path to invoke compile does not consult `isLoading`.
This allows concurrent requests and out-of-order UI state updates.
### Issue Context
`generateAgentCodeScript(...).then(...).catch(...)` toggles `isLoading/isComplete/isError` and schedules timeouts. With concurrent invocations, later responses can overwrite earlier UI status, and multiple writes may race on the backend.
### Fix Focus Areas
- src/routes/page/agent/[agentId]/agent-components/rules/agent-rule.svelte[257-290]
- src/routes/page/agent/[agentId]/agent-components/rules/agent-rule-item.svelte[186-199]
Suggested fix:
- Add a guard at the top of `generateCodeScript`: `if (isLoading) return;`
- Disable/hide the compile icon (or the confirm button) while `isLoading` is true.
- Optionally store/clear timeout IDs to avoid overlapping timers from successive runs.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
No description provided.