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

Extract search #303

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/deploy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@ jobs:
| sed s/{CODA_TOKEN}/${{ secrets.CODA_TOKEN }}/ \
| sed s/{CODA_INCOMING_TOKEN}/${{ secrets.CODA_INCOMING_TOKEN }}/ \
| sed s/{CODA_WRITES_TOKEN}/${{ secrets.CODA_WRITES_TOKEN }}/ \
| sed s/{ALLOW_ORIGINS}// \
> wrangler.toml
npm ci
npm run build --prefix stampy-search
npm run deploy
- name: 'Debug: list files in build directory'
run: ls -R build public/build
1 change: 1 addition & 0 deletions .github/workflows/pr-check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@ jobs:
- name: lint
run: |
npm ci
npm run build --prefix stampy-search
npm run lint
npm run test
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,6 @@ node_modules

.DS_store
wrangler.toml

stampy-search/dist
stampy-search/example/stampySearch.min.js
87 changes: 57 additions & 30 deletions app/components/search.tsx
Original file line number Diff line number Diff line change
@@ -1,36 +1,69 @@
import {useState, useEffect, useRef, MutableRefObject, FocusEvent} from 'react'
import {useState, useEffect, useCallback, useRef, MutableRefObject, FocusEvent} from 'react'
import debounce from 'lodash/debounce'
import {Searcher, Question as QuestionType, SearchResult} from 'stampy-search'
import {AddQuestion} from '~/routes/questions/add'
import {Action, ActionType} from '~/routes/questions/actions'
import {MagnifyingGlass, Edit} from '~/components/icons-generated'
import {useSearch, Question as QuestionType, SearchResult} from '~/hooks/search'
import AutoHeight from 'react-auto-height'
import Dialog from '~/components/dialog'

type Props = {
onSiteAnswersRef: MutableRefObject<QuestionType[]>
initialQuery?: string
openQuestionTitles: string[]
onSelect: (pageid: string, title: string) => void
}

const empty: [] = []

