-
Notifications
You must be signed in to change notification settings - Fork 252
Initial issues dashboard with filters #1860
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
base: main
Are you sure you want to change the base?
Conversation
c08e609 to
d4630fa
Compare
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.
This monster query deserves tests. However, I won't be doing that in this PR, so I want to move on to another thing. Anyway this is not yet released
d4630fa to
70b99c2
Compare
70b99c2 to
b946a8a
Compare
| /** | ||
| * SWR cache key for issues list based on filters and sorting | ||
| */ | ||
| export function buildIssuesCacheKey({ |
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.
This is more simpler if you just add the query string to the SWR cache key.... look how eval results v2 store work
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'll keep this way for now
...private)/projects/[projectId]/versions/[commitUuid]/issues/_components/IssuesTitle/index.tsx
Outdated
Show resolved
Hide resolved
...private)/projects/[projectId]/versions/[commitUuid]/issues/_components/IssuesTitle/index.tsx
Outdated
Show resolved
Hide resolved
...private)/projects/[projectId]/versions/[commitUuid]/issues/_components/IssuesTable/index.tsx
Outdated
Show resolved
Hide resolved
| const status = filters.status ?? serverParams.filters.status | ||
| const showStatus = status !== 'active' | ||
|
|
||
| if (noData) return <TableBlankSlate description='No issues in this project' /> |
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.
| if (noData) return <TableBlankSlate description='No issues in this project' /> | |
| if (noData) return <TableBlankSlate description='No issues found for this prompt yet' /> |
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.
It could be for all prompts if no document is selected
| }: { | ||
| workspace: Workspace | ||
| project: Project | ||
| documentUuid: string |
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.
why not pass the document 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.
I'll change when I use it in the next PR
| }, | ||
| transaction = new Transaction(), | ||
| ) { | ||
| return transaction.call(async (tx) => { |
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.
we should publish an issueCreated event maybe?
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.
Same
| `, | ||
| ) | ||
| .as('recentCount'), | ||
| lastSeenDate: sql<Date>`MAX(${issueHistograms.date})`.as( |
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.
you need firstSeenDat with MIN on issueHistograms.date. That should be the first seen at date for the issue!
| ), | ||
| }) | ||
| .from(issueHistograms) | ||
| .where( |
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.
this where is lacking the filter on date by the seenAt range date no?
| inArray(issueHistograms.commitId, commitIds), | ||
| ), | ||
| ) | ||
| .groupBy(issueHistograms.issueId) |
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.
add a having that checks if the total count > 0
| } | ||
|
|
||
| if (filters.firstSeen) { | ||
| conditions.push(gte(issues.createdAt, filters.firstSeen)) |
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.
no, you should use the firstSeenAt of the subquery
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.
Also you should compare the last seen issue date to the first seen of the range calendar.
| ) | ||
| } | ||
|
|
||
| if (filters.lastSeen) { |
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.
this is lacking the first seenAt
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.
Also you should compare the first seen issue date to the last seen of the range calendar.
| if (filters.lastSeen) { | ||
| const toEndOfDay = endOfDay(filters.lastSeen) | ||
| const condition = sql`${sql.raw(`"${HISTOGRAM_SUBQUERY_ALIAS}"."lastSeenDate"`)} <= ${toEndOfDay}` | ||
| if (condition) conditions.push(condition) |
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.
why this if?
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.
To make typescript happy
| * Returns the common GROUP BY clause fields for issues queries with histogram subquery. | ||
| * This includes all issue fields and subquery fields that are selected in the main query. | ||
| */ | ||
| private buildGroupByClause( |
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.
this group by is useless because you are just selecting all columns of the table... do a normal select no?
| .leftJoin(subquery, eq(subquery.issueId, issues.id)) | ||
| .where(and(...whereConditions)) | ||
| .groupBy(...this.buildGroupByClause(subquery)) | ||
| .having( |
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.
this having should be a where no?
| .from(issues) | ||
| .leftJoin(subquery, eq(subquery.issueId, issues.id)) | ||
| .where(and(...whereConditions)) | ||
| .groupBy(...this.buildGroupByClause(subquery)) |
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.
why group by?
| .from(issues) | ||
| .leftJoin(subquery, eq(subquery.issueId, issues.id)) | ||
| .where(and(...where)) | ||
| .groupBy(...this.buildGroupByClause(subquery)) |
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.
why group by
| .leftJoin(subquery, eq(subquery.issueId, issues.id)) | ||
| .where(and(...where)) | ||
| .groupBy(...this.buildGroupByClause(subquery)) | ||
| .having(having.length > 0 ? and(...having) : undefined) |
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.
this should be a where no?
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.
review the main queries
d9e6c63 to
6575397
Compare
I tried moving lastSeenAt and firstSeenAt to the subquery. But it did not work because you can not reference the subquery alias in the where of the subquery. We can review later |
What?
Implement base files for issue models
TODO
Make back navigation work from the middle.Implemented page/offset finallydirection=backwardescalatingwarning andnewaccent no iconsSeen at11d ago / 3yr oldnormaldefault andsmallwithout fancy layer and 32px max like a normal button.Last seen up to ...