ENGG-4624 Variable flow edgecases #3835
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces context-aware behavior to the VariablePopover component system. The changes add an Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableNotFound.tsx(2 hunks)app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/hooks/useScopeOptions.ts(5 hunks)app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx(5 hunks)app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/types.ts(1 hunks)
🔇 Additional comments (8)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/types.ts (1)
31-31: LGTM!The type extension is clean and properly integrates the noop context flag into the props interface.
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/hooks/useScopeOptions.ts (3)
71-72: LGTM!The noop context detection logic is correct and clearly determines when to apply context-aware scope restrictions.
92-92: LGTM!The scope logic correctly handles noop contexts by disabling GLOBAL scope and defaulting to RUNTIME, which aligns with the PR objective of preventing variable creation errors in contexts without proper variable stores.
Also applies to: 104-106
115-115: LGTM!Properly includes
isNoopContextin the memoization dependencies to ensure scope options recompute when context changes.app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableNotFound.tsx (2)
6-10: LGTM!The component signature correctly accepts the
isNoopContextprop for context-aware rendering.
21-25: LGTM!The conditional rendering correctly hides the "Switch environment" button in noop contexts where environment switching is unavailable, addressing the PR objective.
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (2)
35-36: LGTM!The noop context detection logic correctly identifies when the component is operating in a noop context.
204-227: LGTM!The conditional rendering correctly hides the variable header and edit controls in noop contexts, which is appropriate since scope/environment information is not meaningful in those contexts.
| variable: variableData, | ||
| }} | ||
| onEditClick={handleEditClick} | ||
| isNoopContext |
There was a problem hiding this comment.
Critical: Pass the computed isNoopContext variable instead of hardcoding it to true.
Lines 82 and 90 use JSX shorthand syntax (isNoopContext without a value), which is equivalent to isNoopContext={true}. This hardcodes the prop to always true, ignoring the computed isNoopContext value from line 36. This would cause all contexts to be treated as noop contexts, breaking normal variable flows and hiding UI elements that should be visible.
Apply this diff to pass the actual variable value:
<VariableInfo
params={{
name: hoveredVariable,
variable: variableData,
}}
onEditClick={handleEditClick}
- isNoopContext
+ isNoopContext={isNoopContext}
/>- return (
- <VariableNotFound
- isNoopContext
- onCreateClick={handleCreateClick}
- onSwitchEnvironment={handleSwitchEnvironment}
- />
- );
+ return (
+ <VariableNotFound
+ isNoopContext={isNoopContext}
+ onCreateClick={handleCreateClick}
+ onSwitchEnvironment={handleSwitchEnvironment}
+ />
+ );Also applies to: 90-90
🤖 Prompt for AI Agents
In
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
around lines 82 and 90, the JSX uses the shorthand isNoopContext which hardcodes
isNoopContext={true} and ignores the computed variable from line 36; change both
usages to pass the actual variable value (isNoopContext={isNoopContext}) so the
prop reflects the computed boolean instead of always true, ensuring correct
context behavior and UI rendering.
…estly into ENGG-4624-variable-edgecases
…ionality (#3820) * Add edit variable functionality to VariablePopOver - Allow editing variables via a new edit form mode - Add edit button to variable info header (except runtime scope) - Update CreateVariableView to support both create and edit modes - Adjust styles for edit mode header and actions * Update getSourceId to return TabSourceMetadata type * Refactor variable scope icon logic and update popover UI * Add useUpdateVariable hook for variable editing * Add edit mode to variable popover and creation view * Update variable popover UI and add scope name prop - Pass scopeName to CreateVariableView for improved labeling - Adjust button sizes and loading states for consistency - Refine header and action styles for better alignment - Add debug console logs for variable data and options * Refactor variable popover to use upsert logic - Split create and edit variable views into separate components - Add useUpsertVariable hook for unified create/update logic - Extract VariableFormFields for shared form UI - Update popover state transitions and rendering - Remove legacy create/update hooks and props * Remove useCreateVariable and useUpdateVariable hooks * Update VariablePopover to handle open state and trigger Remove tab change popover logic from SingleLineEditor * Add border radius to buttons and select elements * Show error toast when variable update fails * [ENGG-4624]: Hide environment actions in variable popover for Noop context (#3835) --------- Co-authored-by: Rohan Mathur <mathurrohan04@gmail.com>
* Add variable creation flow to VariablePopOver - Add CreateVariableView and VariableNotFound components - Implement useCreateVariable and useScopeOptions hooks - Update VariablePopOver to support creating variables when not found - Add types for popover views and variable creation - Update styles for new popover views and form - Integrate popover pinning logic in SingleLineEditor * Support collection-scoped variable creation in popover * Fix runtime variable creation to use add with generated ID Generate a unique ID for new runtime variables and use the add method instead of update to ensure proper creation. Set isPersisted to true. * Add type-specific inputs and validation to variable creation - Support string, secret, number, and boolean variable types - Add type-specific input components and validation logic - Improve scope dropdown icons and styling - Implement environment switcher trigger from variable popover - Refine sidebar and environment switcher to respond to trigger event - Update styles for new input types and dropdowns * Improve number validation and update form value types * Fix variable value validation and input styles in popover * Extract collectionId from API record instead of variables * Remove debug console.log from VariablePopover * Refactor variable popover to use generic state for collectionId * Refactor source ID handling in generic state and variable creation - Change getSourceId to return metadata object instead of string - Update CreateVariableView to derive collectionId from record metadata - Adjust TabItem to use metadata for source ID - Update GenericState interface and default implementation * Support boolean and number types in variable creation * Fix runtime variable ID generation logic * Refactor variable popover and creation flow - Update CreateVariableView and VariableNotFound to define props inline - Refactor useCreateVariable to use unified status object - Improve type handling for variable values - Add explicit popover view state transitions - Use useCallback for event handlers in popover and editor - Remove unused interface exports from types * Fix getSourceId to return record id instead of metadata object * Close popover when active tab changes in SingleLineEditor * Improve number field validation in variable creation form * Remove number input validation from variable creation * Pass contextId in trigger-env-switcher event Ensure environment switcher only responds to relevant contextId. MultiWorkspaceSidebar now ignores context-specific events. * Remove redundant type prop from InputNumber component * [ENGG-2769] Feat: Enhance variable popover with create and edit functionality (#3820) * Add edit variable functionality to VariablePopOver - Allow editing variables via a new edit form mode - Add edit button to variable info header (except runtime scope) - Update CreateVariableView to support both create and edit modes - Adjust styles for edit mode header and actions * Update getSourceId to return TabSourceMetadata type * Refactor variable scope icon logic and update popover UI * Add useUpdateVariable hook for variable editing * Add edit mode to variable popover and creation view * Update variable popover UI and add scope name prop - Pass scopeName to CreateVariableView for improved labeling - Adjust button sizes and loading states for consistency - Refine header and action styles for better alignment - Add debug console logs for variable data and options * Refactor variable popover to use upsert logic - Split create and edit variable views into separate components - Add useUpsertVariable hook for unified create/update logic - Extract VariableFormFields for shared form UI - Update popover state transitions and rendering - Remove legacy create/update hooks and props * Remove useCreateVariable and useUpdateVariable hooks * Update VariablePopover to handle open state and trigger Remove tab change popover logic from SingleLineEditor * Add border radius to buttons and select elements * Show error toast when variable update fails * [ENGG-4624]: Hide environment actions in variable popover for Noop context (#3835) --------- Co-authored-by: Rohan Mathur <mathurrohan04@gmail.com> * Move success toast after onSave and simplify error toast messages * Fix isNoopContext prop passing in VariablePopover components * Refactor collection ID lookup into shared utility function * Allow transitions from IDLE to VARIABLE_INFO and NOT_FOUND * Replace console.error with Sentry captureMessage for popover transitions * Remove global environment switcher event handling from sidebar Environment switcher now only responds to events for its own context. * Remove popover pinning logic from variable editor components * Standardize input heights and font sizes in VariablePopOver * add pinning back * Show variable info header for noop context variables * analytics * Update trackEvent import to use modules/analytics * Add variable scope and source to analytics events * Refactor variable analytics source to use enum in types * Update trackEvent import to use analytics module * Fix variable tracking source in EditVariableView * Close dropdown on environment switch actions --------- Co-authored-by: Rohan Mathur <mathurrohan04@gmail.com>
Closes issue: ENGG-4624
📜 Summary of changes:
Fixes variable creation errors in Traffic Interceptor Edit & Replay
and Mock Editor Test flows where NoopContext is used instead of
real workspace context.
🎥 Demo Video:
✅ Checklist:
🧪 Test instructions:
🔗 Other references:
Summary by CodeRabbit
Release Notes