feat:connecting trawl lines between linked buoys #1276

JoshuaVulcan · 2025-04-08T18:14:19Z

What does this PR do?

Sketches some basic trawl lines on the map for Buoy devices.

How does it look

Relevant link(s)

🤷

Where / how to start reviewing (optional)

The logic is a little funny; we can't rely on the coordinates provided by the devices array in a subject's additional properties, so instead we match the device_ids listed to their subject names and then use the last known position for that subject. @andrejiron21 @doug-nimblegravity should definitely add story work to keep that devices data better refreshed to avoid weird data sync issues in the future.

Copilot

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

luixlive · 2025-04-08T18:22:40Z

src/BuoyTrawlLineLayer/index.js

+const subjectIsBuoyLineEligible = (subjectFeature = {}, _index, allSubjects = []) => {
+  const subject = subjectFeature.properties;
+
+  const is_buoy = subject.subject_subtype === 'ropeless_buoy_device';


This string should be a constant. Maybe we won't need it anywhere else so at least putting it at the top of the file like: BUOY_SUBJECT_SUBTYPE is enough.

Second question: why snake case here (and in all other variables of this method)? It's not our usual approach 🤔

The snake case and non-constantified strings are just me doing this as a quickie/prototype.

luixlive · 2025-04-08T18:22:58Z

src/BuoyTrawlLineLayer/index.js

+import { useSelector } from 'react-redux';
+
+import { getMapSubjectFeatureCollectionWithVirtualPositioning } from '../selectors/subjects';
+import { lineString } from '@turf/turf';


nit: this import should go in the first block.

luixlive · 2025-04-08T18:26:25Z

src/BuoyTrawlLineLayer/index.js

+
+  if (!is_line) return false;
+
+  const line_contains_valid_subjects = devices.every(({ device_id }) => allSubjects.find(({ properties }) => properties.name === device_id));


This variable name sounds odd, it almost sounds like a boolean for a condition: if (lineContainsValidSubjects) ... but it is not, it's just a list of subjects contained by the line. I guess it needs to be reworded, or maybe just return the one-liner since it's kind of easy to read.

This is a boolean for a condition -- it's just using Array.prototype.every to check device presence in the subject list.

Ohh right, I just understood the every and where this is being used 👍

luixlive · 2025-04-08T18:31:39Z

src/BuoyTrawlLineLayer/index.js

+};
+
+const createTrawlLineGeoJSON = (buoySubjectFeatures) => {
+  return buoySubjectFeatures.reduce((accumulator, { properties }) => {


Honestly, I've stared at this function for a while haha. I feel like we could make it a bit easier to understand maybe by changing some variable names or not extracting properties from objects like: ({ properties }) => properties.additional... -> (buoySubjectFeature) => buoySubjectFeature.properties.additional etc...

Summary version:

We can say that the devices listed for a buoy feature's additional.devices should be the names of the subjects comprising that trawl.

We can't say that the coordinates included in the additional data devices array data is up-to-date.

So the eligibility-checker function first ensures a trawl line is completely valid and comprised of available subjects for the user.

Then createTrawlLineGeoJSON maps those device names back to their subject's last known position and builds out the linestring coordinates from there.

That makes sense. Maybe a couple tweaks or a brief comment could make that more obvious in the code for future reference.

Added a descriptive inline comment

luixlive · 2025-04-08T18:33:40Z

src/BuoyTrawlLineLayer/index.js

+  const buoySubjects = mapSubjects.features.filter(subjectIsBuoyLineEligible);
+  const trawlLineGeoJSON = createTrawlLineGeoJSON(buoySubjects);
+
+  useMapSources([{ id: 'trawl-lines-source', data: trawlLineGeoJSON }]);


We have constants for sources and layers that we should use to keep our code consistent and the ids accessible. Also, I don't know if you tested the properly addition, update and removal of these sources and layers (seeing Chromes console for warnings and errors), but as I mentioned recently, these hooks don't follow Mapbox sources and layers flows correctly from beginning to end. That's why I think it would be better to stop using them and instead use the map API directly.

I did test absence/presence/filtering etc, and timesliding, and it all works as expected.

I kept the names out of our constants file because I'd like this to be as self-isolated as possible. I will however make the strings constants within this file.

Oh with tested I meant manually. In unit testing we usually use mocks for the map and other methods and the real flow of addition, interaction and removal (or not removal, causing memory leaks) of sources and layers are obscured by all the mocking. Anyway, we agreed this is out of the scope.

About the layers and sources being isolated, why's that 🤔 ? I know this is a feature for the specific scenario of buoy, but I don't get why we don't want to treat it as part of our app if now it is. Just like stationary subjects, or patrols, which are features not everyone use. I feel I'm missing some information or something haha.

Oh with tested I meant manually.

Me too :-)

"Features not everyone uses" can be considered a spectrum -- with this Buoy work being at one pretty extreme end of the spectrum, "features designed for a specific site and fringe use-cases". It's so specifically for a special sub-project, that I'd rather not pollute the broader app with random BUOY_ variables and references.

luixlive

The source + layer is very simple and the processing of the source data is clear and well tested 👍

luixlive · 2025-04-09T16:00:46Z

src/BuoyTrawlLineLayer/index.js

+  }, { type: 'FeatureCollection', features: [] });
+};
+
+const BuoyLineLayer = (_props) => {


nit: why the (_props) => { instead of simply () => {?

just a matter of preference!

I mean, it's a default linter rule (no-unused-vars) that we explicitly skip by prefixing the _. It feels weird to add it intentionally for nothing 🤔

feat:connecting trawl lines between linked buoys

524aab2

JoshuaVulcan requested review from AlanCalvillo, luixlive, gaboDev, caciolucas, irvingsoto and Copilot April 8, 2025 18:14

Copilot AI reviewed Apr 8, 2025

View reviewed changes

luixlive requested changes Apr 8, 2025

View reviewed changes

PADAS deleted a comment from Copilot AI Apr 8, 2025

JoshuaVulcan added 2 commits April 8, 2025 11:47

fix:PR feedback for naming convention and constants

49cfbaa

fix:PR feedback for import block

5f8516a

JoshuaVulcan requested a review from luixlive April 8, 2025 18:54

fix:descriptive comment as per PR suggestion

c95d9f2

JoshuaVulcan added the deploy label Apr 8, 2025

JoshuaVulcan deployed to joshua-experimenting-buoy-lines April 8, 2025 20:44 — with GitHub Actions View deployment

JoshuaVulcan temporarily deployed to padas-app April 8, 2025 20:44 — with GitHub Actions Inactive

luixlive approved these changes Apr 9, 2025

View reviewed changes

JoshuaVulcan merged commit c9d6107 into develop Apr 23, 2025
5 checks passed

JoshuaVulcan deleted the joshua-experimenting-buoy-lines branch April 23, 2025 16:58


		if (!is_line) return false;

		const line_contains_valid_subjects = devices.every(({ device_id }) => allSubjects.find(({ properties }) => properties.name === device_id));

feat:connecting trawl lines between linked buoys #1276

feat:connecting trawl lines between linked buoys #1276

Uh oh!

Conversation

JoshuaVulcan commented Apr 8, 2025

What does this PR do?

How does it look

Relevant link(s)

Where / how to start reviewing (optional)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaVulcan Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaVulcan Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaVulcan Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luixlive left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JoshuaVulcan Apr 8, 2025 •

edited

Loading

JoshuaVulcan Apr 8, 2025 •

edited

Loading

JoshuaVulcan Apr 9, 2025 •

edited

Loading