feat#6256: Remember last value for Prompt Variables and provide it as a suggestion next time #6279
Conversation
NikHillAgar
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
WalkthroughAdds an opt-in "remember prompt value" toggle and plumbing: Modal gains new props and footer toggle; PromptVariables provider holds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js (2)
68-68: Simplify the comment per coding guidelines.The comment is too verbose. Coding guidelines specify meaningful comments over obvious ones and avoiding hand-holding explanations.
-// autoFocus={index === 0} --> AutoFocus breaks the code. AutoFocus does not seem to provide such a big enhancement to make the code more complicated +// autoFocus removed to prevent conflicts with suggestion dropdown
100-100: Use null instead of empty fragment.Per coding guidelines, prefer
nullfor empty JSX returns.-) : <></>} +)}Or if you need the ternary:
-) : <></>} +) : null}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js(3 hunks)packages/bruno-app/src/providers/PromptVariables/index.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.jspackages/bruno-app/src/providers/PromptVariables/index.js
🧬 Code graph analysis (1)
packages/bruno-app/src/providers/PromptVariables/index.js (1)
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js (1)
values(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Windows
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
🔇 Additional comments (4)
packages/bruno-app/src/providers/PromptVariables/index.js (2)
47-47: LGTM!Passing
promptVariablesas a prop correctly threads the persisted state to the modal component.
2-2: Verify the useLocalStorage hook exists and review its implementation.The code imports and uses
useLocalStoragefromhooks/useLocalStorage. Ensure this hook is properly implemented and handles edge cases like storage quota exceeded or invalid JSON.packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js (2)
1-1: LGTM!The import and function signature changes correctly add support for the
promptVariablesprop and theuseRefhook.Also applies to: 7-7
93-93: Good use of onMouseDown to prevent blur.The
onMouseDownwithpreventDefault()correctly handles the click-vs-blur race condition, and the comment provides valuable context with a reference.
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js (2)
15-29: Consider state-based visibility instead of direct DOM manipulation.While the current implementation works, directly modifying
node.style.displaybypasses React's declarative model. A state-based approach (e.g.,visibleSuggestionsobject or Set) would be more idiomatic, though the current method does avoid re-renders.Example state-based alternative:
-const suggestionsRef = useRef(); +const [visibleSuggestions, setVisibleSuggestions] = useState(new Set()); const handleOnFocus = (prompt) => { - const map = getSuggestionMap(); - if (!map.has(prompt)) return; - - const node = map.get(prompt); - node.style.display = 'block'; + if (prompt in promptVariables) { + setVisibleSuggestions((prev) => new Set(prev).add(prompt)); + } }; const handleOnBlur = (prompt) => { - const map = getSuggestionMap(); - if (!map.has(prompt)) return; - - const node = map.get(prompt); - node.style.display = 'none'; + setVisibleSuggestions((prev) => { + const next = new Set(prev); + next.delete(prompt); + return next; + }); };Then use
style={{ display: visibleSuggestions.has(prompt) ? 'block' : 'none' }}in the suggestion div.
96-98: Consider simplifying the suggestion markup.Using
<ul><li>for a single item is semantically unusual. A simple<div>would be cleaner and more appropriate.-<ul> - <li>{promptVariables[prompt]}</li> -</ul> +<div>{promptVariables[prompt]}</div>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js
🧠 Learnings (1)
📚 Learning: 2025-12-02T20:48:05.634Z
Learnt from: NikHillAgar
Repo: usebruno/bruno PR: 6279
File: packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js:79-86
Timestamp: 2025-12-02T20:48:05.634Z
Learning: In React 19+, ref callbacks support returning cleanup functions. When a ref callback returns a function, React will call it when the element unmounts or the ref changes, similar to useEffect cleanup. This is documented at https://react.dev/learn/manipulating-the-dom-with-refs#how-to-manage-a-list-of-refs-using-a-ref-callback
Applied to files:
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js
🧬 Code graph analysis (1)
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js (1)
packages/bruno-app/src/providers/PromptVariables/index.js (2)
prompt(11-15)promptVariables(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: CLI Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - macOS
🔇 Additional comments (5)
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js (5)
1-1: LGTM!Import addition is clean and necessary for the ref-based suggestion tracking.
7-7: LGTM!The new
promptVariablesprop integrates cleanly with existing props.
9-9: LGTM!Ref initialization for the suggestions map is correct.
31-37: LGTM!Lazy initialization pattern for the ref-based Map is correct.
68-70: LGTM!The inline comment explaining the autoFocus removal is helpful, and the event handlers are correctly wired.
| {prompt in promptVariables | ||
| ? ( | ||
| <div | ||
| ref={(node) => { | ||
| const map = getSuggestionMap(); | ||
| map.set(prompt, node); | ||
|
|
||
| return () => { | ||
| map.delete(prompt); | ||
| }; | ||
| }} | ||
| data-testid="suggestion-container" | ||
| className="bg-black rounded-md p-3 text-white" | ||
| style={{ display: 'none', cursor: 'pointer' }} | ||
| // If only onClick is used onBlur is triggered first | ||
| // Below stackoverflow comment has the solution with explaination | ||
| // https://stackoverflow.com/questions/17769005/onclick-and-onblur-ordering-issue/57630197#57630197 | ||
| onMouseDown={(e) => e.preventDefault()} | ||
| onClick={() => handleChange(prompt, promptVariables[prompt])} | ||
| > | ||
| <ul> | ||
| <li>{promptVariables[prompt]}</li> | ||
| </ul> | ||
| </div> | ||
| ) : null} |
There was a problem hiding this comment.
Add accessibility features for the suggestion dropdown.
The suggestion dropdown is missing critical accessibility attributes and keyboard navigation:
- ARIA attributes: The input should have
aria-controlspointing to the suggestion container, and the container needsrole="listbox"or similar - Keyboard navigation: Users should be able to navigate with arrow keys and select with Enter
- Screen reader announcements: The suggestion availability should be announced when the input receives focus
These gaps prevent keyboard-only and screen-reader users from accessing the suggestion feature.
Example ARIA improvements:
<input
id={`prompt-${index}`}
+ aria-controls={`suggestion-${index}`}
+ aria-expanded={/* track if suggestion is visible */}
+ aria-autocomplete="list"
type="text"
...
/>
<div
+ id={`suggestion-${index}`}
+ role="listbox"
+ aria-label="Suggestion"
ref={(node) => { ... }}
...
>
- <ul>
- <li>{promptVariables[prompt]}</li>
- </ul>
+ <div role="option" aria-selected="false">
+ {promptVariables[prompt]}
+ </div>
</div>Additionally, consider handling keyboard events (ArrowDown/Enter to select, Escape to close).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js
around lines 76-100, the suggestion dropdown lacks ARIA attributes and keyboard
handling; add an id to the suggestion container and set role="listbox" and
data-testid as already present, add aria-controls on the related input (pointing
to that id) and manage aria-activedescendant on the input to reflect the
highlighted suggestion; implement keyboard handlers on the input to open the
list, move a focused index with ArrowDown/ArrowUp, select with Enter (calling
handleChange), and close with Escape while preventing default mouse down
behavior as you already do; finally provide live region announcements for when
suggestions appear (aria-live="polite" or a visually-hidden announcer) and
ensure each suggestion item has role="option" and an id that matches
aria-activedescendant so screen readers and keyboard users can navigate and
select items.
|
Hey Thanks for the PR @NikHillAgar I think @giuseppemilicia in #6256 is talking about some sort of checkbox to toggle in the form to auto-remember these fields instead of providing dropdown options |
|
@helloanoop My bad. The code can be changed so instead of showing drop-down it will automatically fill the last value used. Also I can give the toggle in the settings to auto-remember these fields. If not selected prompt values will not be auto filled Also should the app restricts on how many of these prompt variables it will store in the local storage? |
|
Auto toggle is not needed for each field. It'll be for the whole modal, in the bottom left corner. |
|
Auto Remember Toggle:
UI: It would kind of look like below |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/bruno-app/src/components/Modal/index.js(4 hunks)packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js(2 hunks)packages/bruno-app/src/providers/PromptVariables/index.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-app/src/providers/PromptVariables/index.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation, never tabs
Use single quotes for strings instead of double quotes
Always add semicolons at the end of statements
No trailing commas in code
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line with a minimum of 2 elements for multiline formatting
No newlines inside function parentheses
Space before and after the arrow in arrow functions:() => {}
No space between function name and parentheses:func()notfunc ()
Semicolons should go at the end of the line, not on a new line
Function names should be concise and descriptive
Add JSDoc comments to provide details to abstractions
Avoid single-line abstractions where all that is being done is increasing the call stack with one additional function
Add meaningful comments to explain complex code flow instead of obvious comments
Files:
packages/bruno-app/src/components/Modal/index.jspackages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js
🧠 Learnings (1)
📚 Learning: 2025-12-02T20:48:05.634Z
Learnt from: NikHillAgar
Repo: usebruno/bruno PR: 6279
File: packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js:79-86
Timestamp: 2025-12-02T20:48:05.634Z
Learning: In React 19+, ref callbacks support returning cleanup functions. When a ref callback returns a function, React will call it when the element unmounts or the ref changes, similar to useEffect cleanup. This is documented at https://react.dev/learn/manipulating-the-dom-with-refs#how-to-manage-a-list-of-refs-using-a-ref-callback
Applied to files:
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js
🧬 Code graph analysis (2)
packages/bruno-app/src/components/Modal/index.js (1)
packages/bruno-app/src/providers/PromptVariables/index.js (1)
savePromptValues(9-9)
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js (1)
packages/bruno-app/src/providers/PromptVariables/index.js (2)
savePromptValues(9-9)promptValues(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - macOS
- GitHub Check: Playwright E2E Tests
- GitHub Check: Unit Tests
- GitHub Check: CLI Tests
🔇 Additional comments (7)
packages/bruno-app/src/components/Modal/index.js (3)
22-34: LGTM!The new props are integrated cleanly into ModalFooter's signature.
67-89: LGTM!Default prop values are sensible: the toggle is hidden by default, savePromptValues starts false, and the setter is a no-op.
146-158: LGTM!Props are correctly passed through to ModalFooter.
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js (4)
7-7: LGTM!The new props extend the API cleanly to support the remember-prompt feature.
8-10: LGTM!The lazy initialization pattern is efficient, and the conditional logic correctly applies promptValues only when savePromptValues is enabled.
34-34: LGTM!Adding bottom margin balances the spacing now that the footer contains the toggle.
25-27: Verify persistence mechanism aligns with requirements.The PR description states that prompt variables are stored in local storage for persistence across sessions, but helloanoop suggested in-memory storage without persistence across app restarts. The code snippet shows only
useStatewith nolocalStoragecalls visible. Confirm where and how persistence is implemented—whether through a custom hook, a provider context, or another mechanism—and ensure it matches the agreed requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/bruno-app/src/components/Modal/index.js (2)
44-47: Checkbox wiring and accessibility are now correct; consider unique IDs if multiple modals can co-existThe checkbox now uses a boolean
checked={savePromptValues}and is correctly associated with its text viaid/htmlFor, which resolves the earlier correctness and a11y concerns. If there is any chance of more than one modal with this toggle being mounted at the same time, consider deriving a per-instance ID (e.g. viauseId) instead of the hard-codedpromptToggleto avoid duplicate IDs in the DOM.
67-89: New Modal defaults maintain existing behavior while enabling the toggleDefaulting
hidePromptToggletotrueandsavePromptValuestofalse, with a no-opsetSavePromptValues, preserves all existing usages while allowing callers likePromptVariablesModalto opt in. Just ensure any caller that setshidePromptToggle={false}always provides a realsetSavePromptValuesto avoid a silent no-op on toggle.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-app/src/components/Modal/index.js(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation, never tabs
Use single quotes for strings instead of double quotes
Always add semicolons at the end of statements
No trailing commas in code
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line with a minimum of 2 elements for multiline formatting
No newlines inside function parentheses
Space before and after the arrow in arrow functions:() => {}
No space between function name and parentheses:func()notfunc ()
Semicolons should go at the end of the line, not on a new line
Function names should be concise and descriptive
Add JSDoc comments to provide details to abstractions
Avoid single-line abstractions where all that is being done is increasing the call stack with one additional function
Add meaningful comments to explain complex code flow instead of obvious comments
Files:
packages/bruno-app/src/components/Modal/index.js
🧬 Code graph analysis (1)
packages/bruno-app/src/components/Modal/index.js (1)
packages/bruno-app/src/providers/PromptVariables/index.js (1)
savePromptValues(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: CLI Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - Linux
🔇 Additional comments (2)
packages/bruno-app/src/components/Modal/index.js (2)
22-35: ModalFooter prop surface extension looks sound and backward compatibleAdding
hidePromptToggle,savePromptValues, andsetSavePromptValueshere is clean and keeps existing callers working since defaults are handled at theModallevel. No issues from a typing or behavioral standpoint in this file.
145-158: Prop plumbing from Modal to ModalFooter is correct
hidePromptToggle,savePromptValues, andsetSavePromptValuesare forwarded as expected alongside the existing footer props, so the footer toggle will track the same state that the provider manages. This matches the provider snippet (useState(false)) and should integrate cleanly.
|
@helloanoop I have made the changes. Please review. |
|
@NikHillAgar The checkbox should be in the left side of the footer of the modal |
|
@helloanoop Below is the screenshot of updated UI. Checkbox is in the left side of the footer |
|
@helloanoop @bijin-bruno @lohit-bruno @naman-bruno Please review |
2 participants
Description
This PR addresses the enhancement request# 6256.
The changes store the prompt variables in-memory.
feat6256.mp4
Contribution Checklist:
Summary by CodeRabbit
New Features
Style / UI
✏️ Tip: You can customize this high-level summary in your review settings.