----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58872/#review174374 -----------------------------------------------------------
Fix it, then Ship it! Before you commit this, it would be great if you could test this (I can't tell how you tested this from the testing done section), e.g.: * Clicking on a link from the top level task page (state should already be loaded) * Navigating directly to a valid URL (state should not be loaded yet) * Navigating directly to an invalid URL (e.g. bad agent ID) (state should not be loaded) To make sure that these all work as you expect. src/webui/master/static/js/controllers.js Lines 1016-1017 (patched) <https://reviews.apache.org/r/58872/#comment247501> I'm a bit confused as to why we would want to set up the listener if we already called reroute(). Does this belong in the else case? ``` if ($scope.state) { reroute(); } else { var removeListener = $scope.$on('state_updated', reroute); $scope.$on('$routeChangeStart', removeListener); } ``` Also, why is it safe to call reroute on every state update? It seems there are a few cases to consider: Case 1: reroute actually routes away from the page, then we're ok because the listener is removed Case 2: reroute calls goBack, we're ok because the listener is removed (there is a separate question we should consider later, which is whether going back is the best thing to do) Case 3: reroute navigates "home" (/) if we get an error response, this is ok since the listener is removed How about adding a comment here that reroute is expected to always route away from the page and so the listener is removed after the first state update? - Benjamin Mahler On May 3, 2017, 4:15 a.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58872/ > ----------------------------------------------------------- > > (Updated May 3, 2017, 4:15 a.m.) > > > Review request for mesos, Benjamin Mahler and Tomasz Janiszewski. > > > Bugs: MESOS-4992 > https://issues.apache.org/jira/browse/MESOS-4992 > > > Repository: mesos > > > Description > ------- > > Open sandbox link in a new tab would fail since it depends on agents > information loading finish. This patch fixes it by ensuring the agents > information loaded first and then rerouting the sandbox request. > > > Diffs > ----- > > src/webui/master/static/js/controllers.js > a021962573d452de1581e6a7717016eac7d0cd85 > > > Diff: https://reviews.apache.org/r/58872/diff/2/ > > > Testing > ------- > > > Thanks, > > haosdent huang > >
