Feature/insights filters [CONSOLE-1607]#7662
Conversation
Summary of ChangesHello @jonathanawesome, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new 'Saved Filters' feature, enabling users to persist and reuse filter configurations for insights. It includes GraphQL API endpoints for managing these filters, database schema changes, integration tests, and audit logging to ensure proper tracking and security. The feature enhances user experience by allowing quick access to frequently used filter settings. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📚 Storybook DeploymentThe latest changes are available as preview in: https://pr-7662.hive-storybook.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature for saved filters, including database migrations, storage logic, GraphQL schema, and resolvers, and extends operations filtering with client version support. A critical security concern has been identified: a lack of input validation on array sizes could lead to a Denial of Service (DoS) attack by providing excessively large arrays of operation IDs or client versions, causing resource exhaustion. Additionally, while the implementation is comprehensive with good test coverage, there are areas for improvement regarding adherence to architectural guidelines, query performance, and input validation consistency.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/services/storage/src/index.ts (3973-4202)
Adding new data access logic for the saved_filters table directly into packages/services/storage/src/index.ts appears to deviate from the repository's architectural guidelines. According to the style guide (lines 60-66), new database logic should be encapsulated in smaller, dedicated classes within the corresponding GraphQL module to avoid bloating the legacy storage package.
To align with the preferred architecture, consider refactoring this new data access logic into a separate SavedFiltersStorage class within the packages/services/api/src/modules/saved-filters/ directory.
References
- The repository style guide discourages adding new logic for new database tables to the legacy
/packages/services/storagemodule. Instead, it recommends encapsulating database interactions in smaller classes within the relevant GraphQL module. (link) - New database interaction logic should be encapsulated in smaller classes within the corresponding GraphQL modules, not added to the legacy
/packages/services/storagemodule.
packages/services/api/src/modules/saved-filters/providers/saved-filters.provider.ts (22)
The versions array within clientFilters lacks a maximum size limit in the Zod validation model. An attacker could provide an extremely large array of versions, leading to resource exhaustion (DoS) when the filter is processed or used to construct database queries. It is recommended to enforce a reasonable limit (e.g., 100 elements) on this array.
packages/services/api/src/modules/operations/resolvers/Target.ts (67-71)
The clientVersionFilters input is mapped and passed to the operations manager without any validation of the array sizes (both the number of filters and the number of versions per filter). This data eventually reaches the OperationsReader where it is used to build complex SQL queries with large IN clauses. This can be exploited to cause a Denial of Service on the database or the API service. Please implement size limits for these arrays.
packages/services/api/src/modules/saved-filters/providers/saved-filters.provider.ts (39)
The name field is defined as nullable in the UpdateSavedFilterInputModel, but the corresponding database column saved_filters.name is NOT NULL. This inconsistency can lead to unexpected behavior where providing name: null in a mutation silently does nothing instead of raising an error. To ensure data integrity and provide clear feedback to the user, the validation should not allow null for the name.
name: zod.string().min(1).max(100).optional(),
packages/services/storage/src/index.ts (4032-4035)
The OR condition used to handle filter visibility ("visibility" = 'shared' OR ("visibility" = 'private' AND ...)), combined with pagination, can lead to suboptimal query performance, especially as the number of saved filters grows. The database might struggle to use the saved_filters_project_type_visibility_pagination index effectively with this OR logic.
A more performant approach would be to refactor this query using UNION ALL to handle the two visibility cases separately. This allows the database to use the index efficiently for both parts of the query, which should improve performance.
jonathanawesome
changed the title
|
🐋 This PR was built and pushed to the following Docker images: Targets: Platforms: Image Tag: |
💻 Website PreviewThe latest changes are available as preview in: https://pr-7662.hive-landing-page.pages.dev |
Create SavedFiltersStorage class in saved-filters module Update SavedFiltersProvider to use new storage class Remove saved filters methods from legacy storage
…columns are NOT NULL
|
Some additional feedback, nothing that should be part of this PR though. I am spoiled from the linear "type to search" feature in the context menu and would love to see the same in ours. 😁 I could see that happen in several ways. E.g. when I am in a context submenu menu with a search field, startign typing would automatically focus and populate the Or more like linear "AI Filters", where on the top level context menu, we already had a search input that can find operations, clients, filters etc. |
4 participants
This PR implements a saved filters feature for Insights.
The feature has the following requirements as per internal and external stakeholders:
As a reminder, we decided early on in this discussion that we should invest in this opportunity to improve our filtering UI. This resulted in significant UI changes with the following implications:
components/base@base-ui/reactas an alternative to headless/radix/etcclassNamein favor of providing strict style variantsSheetswith a Linear-like approach to filtering