feat(code): Inbox 2.0#2547
Conversation
|
React Doctor found 26 issues in 15 files · 26 warnings. 26 warnings
Reviewed by React Doctor for commit |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
joshsny
left a comment
joshsny
left a comment
There was a problem hiding this comment.
LGTM, your change to the remote debugging port looks unintentional
oliverb123
left a comment
oliverb123
left a comment
There was a problem hiding this comment.
Did the usual and just dump3e my ui feedback somewhere vaguely sensible, and let you route it properly to the relevant code.
Seems like the major things are the fetching tasks associated with a particular report is broken, and you deleted functionality around changing the suggested reviewers list. Also I think we could go 2 column in the detail view
| import { ConventionalCommitScopeTag } from "@features/inbox/components/ConventionalCommitScopeTag"; | ||
| import { InboxCardSourceMeta } from "@features/inbox/components/InboxCardSourceMeta"; | ||
| import { InboxCardTitle } from "@features/inbox/components/InboxCardTitle"; | ||
| import { SuggestedReviewerAvatarStack } from "@features/inbox/components/SuggestedReviewerAvatarStack"; |
There was a problem hiding this comment.
I dislike the the variance in corner radius here, particularly this close together and not nested, and generally I like the idea of using icons here but making them look a bit like buttons is ugly imo
There was a problem hiding this comment.
Late addition to the PR with those icons, got messed up indeed!
| import type { SignalReport } from "@shared/types"; | ||
| import { Link, useNavigate } from "@tanstack/react-router"; | ||
| import { PriorityMonogram } from "./PriorityMonogram"; | ||
|
|
There was a problem hiding this comment.
I really want a link to the relevant commit here, or at least it's short hash so I can go look at it.
| } | ||
|
|
||
| export function ReportCard({ | ||
| report, |
There was a problem hiding this comment.
I see the open in GitHub button but no way to view the associated tasks? Seems like either the pulling of associated tasks is broken, or this is missing?
| report, | ||
| detailRoute, | ||
| ); | ||
| const navigate = useNavigate(); |
There was a problem hiding this comment.
Feels wider than it needs to be, and puts the evidence and artifacts at the bottom. Particularly once we/I move towards "artifacts as log", we should make this two-column (dynamic to single column when the screen is too narrow) imo, otherwise your burying all the rich data (diffs, tasks, artefacts) below this wall of text
There was a problem hiding this comment.
Agreed, should be no more than 100 chars wide really
So, I like "artifacts" as long. It's kind of a matter of responsive design. If we have the space, via a container query, we can have the description on the left, and the other details to the right. Slightly like a Facebook page UI 🤔
| const { data: artefactsResp } = useInboxReportArtefacts(report.id, { | ||
| staleTime: 5 * 60 * 1000, | ||
| refetchOnWindowFocus: false, | ||
| }); |
There was a problem hiding this comment.
My impression is "runs" is only meant to show ongoing tasks? Seems like a bit of an arbitrary limitation imo, but given it, I'd call it "running" or "live" instead
There was a problem hiding this comment.
Primarily for ongoing runs, but it makes sense to me to show the ongoing ones first, and finished ones lower down
| }); | ||
| const repoSlug = extractRepoSelectionRepository(artefactsResp?.results); | ||
| const firstSource = report.source_products?.[0]; | ||
| const sourceMeta = firstSource ? SOURCE_PRODUCT_META[firstSource] : null; |
There was a problem hiding this comment.
UI seems to be lying about there being only 100 items (what items? reports? runs?) "for me"
Also this feels like a weird place to put this, I thought it would be a tab of curated reports or something, not a filtering UI
There was a problem hiding this comment.
Did feel too tab-like. Made it a segmented button on the right:
There was a problem hiding this comment.
As for the UI lying about the count, you are correct. I'll leave it be for now though, as the fix requires a backend change to be able to query "PRs" and "Reports" separately (i.e. separate list request for reports with code vs. without)
| undefined, | ||
| { month: "short", day: "numeric" }, | ||
| ); | ||
| const isReady = report.status === "ready"; |
There was a problem hiding this comment.
Spacing between the "findings" header and the first finding is bad, count is in a pill but the rest of the ui uses round rects, count pill has a BG color that matches the rest of the UI but icon doesn't
Also feels weird to call them "findings" here but "evidence" in the PR version, unless I'm confused about what this is displaying (it's just a single, right?)
| const conventionalTitle = parseConventionalCommitTitle(report.title); | ||
| const cardTitle = displayConventionalCommitTitle( | ||
| report.title, | ||
| "Untitled report", |
There was a problem hiding this comment.
This header in general could be removed imo, but failing that, "this is your inbox" is really odd and should be removed
There was a problem hiding this comment.
I think the header feels like pretty solid grounding of what you're looked at, so leaving here, but cleaned up descriptions
There was a problem hiding this comment.
BTW the "This is your inbox" got lifted from the mocks I did https://posthog-self-driving.pages.dev/ lol
| ); | ||
|
|
||
| const openDetail = () => { | ||
| prefetch(); |
There was a problem hiding this comment.
No way to view suggested reviewers inside a report, even though we display them in the list view, and no way to modify suggested reviewers on a report OR a PR detail page (serious regression I'd actually block merging on)
There was a problem hiding this comment.
Missed the reviewer editing as it's newer, restored and unified with reports!
Twixes
force-pushed
the
inbox-v2-pr
branch
3 times, most recently
from
f6a3a4b to
01f75cd
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Twixes
force-pushed
the
inbox-v2-pr
branch
8 times, most recently
from
3f3aef4 to
d895363
Compare
5 participants
Summary
Replaces the legacy Inbox list with the v2 Self-driving: Pull requests, Reports, Agents, plus scoped For you filtering.
Polished list/detail UI for PRs, reports, and agent runs, including repo/source metadata, reviewer affordances, and task-log-backed run details.
Promotes agent configuration into a first-class view.