BL-16069 Disable tools during Change Layout mode#7828
BL-16069 Disable tools during Change Layout mode#7828
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/BloomBrowserUI/app/EditTabPane.tsx">
<violation number="1" location="src/BloomBrowserUI/app/EditTabPane.tsx:46">
P1: Toolbox visibility is not restored when re-enabled. When entering Change Layout mode, `toolboxIsShowing` is set to `false`, but when `enabled` becomes `true` again the handler doesn't restore it. This means after exiting Change Layout mode the toolbox stays hidden even though the toggle is re-enabled.
Consider saving the previous `toolboxIsShowing` value and restoring it when re-enabled.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
|
|
||
| React.useEffect(() => { | ||
| // We will use this to disable and hide the toolbox when in Change Layout mode (BL-16069) | ||
| setToolboxEnabledHandler((enabled) => { |
There was a problem hiding this comment.
P1: Toolbox visibility is not restored when re-enabled. When entering Change Layout mode, toolboxIsShowing is set to false, but when enabled becomes true again the handler doesn't restore it. This means after exiting Change Layout mode the toolbox stays hidden even though the toggle is re-enabled.
Consider saving the previous toolboxIsShowing value and restoring it when re-enabled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/BloomBrowserUI/app/EditTabPane.tsx, line 46:
<comment>Toolbox visibility is not restored when re-enabled. When entering Change Layout mode, `toolboxIsShowing` is set to `false`, but when `enabled` becomes `true` again the handler doesn't restore it. This means after exiting Change Layout mode the toolbox stays hidden even though the toggle is re-enabled.
Consider saving the previous `toolboxIsShowing` value and restoring it when re-enabled.</comment>
<file context>
@@ -39,6 +41,20 @@ export const EditTabPane: React.FunctionComponent<{ active: boolean }> = (
+ React.useEffect(() => {
+ // We will use this to disable and hide the toolbox when in Change Layout mode (BL-16069)
+ setToolboxEnabledHandler((enabled) => {
+ setToolboxIsEnabled(enabled);
+ if (!enabled) {
</file context>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| setToolboxEnabledHandler((enabled) => { | ||
| setToolboxIsEnabled(enabled); | ||
| if (!enabled) { | ||
| setToolboxIsShowing(false); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🔴 Toolbox showing state not restored when toolbox is re-enabled after exiting Change Layout mode
When the toolbox is disabled (entering Change Layout mode), the handler at EditTabPane.tsx:48-50 sets toolboxIsShowing to false. However, when the toolbox is re-enabled (exiting Change Layout mode), the handler only calls setToolboxIsEnabled(true) — it never restores toolboxIsShowing back to true. This means the toolbox remains hidden after leaving Change Layout mode, even though it was visible before entering it. The user must manually click the toggle label to show the toolbox again.
The frameSources fetch that initializes toolboxIsShowing from the server (EditTabPane.tsx:40) only runs when props.active changes, not on page reloads within the edit tab, so the page reload triggered by postThatMightNavigate in origami.ts:191 does not restore the state either.
| setToolboxEnabledHandler((enabled) => { | |
| setToolboxIsEnabled(enabled); | |
| if (!enabled) { | |
| setToolboxIsShowing(false); | |
| } | |
| }); | |
| setToolboxEnabledHandler((enabled) => { | |
| setToolboxIsEnabled(enabled); | |
| if (!enabled) { | |
| setToolboxIsShowing(false); | |
| } else { | |
| setToolboxIsShowing(true); | |
| } | |
| }); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| export function setToolboxEnabledHandler( | ||
| handler: ((enabled: boolean) => void) | undefined, | ||
| ): void { | ||
| toolboxEnabledHandler = handler; | ||
| toolboxEnabledHandler?.(toolboxEnabled); | ||
| } | ||
|
|
||
| export function setToolboxEnabled(enabled: boolean): void { | ||
| toolboxEnabled = enabled; | ||
| toolboxEnabledHandler?.(enabled); | ||
| } |
There was a problem hiding this comment.
🔴 Missing comments on new exported functions setToolboxEnabledHandler and setToolboxEnabled
Per AGENTS.md: "All public methods should have a comment. So should most private ones!" Both setToolboxEnabledHandler (line 196) and setToolboxEnabled (line 203) are exported (public) functions without individual doc comments. The comment at line 192 is a section-level comment for the variable declarations, not a function-level doc comment for either function.
| export function setToolboxEnabledHandler( | |
| handler: ((enabled: boolean) => void) | undefined, | |
| ): void { | |
| toolboxEnabledHandler = handler; | |
| toolboxEnabledHandler?.(toolboxEnabled); | |
| } | |
| export function setToolboxEnabled(enabled: boolean): void { | |
| toolboxEnabled = enabled; | |
| toolboxEnabledHandler?.(enabled); | |
| } | |
| // Registers a handler that will be called whenever the toolbox enabled state changes. | |
| // The handler is immediately invoked with the current enabled state when registered. | |
| export function setToolboxEnabledHandler( | |
| handler: ((enabled: boolean) => void) | undefined, | |
| ): void { | |
| toolboxEnabledHandler = handler; | |
| toolboxEnabledHandler?.(toolboxEnabled); | |
| } | |
| // Sets whether the toolbox is enabled. When disabled (e.g. in Change Layout mode), | |
| // the toolbox toggle is grayed out and the toolbox panel is hidden. | |
| export function setToolboxEnabled(enabled: boolean): void { | |
| toolboxEnabled = enabled; | |
| toolboxEnabledHandler?.(enabled); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| React.useEffect(() => { | ||
| // We will use this to disable and hide the toolbox when in Change Layout mode (BL-16069) | ||
| setToolboxEnabledHandler((enabled) => { | ||
| setToolboxIsEnabled(enabled); | ||
| if (!enabled) { | ||
| setToolboxIsShowing(false); | ||
| } | ||
| }); | ||
|
|
||
| return () => { | ||
| setToolboxEnabledHandler(undefined); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🚩 Handler registration uses stale closure for reading state but this doesn't affect current code
The handler registered in the useEffect([], []) at line 44-56 captures the initial values of any local variables. However, the handler only calls state setters (setToolboxIsEnabled, setToolboxIsShowing) which are stable references and work correctly regardless of closure staleness. If future code needs to read toolboxIsShowing inside this handler (e.g., to save it before hiding), a useRef would be needed to avoid stale reads. This isn't a bug today but is worth noting for the suggested fix of BUG-0001 — a naive fix that tries to restore the previous showing state by reading toolboxIsShowing inside the handler would get the stale initial value.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This change is