Conversation
|
🚅 Deployed to the rivet-pr-4325 environment in rivet-frontend
|
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR Review: feat: dev toolbarOverall this is a nice, well-scoped dev utility. The gating mechanism (localStorage key + cloud-only) is a reasonable approach for an internal debug tool. A few things worth addressing before merge: Bug: Incorrect shortcut badge for "load toolbar"The registered hotkey sequence and the displayed badge don't match: // Registered as 3-key sequence: Mod+Shift+G → R → L
useHotkeySequence([debugSequence, "R", "L"], () => {
posthog.toolbar.loadToolbar();
});
// But badge shows only 2-key sequence (missing "L")
<ShortcutBadge hotkey="Mod+Shift+G R" /> // ← should be "Mod+Shift+G R L"The other shortcuts are consistent between their registered sequences and badges, so this looks like a copy-paste oversight. Performance: Unconditional 1-second re-renderThe forced re-render every second is a code smell. The only thing that seems to need live updates is the PostHog recording status. Consider subscribing directly to posthog state instead, or at minimum only running the interval when the toolbar is actually enabled: const [, setState] = useState({}); // used just to trigger re-render every second
useEffect(() => {
const interval = setInterval(() => {
setState({});
}, 1000);
return () => clearInterval(interval);
}, []);Each tick creates a new object reference and schedules a re-render even when nothing has changed. If there's no reactive way to get Minor: Trailing space in classNameconst Sep = () => <span className=" text-muted-foreground">⋅</span>;
// ^ extra leading spaceDependency: Pre-1.0 package with pinned peer deps
Nit:
|
1 participant
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: