fix: Show Final Request Variables#7789
Open
karthikeyansundaram2 wants to merge 1 commit intousebruno:mainfrom
Open
fix: Show Final Request Variables#7789karthikeyansundaram2 wants to merge 1 commit intousebruno:mainfrom
karthikeyansundaram2 wants to merge 1 commit intousebruno:mainfrom
Conversation
karthikeyansundaram2
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
WalkthroughAdds a feature to display effective request variables in the Variables tab, showing final resolved values after applying precedence rules (Request > Folder > Collection > Environment). Includes a new utility function to compute effective variables, a React component to render them in a table with source attribution, and styling for the display. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/bruno-app/src/components/RequestPane/Vars/EffectiveVarsTable/index.js (2)
9-22:useMemodep list is doing a lot of heavy lifting.Deps include
item?.request,collection?.root,collection?.draft,collection?.items,collection?.environments— all object references. This relies on Redux giving you fresh references on every relevant update, which is generally true in this codebase but is a subtle coupling. SincegetEffectiveRequestVariablesis quite cheap (one Map traversal per scope), consider dropping the memo entirely and computing inline — simpler and no risk of stale values if a reducer ever mutates in place.♻️ Proposed simplification
- const variables = useMemo( - () => getEffectiveRequestVariables(collection, item), - [ - item?.uid, - item?.draft, - item?.request, - collection?.uid, - collection?.activeEnvironmentUid, - collection?.environments, - collection?.root, - collection?.draft, - collection?.items - ] - ); + const variables = getEffectiveRequestVariables(collection, item);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/RequestPane/Vars/EffectiveVarsTable/index.js` around lines 9 - 22, The useMemo around getEffectiveRequestVariables (creating variables) relies on many object-refs and is fragile; remove the useMemo and compute variables inline by directly calling getEffectiveRequestVariables(collection, item) where variables is used (replace the useMemo block that defines variables) so the value is always derived from current props/state without depending on item?.request, collection?.root, collection?.draft, collection?.items, or collection?.environments references.
39-53: Add adata-testidfor Playwright and consider a real<button>.Two small things:
- As per coding guidelines ("Add
data-testidto testable elements for Playwright"), the secret-toggle is a prime candidate.role="button"+tabIndex={0}+ manual Enter/Space handling is reinventing<button type="button">. A real button gives you keyboard behavior, focus ring, and disabled semantics for free.♻️ Proposed change
- <div - className="secret-toggle mb-2" - role="button" - tabIndex={0} - onClick={() => setShowSecret((s) => !s)} - onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - setShowSecret((s) => !s); - } - }} - > + <button + type="button" + className="secret-toggle mb-2" + data-testid="effective-vars-secret-toggle" + onClick={() => setShowSecret((s) => !s)} + > {showSecret ? <IconEyeOff size={16} strokeWidth={1.5} /> : <IconEye size={16} strokeWidth={1.5} />} <span>{showSecret ? 'Hide secret variable values' : 'Show secret variable values'}</span> - </div> + </button>As per coding guidelines: "Add
data-testidto testable elements for Playwright."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/RequestPane/Vars/EffectiveVarsTable/index.js` around lines 39 - 53, Replace the clickable div with a proper interactive element: change the element using className "secret-toggle" to a <button type="button"> (keep onClick handler that calls setShowSecret and preserve IconEye/IconEyeOff and the label span), remove role="button", tabIndex and manual onKeyDown logic, and add a data-testid attribute like data-testid="secret-toggle" for Playwright; also set an ARIA state such as aria-pressed={showSecret} to reflect the toggle state so assistive tech stays accurate.packages/bruno-app/src/utils/collections/index.js (1)
1337-1344: Minor: consistency withmergeVarsfor request-var extraction.
mergeVarsabove handles the request item via the tree-path loop (non-folder branch). Here you re-read request vars offitemdirectly. Functionally equivalent for the current call sites, but ifitemis ever absent fromrequestTreePath(e.g., transient/detached items), behavior could diverge. Not blocking — just flagging for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/collections/index.js` around lines 1337 - 1344, The request variable extraction repeats direct access to item (const requestVars = item.draft ? get(item, 'draft.request.vars.req', []) : get(item, 'request.vars.req', [])) which can diverge from the earlier mergeVars logic that uses the requestTreePath traversal; change this block to obtain the request node via the same requestTreePath loop used in mergeVars (use the same traversal logic to find the request node before reading vars) and then iterate that node's 'request.vars.req' (preserving the draft vs non-draft selection and filtering by v.name && v.enabled) before calling result.set, so behavior remains consistent even for transient/detached items (refer to mergeVars and requestTreePath handling to mirror the lookup).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/components/RequestPane/Vars/index.js`:
- Around line 22-27: The displayed precedence label in the Vars component is
inconsistent with the canonical order referenced in utils/collections/index.js
and getEffectiveRequestVariables; update the label text in the JSX (the span
inside the "Effective Variables" header) to match the confirmed canonical order
(either "Request > Folder > Collection > Environment" or the opposite), and then
ensure getEffectiveRequestVariables, its JSDoc, and any resolver in
utils/collections/index.js use the exact same ordering; pick the canonical
precedence, update the header string, update the JSDoc comment for
getEffectiveRequestVariables, and adjust the resolver logic in
utils/collections/index.js so all three (label, JSDoc, resolver) match exactly.
---
Nitpick comments:
In
`@packages/bruno-app/src/components/RequestPane/Vars/EffectiveVarsTable/index.js`:
- Around line 9-22: The useMemo around getEffectiveRequestVariables (creating
variables) relies on many object-refs and is fragile; remove the useMemo and
compute variables inline by directly calling
getEffectiveRequestVariables(collection, item) where variables is used (replace
the useMemo block that defines variables) so the value is always derived from
current props/state without depending on item?.request, collection?.root,
collection?.draft, collection?.items, or collection?.environments references.
- Around line 39-53: Replace the clickable div with a proper interactive
element: change the element using className "secret-toggle" to a <button
type="button"> (keep onClick handler that calls setShowSecret and preserve
IconEye/IconEyeOff and the label span), remove role="button", tabIndex and
manual onKeyDown logic, and add a data-testid attribute like
data-testid="secret-toggle" for Playwright; also set an ARIA state such as
aria-pressed={showSecret} to reflect the toggle state so assistive tech stays
accurate.
In `@packages/bruno-app/src/utils/collections/index.js`:
- Around line 1337-1344: The request variable extraction repeats direct access
to item (const requestVars = item.draft ? get(item, 'draft.request.vars.req',
[]) : get(item, 'request.vars.req', [])) which can diverge from the earlier
mergeVars logic that uses the requestTreePath traversal; change this block to
obtain the request node via the same requestTreePath loop used in mergeVars (use
the same traversal logic to find the request node before reading vars) and then
iterate that node's 'request.vars.req' (preserving the draft vs non-draft
selection and filtering by v.name && v.enabled) before calling result.set, so
behavior remains consistent even for transient/detached items (refer to
mergeVars and requestTreePath handling to mirror the lookup).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7100c760-43e5-4a4f-b977-f42c3e63ab90
📒 Files selected for processing (4)
packages/bruno-app/src/components/RequestPane/Vars/EffectiveVarsTable/StyledWrapper.jspackages/bruno-app/src/components/RequestPane/Vars/EffectiveVarsTable/index.jspackages/bruno-app/src/components/RequestPane/Vars/index.jspackages/bruno-app/src/utils/collections/index.js
Comment on lines
+22
to
+27
| <div className="mt-3 mb-3 title text-xs"> | ||
| Effective Variables | ||
| <span className="ml-2 text-xs font-normal opacity-70"> | ||
| Request > Folder > Environment > Collection | ||
| </span> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Label precedence disagrees with issue #5456.
The header reads Request > Folder > Environment > Collection, which matches the implementation in getEffectiveRequestVariables but contradicts the linked issue (Request > Folder > Collection > Environment). Align this with whichever precedence is confirmed canonical (see the related comment on utils/collections/index.js). Whatever you pick, the label, the JSDoc, and the resolver must all agree.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/RequestPane/Vars/index.js` around lines 22
- 27, The displayed precedence label in the Vars component is inconsistent with
the canonical order referenced in utils/collections/index.js and
getEffectiveRequestVariables; update the label text in the JSX (the span inside
the "Effective Variables" header) to match the confirmed canonical order (either
"Request > Folder > Collection > Environment" or the opposite), and then ensure
getEffectiveRequestVariables, its JSDoc, and any resolver in
utils/collections/index.js use the exact same ordering; pick the canonical
precedence, update the header string, update the JSDoc comment for
getEffectiveRequestVariables, and adjust the resolver logic in
utils/collections/index.js so all three (label, JSDoc, resolver) match exactly.
Author
|
Thanks for the review! I'll look into the suggestions:
Will push an update if the suggestions improve correctness or edge case handling. |
Contributor
|
Hey @karthikeyansundaram2 Thanks for the PR! It would be great if you could share a demo video. Also, just a heads-up that this issue is currently in our backlog, and it may take some time before our team is able to pick it up. |
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Closes #5456
Addresses: Show Final Request Variables
Changes
Contribution by @karthikeyansundaram2
Summary by CodeRabbit