Cannot set properties of null (setting 'innerHTML') in SatInfoBoxOrbital.updateOrbitData_

[thkruz]

What happened?

Stack:
TypeError: Cannot set properties of null (setting 'innerHTML')
at SatInfoBoxOrbital.updateOrbitData_ (https://app.keeptrack.space/js/src_plugins_plugins_ts.ad6709c6a55c6f02.js:13162:89)
at https://app.keeptrack.space/js/main.3e4e59a9fdf3c473.js:138888:47
at Array.forEach ()
at KeepTrackApi.emit (https://app.keeptrack.space/js/main.3e4e59a9fdf3c473.js:138888:28)
at SelectSatManager.selectSat (https://app.keeptrack.space/js/main.3e4e59a9fdf3c473.js:148064:69)
at TouchInput.tap (https://app.keeptrack.space/js/main.3e4e59a9fdf3c473.js:168669:178)
at TouchInput.canvasTouchEnd (https://app.keeptrack.space/js/main.3e4e59a9fdf3c473.js:168587:22)
at HTMLCanvasElement. (https://app.keeptrack.space/js/main.3e4e59a9fdf3c473.js:168573:22)
at HTMLCanvasElement.c (https://app.keeptrack.space/cdn-cgi/scripts/7d0fa10a/cloudflare-static/rocket-loader.min.js:1:9405)

[Le-Roi777]

Hey TK! : )

Is this issue still present? If so, can you share a quick couple of reproduction points then I can also have a look and get it fixed up.

related: #1177

[thkruz]

@Le-Roi777 very happy to see you still checking up on KeepTrack!

Unfortunately this isn't actually my bug report. The production application reports errors to my server and I review them manually. If it is unique I post them here and add any comments I can infer from the stack trace.

I didn't get to this yet. Looking at sat-info-box-orbital.ts there either needs to be element validation or a flag added to createOrbitalSection to ensure the elements are there before an update is called.

Usually the culprit is starting the application with a satellite already selected and then it induces a race condition because it selects the satellite and tries to update its data before everything is loaded (also on the todo list to fix).

Definitely branch from/PR to the develop branch since I am in the middle of finishing up version 11 and there a ton of changes.

#1177 is similar but different. In general the sat-info-box series of classes need to verify the elements have been created before trying to edit them.

The satInfoBoxInit event is where html is added.
The satInfoBoxAddListeners event is where the listeners are attached to the elements.
The selectSatData event...in theory...shouldn't be called until after both of those two.

Let me know if I can clarify anything. Making sure there is enough documentation/intuitive names for you to help is a bigger priority for me than fixing the actual bug.

[Le-Roi777]

@thkruz I'm not able to create a branch for the issue directly so I created a new branch from the dev branch manually on my side for now #1195, so it wont interfere with the main repo until you finalized v11

I found that several UI update paths write to DOM nodes using non-null assertions on elements that can legitimately be missing.

So indeed, as you mentioned, certain client side actions like a satellite being selected on app load/reload or at this time unknow actions like, opening and closing UI panels/overlays can sometimes unexpectedly cause this issue.

Many part of the app already guard against the missing elements, however, there are still some parts using non-null assertions directly. My suggestion at this time... to further investigate and go through the code base where this is till present and update those parts.

For example:

instead of:

@ src/plugins/sat-info-box-orbital/sat-info-box-orbital.ts
line start 108 `updateOrbitData_`

//...
private updateOrbitData_(obj: BaseObject): void {

    if (obj === null || typeof obj === 'undefined') {
      return;
    }

    if (obj instanceof DetailedSatellite) {
      getEl(EL.APOGEE)!.innerHTML = `${obj.apogee.toFixed(0)} km`;
      getEl(EL.PERIGEE)!.innerHTML = `${obj.perigee.toFixed(0)} km`;
      getEl(EL.INCLINATION)!.innerHTML = `${obj.inclination.toFixed(2)}°`;
      getEl(EL.ECCENTRICITY)!.innerHTML = obj.eccentricity.toFixed(3);
      getEl(EL.RAAN)!.innerHTML = `${obj.rightAscension.toFixed(2)}°`;
      getEl(EL.ARG_PE)!.innerHTML = `${obj.argOfPerigee.toFixed(2)}°`;
    }
}
//...

make use of existing setInnerHtml(id, html) wrapper:

@ src/engine/utils/get-el.ts

export const setInnerHtml = (id: string, html: string) => {
  const el = getEl(id);

  if (!el) {
    errorManagerInstance.debug(`Element with id ${id} not found!`);

    return;
  }
  el.innerHTML = html;
};

and update to:

@ src/plugins/sat-info-box-orbital/sat-info-box-orbital.ts
line start 108 `updateOrbitData_`

//...
private updateOrbitData_(obj: BaseObject): void {

    if (obj === null || typeof obj === 'undefined') {
      return;
    }

  if (obj instanceof DetailedSatellite) {
        setInnerHtml(EL.APOGEE, `${obj.apogee.toFixed(0)} km`);
        setInnerHtml(EL.PERIGEE, `${obj.perigee.toFixed(0)} km`);
        setInnerHtml(EL.INCLINATION, `${obj.inclination.toFixed(2)}°`);
        setInnerHtml(EL.ECCENTRICITY, obj.eccentricity.toFixed(3));
        setInnerHtml(EL.RAAN, `${obj.rightAscension.toFixed(2)}°`);
        setInnerHtml(EL.ARG_PE, `${obj.argOfPerigee.toFixed(2)}°`);
  }
}
//...

[thkruz]

@Le-Roi777 I love it.

Almost all of the html elements are created at initialization and should be present once the application starts unless they are part of an optional plugin and referenced from outside that plugin.

Example is when something tried to access the top menu it is always marked as optional since that plugin can be disabled.

Here the method is referencing it's own elements that are created within init and should be made before any other method is called.

That is why it asserts they aren't null. This worked great until I started adding persistence and GET parameters that modify things during initialization. The problem is race conditions where that assumption about init always going first fails.

I have no recollection of ever writing setInnerHtml, but obviously it was made for this exact problem.

getEl will throw an error during debug if it can't find an element unless it is explicitly marked as possibly missing (and then assumably using optional chaining). That shouldn't matter here since setting innerHtml can't be part of an optional chain.

It also opens the door to have a centralized check that the values are always present during testing since currently it will pass even if setting the html to undefined.

I will investigate why you can't branch off of develop when I get home - that isn't intended behavior. Please feel free to implement setInnerHtml anywhere you think it needs it and I will start doing the same.

[Le-Roi777]

@thkruz Thanks for the explanation. I’ve identified the likely components causing the issue and added null-safe checks where needed. Let me know if you find anything specific related to the develop branch issue, in the meantime my changes are in: commit