Skip to content

fix: env variable overwrite and action menu clipping in create function wizard#3097

Merged
HarshMN2345 merged 15 commits into
mainfrom
fix-create-function-env-variable-bugs
Jun 29, 2026
Merged

fix: env variable overwrite and action menu clipping in create function wizard#3097
HarshMN2345 merged 15 commits into
mainfrom
fix-create-function-env-variable-bugs

Conversation

@HarshMN2345

@HarshMN2345 HarshMN2345 commented Jun 29, 2026

Copy link
Copy Markdown
Member

Summary

  • Env variable overwrite bug: In the Create function wizard, variables are stored client-side with no `$id` (not yet saved to the API). The update modal matched variables by `variable.$id === selectedVar.$id` — since all unsaved variables have `$id === undefined`, every variable matched and got overwritten with the edited one's values. Fixed by falling back to key-based matching when `$id` is absent.

Root cause

`updateVariableModal.svelte` line 25:
```js
// Before — undefined === undefined matches ALL unsaved variables
if (variable.$id === selectedVar.$id) { return pair; }

// After — fall back to key match when no $id exists
const match = selectedVar.$id
? variable.$id === selectedVar.$id
: variable.key === selectedVar.key;
return match ? pair : variable;
```

Test plan

  • Create function wizard: add 3 variables with different keys/values, edit one → only that variable should change
  • Existing saved variables (with `$id`) still update correctly via the function settings page

…verwritten, add portal to fix action menu clipping
@greptile-apps

greptile-apps Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes two bugs in the Create Function wizard's variable management: the updateVariableModal now matches unsaved variables by key instead of $id (preventing all-variables overwrite), and a new variableActionMenu component portals the action menu to document.body via floating-ui to escape the accordion's overflow context.

  • updateVariableModal.svelte: The fix correctly falls back to variable.key === selectedVar.key when selectedVar.$id is absent; the modal is rendered inside {#if showUpdate} so pair always re-initialises from the current selectedVar, keeping the fix sound.
  • variableActionMenu.svelte: A hand-rolled portal + floating-ui dropdown replaces the Popover component; the autoUpdate cleanup is assigned to a module-level cleanup variable rather than returned from $effect, leaving scroll/resize listeners attached if the component is destroyed while the menu is open (noted in a prior review round).
  • environmentVariables.svelte: Delegates the ... action cell to VariableActionMenu; adds max column-width constraints and an icon-only mode for the Create button on small viewports.

Confidence Score: 4/5

The env-variable overwrite fix is correct and self-contained. The new variableActionMenu component ships with an unresolved autoUpdate cleanup gap that can leave stale event listeners when a row is removed while the menu is open.

The matching-logic fix in updateVariableModal is correct and low-risk — the modal is always destroyed and re-created, so pair initialises fresh on every open. The variableActionMenu replacement works well in the common path, but the autoUpdate cleanup is stored in a module-level variable rather than returned from the $effect, meaning Svelte cannot invoke it on component teardown when open is still true.

src/lib/components/variables/variableActionMenu.svelte — specifically the $effect at line 47 that manages the autoUpdate lifecycle.

Important Files Changed

Filename Overview
src/lib/components/variables/updateVariableModal.svelte Core bug fix: falling back to key-based matching when $id is undefined (unsaved wizard variables) correctly prevents all-variables overwrite; logic is sound since the modal is destroyed/recreated via {#if showUpdate} so pair always initialises fresh.
src/lib/components/variables/variableActionMenu.svelte New custom portal component using floating-ui; fixes overflow/clipping; the autoUpdate cleanup is stored in a module-level variable rather than returned from the $effect, so listeners leak when the component is destroyed while the menu is open (flagged in a prior review round).
src/lib/components/variables/environmentVariables.svelte Replaces inline Popover + ActionMenu markup with the new VariableActionMenu component; adjusts column widths to add max constraints; adds icon-only mode for the Create button on small viewports. Changes look correct.

Reviews (9): Last reviewed commit: "fix: remove stopPropagation so outside-c..." | Re-trigger Greptile

Comment thread src/lib/components/variables/variableActionMenu.svelte
Comment thread src/lib/components/variables/variableActionMenu.svelte
@HarshMN2345 HarshMN2345 merged commit 7331b3a into main Jun 29, 2026
4 checks passed
@HarshMN2345 HarshMN2345 deleted the fix-create-function-env-variable-bugs branch June 29, 2026 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants