Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Environments order #4048

Merged
merged 6 commits into from
Oct 13, 2021
Merged

Environments order #4048

merged 6 commits into from
Oct 13, 2021

Conversation

vincendep
Copy link
Contributor

@vincendep vincendep commented Sep 23, 2021

Closes #3922
Closes #3822
Closes INS-954

@vincendep vincendep changed the title Use environment index as metaSortKey value Environments order Sep 23, 2021
Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I tested using the steps in #3822 and can confirm that this PR fixes what's described in those steps.

Here's what I tested with:
Screenshot_20211011_154712

@vincendep
Copy link
Contributor Author

Hi @dimitropoulos. I also include a change to address issue #3922, but I was not sure if it was a good idea since there were no feedback from you under that

@dimitropoulos
Copy link
Contributor

@vincendep is it something different than this fix? if so, it'd be great if you knew the reproduction steps (and would open a new PR for that issue so we can handle separately).

@vincendep
Copy link
Contributor Author

It is quite related, since I had to update my previous changes.

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On testing this seems to work well! A couple of notes however, would love your thoughts!

@@ -76,6 +76,7 @@ export async function getOrCreateForParentId(parentId: string) {
// Deterministic base env ID. It helps reduce sync complexity since we won't have to
// de-duplicate environments.
_id: `${prefix}_${crypto.createHash('sha1').update(parentId).digest('hex')}`,
metaSortKey: 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just curious on what purpose this serves specifically? It's worth writing a comment here to explain why the metaSortKey for the base environment should be hard-coded to 0.

environment,
{ metaSortKey: 1 },
{ metaSortKey: idx + 1 },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think setting this to just idx should do the job. Looking at where this regression was introduced, that was the existing behavior: https://github.com/Kong/insomnia/pull/2891/files?authenticity_token=NotWiXgjksFGynH9ySdJfzuPFfvny1ipT861Ia0PCdxxQciOP08il4GzvolDtfnjCbgSLI7MzW3fm3gge3EmMQ%3D%3D&file-filters%5B%5D=.tsx#diff-83bf0bed6b835cb203957ba0b8b615b662856919617cab840b6ced94a9d02708L337-R375. During the refactor, metaSortKey: i was mistakenly changed to metaSortKey: 1.

for (let i = 0; i < newSubEnvironments.length; i++) {
const environment = newSubEnvironments[i];
await this._updateEnvironment(
environment,
{
metaSortKey: i,
},
false,
);
}

Is the + 1 strictly necessary?

@vincendep
Copy link
Contributor Author

Hi @develohpanda, I introduced that changes after looking at issue #3922. It is to keep the base environment metaSortKey always lower than the sub environment ones.

@develohpanda
Copy link
Contributor

develohpanda commented Oct 13, 2021

Hi @develohpanda, I introduced that changes after looking at issue #3922. It is to keep the base environment metaSortKey always lower than the sub environment ones.

Gotcha, thanks! I think we should remove the hard-coded value there and stick with what was there before, for the base environment. The metaSortKey for the base environment holds no significance, so while it's likely harmless to change it it doesn't feel like there's a compelling reason to change it.

The extra note in #3922 mentions the re-ordering issue, which is fixed by this PR, but that's not directly related to the base environment metaSortKey, only the sub environment ones.

How does that sound?

@vincendep
Copy link
Contributor Author

vincendep commented Oct 13, 2021

It sounds good to me, indeed I was not sure of the utility to have the metaSotKey of the base environment lower than others. So I'll remove the hard coded one and set back the others to index instead of index plus 1

This reverts commit 110982c.

# Conflicts:
#	packages/insomnia-app/app/models/environment.ts
#	packages/insomnia-app/app/ui/components/modals/workspace-environments-edit-modal.tsx
@vercel vercel bot temporarily deployed to Preview October 13, 2021 11:32 Inactive
Copy link
Contributor

@gatzjames gatzjames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one! LGTM!

@develohpanda develohpanda enabled auto-merge (squash) October 13, 2021 18:45
@develohpanda develohpanda merged commit cbc1cfc into Kong:develop Oct 13, 2021
@vincendep vincendep deleted the fix/env-order branch October 17, 2021 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants