Uh oh!
There was an error while loading. Please reload this page.
sync agent rule - #476
Merged
Merged
Conversation
PR Summary by QodoAdd agent rule criteria mode, message, and code-scripts deep link
AI Description
Diagram
High-Level Assessment
Files changed (8) |
Code Review by Qodo
1. Criteria whitespace preserved |
Comment on lines
+77
to
+80
| return { | ||
| mode: mode.trim() || null, | ||
| criteria: text || null | ||
| }; |
There was a problem hiding this comment.
1. Criteria whitespace preserved 🐞 Bug≡ Correctness
normalizeCriteria() checks text.trim() to detect a blank criteria, but returns the untrimmed text, so a whitespace-only criteria is still persisted whenever mode is set. This can produce semantically blank-but-present criteria values in agent.rules payloads.
Agent Prompt
### Issue description
`normalizeCriteria()` trims `mode` but returns `criteria: text || null` without trimming. If the user enters only whitespace in Criteria Text while selecting a mode, the criteria object will be saved with whitespace content.
### Issue Context
`fetchRules()` uses `normalizeCriteria()` to build the rules array that is saved back into `agent.rules`.
### Fix Focus Areas
- src/routes/page/agent/[agentId]/agent-components/rules/agent-rule.svelte[66-81]
### Suggested change
Return `criteria: text.trim() || null` (and optionally set `const trimmedText = text.trim()` once) so whitespace-only criteria does not get persisted.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+53
to
+56
| export function restoreSessionFromOpener() { | ||
| if (!browser || openerSessionRestored) return; | ||
| openerSessionRestored = true; | ||
There was a problem hiding this comment.
2. Opener restore is one-shot 🐞 Bug☼ Reliability
restoreSessionFromOpener() sets openerSessionRestored=true before copying any keys, so later calls will not retry copying keys that were missing during the first attempt. This can leave the new tab without tenant_id/tenant_name (or other keys) if the opener populates them after the first restore attempt.
Agent Prompt
### Issue description
`restoreSessionFromOpener()` is guarded by a global `openerSessionRestored` flag that is set to `true` before attempting any key copies. If the first call occurs before the opener has populated some keys (e.g., tenant_id/tenant_name set later), subsequent calls will never attempt to copy those missing keys.
### Issue Context
`getUserStore()` calls this only when `user` is absent; `getTenantId()`/`getTenantName()` call it unconditionally, but are blocked after the first attempt.
### Fix Focus Areas
- src/lib/helpers/store.js[40-74]
- src/lib/helpers/store.js[101-128]
### Suggested change
Allow repeated attempts to copy *still-missing* keys without overwriting existing values. For example:
- Remove the global one-shot flag entirely, or
- Replace it with per-key tracking (e.g., `restoredKeys` set) and only skip keys already present or already successfully restored, or
- Only set `openerSessionRestored=true` after a same-origin opener is confirmed and the copy loop has run, and allow retrying when any of `[userKey, tenantKey, tenantNameKey]` remains missing.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+123
to
+126
| if (!codeScriptUrl) return; | ||
| e.preventDefault(); | ||
| window.open(codeScriptUrl, '_blank'); |
There was a problem hiding this comment.
3. Opener kept for new tab 🐞 Bug⛨ Security
openCodeScripts() uses window.open(url, '_blank') specifically to preserve window.opener for session restoration, which means the opened tab can navigate the opener (reverse-tabnabbing/opener manipulation) if it ever reaches attacker-controlled content. This is a security tradeoff introduced by relying on opener for credential/session transfer.
Agent Prompt
### Issue description
The new tab is intentionally opened with an opener reference so it can read `opener.sessionStorage` and restore the session. Keeping `window.opener` increases exposure to reverse-tabnabbing/opener manipulation if the opened tab ever navigates to untrusted content (e.g., via an XSS, open redirect, or user-driven navigation).
### Issue Context
This behavior is paired with `restoreSessionFromOpener()` which reads `window.opener.sessionStorage`.
### Fix Focus Areas
- src/routes/page/agent/[agentId]/agent-components/rules/agent-rule-item.svelte[113-127]
- src/lib/helpers/store.js[43-74]
### Safer alternatives
Prefer a session-sharing mechanism that doesn’t require opener (so you can use `noopener`):
- Use `localStorage` + `storage` event / `BroadcastChannel` to request/response a session copy.
- Use a short-lived one-time token in the URL (issued server-side) rather than copying sessionStorage.
If opener must be used, consider constraining capabilities (e.g., immediately `window.opener = null` after the child has copied needed values, if feasible in the child page).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+83
to
+86
| if (!agentOptions.some(x => x.value === agentId)) { | ||
| syncAgentIdToUrl(null); | ||
| return; | ||
| } |
There was a problem hiding this comment.
4. Deep link lost on errors 🐞 Bug☼ Reliability
The code-scripts page removes ?agentId= from the URL whenever the current agentId is not found in agentOptions; if agentOptions fails to load and remains empty, valid deep links will be stripped during transient errors. This breaks the new deep-linking behavior under network/API failure conditions.
Agent Prompt
### Issue description
On mount, agent options are loaded in a try/catch; regardless of failure, `applyAgentIdFromUrl()` runs and treats any `agentId` as invalid when `agentOptions` is empty, then calls `syncAgentIdToUrl(null)` which drops the query param.
### Issue Context
This defeats deep links like `/page/agent/code-scripts?agentId=...` exactly when options loading fails (e.g., transient backend outage).
### Fix Focus Areas
- src/routes/page/agent/code-scripts/+page.svelte[57-68]
- src/routes/page/agent/code-scripts/+page.svelte[79-95]
### Suggested change
Gate URL validation on successful options load, e.g.:
- Track `agentOptionsLoadedSuccessfully` and in `applyAgentIdFromUrl()` return early if options aren’t loaded.
- Or if options load fails, keep `selectedAgentId = agentId` and defer validation until options are available (don’t call `syncAgentIdToUrl(null)` in the failure case).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
No description provided.