fix/wsi-viewport-preserve-zoom-on-resize#2807
Conversation
📝 WalkthroughWalkthrough
ChangesWSI viewport resize
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/RenderingEngine/WSIViewport.ts`:
- Around line 459-473: Update the viewport resize logic around `relativeZoom`,
`oldSize`, and `newSize` to require a non-null projection extent and strictly
positive width and height for both map sizes before calling
`getResolutionForExtent`. Validate that the computed `relativeZoom` is finite
before applying `view.setResolution`, while preserving the existing behavior for
valid dimensions and extents.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 144b37b3-f24b-473f-aa4d-61159f6d81a0
📒 Files selected for processing (1)
packages/core/src/RenderingEngine/WSIViewport.ts
| const extent = view.getProjection().getExtent(); | ||
| const oldSize = map.getSize(); | ||
| const oldResolution = view.getResolution(); | ||
| let relativeZoom; | ||
| if (oldSize && oldResolution) { | ||
| relativeZoom = | ||
| view.getResolutionForExtent(extent, oldSize) / oldResolution; | ||
| } | ||
| map.updateSize(); | ||
| const newSize = map.getSize(); | ||
| if (relativeZoom && newSize) { | ||
| view.setResolution( | ||
| view.getResolutionForExtent(extent, newSize) / relativeZoom | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Guard against zero dimensions and invalid extents to prevent OpenLayers crashes.
When the viewport container is temporarily hidden (e.g., inside a collapsed panel or display: none) or initialized with zero dimensions, map.getSize() returns [0, 0]. Since arrays are truthy in JavaScript, the code proceeds and computes relativeZoom as Infinity (due to division by zero in getResolutionForExtent).
When the container is later shown and resized, dividing by Infinity leads to view.setResolution(0), which corrupts the OpenLayers view state. Additionally, view.getProjection().getExtent() can return null, which causes getResolutionForExtent to throw an error.
Ensure the sizes strictly have width and height > 0 and that the resulting relativeZoom is finite before applying it.
🔒️ Proposed fix to add robust boundary checks
- const extent = view.getProjection().getExtent();
+ const extent = view.getProjection()?.getExtent();
const oldSize = map.getSize();
const oldResolution = view.getResolution();
let relativeZoom;
- if (oldSize && oldResolution) {
+ if (extent && oldSize && oldSize[0] > 0 && oldSize[1] > 0 && oldResolution > 0) {
relativeZoom =
view.getResolutionForExtent(extent, oldSize) / oldResolution;
}
map.updateSize();
const newSize = map.getSize();
- if (relativeZoom && newSize) {
+ if (relativeZoom > 0 && Number.isFinite(relativeZoom) && newSize && newSize[0] > 0 && newSize[1] > 0) {
view.setResolution(
view.getResolutionForExtent(extent, newSize) / relativeZoom
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const extent = view.getProjection().getExtent(); | |
| const oldSize = map.getSize(); | |
| const oldResolution = view.getResolution(); | |
| let relativeZoom; | |
| if (oldSize && oldResolution) { | |
| relativeZoom = | |
| view.getResolutionForExtent(extent, oldSize) / oldResolution; | |
| } | |
| map.updateSize(); | |
| const newSize = map.getSize(); | |
| if (relativeZoom && newSize) { | |
| view.setResolution( | |
| view.getResolutionForExtent(extent, newSize) / relativeZoom | |
| ); | |
| } | |
| const extent = view.getProjection()?.getExtent(); | |
| const oldSize = map.getSize(); | |
| const oldResolution = view.getResolution(); | |
| let relativeZoom; | |
| if (extent && oldSize && oldSize[0] > 0 && oldSize[1] > 0 && oldResolution > 0) { | |
| relativeZoom = | |
| view.getResolutionForExtent(extent, oldSize) / oldResolution; | |
| } | |
| map.updateSize(); | |
| const newSize = map.getSize(); | |
| if (relativeZoom > 0 && Number.isFinite(relativeZoom) && newSize && newSize[0] > 0 && newSize[1] > 0) { | |
| view.setResolution( | |
| view.getResolutionForExtent(extent, newSize) / relativeZoom | |
| ); | |
| } |
🧰 Tools
🪛 GitHub Actions: Docusaurus Build / 0_docusaurus-build.txt
[error] 461-461: TypeScript (tsc) failed with TS2339: Property 'getSize' does not exist on type 'WSIMapLike'.
🪛 GitHub Actions: Docusaurus Build / docusaurus-build
[error] 461-461: TypeScript (tsc) failed: TS2339: Property 'getSize' does not exist on type 'WSIMapLike'.
🪛 GitHub Actions: Validate ESM Packaging / 1_build-packages.txt
[error] 461-461: TypeScript (tsc) failed with TS2339: Property 'getSize' does not exist on type 'WSIMapLike'.
🪛 GitHub Actions: Validate ESM Packaging / build-packages
[error] 461-461: TypeScript compilation failed (tsc). TS2339: Property 'getSize' does not exist on type 'WSIMapLike'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/RenderingEngine/WSIViewport.ts` around lines 459 - 473,
Update the viewport resize logic around `relativeZoom`, `oldSize`, and `newSize`
to require a non-null projection extent and strictly positive width and height
for both map sizes before calling `getResolutionForExtent`. Validate that the
computed `relativeZoom` is finite before applying `view.setResolution`, while
preserving the existing behavior for valid dimensions and extents.
1 participant
Context
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit