chore(refactor): add unit selectors and utils #2220
+1,757
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Demo
Jenkins 
demos.haus 
huwshimi
force-pushed
the
allwatcher-unit-selectors
branch
from
f6f4875 to
aafb632
Compare
andogq
reviewed
Jan 29, 2026
Member
andogq
left a comment
andogq
left a comment
There was a problem hiding this comment.
- With all the unit selectors, the extra machinery to handle parent/subordinate really complicates things. Is it possible to unify/simplify the logic dealing with subordinates, even just within each method?
Comment on lines
+11
to
+31
| export const getAppScale = ( | ||
| appId: string, | ||
| applications: Record<string, ApplicationStatus>, | ||
| ): number => { | ||
| const app = appId in applications ? applications[appId] : null; | ||
| if (!app) { | ||
| return 0; | ||
| } | ||
| return app["subordinate-to"].length | ||
| ? app["subordinate-to"].reduce<number>( | ||
| (count, parentId) => | ||
| count + | ||
| (parentId in applications | ||
| ? // A subordinate exists on every parent unit, so just count the parent units rather than | ||
| // checking for the subordinate id. | ||
| Object.keys(applications[parentId].units ?? {}).length | ||
| : 0), | ||
| 0, | ||
| ) | ||
| : Object.keys(app.units ?? {}).length; | ||
| }; |
Member
There was a problem hiding this comment.
I'm a little bit of a ternary hater in favour of clarity, but feel free to take it or leave it.
Suggested change
| export const getAppScale = ( | |
| appId: string, | |
| applications: Record<string, ApplicationStatus>, | |
| ): number => { | |
| const app = appId in applications ? applications[appId] : null; | |
| if (!app) { | |
| return 0; | |
| } | |
| return app["subordinate-to"].length | |
| ? app["subordinate-to"].reduce<number>( | |
| (count, parentId) => | |
| count + | |
| (parentId in applications | |
| ? // A subordinate exists on every parent unit, so just count the parent units rather than | |
| // checking for the subordinate id. | |
| Object.keys(applications[parentId].units ?? {}).length | |
| : 0), | |
| 0, | |
| ) | |
| : Object.keys(app.units ?? {}).length; | |
| }; | |
| export const getAppUnits = (app: ApplicationStatus): number => { | |
| return Object.keys(app.units ?? {}).length; | |
| } | |
| export const getAppScale = ( | |
| appId: string, | |
| applications: Record<string, ApplicationStatus>, | |
| ): number => { | |
| if (!(appId in applications)) { | |
| return 0; | |
| } | |
| const app = applications[appId]; | |
| if (app["subordinate-to"].length > 0) { | |
| return app["subordinate-to"] | |
| .map((parentId) => applications[parentId]) | |
| .filter((parent) => parent !== undefined) | |
| // A subordinate exists on every parent unit, so just count the parent units rather than | |
| // checking for the subordinate id. | |
| .reduce((count, parent) => count + getAppUnits(parent), 0); | |
| } else { | |
| return getAppUnits(app); | |
| } | |
| }; |
Comment on lines
+38
to
+42
| applications: null | Record<string, ApplicationStatus>, | ||
| ): number => { | ||
| if (!applications) { | ||
| return 0; | ||
| } |
Member
There was a problem hiding this comment.
I'm not sure where this is called (I assume in the next PR), but can we avoid having applications potentially be null? This check feels better suited at the callsite, and would keep things consistent with getAppScale and other calls.
Suggested change
| applications: null | Record<string, ApplicationStatus>, | |
| ): number => { | |
| if (!applications) { | |
| return 0; | |
| } | |
| applications: Record<string, ApplicationStatus>, | |
| ): number => { |
Comment on lines
+51
to
+77
| if (applications[appId]["subordinate-to"].length) { | ||
| return ( | ||
| count + | ||
| getAppScale( | ||
| appId, | ||
| // Only include the parent if its id is not in the applicationIds list. | ||
| applications[appId]["subordinate-to"].reduce< | ||
| Record<string, ApplicationStatus> | ||
| >( | ||
| (parents, subordinateTo) => { | ||
| if ( | ||
| applicationIds.includes(subordinateTo) || | ||
| counted.includes(subordinateTo) | ||
| ) { | ||
| // Ignore it if the parent application will be counted. | ||
| return parents; | ||
| } | ||
| counted.push(subordinateTo); | ||
| parents[subordinateTo] = applications[subordinateTo]; | ||
| return parents; | ||
| }, | ||
| { [appId]: applications[appId] }, | ||
| ), | ||
| ) | ||
| ); | ||
| } | ||
| return count + getAppScale(appId, applications); |
Member
There was a problem hiding this comment.
This is quite difficult to follow. Potentially breaking the selection of applications out may help, but I'm not 100% sure. The interaction between this method pre-empting work done by the getAppScale method (in terms of searching for parent units) feels quite complex, and some of the logic feels very similar. Could these two be simplified or unified in some way?
Suggested change
| if (applications[appId]["subordinate-to"].length) { | |
| return ( | |
| count + | |
| getAppScale( | |
| appId, | |
| // Only include the parent if its id is not in the applicationIds list. | |
| applications[appId]["subordinate-to"].reduce< | |
| Record<string, ApplicationStatus> | |
| >( | |
| (parents, subordinateTo) => { | |
| if ( | |
| applicationIds.includes(subordinateTo) || | |
| counted.includes(subordinateTo) | |
| ) { | |
| // Ignore it if the parent application will be counted. | |
| return parents; | |
| } | |
| counted.push(subordinateTo); | |
| parents[subordinateTo] = applications[subordinateTo]; | |
| return parents; | |
| }, | |
| { [appId]: applications[appId] }, | |
| ), | |
| ) | |
| ); | |
| } | |
| return count + getAppScale(appId, applications); | |
| let candidateApplications = applications; | |
| if (applications[appId]["subordinate-to"].length) { | |
| // Only include the parent if its id is not in the applicationIds list. | |
| candidateApplications = applications[appId]["subordinate-to"].reduce< | |
| Record<string, ApplicationStatus> | |
| >( | |
| (parents, subordinateTo) => { | |
| if ( | |
| !applicationIds.includes(subordinateTo) && | |
| !counted.includes(subordinateTo) | |
| ) { | |
| // Only count if the parent application won't be counted. | |
| counted.push(subordinateTo); | |
| parents[subordinateTo] = applications[subordinateTo]; | |
| } | |
| return parents; | |
| }, | |
| { [appId]: applications[appId] }, | |
| ); | |
| } | |
| return count + getAppScale(appId, candidateApplications); |
Comment on lines
+86
to
+91
| applications: null | Record<string, ApplicationStatus>, | ||
| ): Record<string, ApplicationStatus> => { | ||
| const appsOnMachine: Record<string, ApplicationStatus> = {}; | ||
| if (!applications) { | ||
| return appsOnMachine; | ||
| } |
Member
There was a problem hiding this comment.
Same deal here with applications being null
| if (!applications) { | ||
| return appsOnMachine; | ||
| } | ||
| Object.entries(applications).forEach(([appName, app]) => { |
Member
There was a problem hiding this comment.
🚨🚨🚨 MEGA nit 🚨🚨🚨, but using a normal for loop is significantly faster than .forEach (or any iterable method for that matter) (same goes for below)
Comment on lines
+97
to
+116
| for (const unitInfo of Object.values(app.units ?? {})) { | ||
| if (machineId === unitInfo.machine) { | ||
| appsOnMachine[appName] = app; | ||
| Object.keys(unitInfo.subordinates ?? {}).forEach((subordinateName) => { | ||
| const [parentAppName] = subordinateName.split("/"); | ||
| const parentApp = | ||
| parentAppName in applications ? applications[parentAppName] : null; | ||
| if ( | ||
| parentApp && | ||
| parentAppName in applications && | ||
| !(parentAppName in appsOnMachine) | ||
| ) { | ||
| appsOnMachine[parentAppName] = parentApp; | ||
| } | ||
| }); | ||
| // Only one unit of each application can be on the machine | ||
| // so exit the loop if a unit was found. | ||
| break; | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
This feels a little difficult to follow. Could it be split into two parts, first finding any applications with a unit on the machine, and then finding any parent applications for those applications?
andogq
reviewed
Jan 29, 2026
Member
andogq
left a comment
andogq
left a comment
There was a problem hiding this comment.
- With all the unit selectors, the extra machinery to handle parent/subordinate really complicates things. Is it possible to unify/simplify the logic dealing with subordinates, even just within each method?
- Iterable
forEachcould probably be swapped out for a standardforloop. Similar or other iterable methods wouldn't hurt either applicationsparameter beingnullshould be handled at the callsite, rather than being an allowed type- There are a lot of checks for a given
appIdbeing present inapplications. Is it actually possible forapplicationsto be partially populated, missing an application that's defined elsewhere (aside from that one method where we selectively add applications to search) collection && key in collection ? collection[key] : nullcould probably be simplified tocollection?.[key] ?? null(unless out eslint rules prevent this, can't remember what they let us do in this circumstance)
Comment on lines
+97
to
+98
| for (const unitInfo of Object.values(app.units ?? {})) { | ||
| if (machineId === unitInfo.machine) { |
Member
There was a problem hiding this comment.
Can this method be implemented using getMachineUnits? Logic seems very similar
| */ | ||
| export const getMachineUnits = ( | ||
| machineId: string, | ||
| applications: null | Record<string, ApplicationStatus>, |
Comment on lines
+138
to
+142
| Object.entries(unitInfo.subordinates ?? {}).forEach( | ||
| ([subordinateId, subordinate]) => { | ||
| unitsOnMachine[subordinateId] = subordinate; | ||
| }, | ||
| ); |
Member
There was a problem hiding this comment.
Could use Object.assign here instead
Comment on lines
+163
to
+167
| const [appId] = unitId.split("/"); | ||
| const app = appId in applications ? applications[appId] : null; | ||
| if (!app?.["subordinate-to"].length) { | ||
| return null; | ||
| } |
Member
There was a problem hiding this comment.
This similar logic for selecting a parent application is also present in getMachineApps. Perhaps another selector could be used to simplify this?
Comment on lines
+175
to
+181
| for (const subordinateName of Object.keys( | ||
| parentUnit.subordinates ?? {}, | ||
| )) { | ||
| if (subordinateName === unitId) { | ||
| return parentUnit; | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Am I misunderstanding this, or is it searching through the parent unit's subordinates to verify that the unitId is present?
Won't this always be true if an app's subordinate-to is populated? Or is it possible for these to be mis-matched, hence the extra check required?
| /** | ||
| * Get the application for a unit. | ||
| */ | ||
| export const getUnitApp = ( |
Member
There was a problem hiding this comment.
This probably could be used in quite a few of the other selectors
| /** | ||
| * Get a unit from its parent if it is a subordinate, or else get the unit from the unit's application. | ||
| */ | ||
| export const getUnitMachine = ( |
Member
There was a problem hiding this comment.
I also believe this could be used in some other methods.
| const application = | ||
| applications && appId in applications ? applications[appId] : null; | ||
| if (!applications || !machines || !application) { | ||
| return machinesForApp; |
Member
There was a problem hiding this comment.
This seems a little inconsistent, as in other methods null is returned as the default case. I don't mind which is used (null vs empty object), but I feel like keeping them the same wouldn't hurt.
Comment on lines
+269
to
+286
| if (application["subordinate-to"].length) { | ||
| application["subordinate-to"].forEach((appName) => { | ||
| const app = appName in applications ? applications[appName] : null; | ||
| if (app) { | ||
| Object.values(app.units ?? {}).forEach((parentUnit) => { | ||
| if (parentUnit.machine in machines) { | ||
| machinesForApp[parentUnit.machine] = machines[parentUnit.machine]; | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| } else { | ||
| Object.values(application.units ?? {}).forEach((unitData) => { | ||
| if (unitData.machine in machines) { | ||
| machinesForApp[unitData.machine] = machines[unitData.machine]; | ||
| } | ||
| }); | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| if (application["subordinate-to"].length) { | |
| application["subordinate-to"].forEach((appName) => { | |
| const app = appName in applications ? applications[appName] : null; | |
| if (app) { | |
| Object.values(app.units ?? {}).forEach((parentUnit) => { | |
| if (parentUnit.machine in machines) { | |
| machinesForApp[parentUnit.machine] = machines[parentUnit.machine]; | |
| } | |
| }); | |
| } | |
| }); | |
| } else { | |
| Object.values(application.units ?? {}).forEach((unitData) => { | |
| if (unitData.machine in machines) { | |
| machinesForApp[unitData.machine] = machines[unitData.machine]; | |
| } | |
| }); | |
| } | |
| let targetUnits; | |
| if (application["subordinate-to"].length) { | |
| targetUnits = application["subordinate-to"] | |
| .map((appName) => applications[appName]?.units ?? {}) | |
| .reduce((targetUnits, units) => Object.assign(targetUnits, units), {}); | |
| } else { | |
| targetUnits = application.units; | |
| } | |
| for (const unitData of Object.values(targetUnits)) { | |
| if (unitData.machine in machines) { | |
| machinesForApp[unitData.machine] = machines[unitData.machine]; | |
| } | |
| } |
Separating the selection of units from the processing of them might help cut down on duplicated code.
Comment on lines
+302
to
+320
| if (application["subordinate-to"].length) { | ||
| const unitsForApp: Record<string, UnitStatus> = {}; | ||
| application["subordinate-to"].forEach((appName) => { | ||
| const app = appName in applications ? applications[appName] : null; | ||
| if (app) { | ||
| Object.values(app.units ?? {}).forEach((parentUnit) => { | ||
| Object.entries(parentUnit.subordinates ?? {}).forEach( | ||
| ([subordinateName, subordinateUnit]) => { | ||
| if (subordinateName.split("/")[0] === appId) { | ||
| unitsForApp[subordinateName] = subordinateUnit; | ||
| } | ||
| }, | ||
| ); | ||
| }); | ||
| } | ||
| }); | ||
| return unitsForApp; | ||
| } | ||
| return application.units; |
Member
There was a problem hiding this comment.
Could getParentOrUnit be used here instead?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Done
Precursor to #2219