Conversation
Verbose, but nice for these edge cases! Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ups issue; removing unused footer
Clean up app/map async: resolve missing slider widget issue
There was a problem hiding this comment.
Pull request overview
Improves robustness of UI initialization/state restoration in the visualize planner by replacing brittle setTimeout sequencing with polling-based readiness checks, addressing KO binding conflicts, and preventing duplicate multilayer slider initialization.
Changes:
- Add
#modal-containerwrapper to scope Knockout bindings more narrowly and add an emptyfooterblock for extensibility. - Replace several
setTimeout-based flows with polling helpers to wait forapp.menuModel,app.map.zoom, and tab elements before proceeding. - Add a
_sliderBuildingflag on multilayer parent layers to prevent duplicate slider build triggers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| visualize/templates/visualize/planner.html | Wraps modal includes in #modal-container and introduces a footer block to support targeted KO bindings. |
| visualize/static/js/app.js | Replaces delayed ko.applyBindings with polling that waits for menus/menuModel readiness and applies bindings to scoped roots. |
| visualize/static/js/state.js | Polls for app.map.zoom before activating hash-state layers; adds a helper to wait for tabs before showing them. |
| visualize/static/js/models.js | Prevents duplicate multilayer slider initialization via _sliderBuilding flag. |
| visualize/static/js/map.js | Removes an obsolete commented-out context menu init line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| ko.applyBindings(app.viewModel, document.querySelector('#primary-content')); | ||
| } catch (e) { | ||
| console.error('Error applying KO bindings:', e); | ||
| } | ||
| try { | ||
| ko.applyBindings(app.viewModel, document.querySelector('#modal-container')); | ||
| } catch (e) { | ||
| console.error('Error applying KO bindings:', e); | ||
| } |
There was a problem hiding this comment.
ko.applyBindings is only applied to #primary-content / #modal-container. On pages that include this app.js but don’t have #primary-content (e.g. visualize/templates/visualize/mobile-map.html), document.querySelector('#primary-content') will be null, ko.applyBindings will throw, and the catch will prevent any bindings from being applied at all (breaking all data-bind UI on that page). Suggestion: detect the presence of these containers and (a) skip binding/logging for #modal-container when it’s absent, and (b) fall back to applying bindings on an appropriate root (e.g. document.body or a page-specific container) when #primary-content isn’t present, so non-planner templates keep working.
| try { | |
| ko.applyBindings(app.viewModel, document.querySelector('#primary-content')); | |
| } catch (e) { | |
| console.error('Error applying KO bindings:', e); | |
| } | |
| try { | |
| ko.applyBindings(app.viewModel, document.querySelector('#modal-container')); | |
| } catch (e) { | |
| console.error('Error applying KO bindings:', e); | |
| } | |
| var primaryContent = document.querySelector('#primary-content'); | |
| var modalContainer = document.querySelector('#modal-container'); | |
| var primaryBindingRoot = primaryContent || document.body; | |
| if (!primaryContent && primaryBindingRoot) { | |
| console.warn('#primary-content not found; applying KO bindings to document.body instead.'); | |
| } | |
| if (primaryBindingRoot) { | |
| try { | |
| ko.applyBindings(app.viewModel, primaryBindingRoot); | |
| } catch (e) { | |
| console.error('Error applying KO bindings to primary root:', e); | |
| } | |
| } else { | |
| console.warn('No root element available for KO bindings.'); | |
| } | |
| if ( | |
| modalContainer && | |
| modalContainer !== primaryBindingRoot && | |
| !primaryBindingRoot.contains(modalContainer) | |
| ) { | |
| try { | |
| ko.applyBindings(app.viewModel, modalContainer); | |
| } catch (e) { | |
| console.error('Error applying KO bindings to #modal-container:', e); | |
| } | |
| } |
| setTimeout(checkElement, 50); // Check every 50ms | ||
| return false; | ||
| } else { | ||
| console.warn('app.menus not found after waiting:', maxWaitTime/1000, 'seconds'); |
There was a problem hiding this comment.
The timeout warning message is misleading: the condition gating success checks both app.menus and app.menuModel.menuItems, but the warning always says only app.menus not found. This can send debugging in the wrong direction when menuModel is the missing piece. Suggest updating the warning to reflect both prerequisites (or log which one is still missing).
| console.warn('app.menus not found after waiting:', maxWaitTime/1000, 'seconds'); | |
| var missingRequirements = []; | |
| if (!(typeof app !== 'undefined' && app.hasOwnProperty('menus') && typeof app.menus === 'object')) { | |
| missingRequirements.push('app.menus'); | |
| } | |
| if (!(typeof app !== 'undefined' && app.hasOwnProperty('menuModel') && typeof app.menuModel !== 'undefined')) { | |
| missingRequirements.push('app.menuModel'); | |
| } else if (!(app.menuModel.hasOwnProperty('menuItems') && typeof app.menuModel.menuItems === 'function')) { | |
| missingRequirements.push('app.menuModel.menuItems'); | |
| } | |
| console.warn('Required KO binding prerequisites not found after waiting:', maxWaitTime/1000, 'seconds. Missing:', missingRequirements.join(', ')); |
| return; | ||
| } | ||
|
|
||
| if (app._waitingForMapZoom) { |
There was a problem hiding this comment.
app._waitingForMapZoom is a great addition for improved performance!
| checkElement(); | ||
| } | ||
|
|
||
| waitForMenusLoadToApplyKOBindings(); |
There was a problem hiding this comment.
I guess Anna's work only removed Knockout from the data layer menu 😢 .
This pull request focuses on improving the reliability and robustness of UI initialization and state restoration in the application's visualization interface. The main changes replace brittle
setTimeout-based logic with polling functions that wait for required DOM elements and JavaScript objects to be available before proceeding with Knockout.js bindings, layer activation, and tab switching. This approach helps prevent race conditions and ensures smoother user experience, especially during asynchronous loading scenarios.Resolve long-standing
Error applying KO bindings: Error: You cannot apply bindings multiple times to the same element.Improvements to UI Initialization and State Restoration:
Replaced
setTimeout-based Knockout.js bindings with a polling function (waitForMenusLoadToApplyKOBindings) that waits forapp.menusandapp.menuModel.menuItemsto be ready before applying bindings to#primary-contentand the new#modal-containerelements. This reduces the risk of partial UI loads and binding errors. [1] [2]Updated state restoration logic to remove arbitrary timeouts and instead poll for the existence of
app.map.zoombefore activating layers, preventing errors that occurred when layers were activated before the map was ready. Only one polling loop is allowed at a time to avoid redundant checks. [1] [2] [3]Improved tab restoration after loading compressed state by introducing a helper function (
waitForElementAndShowTab) that waits for the relevant tab element and its.tab()method to be available before calling.tab('show'). This eliminates race conditions related to tab switching and ensures the correct tab is shown after state restoration. [1] [2]Enhancements to Layer Slider Initialization:
_sliderBuilding) to each multilayer parent to prevent duplicate slider initialization calls. This ensures that the slider is only built once, even if multiple asynchronous triggers occur. The flag is reset after completion or failure.Template and Structural Updates:
#modal-containerdiv in the main planner template to allow for more targeted Knockout.js binding. Also added empty{% block footer %}for future extensibility. [1] [2]These changes collectively make the UI initialization process more robust, reduce the likelihood of errors caused by asynchronous loading, and improve maintainability.