-
Notifications
You must be signed in to change notification settings - Fork 390
[HDX-2300] introduce Shared Filters for team-wide filter visibility and discoverability #2047
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
Open
brandon-pereira
wants to merge
24
commits into
main
Choose a base branch
from
brandon/global-pinned-filters
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+2,131
−306
Open
Changes from 22 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
5248bf5
feat: store pinned filters in MongoDB for team-wide sharing (HDX-2300)
brandon-pereira 07769c6
add changeset, fix knip
brandon-pereira 203ce6c
Merge branch 'main' into brandon/global-pinned-filters
brandon-pereira c9210b5
fix spacing
brandon-pereira 67e802c
Merge branch 'brandon/global-pinned-filters' of https://github.com/hy…
brandon-pereira 96fe754
claude feedback
brandon-pereira 7a2ff54
fix: fix failing E2E and integration tests for pinned filters
brandon-pereira 68999fe
fix(e2e): increase assertion timeouts for slow CI
brandon-pereira 5111013
improve ux on shared vs pinned icons
brandon-pereira cbe14bb
clean up reset logic
brandon-pereira 9f2e735
separate clear icons
brandon-pereira e817a9c
attempting to reduce duplicate code
brandon-pereira a30acf9
Merge remote-tracking branch 'origin/main' into brandon/global-pinned…
brandon-pereira 61ee88d
claude feedback / knip fixes / int test fixes
brandon-pereira 16da565
remove scoping test since no longer needed
brandon-pereira c418004
more claude fixes
brandon-pereira 6ecbed4
more feedback
brandon-pereira 232baea
update changeset and fix e2e tests
brandon-pereira ce2b797
fix failing tests
brandon-pereira 721f5d6
improve flakeyness of tests in ci
brandon-pereira 657184a
more claude feedback :P
brandon-pereira 9c8a0f0
attempt to improve test
brandon-pereira 5a3a3bd
pr feedback
brandon-pereira 8ab5199
Merge origin/main - resolve localStore conflict, drop unused hashCode…
brandon-pereira File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@hyperdx/common-utils": minor | ||
| "@hyperdx/api": minor | ||
| "@hyperdx/app": minor | ||
| --- | ||
|
|
||
| Introduces Shared Filters, enabling teams to pin and surface common filters across all members. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import type { PinnedFiltersValue } from '@hyperdx/common-utils/dist/types'; | ||
| import mongoose from 'mongoose'; | ||
|
|
||
| import type { ObjectId } from '@/models'; | ||
| import PinnedFilterModel from '@/models/pinnedFilter'; | ||
|
|
||
| /** | ||
| * Get team-level pinned filters for a team+source combination. | ||
| */ | ||
| export async function getPinnedFilters( | ||
| teamId: string | ObjectId, | ||
| sourceId: string | ObjectId, | ||
| ) { | ||
| return PinnedFilterModel.findOne({ | ||
| team: new mongoose.Types.ObjectId(teamId), | ||
| source: new mongoose.Types.ObjectId(sourceId), | ||
| user: null, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Upsert team-level pinned filters for a team+source. | ||
| */ | ||
| export async function updatePinnedFilters( | ||
| teamId: string | ObjectId, | ||
| sourceId: string | ObjectId, | ||
| data: { fields: string[]; filters: PinnedFiltersValue }, | ||
| ) { | ||
| const filter = { | ||
| team: new mongoose.Types.ObjectId(teamId), | ||
| source: new mongoose.Types.ObjectId(sourceId), | ||
| user: null, | ||
| }; | ||
|
|
||
| return PinnedFilterModel.findOneAndUpdate( | ||
| filter, | ||
| { | ||
| ...filter, | ||
| fields: data.fields, | ||
| filters: data.filters, | ||
| }, | ||
| { upsert: true, new: true }, | ||
| ); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| import type { PinnedFiltersValue } from '@hyperdx/common-utils/dist/types'; | ||
| import mongoose, { Schema } from 'mongoose'; | ||
|
|
||
| import type { ObjectId } from '.'; | ||
|
|
||
| interface IPinnedFilter { | ||
| _id: ObjectId; | ||
| team: ObjectId; | ||
| source: ObjectId; | ||
| user: ObjectId | null; // null = team-level, non-null = personal | ||
| fields: string[]; | ||
| filters: PinnedFiltersValue; | ||
| createdAt: Date; | ||
| updatedAt: Date; | ||
| } | ||
|
|
||
| const PinnedFilterSchema = new Schema<IPinnedFilter>( | ||
| { | ||
| team: { | ||
| type: mongoose.Schema.Types.ObjectId, | ||
| required: true, | ||
| ref: 'Team', | ||
| }, | ||
| source: { | ||
| type: mongoose.Schema.Types.ObjectId, | ||
| required: true, | ||
| ref: 'Source', | ||
| }, | ||
| user: { | ||
| type: mongoose.Schema.Types.ObjectId, | ||
| default: null, | ||
| ref: 'User', | ||
| }, | ||
| fields: { | ||
| type: [String], | ||
| default: [], | ||
| }, | ||
| filters: { | ||
| type: Schema.Types.Mixed, | ||
| default: {}, | ||
| }, | ||
| }, | ||
| { | ||
| timestamps: true, | ||
| toJSON: { getters: true }, | ||
| }, | ||
| ); | ||
|
|
||
| // One document per team+source+user combination | ||
| // user=null means team-level pins | ||
| PinnedFilterSchema.index({ team: 1, source: 1, user: 1 }, { unique: true }); | ||
|
|
||
| export default mongoose.model<IPinnedFilter>( | ||
| 'PinnedFilter', | ||
| PinnedFilterSchema, | ||
| ); |
214 changes: 214 additions & 0 deletions
214
packages/api/src/routers/api/__tests__/pinnedFilters.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,214 @@ | ||
| import { SourceKind, TSource } from '@hyperdx/common-utils/dist/types'; | ||
| import { Types } from 'mongoose'; | ||
|
|
||
| import { getLoggedInAgent, getServer } from '@/fixtures'; | ||
| import { Source } from '@/models/source'; | ||
|
|
||
| const MOCK_SOURCE: Omit<Extract<TSource, { kind: 'log' }>, 'id'> = { | ||
| kind: SourceKind.Log, | ||
| name: 'Test Source', | ||
| connection: new Types.ObjectId().toString(), | ||
| from: { databaseName: 'test_db', tableName: 'test_table' }, | ||
| timestampValueExpression: 'timestamp', | ||
| defaultTableSelectExpression: 'body', | ||
| }; | ||
|
|
||
| describe('pinnedFilters router', () => { | ||
| const server = getServer(); | ||
| let agent: Awaited<ReturnType<typeof getLoggedInAgent>>['agent']; | ||
| let team: Awaited<ReturnType<typeof getLoggedInAgent>>['team']; | ||
| let sourceId: string; | ||
|
|
||
| beforeAll(async () => { | ||
| await server.start(); | ||
| }); | ||
|
|
||
| beforeEach(async () => { | ||
| const result = await getLoggedInAgent(server); | ||
| agent = result.agent; | ||
| team = result.team; | ||
|
|
||
| // Create a real source owned by this team | ||
| const source = await Source.create({ ...MOCK_SOURCE, team: team._id }); | ||
| sourceId = source._id.toString(); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await server.clearDBs(); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await server.stop(); | ||
| }); | ||
|
|
||
| describe('GET /pinned-filters', () => { | ||
| it('returns null when no pinned filters exist', async () => { | ||
| const res = await agent | ||
| .get(`/pinned-filters?source=${sourceId}`) | ||
| .expect(200); | ||
|
|
||
| expect(res.body.team).toBeNull(); | ||
| }); | ||
|
|
||
| it('rejects invalid source id', async () => { | ||
| await agent.get('/pinned-filters?source=not-an-objectid').expect(400); | ||
| }); | ||
|
|
||
| it('rejects missing source param', async () => { | ||
| await agent.get('/pinned-filters').expect(400); | ||
| }); | ||
|
|
||
| it('returns 404 for a source not owned by the team', async () => { | ||
| const foreignSourceId = new Types.ObjectId().toString(); | ||
| await agent.get(`/pinned-filters?source=${foreignSourceId}`).expect(404); | ||
| }); | ||
| }); | ||
|
|
||
| describe('PUT /pinned-filters', () => { | ||
| it('can create pinned filters', async () => { | ||
| const res = await agent | ||
| .put('/pinned-filters') | ||
| .send({ | ||
| source: sourceId, | ||
| fields: ['ServiceName', 'SeverityText'], | ||
| filters: { ServiceName: ['web', 'api'] }, | ||
| }) | ||
| .expect(200); | ||
|
|
||
| expect(res.body.fields).toEqual(['ServiceName', 'SeverityText']); | ||
| expect(res.body.filters).toEqual({ ServiceName: ['web', 'api'] }); | ||
| expect(res.body.id).toBeDefined(); | ||
| }); | ||
|
|
||
| it('upserts on repeated PUT', async () => { | ||
| await agent | ||
| .put('/pinned-filters') | ||
| .send({ | ||
| source: sourceId, | ||
| fields: ['ServiceName'], | ||
| filters: { ServiceName: ['web'] }, | ||
| }) | ||
| .expect(200); | ||
|
|
||
| const res = await agent | ||
| .put('/pinned-filters') | ||
| .send({ | ||
| source: sourceId, | ||
| fields: ['ServiceName', 'SeverityText'], | ||
| filters: { ServiceName: ['web', 'api'], SeverityText: ['error'] }, | ||
| }) | ||
| .expect(200); | ||
|
|
||
| expect(res.body.fields).toEqual(['ServiceName', 'SeverityText']); | ||
| expect(res.body.filters).toEqual({ | ||
| ServiceName: ['web', 'api'], | ||
| SeverityText: ['error'], | ||
| }); | ||
| }); | ||
|
|
||
| it('rejects invalid source id', async () => { | ||
| await agent | ||
| .put('/pinned-filters') | ||
| .send({ source: 'not-valid', fields: [], filters: {} }) | ||
| .expect(400); | ||
| }); | ||
|
|
||
| it('returns 404 for a source not owned by the team', async () => { | ||
| const foreignSourceId = new Types.ObjectId().toString(); | ||
| await agent | ||
| .put('/pinned-filters') | ||
| .send({ source: foreignSourceId, fields: [], filters: {} }) | ||
| .expect(404); | ||
| }); | ||
| }); | ||
|
|
||
| describe('GET + PUT round-trip', () => { | ||
| it('returns data after PUT', async () => { | ||
| await agent | ||
| .put('/pinned-filters') | ||
| .send({ | ||
| source: sourceId, | ||
| fields: ['ServiceName'], | ||
| filters: { ServiceName: ['web'] }, | ||
| }) | ||
| .expect(200); | ||
|
|
||
| const res = await agent | ||
| .get(`/pinned-filters?source=${sourceId}`) | ||
| .expect(200); | ||
|
|
||
| expect(res.body.team).not.toBeNull(); | ||
| expect(res.body.team.fields).toEqual(['ServiceName']); | ||
| expect(res.body.team.filters).toEqual({ ServiceName: ['web'] }); | ||
| }); | ||
|
|
||
| it('can reset by sending empty fields and filters', async () => { | ||
| await agent | ||
| .put('/pinned-filters') | ||
| .send({ | ||
| source: sourceId, | ||
| fields: ['ServiceName'], | ||
| filters: { ServiceName: ['web'] }, | ||
| }) | ||
| .expect(200); | ||
|
|
||
| await agent | ||
| .put('/pinned-filters') | ||
| .send({ source: sourceId, fields: [], filters: {} }) | ||
| .expect(200); | ||
|
|
||
| const res = await agent | ||
| .get(`/pinned-filters?source=${sourceId}`) | ||
| .expect(200); | ||
|
|
||
| expect(res.body.team).not.toBeNull(); | ||
| expect(res.body.team.fields).toEqual([]); | ||
| expect(res.body.team.filters).toEqual({}); | ||
| }); | ||
| }); | ||
|
|
||
| describe('source scoping', () => { | ||
| it('pins are scoped to their source', async () => { | ||
| const source2 = await Source.create({ ...MOCK_SOURCE, team: team._id }); | ||
|
|
||
| await agent | ||
| .put('/pinned-filters') | ||
| .send({ | ||
| source: sourceId, | ||
| fields: ['ServiceName'], | ||
| filters: { ServiceName: ['web'] }, | ||
| }) | ||
| .expect(200); | ||
|
|
||
| const res = await agent | ||
| .get(`/pinned-filters?source=${source2._id}`) | ||
| .expect(200); | ||
|
|
||
| expect(res.body.team).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| // Note: cross-team isolation (Team B cannot read Team A's pins) is enforced | ||
| // by the MongoDB query filtering on teamId AND the source ownership check | ||
| // (getSource validates source.team === teamId). Multi-team integration tests | ||
| // are not possible in this single-team environment (register returns 409). | ||
|
|
||
| describe('filter values with booleans', () => { | ||
| it('supports boolean values in filters', async () => { | ||
| await agent | ||
| .put('/pinned-filters') | ||
| .send({ | ||
| source: sourceId, | ||
| fields: ['isRootSpan'], | ||
| filters: { isRootSpan: [true, false] }, | ||
| }) | ||
| .expect(200); | ||
|
|
||
| const res = await agent | ||
| .get(`/pinned-filters?source=${sourceId}`) | ||
| .expect(200); | ||
|
|
||
| expect(res.body.team.filters).toEqual({ isRootSpan: [true, false] }); | ||
| }); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I see that the code includes the capability to store user-level pinned filters, but I don't see that implemented anywhere.
IMO, we shouldn't remove user-specific pinned filters, since different users are likely to care about different filter keys and values. Having no special UI for "pin for team" vs "pin for me" is likely to result in various teammates modifying the filters for the entire team without realizing they're doing so.
It's also odd that the migration from local --> team/db filters is automatic, and applies to the entire team. The first user to login after deploying this will have their filters set for the entire team, possibly unexpectedly.
I would suggest we implement both user-level and team-level filters, and have the migration create user-level pinned filters.
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.
@pulpdrew you're right, I was trying to keep the scope small and minimize UX changes, but it didn't make sense.
I have changed the UX of this so that clicking the pin icons opens a menu where you can select between pin and share. I have updated the PR description with a video.
Previously localStorage was still used for local mode, but now localStorage is used for pinning and mongo is used for sharing with team.
I have also removed the migration since it's not needed.
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.
Makes sense, though with user-specific pinned filters always using local storage, should we remove the user field from this model now?