-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
chore: update project tree scroll to current ID context menu, top bar #29423
Conversation
…older id working again, updated project tree logic with sensible defaults (removing useless state)
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Michael Matloka <michael@matloka.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
…dated tree notice to remove command note
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Ben White <ben@posthog.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Size Change: -6 B (0%) Total Size: 9.73 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR enhances the project tree navigation experience with improved context retention and user guidance features.
- Added context persistence by saving clicked items and scrolling to active items when navigating or refreshing pages
- Implemented a context menu system with right-click functionality, replacing the previous keyboard modifier approach for drag-and-drop
- Added a top navigation bar with edit/cancel/save buttons to explicitly toggle drag-and-drop mode and manage pending changes
- Improved user guidance with a dismissable banner explaining drag-and-drop functionality
- Refactored import paths throughout the codebase, moving
sync_execute
fromposthog.client
toposthog.clickhouse.client
for better code organization
325 file(s) reviewed, 43 comment(s)
Edit PR Review Bot Settings | Greptile
# Test with enterprise taxonomy enabled - hidden events should be excluded when exclude_hidden=true | ||
self.demo_team.organization.available_product_features = [ | ||
{"key": AvailableFeature.INGESTION_TAXONOMY, "name": "ingestion-taxonomy"} | ||
] | ||
self.demo_team.organization.save() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Using self.demo_team.organization
is redundant since you already have self.organization
available. Consider using self.organization.available_product_features = ...
for consistency with line 381.
# Test with enterprise taxonomy enabled - hidden events should be excluded when exclude_hidden=true | |
self.demo_team.organization.available_product_features = [ | |
{"key": AvailableFeature.INGESTION_TAXONOMY, "name": "ingestion-taxonomy"} | |
] | |
self.demo_team.organization.save() | |
# Test with enterprise taxonomy enabled - hidden events should be excluded when exclude_hidden=true | |
self.organization.available_product_features = [ | |
{"key": AvailableFeature.INGESTION_TAXONOMY, "name": "ingestion-taxonomy"} | |
] | |
self.organization.save() |
def validate(self, data): | ||
validated_data = super().validate(data) | ||
|
||
if "hidden" in validated_data and "verified" in validated_data: | ||
if validated_data["hidden"] and validated_data["verified"]: | ||
raise serializers.ValidationError("A property cannot be both hidden and verified") | ||
|
||
# If setting hidden=True, ensure verified becomes false | ||
if validated_data.get("hidden", False): | ||
validated_data["verified"] = False | ||
# If setting verified=True, ensure hidden becomes false | ||
elif validated_data.get("verified", False): | ||
validated_data["hidden"] = False | ||
|
||
return validated_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The validation logic here is duplicated in the update method. Consider extracting this logic to a helper method to avoid inconsistencies if one part is updated but not the other.
# If setting hidden=True, ensure verified becomes false | ||
if validated_data.get("hidden", False): | ||
validated_data["verified"] = False | ||
validated_data["verified_by"] = None | ||
validated_data["verified_at"] = None | ||
# If setting verified=True, ensure hidden becomes false | ||
elif validated_data.get("verified", False): | ||
validated_data["hidden"] = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: This logic duplicates what's already done in the validate method. Since validate is called before update, this code may be redundant and could be removed.
# This is an empty endpoint for the Slack interactivity callback. | ||
# We don't verify the request, as we don't do anything with the submitted data. | ||
# We only use it to supress the warnings when users press buttons in Slack messages. | ||
# In case we decide to do something with it, please add the verification process here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding request validation for security, even if you're not using the data. Without validation, this endpoint could potentially be abused.
tooltip={ | ||
dragAndDropEnabled | ||
? 'Click to cancel editing and changes' | ||
: 'Click to editinga and drag and drop' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: Typo in tooltip text: 'editinga' should be 'editing'
: 'Click to editinga and drag and drop' | |
: 'Click to editing and drag and drop' |
import { loadPluginsFromDB } from './loadPluginsFromDB' | ||
import { loadSchedule } from './loadSchedule' | ||
import { teardownPlugins } from './teardown' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: The import for loadSchedule
has been removed, but there's no indication in this file where this functionality has been moved to. This could potentially break scheduled tasks functionality if not handled elsewhere.
'ingestionV2', | ||
'ingestionV2Combined', | ||
]) | ||
const PROCESS_EVENT_CAPABILITIES = new Set<keyof PluginServerCapabilities>(['ingestionV2', 'ingestionV2Combined']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: The PROCESS_EVENT_CAPABILITIES set has been reduced to only include 'ingestionV2' and 'ingestionV2Combined', removing 'ingestion', 'ingestionOverflow', and 'ingestionHistorical'. Make sure this doesn't break compatibility with plugins that rely on these older capabilities.
def list_channels(self, authed_user) -> list[dict]: | ||
# NOTE: Annoyingly the Slack API has no search so we have to load all channels... | ||
# We load public and private channels separately as when mixed, the Slack API pagination is buggy | ||
public_channels = self._list_channels_by_type("public_channel") | ||
private_channels = self._list_channels_by_type("private_channel") | ||
public_channels = self._list_channels_by_type("public_channel", authed_user) | ||
private_channels = self._list_channels_by_type("private_channel", authed_user) | ||
channels = public_channels + private_channels | ||
|
||
return sorted(channels, key=lambda x: x["name"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The authed_user
parameter is now required but lacks type annotation. Consider adding a type hint for better code clarity.
def list_channels(self, authed_user) -> list[dict]: | |
# NOTE: Annoyingly the Slack API has no search so we have to load all channels... | |
# We load public and private channels separately as when mixed, the Slack API pagination is buggy | |
public_channels = self._list_channels_by_type("public_channel") | |
private_channels = self._list_channels_by_type("private_channel") | |
public_channels = self._list_channels_by_type("public_channel", authed_user) | |
private_channels = self._list_channels_by_type("private_channel", authed_user) | |
channels = public_channels + private_channels | |
return sorted(channels, key=lambda x: x["name"]) | |
def list_channels(self, authed_user: str) -> list[dict]: | |
# NOTE: Annoyingly the Slack API has no search so we have to load all channels... | |
# We load public and private channels separately as when mixed, the Slack API pagination is buggy | |
public_channels = self._list_channels_by_type("public_channel", authed_user) | |
private_channels = self._list_channels_by_type("private_channel", authed_user) | |
channels = public_channels + private_channels | |
return sorted(channels, key=lambda x: x["name"]) |
if data is None: | ||
return None | ||
return bytes(data).decode("utf-8") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding error handling for potential UTF-8 decoding errors that might occur with malformed JSON data.
try: | ||
# We want to use pyarrow arrays where possible to optimise on memory usage | ||
columnar_table_data[col] = pa.array(values) | ||
except: | ||
# Some values can't be interpreted by pyarrows directly | ||
columnar_table_data[col] = np.array(values, dtype=object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The try-except block here is very broad and doesn't specify which exceptions it's catching. Consider catching specific exceptions or at least logging the error to help with debugging.
Problem
When an project tree/lemon tree item was clicked we went to that URL, but when we refreshed, we'd lose that context in the tree.
Also, drag and drop (currently) is initiated by holding command key/use context menu, which is not obvious, so I added a dismissable banner to help users know what's going on.
Changes
Does this work well for both Cloud and self-hosted?
It should
How did you test this code?
Locally