I don't think using commAppURLFacts.baseDomain is a good long-term approach here, since we'll want people to be able to set up a keyserver without specifying commAppURLFacts. I know you had some plans to figure config out as a follow-up... can you link the task you're tracking that in?

It appears that you're setting CORS for both commAppRouter and squadCalRouter, but I'm not sure why you need to.

I think you should only set CORS for one if you don't need it for both. I suspect you just need to set CORS for the future keyserver endpoints (squadCalRouter) to allow for calls from commAppURLFacts.baseDomain, but I may be wrong.

Let me know if I'm missing something!

commAppURLFacts is used for configuring the web app hosting. There should be no expectation that the keyserver and the web app are hosted in the same place. I don't think it makes sense to use commAppURLFacts.baseDomain here. Can you identify a solution that doesn't use commAppURLFacts.baseDomain, and either implement it, or create a follow-up task / follow-up diff for it?

It's confusing to introduce issues in one diff and resolve it in a later diff. I'm not even on the review for D9452. There should be no expectation that a reviewer needs to read a later diff in order to understand an earlier diff, ESPECIALLY when they're not even on the review for the later diff.

Please address this issue here.

I need a deeper explanation. If localhost:3000 was just connecting to localhost:3000, why did we even have this in the first place?

High-level feedback – if somebody asks a question in code review, it's probably because something is non-obvious. Responding with a couple sentences like this is frankly not helpful, and will lead to your work being delayed further.

Can you add a comment explaining why you're checking this?

Alternately, instead of a comment it might be more readable to define a function.

Something like:

function isDockerEnvironment(): boolean {
  return !!process.env.COMM_DATABASE_HOST;
}

In summary: uploads in development mode use the ip address instead of localhost (http://xx.xx.xx.xx:3000/comm/upload/123/123abc). From what I understand this is because of native media download issues.

This explanation was still insufficient for me, even after asking for a deeper explanation. As a result, I had to do further research, which is why this diff's review was further delayed.

I'd like to repeat what I said in my last comment:

High-level feedback – if somebody asks a question in code review, it's probably because something is non-obvious. Responding with a couple sentences like this is frankly not helpful, and will lead to your work being delayed further.

@michal, please understand the feedback here. It is the responsibility of the diff author to make the review as easy as possible. Your diff reviewers are generally more senior people, in this case the CEO of the company. I do not have as much time as you, and forcing me to do independent research to review your diff is a recipe for (1) frustrating me, (2) making your diff review take longer.

In the future, please try *much* harder to find deeper explanations. Your current efforts are insufficient.

With the high-level feedback out of the way, let's get into specifics:

From what I understand this is because of native media download issues.

"Native media download issues" is extremely vague. This is not a helpful answer because it doesn't really explain anything, and just forces the reader to ask more questions.

To answer my own question, I had to take a look at getUploadURL: link. We can see that there is a special condition that checks ip.v4.sync() there, which is why the IP address is used instead of localhost. Looking at git blame, we can see that was introduced in D6587, which provides an excellent explanation. We need to use the IP address in order to support devs testing the mobile app from their physical device.

This is the explanation you should have provided.

Now that we handle cors globally we can remove this upload-specific code.

Once again there is simply not enough detail in this statement. You need to write and explain more.

1. What testing did you do to make sure this change is safe? Why isn't it listed in the Test Plan?
2. How does "handling CORS globally" address the original issue?

Here is the explanation you should have given instead of this single sentence:

This CORS rule is only necessary for the dev environment on web. On native CORS rules are ignored, and in production getUploadURL will just use the URL from the URL facts (the keyserver URL), which in the past was the same URL that the web app is being served on, and in the future will be handled by the CORS rules introduced in this diff.

Now that we've narrowed down the purpose of this CORS rule, let's see if we still need it for the dev environment on web.

In the dev environment we always access the web app via http://localhost:3000. This is why these CORS rule were explicitly written to reference that address. However, the CORS rules we're introducing should actually protect this case. That's because the upload URL is a keyserver URL, and the web app must already be set up to be able to access keyserver URLs via CORS rules on the keyserver endpoints.

As such, we can conclude that the new CORS rules obviate the need for these old rules. Additionally, I've tested and confirmed that media uploads load fine in the web app under this new setup.