export default function Search({onSiteAnswersRef, openQuestionTitles, onSelect}: Props) {
const [showResults, setShowResults] = useState(false)
export default function Search({
onSiteAnswersRef,
initialQuery,
openQuestionTitles,
onSelect,
}: Props) {
const [showResults, setShowResults] = useState(initialQuery !== undefined)
const [showMore, setShowMore] = useState(false)
const searchInputRef = useRef('')

const {search, arePendingSearches, results} = useSearch(onSiteAnswersRef)
const [arePendingSearches, setPendingSearches] = useState(false)
const [results, setResults] = useState([] as SearchResult[])
const [searcher, setSearcher] = useState<Searcher>()

useEffect(() => {
setSearcher(
new Searcher({
getAllQuestions: () => onSiteAnswersRef.current,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a very useful exercise to find out how reusable is current architecture, and I'd be happy for this code to live on a branch or fork for experimental deployments.

However, I am not able to confirm this is the right step for the master branch and production deployment, where we should aim for more maintainability and simplified architecture. Adding more features on top of previous hacks does not sound sustainable to me.

For 3rd party users of this feature, creating new class instances to spin of workers sensitive to CORS and special API calls while having to provide your own getAllQuestions, then dealing with optional-results success callbacks without error handling and quite complex semantics ... that doesn't sound very simple to use / modular / ...

If there is someone else who wants to take responsibility for long term maintenance of this repo with occational firefighting and debugging if the site stops working, I'd be happy to hand over the "architecture oversight".

If not, I will rather focus on figuring out how to include stampy search on 3rd party sites using some simpler API, without this new PoC feature 🤔 Ideally something drop-in replacable with API from https://nlp.stampy.ai/ (or whatever is / will be the current url / name for that feature)

onResolveCallback: (query?: string, res?: SearchResult[] | null) => {
if (res) {
setPendingSearches(false)
setResults(res)
}
},
})
)
}, [onSiteAnswersRef])

const searchFn = (rawValue: string) => {
const value = rawValue.trim()
if (value === searchInputRef.current) return
const searchFn = useCallback(
(rawValue: string) => {
const value = rawValue.trim()
if (value === searchInputRef.current) return

searchInputRef.current = value
searchInputRef.current = value

search(value)
logSearch(value)
}
setPendingSearches(true)
searcher && searcher.searchLive(value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is probably mostly for TS reasons, but it shouldn't be a valid option to NOT perform the search at this step, if it "always works" it should throw an exception after future changes that make it suddenly "not work"

logSearch(value)
},
[searcher]
)

// Show the url query if defined
useEffect(() => {
if (initialQuery && searcher) {
initialQuery && searchFn(initialQuery)
}
}, [initialQuery, searcher, searchFn])

const handleChange = debounce(searchFn, 100)

Expand Down Expand Up @@ -63,6 +96,7 @@ export default function Search({onSiteAnswersRef, openQuestionTitles, onSelect}:
name="searchbar"
placeholder="Search for more questions here..."
autoComplete="off"
defaultValue={searchInputRef.current}
onChange={(e) => handleChange(e.currentTarget.value)}
onKeyDown={(e) => e.key === 'Enter' && searchFn(e.currentTarget.value)}
/>
Expand Down Expand Up @@ -107,12 +141,13 @@ export default function Search({onSiteAnswersRef, openQuestionTitles, onSelect}:
</div>
</AutoHeight>
</div>
{showMore && (
{showMore && searcher && (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the searcher gets created as soon as the component is loaded. It's not properly configured then, as the worker still has to start up, but it's around, so this will be displayed as soon as the component gets loaded

<ShowMoreSuggestions
onClose={() => {
setShowMore(false)
setHide(true)
}}
searcher={searcher}
question={searchInputRef.current}
relatedQuestions={results.map(({title}) => title)}
/>
Expand Down Expand Up @@ -161,33 +196,25 @@ const ShowMoreSuggestions = ({
question,
relatedQuestions,
onClose,
searcher,
}: {
question: string
relatedQuestions: string[]
onClose: (e: unknown) => void
searcher: Searcher
}) => {
const [extraQuestions, setExtraQuestions] = useState<SearchResult[]>(empty)
const [error, setError] = useState<string>()

useEffect(() => {
const getResults = async (question: string) => {
try {
const result = await fetch(`/questions/search?question=${encodeURIComponent(question)}`)

if (result.status == 200) {
const questions = await result.json()
setExtraQuestions(questions) // don't set on API errors
} else {
console.error(await result.text())
setError('Error while searching for similar questions')
}
} catch (e) {
searcher
.searchUnpublished(question, 5)
.then((res: SearchResult[] | null) => res && setExtraQuestions(res))
.catch((e: any) => {
console.error(e)
setError(e instanceof Error ? e.message : '')
}
}
getResults(question)
}, [question])
setError(e)
})
}, [question, searcher])

if (extraQuestions === empty) {
return (
Expand Down
181 changes: 0 additions & 181 deletions app/hooks/search.tsx

This file was deleted.

2 changes: 2 additions & 0 deletions app/routes/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ const Bottom = ({

export default function App() {
const minLogo = useOutletContext<boolean>()
const [remixSearchParams] = useSearchParams()
const {initialQuestionsData} = useLoaderData<ReturnType<typeof loader>>()
const {data: initialQuestions = [], timestamp} = initialQuestionsData ?? {}

Expand Down Expand Up @@ -183,6 +184,7 @@ export default function App() {
onSiteAnswersRef={onSiteAnswersRef}
openQuestionTitles={openQuestionTitles}
onSelect={selectQuestion}
initialQuery={remixSearchParams.get('query') || ''}
/>

{/* Add an extra, draggable div here, so that questions can be moved to the top of the list */}
Expand Down
7 changes: 4 additions & 3 deletions app/routes/questions/search.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@ export const loader = async ({request}: LoaderArgs) => {
const url = new URL(request.url)
const question = url.searchParams.get('question')
const onlyLive = url.searchParams.get('onlyLive') == 'true'
const numResults = parseInt(url.searchParams.get('numResults') || '5', 10)

if (!question) return []

const results = await search(question, onlyLive)
const results = await search(question, onlyLive, numResults)
return jsonCORS(results)
}

export function search(question: string, onlyLive: boolean) {
const url = `${NLP_SEARCH_ENDPOINT}/api/search?query=${question}&top=5&showLive=${
export function search(question: string, onlyLive: boolean, numResults = 5) {
const url = `${NLP_SEARCH_ENDPOINT}/api/search?query=${question}&top=${numResults}&showLive=${
onlyLive ? 1 : 0
}`
return fetch(url).then(async (response) => {
Expand Down
Loading