[ENGG-5092]: Added Eye button to make secrets visible in variable popover #4050
Conversation
WalkthroughThis change updates VariablePopOver to compute memoized info fields via Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/variable-popover.scss (1)
212-216: Consider responsive behavior for the min-width constraint.The fixed
min-width: 150pxon.value-contentcould cause horizontal overflow in narrow popovers or small viewports, especially since the popover width is constrained to 300px (line 88).Consider adjusting to a smaller min-width or using a flexible approach:
.value-content { - min-width: 150px; + min-width: 100px; overflow: hidden; text-overflow: ellipsis; }app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (3)
208-216: Boolean to string conversion is intentional but could be clearer.The
getValueStringsfunction converts the booleanisPersistedto a string via template literal coercion. While this works, it could benefit from explicit conversion for clarity.Consider explicit boolean conversion:
function getValueStrings(variable: VariableData) { - const makeRenderable = (value: VariableValueType) => `${value}`; + const makeRenderable = (value: VariableValueType) => { + if (typeof value === 'boolean') return value ? 'Yes' : 'No'; + return `${value}`; + }; return { syncValue: makeRenderable(variable.syncValue ?? ""), localValue: makeRenderable(variable.localValue ?? ""), isPersisted: makeRenderable(variable.isPersisted ?? true), }; }
313-322: Add accessibility attributes to the eye toggle button.The toggle button lacks ARIA attributes, making it difficult for screen reader users to understand its purpose and state.
Apply this diff to improve accessibility:
{field.isSecret && ( <div className="eye-toggle-button"> <RQButton type="transparent" size="small" icon={revealedSecrets[field.id] ? <RiEyeLine /> : <RiEyeOffLine />} onClick={() => toggleVisibility(field.id)} + aria-label={revealedSecrets[field.id] ? "Hide secret value" : "Show secret value"} + aria-pressed={revealedSecrets[field.id]} /> </div> )}
302-310: Consider dynamic bullet count for better UX.The masked secret always displays 13 bullets (
•••••••••••••) regardless of the actual secret length, which doesn't provide visual feedback about the value's approximate length.Consider matching the bullet count to the secret length:
{field.isSecret ? ( revealedSecrets[field.id] ? ( <span className="secret-revealed">{String(field.value)}</span> ) : ( - <span className="secret-masked">•••••••••••••</span> + <span className="secret-masked">{'•'.repeat(Math.min(String(field.value).length, 20))}</span> ) ) : ( <span>{String(field.value)}</span> )}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx(6 hunks)app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/variable-popover.scss(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-27T14:31:06.274Z
Learnt from: Aarushsr12
Repo: requestly/requestly PR: 3719
File: app/src/features/apiClient/screens/apiClient/components/views/components/Collection/components/CollectionRunnerView/components/RunConfigView/RunConfigSettings/runConfigSettings.scss:85-113
Timestamp: 2025-10-27T14:31:06.274Z
Learning: In the requestly/requestly codebase, do not suggest adding fallback values to CSS/SCSS variable usage (e.g., var(--requestly-color-error-darker)). The team relies on their design token system to ensure all tokens are defined, and fallbacks are not needed.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/variable-popover.scss
📚 Learning: 2025-11-20T06:20:26.538Z
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 3820
File: app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableFormFields.tsx:6-16
Timestamp: 2025-11-20T06:20:26.538Z
Learning: In the VariableFormFields component (app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableFormFields.tsx), the scopeOptions prop is guaranteed to always contain at least one option, so defensive checks for empty arrays are not necessary.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
📚 Learning: 2025-12-11T06:43:01.328Z
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 4000
File: app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts:6-14
Timestamp: 2025-12-11T06:43:01.328Z
Learning: In app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts, the useApiClientFeatureContext hook uses useMemo without reactivity intentionally. Contexts are expected to be registered before component mount, and reactivity to registry updates is not needed.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
🔇 Additional comments (6)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/variable-popover.scss (2)
70-72: LGTM!The transparent background on hover for ghost buttons ensures consistent styling with the design system.
115-119: LGTM!The edit icon styling correctly uses design tokens for consistent theming.
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (4)
1-1: LGTM!The imports are appropriate:
useMemofor optimizing field configuration, and eye icons for the visibility toggle UI.Also applies to: 14-15
37-49: LGTM!The
FieldKeyenum andFieldConfiginterface provide good type safety and a clear structure for field-based rendering with optional secret handling.
236-270: LGTM!The memoized field configuration appropriately handles different scopes and applies secret masking only to value fields. The dependency array is comprehensive and correct.
279-291: LGTM!The UI structural changes (scope icon, separator, header name, and edit icon with class) properly integrate with the corresponding SCSS styling.
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
Outdated
Show resolved
Hide resolved
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
Outdated
Show resolved
Hide resolved
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
Outdated
Show resolved
Hide resolved
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
Outdated
Show resolved
Hide resolved
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (1)
308-317: Consider the eye icon UX convention.The current icon logic shows
RiEyeLine(open eye) when the secret is revealed andRiEyeOffLine(crossed eye) when hidden. The more common password field convention is the opposite: show an open eye when hidden (indicating "click to reveal") and a crossed eye when revealed (indicating "click to hide").This isn't necessarily wrong—both conventions exist—but it's worth verifying this aligns with other visibility toggles in the Requestly UI for consistency.
🔎 If you want to follow the password-field convention
<RQButton type="transparent" size="small" - icon={revealedSecrets[field.label] ? <RiEyeLine /> : <RiEyeOffLine />} + icon={revealedSecrets[field.label] ? <RiEyeOffLine /> : <RiEyeLine />} onClick={() => toggleVisibility(field.label)} />
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-20T06:20:26.538Z
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 3820
File: app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableFormFields.tsx:6-16
Timestamp: 2025-11-20T06:20:26.538Z
Learning: In the VariableFormFields component (app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableFormFields.tsx), the scopeOptions prop is guaranteed to always contain at least one option, so defensive checks for empty arrays are not necessary.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
📚 Learning: 2025-12-11T06:43:01.328Z
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 4000
File: app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts:6-14
Timestamp: 2025-12-11T06:43:01.328Z
Learning: In app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts, the useApiClientFeatureContext hook uses useMemo without reactivity intentionally. Contexts are expected to be registered before component mount, and reactivity to registry updates is not needed.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
🧬 Code graph analysis (1)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (2)
app/src/features/apiClient/helpers/modules/scriptsV2/worker/script-internals/variableScope.ts (1)
VariableScope(43-84)app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/hooks/useScopeOptions.ts (1)
getScopeIcon(42-67)
🔇 Additional comments (5)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (5)
1-14: LGTM!The new imports (
useMemo,RiEyeLine,RiEyeOffLine) are all properly utilized in the component for memoization and the visibility toggle UI.
36-47: LGTM!The
InfoFieldLabelenum andInfoFieldConfiginterface provide clear structure for the field configuration. Using the enum as both display labels and keys for therevealedSecretsstate is a clean approach.
206-214: LGTM!The
getValueStringsfunction correctly normalizes variable values to renderable strings with sensible defaults.
231-269: LGTM!The
useMemohook correctly memoizesinfoFieldswith appropriate dependencies. The conditional logic based onsource.scopeproperly distinguishes between runtime variables (showing current value + persistence) and other scopes (showing both initial and current values). ThetoggleVisibilitycallback uses the functional update pattern correctly.
295-305: LGTM on the masking implementation.The masking logic correctly caps the bullet characters at 15 using
Math.min(String(field.value).length, 15), addressing the previous review feedback. The conditional rendering for secret vs non-secret values is clean and correct.
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
Outdated
Show resolved
Hide resolved
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (1)
231-234: Consider using stricter type forrevealedLabels.The current
Record<string, boolean>type allows any string key. A stricter type would prevent accidental misuse:🔎 Proposed type refinement
- const [revealedLabels, setRevealedLabels] = useState<Record<string, boolean>>({ + const [revealedLabels, setRevealedLabels] = useState< + Partial<Record<InfoFieldLabel.CURRENT_VALUE | InfoFieldLabel.INITIAL_VALUE, boolean>> + >({ [InfoFieldLabel.INITIAL_VALUE]: false, [InfoFieldLabel.CURRENT_VALUE]: false, });Note: This aligns with the past review feedback suggesting stricter typing.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-20T06:20:26.538Z
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 3820
File: app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableFormFields.tsx:6-16
Timestamp: 2025-11-20T06:20:26.538Z
Learning: In the VariableFormFields component (app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableFormFields.tsx), the scopeOptions prop is guaranteed to always contain at least one option, so defensive checks for empty arrays are not necessary.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
📚 Learning: 2025-12-11T06:43:01.328Z
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 4000
File: app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts:6-14
Timestamp: 2025-12-11T06:43:01.328Z
Learning: In app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts, the useApiClientFeatureContext hook uses useMemo without reactivity intentionally. Contexts are expected to be registered before component mount, and reactivity to registry updates is not needed.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
🧬 Code graph analysis (1)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (2)
app/src/features/apiClient/helpers/modules/scriptsV2/worker/script-internals/variableScope.ts (1)
VariableScope(43-84)app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/hooks/useScopeOptions.ts (1)
getScopeIcon(42-67)
🔇 Additional comments (5)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (5)
1-14: LGTM!The new imports are appropriate for the secret visibility toggle feature -
useMemofor memoizing field configuration and the eye icons for the toggle UI.
36-47: Clean type definitions.The
InfoFieldLabelenum andInfoFieldConfiginterface provide good type safety for the field configuration. Using the enum values as keys forrevealedLabelsstate ensures consistency.
206-214: LGTM!The refactored
getValueStringsfunction cleanly separates value extraction from masking logic, which is now correctly handled at render time based on user interaction.
270-272: LGTM!The
toggleVisibilityhandler correctly uses the functional update pattern and an empty dependency array since it doesn't close over any external values.
294-323: Well-structured rendering with proper masking.The field rendering correctly:
- Uses unique keys from
field.label- Caps masked output at 15 characters as requested
- Shows appropriate eye icon based on current visibility state
- Conditionally renders the toggle only for secret fields
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/variable-popover.scss (1)
140-145: LGTM!The spelling correction from "seperator" to "separator" addresses a previous review comment and maintains consistency.
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (1)
231-234: Apply the stricter type as suggested in previous review.The
revealedLabelsstate is currently typed asRecord<string, boolean>, but a previous review comment suggested using the stricter typeRecord<InfoFieldLabel.CURRENT_VALUE | InfoFieldLabel.INITIAL_VALUE, boolean>. This would improve type safety and make the intent clearer that only these two fields support visibility toggling.🔎 Proposed fix
- const [revealedLabels, setRevealedLabels] = useState<Record<string, boolean>>({ + const [revealedLabels, setRevealedLabels] = useState< + Record<InfoFieldLabel.CURRENT_VALUE | InfoFieldLabel.INITIAL_VALUE, boolean> + >({ [InfoFieldLabel.INITIAL_VALUE]: false, [InfoFieldLabel.CURRENT_VALUE]: false, });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx(6 hunks)app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/variable-popover.scss(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-20T06:20:26.538Z
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 3820
File: app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableFormFields.tsx:6-16
Timestamp: 2025-11-20T06:20:26.538Z
Learning: In the VariableFormFields component (app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableFormFields.tsx), the scopeOptions prop is guaranteed to always contain at least one option, so defensive checks for empty arrays are not necessary.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
📚 Learning: 2025-10-27T16:15:11.603Z
Learnt from: Aarushsr12
Repo: requestly/requestly PR: 3719
File: app/src/features/apiClient/screens/apiClient/components/views/components/Collection/components/CollectionRunnerView/components/RunConfigView/ParseFileModal/ParsedTableView.tsx:33-33
Timestamp: 2025-10-27T16:15:11.603Z
Learning: In ParsedTableView.tsx for the collection runner data file preview table, column titles should preserve the original case from the data file (using `k.charAt(0) + k.slice(1)` or just `k`) rather than capitalizing the first letter. This is because column names are used as variables in request bodies and need exact case matching for variable resolution to work correctly.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
📚 Learning: 2025-12-11T06:43:01.328Z
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 4000
File: app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts:6-14
Timestamp: 2025-12-11T06:43:01.328Z
Learning: In app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts, the useApiClientFeatureContext hook uses useMemo without reactivity intentionally. Contexts are expected to be registered before component mount, and reactivity to registry updates is not needed.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
📚 Learning: 2025-10-27T14:31:06.274Z
Learnt from: Aarushsr12
Repo: requestly/requestly PR: 3719
File: app/src/features/apiClient/screens/apiClient/components/views/components/Collection/components/CollectionRunnerView/components/RunConfigView/RunConfigSettings/runConfigSettings.scss:85-113
Timestamp: 2025-10-27T14:31:06.274Z
Learning: In the requestly/requestly codebase, do not suggest adding fallback values to CSS/SCSS variable usage (e.g., var(--requestly-color-error-darker)). The team relies on their design token system to ensure all tokens are defined, and fallbacks are not needed.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/variable-popover.scss
🧬 Code graph analysis (1)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (2)
app/src/features/apiClient/helpers/modules/scriptsV2/worker/script-internals/variableScope.ts (1)
VariableScope(43-84)app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/hooks/useScopeOptions.ts (1)
getScopeIcon(42-67)
🔇 Additional comments (9)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (6)
1-1: LGTM!The addition of
useMemofor memoizing field configuration and the eye icon imports are appropriate for the secret visibility toggle feature.Also applies to: 13-14
36-47: LGTM!The
InfoFieldLabelenum andInfoFieldConfiginterface provide a clean abstraction for the field configuration. The optionalisSecretflag is appropriately used only for fields that support masking (Initial Value and Current Value).Based on learnings, the previous concerns about the optional
isSecretfield and the difference betweenlabelandidhave been addressed by the developer's explanations.
206-214: LGTM!The
getValueStringsfunction correctly produces plain string representations for rendering. The masking is now applied at render time rather than in this function, which provides better separation of concerns.
237-268: LGTM!The memoized
infoFieldsconstruction is well-structured with correct dependencies. The conditional logic for RUNTIME vs other scopes is clear, and theisSecretflag is properly set only for Initial Value and Current Value fields.
270-272: LGTM!The
toggleVisibilitycallback correctly usesuseCallbackfor performance and has the appropriate strict type signature for the two fields that support secret toggling.
294-309: LGTM!The masking logic correctly implements the requested behavior: displaying bullets (•) with length equal to the actual string length, capped at a maximum of 15 characters. The conditional rendering for secret vs non-secret fields is clean and appropriate.
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/variable-popover.scss (3)
70-72: LGTM!The transparent background on hover for ghost buttons maintains consistent styling and prevents unwanted visual changes during interaction.
116-119: LGTM!The edit icon styling with appropriate size and subtle color integrates well with the popover's visual design.
200-226: LGTM!The layout enhancements effectively support the eye toggle feature:
- Flexbox layout enables proper alignment of value content and toggle button
- The
.with-togglevariant adds appropriate padding for the eye button- Hover interaction smoothly reveals the toggle with opacity transition
- Absolute positioning keeps the layout stable regardless of toggle visibility
- The
.value-contentstyling with ellipsis handles overflow gracefullyThese styles integrate seamlessly with the TypeScript component implementation.
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (1)
294-323: Well-implemented dynamic field rendering with secret masking.The rendering logic is clean and correctly handles:
- Dynamic field iteration with proper React keys
- Conditional secret masking with length-aware bullet points (capped at 15)
- Appropriate eye icon semantics (open eye when revealed, closed when hidden)
Note: The type assertion issue at line 317 (
toggleVisibility(field.label)) was already flagged in a past review. If you apply the stricter typing suggested above, you'll need to add a type assertion here:toggleVisibility(field.label as InfoFieldLabel.INITIAL_VALUE | InfoFieldLabel.CURRENT_VALUE).
🧹 Nitpick comments (2)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (2)
231-234: Consider using a stricter type forrevealedLabels.The current
Record<string, boolean>type is looser than necessary. A stricter union type would provide better type safety and self-documentation:- const [revealedLabels, setRevealedLabels] = useState<Record<string, boolean>>({ + const [revealedLabels, setRevealedLabels] = useState< + Record<InfoFieldLabel.CURRENT_VALUE | InfoFieldLabel.INITIAL_VALUE, boolean> + >({ [InfoFieldLabel.INITIAL_VALUE]: false, [InfoFieldLabel.CURRENT_VALUE]: false, });This would also require updating
toggleVisibilityparameter type accordingly.
270-272: Type parameter could be narrowed for clarity.While functionally correct (the
isSecretguard ensures only valid labels reach this function), the parameter type could be narrowed to match the actual usage:- const toggleVisibility = useCallback((key: InfoFieldLabel) => { + const toggleVisibility = useCallback((key: InfoFieldLabel.INITIAL_VALUE | InfoFieldLabel.CURRENT_VALUE) => { setRevealedLabels((prev) => ({ ...prev, [key]: !prev[key] })); }, []);This aligns with the suggested stricter typing for
revealedLabelsstate.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-20T06:20:26.538Z
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 3820
File: app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableFormFields.tsx:6-16
Timestamp: 2025-11-20T06:20:26.538Z
Learning: In the VariableFormFields component (app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableFormFields.tsx), the scopeOptions prop is guaranteed to always contain at least one option, so defensive checks for empty arrays are not necessary.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
📚 Learning: 2025-10-27T16:15:11.603Z
Learnt from: Aarushsr12
Repo: requestly/requestly PR: 3719
File: app/src/features/apiClient/screens/apiClient/components/views/components/Collection/components/CollectionRunnerView/components/RunConfigView/ParseFileModal/ParsedTableView.tsx:33-33
Timestamp: 2025-10-27T16:15:11.603Z
Learning: In ParsedTableView.tsx for the collection runner data file preview table, column titles should preserve the original case from the data file (using `k.charAt(0) + k.slice(1)` or just `k`) rather than capitalizing the first letter. This is because column names are used as variables in request bodies and need exact case matching for variable resolution to work correctly.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
📚 Learning: 2025-12-11T06:43:01.328Z
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 4000
File: app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts:6-14
Timestamp: 2025-12-11T06:43:01.328Z
Learning: In app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts, the useApiClientFeatureContext hook uses useMemo without reactivity intentionally. Contexts are expected to be registered before component mount, and reactivity to registry updates is not needed.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
🧬 Code graph analysis (1)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (2)
app/src/features/apiClient/helpers/modules/scriptsV2/worker/script-internals/variableScope.ts (1)
VariableScope(43-84)app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/hooks/useScopeOptions.ts (1)
getScopeIcon(42-67)
🔇 Additional comments (4)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (4)
36-47: LGTM!The enum and interface are well-structured. Using an enum for field labels ensures consistency and type safety across the component.
206-214: LGTM!The function cleanly converts variable values to renderable strings with sensible defaults.
237-268: LGTM!The memoization is well-structured with appropriate dependencies. The conditional logic correctly differentiates between RUNTIME and non-RUNTIME scopes, and the
isSecretflag is appropriately applied only to value fields.
276-290: LGTM!The header section changes are clean. The separator class name spelling has been corrected, and the layout structure is well-organized.
|
Added RBAC support so viewers:
Screen.Recording.2025-12-21.at.3.42.13.PM.movcc @nafees87n |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (3)
232-273: State management and memoization logic looks correct.The
useMemoforinfoFieldscorrectly computes field configurations based on scope, and thetoggleVisibilitycallback properly manages reveal state. Note that past review comments suggest narrowing the type ofrevealedLabelsto the specific labels that can be revealed.
279-279: Fix spelling: "seperator" should be "separator".This spelling error was flagged in previous review comments and should be corrected in both the TSX and SCSS files.
321-321: Type assertion needed fortoggleVisibilitycall.As noted in previous review comments,
field.labelis typed asInfoFieldLabel(all enum values) whiletoggleVisibilityexpects the narrower unionInfoFieldLabel.INITIAL_VALUE | InfoFieldLabel.CURRENT_VALUE. A type assertion is needed here.
🧹 Nitpick comments (1)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (1)
303-311: Optional: Consider security implications of revealing secret length.Line 307 masks secrets with dots matching the actual length (capped at 15 characters). This reveals the length of the secret, which could be a minor information leak depending on your security requirements. If this is a concern, consider using a fixed number of dots regardless of length.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableNotFound.tsx(2 hunks)app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-20T06:20:26.538Z
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 3820
File: app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableFormFields.tsx:6-16
Timestamp: 2025-11-20T06:20:26.538Z
Learning: In the VariableFormFields component (app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableFormFields.tsx), the scopeOptions prop is guaranteed to always contain at least one option, so defensive checks for empty arrays are not necessary.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsxapp/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableNotFound.tsx
📚 Learning: 2025-10-27T16:15:11.603Z
Learnt from: Aarushsr12
Repo: requestly/requestly PR: 3719
File: app/src/features/apiClient/screens/apiClient/components/views/components/Collection/components/CollectionRunnerView/components/RunConfigView/ParseFileModal/ParsedTableView.tsx:33-33
Timestamp: 2025-10-27T16:15:11.603Z
Learning: In ParsedTableView.tsx for the collection runner data file preview table, column titles should preserve the original case from the data file (using `k.charAt(0) + k.slice(1)` or just `k`) rather than capitalizing the first letter. This is because column names are used as variables in request bodies and need exact case matching for variable resolution to work correctly.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
📚 Learning: 2025-12-11T06:43:01.328Z
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 4000
File: app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts:6-14
Timestamp: 2025-12-11T06:43:01.328Z
Learning: In app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts, the useApiClientFeatureContext hook uses useMemo without reactivity intentionally. Contexts are expected to be registered before component mount, and reactivity to registry updates is not needed.
Applied to files:
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx
🧬 Code graph analysis (1)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (2)
app/src/features/apiClient/helpers/modules/scriptsV2/worker/script-internals/variableScope.ts (1)
VariableScope(43-84)app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/hooks/useScopeOptions.ts (1)
getScopeIcon(42-67)
🔇 Additional comments (4)
app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/components/VariableNotFound.tsx (1)
1-44: LGTM! RBAC integration looks correct.The RBAC permission check for
api_client_environmentcreate is appropriate, and the button is correctly disabled when the user lacks the required permission.app/src/componentsV2/CodeEditor/components/EditorV2/components/VariablePopOver/index.tsx (3)
1-48: LGTM! Type definitions are clean.The imports and type definitions support the new secret visibility feature well. The
InfoFieldConfiginterface with optionalisSecretappropriately models fields that can be masked.
207-215: LGTM! Value string conversion is straightforward.The
getValueStringshelper cleanly converts variable values to renderable strings with appropriate defaults.
314-325: Verify: Should revealing secrets require "update" permission?The eye toggle is wrapped in
RoleBasedComponentwith "update" permission, meaning users without update permission cannot reveal secrets at all. While this might be intentional security design, it could impact UX if users need to see secrets to use them but lack update permission.Please confirm whether:
- Revealing (viewing) a secret should require "update" permission, or
- Any user who can see the variable popover should be able to toggle visibility of secrets they already have access to (just masked).
The PR description doesn't explicitly mention RBAC restrictions on viewing, so clarifying the intended behavior would be helpful.
Closes issue:
📜 Summary of changes:
🎥 Demo Video:
Screen.Recording.2025-12-17.at.1.31.31.PM.mov
✅ Checklist:
🧪 Test instructions:
🔗 Other references:
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.