Skip to content

Conversation

gribnoysup
Copy link
Collaborator

@gribnoysup gribnoysup commented Sep 22, 2025

Adds a new preference that maps to data explorer data access read write and disables a few features in compass that won't work with this preference. As the ticket says, we're probably missing a few, but the goal is to hide the most prominent ones that won't work at all, in particular indexes editing and collection renaming

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll open a ticket to do something about this code, the logic there is minimal and yet we're spreading it across three different files passing down these variables from method to method, it's really hard to navigate this stuff for no good reason

import { InMemoryStorage } from './preferences-in-memory-storage';
import { getActiveUser } from './utils';

const editablePreferences: (keyof UserPreferences)[] = [
Copy link
Collaborator Author

@gribnoysup gribnoysup Sep 23, 2025

Choose a reason for hiding this comment

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

While I think the intention here was good, on practice this doesn't protect us more from someone trying to change this internal state while making e2e testing and dynamically changing these in sandbox more confusing than it should be: often you can see e2e tests for web being skipped for no good reason even though they can be enabled as people don't know where to look for this and so postponing important part of the development process for way too late in the project time. So I'm erring on the side of making the DX better here and removing the checks (and the env var that gates the logic only to e2e tests)

@gribnoysup gribnoysup marked this pull request as ready for review September 24, 2025 10:41
@gribnoysup gribnoysup requested a review from a team as a code owner September 24, 2025 10:41
@nbbeeken
Copy link
Collaborator

should this be a fix? just for the sake of bumping out a patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants