[ERA-8348] Observation/Track Vector Layer#1387
[ERA-8348] Observation/Track Vector Layer#1387JoshuaVulcan wants to merge 34 commits intodevelopfrom
Conversation
JoshuaVulcan
temporarily deployed
to
padas-app
GitHub Actions
Inactive
|
To keep the environment alive:
|
|
🗑️ Environment torn down due to inactivity |
18 similar comments
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
…before first reload. fix:prevent blank screen on logout from bad map lifecycle cleanup
…before first reload. fix:prevent blank screen on logout from bad map lifecycle cleanup
JoshuaVulcan
changed the title
luixlive
left a comment
luixlive
left a comment
There was a problem hiding this comment.
Ok, this is the first pass and I didn't hold on any comments. With all the new sources, layers, filters, and all the old code that was touched, I'd say we really need thorough manual testing here. But the branch env is down: https://era-8348.dev.pamdas.org/ . Is there an env where I can test these changes?
About my review, I see a lot of commented code, Copilot already signaled it, and also lots of new code without a single test. I think the a way to summarize my review is that I notice inconsistencies in the code, between the new code and our existing practices, but also comparing the new code against itself. I guess I've been a bit defensive about moving to a total vibe-code approach and just let the AI do stuff based on only prompting. But I guess we are moving towards that. If we are going to do it, I think we need to establish practices, and definitely go with smaller PRs. This PR has big undocumented code blocks, without tests, and with obscure variable names like x; and some other blocks with a JSDoc twice the size of the function itself. It feels random, and it's hard to review.
| rangeString: '06:01 - 09:00', | ||
| rangeMinutesMin: 361, | ||
| rangeMinutesMax: 54, | ||
| rangeMinutesMax: 540, |
There was a problem hiding this comment.
Why are we modifying these values?
|
|
||
| // When not in timeslider mode, only fetch subjects updated in the last | ||
| // hour — stale subjects are already rendered from the vector tile layer. | ||
| // The 1-hour window matches selectFreshSubjectIds in selectors/subjects. |
There was a problem hiding this comment.
I'd remove the The 1-hour window because we may change that value later in the constant and this comment would be outdated.
Also, defining an object just optionally apend a property and then spread it seems weird. Why not to instead simply calculate the updated_since parameter?
let updated_since = params?.updated_since;
if (!updated_since && !timeSliderActive) {
// The parameters didn't provide an updated_since value and the time slider is inactive
// Set the fresh subject window.
updated_since = new Date(Date.now() - FRESH_SUBJECT_WINDOW_MS).toISOString();
}| return () => { | ||
| map.removeLayer(LAYER_IDS.COORDINATE_REFERENCE_SYSTEM_BBOX); | ||
| map.removeSource(SOURCE_IDS.COORDINATE_REFERENCE_SYSTEM_BBOX); | ||
| safeRemoveMapLayer(map, LAYER_IDS.COORDINATE_REFERENCE_SYSTEM_BBOX); |
There was a problem hiding this comment.
The job is already done, but I think we could have simply added an optional chaining operator right? map?.removeLayer();. Jeje.
Of course that wouldn't help for any errors thrown by map, but we shouldn't hit any if properly handling the sources and layer. Which brings me back to the conversation of how unreliable the useMapLayer and useMapSource hooks are. I have the guess that we needed to add safeRemoveMapLayer just to avoid issues with those hooks poor map management. The name safeRemoveMapLayer implies that we are actually making sure to properly remove the layer, but we are not. If the map is defined and an error is thrown, we just ignore it, post a log, and go on without really cleaning the map layer or source.
| const popup = useSelector(state => state.view.popup); | ||
| const showReportHeatmap = useSelector(state => state.view.showReportHeatmap); | ||
| const showTrackTimepoints = useSelector(state => state.view.showTrackTimepoints); | ||
| // const showTrackTimepoints = useSelector(state => state.view.showTrackTimepoints); |
There was a problem hiding this comment.
Why to keep this code commented? Should we remove it? I see more commented lines below.
| const untilParam = until ? until.toISOString() : new Date().toISOString(); | ||
| const params = { since, until: untilParam }; | ||
| const responses = await Promise.all( | ||
| subjectIds.map((id) => axios.get(TRACKS_API_URL(id), { params, signal })) |
There was a problem hiding this comment.
Why are we doing axios calls here? We should use a thunk action creator and sanitize / store the data in a reducer.
| return total; | ||
| }; | ||
|
|
||
| const TrackDetailsTooltipContent = memo(function TrackDetailsTooltipContent({ |
There was a problem hiding this comment.
Moving this to a subfolder would keep this file cleaner and easier to read! Also, weird to see a function here. This repository favors arrow functions always. AI agent?
|
@luixlive thanks for the review! I'll get going on the first pass of changes, and in the meantime the env should be building correctly now. |
|
I was able to log to the environment now! It's looking great overall, I just have a couple feedback items, all of them very small: 1- The changes in After You can see that before, we had the amount of points information visible and available at all times. Now it is hidden behind a click to that info icon button. And the button itself looks very different from our nice looking and accessible buttons at the right. Those have accessible hover and focus state, and can be navigated with the keyboard. The new one doesn't. 2- Small UI issue, but we should make the numeric inputs of the new controls wider since some numbers don't fit: Also, now that we have a bigger form, I feel like "Track Length" and "Segmentation Settings" texts should be bold or bigger because they get lost with the labels. 3- Playing with the new segmentation controls, I see that we can break the track lines, is that expected? I'm not entirely sure if I understand what the segment settings should do, but I would expect it to filter in / out certain observations, but keeping the lines connected. 4- Not really a feedback point, but worth mentioning. I don't know how to be sure, or tell if I properly tested some of the most important things of this PR: realtime overlay, fresh subject retrieval window, even the segment controls (easy to find and play with them, but I'm not sure if I'm understanding what they should do in order to properly validate it). A little bit of guidance on how to be aware of the functionality of those items would help. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…time-of-day styles with timeslider
|
To keep the environment alive:
|
|
🗑️ Environment torn down due to inactivity |
14 similar comments
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
No description provided.