Add OverlayMetricsManager to avoid workspace shift when flyout is open#508
Add OverlayMetricsManager to avoid workspace shift when flyout is open#508tracygardner wants to merge 3 commits intomainfrom
Conversation
Deploying flockdev with
|
| Latest commit: |
a361e5e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c01e7cc8.flockdev.pages.dev |
| Branch Preview URL: | https://codex-locate-toolbox-workaro.flockdev.pages.dev |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughReplaces a workspace translate monkey-patch with a new Changes
Sequence Diagram(s)sequenceDiagram
participant Workspace
participant OverlayMetricsManager
participant Flyout
participant Toolbox
Workspace->>OverlayMetricsManager: getAbsoluteMetrics()
OverlayMetricsManager->>Flyout: Check existence, visibility, autoClose, dimensions
OverlayMetricsManager->>Toolbox: Query position (LEFT/TOP/RIGHT/BOTTOM)
alt Flyout present and not autoClose
OverlayMetricsManager->>OverlayMetricsManager: Compute offsets / stable toolbox metrics
OverlayMetricsManager-->>Workspace: Return adjusted absolute metrics
else
OverlayMetricsManager-->>Workspace: Return original absolute metrics
end
Workspace->>OverlayMetricsManager: getViewMetrics()
OverlayMetricsManager->>Flyout: Get scaled/visible dimensions
OverlayMetricsManager->>Toolbox: Get position
alt Flyout present and not autoClose
OverlayMetricsManager->>OverlayMetricsManager: Expand/offset view bounds by scaled flyout size
OverlayMetricsManager-->>Workspace: Return adjusted view metrics
else
OverlayMetricsManager-->>Workspace: Return original view metrics
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/blocklyinit.js`:
- Around line 615-617: The code assumes getToolboxMetrics() always returns an
object and immediately accesses .position, which can throw if there's no
toolbox; modify the logic around the toolboxPosition assignment in
blocklyinit.js to first call const toolboxMetrics = this.getToolboxMetrics() and
check for null/undefined before reading toolboxMetrics.position (e.g., set
toolboxPosition to null or a sensible default when toolboxMetrics is falsy), and
ensure subsequent use of toolboxPosition and Blockly.utils.toolbox.Position
handles the missing-toolbox case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const flyoutMetrics = this.getFlyoutMetrics(); | ||
| const toolboxPosition = this.getToolboxMetrics().position; | ||
| const Position = Blockly.utils.toolbox.Position; |
There was a problem hiding this comment.
Potential null access if toolbox is absent.
If getToolboxMetrics() returns null (e.g., workspace without a toolbox), accessing .position will throw. Consider guarding:
const flyoutMetrics = this.getFlyoutMetrics();
const toolboxPosition = this.getToolboxMetrics().position;
+const toolboxMetrics = this.getToolboxMetrics();
+if (!toolboxMetrics) return absolute;
+const toolboxPosition = toolboxMetrics.position;This is a minor edge case but worth defensive handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/blocklyinit.js` around lines 615 - 617, The code assumes
getToolboxMetrics() always returns an object and immediately accesses .position,
which can throw if there's no toolbox; modify the logic around the
toolboxPosition assignment in blocklyinit.js to first call const toolboxMetrics
= this.getToolboxMetrics() and check for null/undefined before reading
toolboxMetrics.position (e.g., set toolboxPosition to null or a sensible default
when toolboxMetrics is falsy), and ensure subsequent use of toolboxPosition and
Blockly.utils.toolbox.Position handles the missing-toolbox case.
1 participant
Motivation
Description
OverlayMetricsManager(subclass ofBlockly.MetricsManager) that overridesgetAbsoluteMetricsandgetViewMetricsto adjust metrics when the flyout is visible andautoCloseis false, takingtoolboxposition and workspacescaleinto account.workspace.setMetricsManager(new OverlayMetricsManager(workspace))immediately afterBlockly.inject.simpleNoBumpTranslatemonkeypatch that alteredworkspace.translateto remove the flyout-width bump.Testing
Codex Task
Summary by CodeRabbit