Clean up app/map async: resolve missing slider widget issue#40
Clean up app/map async: resolve missing slider widget issue#40
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix cases where the multilayer slider widget fails to render when loading the visualize page from a preloaded map state hash/bookmark URL, by removing brittle fixed-delay setTimeout usage and replacing it with readiness checks/polling.
Changes:
- Delay hash-state layer activation until
app.map.zoomis available (polling up to a timeout). - Replace fixed-delay tab selection with a helper that waits for the tab element/plugin before calling
.tab('show'). - Add a guard flag (
_sliderBuilding) to reduce duplicate multilayer slider build attempts; minor cleanup in map context-menu init.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| visualize/static/js/state.js | Reworks state-restore sequencing (layer activation + tab showing) using polling helpers instead of fixed delays |
| visualize/static/js/models.js | Adds _sliderBuilding flag to prevent duplicate multilayer slider build scheduling |
| visualize/static/js/map.js | Removes an obsolete commented-out context menu init line |
| visualize/static/js/app.js | Replaces delayed KO binding with a menus/menuModel readiness poll before applying bindings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| ko.applyBindings(app.viewModel); | ||
| } catch (e) { | ||
| console.warn('Error applying KO bindings:', e); | ||
| console.warn('if this is about "cannot apply bindings multiple times to the same element", this is expected. menuModel should already have been applied to the context menu'); | ||
| } |
There was a problem hiding this comment.
waitForMenusLoadToApplyKOBindings calls ko.applyBindings(app.viewModel) without a root element, which binds the entire document body. Since map.js already applies KO bindings to #context-menu, applying the viewModel to the body will throw "cannot apply bindings multiple times" and (because the exception is caught) can leave the rest of the page unbound. Instead of swallowing the error, scope the viewModel bindings to a container that excludes #context-menu (e.g., #primary-content from planner.html) or otherwise ensure binding roots don’t overlap, and only treat/handle the specific expected error if needed.
| try { | |
| ko.applyBindings(app.viewModel); | |
| } catch (e) { | |
| console.warn('Error applying KO bindings:', e); | |
| console.warn('if this is about "cannot apply bindings multiple times to the same element", this is expected. menuModel should already have been applied to the context menu'); | |
| } | |
| var bindingRoot = document.getElementById('primary-content'); | |
| if (!bindingRoot) { | |
| if (Date.now() - startTime < maxWaitTime) { | |
| setTimeout(checkElement, 50); // Check every 50ms | |
| return false; | |
| } | |
| console.warn('KO binding root #primary-content not found after waiting:', maxWaitTime/1000, 'seconds'); | |
| return false; | |
| } | |
| ko.applyBindings(app.viewModel, bindingRoot); |
There was a problem hiding this comment.
Currently infeasible - More than just #primary-content needs the viewModel binding. Resolving this should be the work of a separate PR (or a full rebuild)
There was a problem hiding this comment.
See commit fa2d09f - not quite build like this solution, but resolves the binding overlap issue (WOO HOO!)
Verbose, but nice for these edge cases! Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ups issue; removing unused footer
Some users reported the slider widget would not render when loading the visaulize page with a pre-loaded map state hash (or possibly bookmark or shortened URL).
This PR removes a lot of bad 'setTimeout' logic and replaces it with better loops that act only when the necessary elements exist (or times out).