Skip to content
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

feat(assistant): Make the sidebar Max's home #28710

Merged
merged 13 commits into from
Feb 19, 2025
Merged

feat(assistant): Make the sidebar Max's home #28710

merged 13 commits into from
Feb 19, 2025

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Feb 14, 2025

Problem

People don't necessarily want to always go to a full-page Max view to get insights. It'd be faster to be able to query while already doing something else, from the sidebar.

Changes

Max is in a side panel now:

0
1
2

This side panel can still be expanded into the full-page view.

How did you test this code?

TODO: Stories See UI snapshot changes.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR integrates Max, PostHog's AI assistant, into the application's sidebar panel, moving it from its previous location in the main navigation menu.

  • Added new SidePanelMax component in /frontend/src/layout/navigation-3000/sidepanel/panels/SidePanelMax.tsx with a minimalist interface and "Open as main focus" button
  • Modified SidePanel.tsx to include Max tab with dynamic icon that switches between hedgehog profile and sparkles based on panel state
  • Enhanced Thread.tsx with container queries (@container/thread) and responsive design improvements for better mobile/sidebar display
  • Updated QuestionInput.tsx and QuestionSuggestions.tsx with improved layout and styling for sidebar integration
  • Added error handling and message management improvements in maxLogic.ts for better API response handling and thread management

13 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile

frontend/src/scenes/max/QuestionInput.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/max/QuestionInput.tsx Show resolved Hide resolved
frontend/src/scenes/max/QuestionInput.tsx Show resolved Hide resolved
frontend/src/scenes/max/Intro.tsx Outdated Show resolved Hide resolved
@Twixes Twixes force-pushed the side-panel-max branch 2 times, most recently from ea8b06d to 958acf2 Compare February 17, 2025 18:47

This comment was marked as outdated.

@posthog-bot

This comment was marked as outdated.

@Twixes Twixes force-pushed the side-panel-max branch 2 times, most recently from 7db8699 to ef79641 Compare February 18, 2025 19:12
@posthog-bot

This comment was marked as outdated.

{rating !== 'bad' && (
<LemonButton
icon={rating === 'good' ? <IconThumbsUpFilled /> : <IconThumbsUp />}
type="tertiary"
size="small"
size="xsmall"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message actions, like thumbs up, are generally more compact now. I think currently even too compact, because the xmsall icon is absolutely tiny. I'll make it a bit larger, but in a separate PR, because it's more of a design system change

Comment on lines +275 to +286
const threadEl = document.getElementsByClassName('@container/thread')[0]
let scrollableEl = threadEl?.parentElement // .Navigation3000__scene or .SidePanel3000__content
if (scrollableEl && !scrollableEl.classList.contains('SidePanel3000__content')) {
// In this case we need to go up to <main>, since .Navigation3000__scene is not scrollable
scrollableEl = scrollableEl.parentElement
}
if (scrollableEl) {
scrollableEl.scrollTo({
top: threadEl.scrollHeight,
behavior: 'smooth',
})
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not as elegant, but this ensures we always scroll the scrollable parent of the thread in both the full-page and side-panel case


import { SidePanelTab } from '~/types'

import type { sidePanelStateLogicType } from './sidePanelStateLogicType'

export const WithinSidePanelContext = React.createContext<boolean>(false)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much like Within3000PageHeader

@Twixes Twixes requested review from skoob13 and Copilot February 18, 2025 22:26

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 19 changed files in this pull request and generated no comments.

Files not reviewed (14)
  • frontend/src/scenes/max/Thread.tsx: Evaluated as low risk
  • frontend/src/scenes/settings/organization/AIConsentPopoverWrapper.tsx: Evaluated as low risk
  • frontend/src/scenes/scenes.ts: Evaluated as low risk
  • frontend/src/layout/navigation-3000/navigationLogic.tsx: Evaluated as low risk
  • frontend/src/layout/navigation-3000/sidepanel/sidePanelStateLogic.tsx: Evaluated as low risk
  • frontend/src/scenes/notebooks/NotebookPanel/NotebookPanel.tsx: Evaluated as low risk
  • frontend/src/lib/lemon-ui/Link/Link.tsx: Evaluated as low risk
  • frontend/src/scenes/max/QuestionInput.tsx: Evaluated as low risk
  • frontend/src/scenes/max/QuestionSuggestions.tsx: Evaluated as low risk
  • frontend/src/types.ts: Evaluated as low risk
  • frontend/src/scenes/max/Intro.tsx: Evaluated as low risk
  • frontend/src/scenes/notebooks/Notebook/NotebookMeta.tsx: Evaluated as low risk
  • frontend/src/layout/navigation-3000/sidepanel/sidePanelLogic.tsx: Evaluated as low risk
  • frontend/src/layout/navigation-3000/sidepanel/SidePanel.tsx: Evaluated as low risk
Comments suppressed due to low confidence (1)

frontend/src/scenes/max/Max.tsx:1

  • Ensure that the 'IconSidePanel' and 'IconArrowLeft' icons are correctly imported and used in the JSX.
import { IconArrowLeft, IconGear, IconSidePanel } from '@posthog/icons'
@posthog-bot

This comment was marked as outdated.

@posthog-bot

This comment was marked as outdated.

@posthog-bot

This comment was marked as outdated.

@posthog-bot

This comment was marked as outdated.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

19 snapshot changes in total. 0 added, 19 modified, 0 deleted:

  • chromium: 0 added, 19 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

@skoob13 skoob13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found two things:

  1. How can I access Max on mobile? The sidebar seems to be unavailable in mobile. As an option, we can just keep Max on its own page.
  2. Insights height is too low. It's 150px now. 240px looks much better IMO.

Neither issue blocks it from shipping though.

@@ -98,6 +98,12 @@ export const Link: React.FC<LinkProps & React.RefAttributes<HTMLElement>> = Reac
href: typeof to === 'string' ? to : undefined,
})

const withinSidePanel = useContext(WithinSidePanelContext)

if (withinSidePanel && target === '_blank' && !isExternalLink(to)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the same effect is possible with event capturing on the Max's level without using the context, but this looks good to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure, thinking more generally here as we'll now be adding more such links/actions

to: urls.max(),
tag: 'beta' as const,
})
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we've discussed, people would expect Max to be in the sidebar navigation for some time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added back, with an info toast:
Screenshot 2025-02-19 at 17 09 01

@@ -350,6 +362,7 @@ export const maxLogic = kea<maxLogicType>([
actions.loadSuggestions({ refresh: 'async_except_on_cache_miss' })
}
}),
permanentlyMount(), // Prevent state from being reset when Max is unmounted, especially key in the side panel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to prioritize the reset button😄

@Twixes Twixes enabled auto-merge (squash) February 19, 2025 16:10
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes Twixes merged commit 2621209 into master Feb 19, 2025
98 checks passed
@Twixes Twixes deleted the side-panel-max branch February 19, 2025 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants