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: add chat search implementation #857

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tobiasbueschel
Copy link

@tobiasbueschel tobiasbueschel commented Mar 10, 2025

This PR implements the ability to search chats, inspired by how it works in the ChatGPT user interface.

  • Supports keyboard shortcuts CMD + K or CTRL + K to open search dialog depending on the operating system used
  • Shows a preview of the search results by matching any message titles or content
  • Allows users to quickly create a new chat from the search dialog
search.mov

Potential future improvements:

  • support semantic search or even something like LLM-powered search
  • better support for searching through tool call results & generating a preview instead of saying "No preview available"
  • removing markdown formatting in search preview so that we can use bold text to highlight the matched search query in the preview
  • adding test cases (Shall I add test cases for e.g., the utils, the chat-search.tsx component to the tests/ folder?)
  • if a search result is found opening it could auto-scroll to the message the user was searching for and highlight the matched search term in the message for a few seconds
  • search filtering similar to how it works in GitHub e.g.,:
    • searching for particular artifact types => artifact:sheet or artifact:text
    • searching for public chats => visibility:public

@jeremyphilemon & @jaredpalmer: please let me know what else you'd like me to add / change in this PR 🙏

Copy link

vercel bot commented Mar 10, 2025

@tobiasbueschel is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

socket-security bot commented Mar 10, 2025

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/[email protected] Transitive: environment +30 1.27 MB paco

View full report↗︎

return {
id: result.id,
title: result.title || "Untitled",
// TODO: Strip any markdown formatting from the preview
Copy link
Author

Choose a reason for hiding this comment

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

We could use something like https://www.npmjs.com/package/remove-markdown or switch react-markdown entirely to use a unified pipeline across the app and then have some custom handler to strip out any formatting (the latter would give more flexibility for other features too).

Copy link
Author

Choose a reason for hiding this comment

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

FYI - I ran pnpm lint:fix and pnpm format to ensure the new code is compliant to all linting and formatting rules and noticed there were a lot of unrelated changes. This is due to the fact that the codebase currently doesn't correctly enforce these rules on new PRs and code that gets merged.

Perhaps it is worth to tackle this in a separate PR and enable biome ci to run in a GitHub Action instead of the current biome lint as described here:

This would ensure that all future code is following the same format & linting rules!

@solomonshalom
Copy link

This is pretty cool

+1

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.

2 participants