fix(search): prevent multiple requests while typing#3910
fix(search): prevent multiple requests while typing#3910Shubb07 wants to merge 15 commits intoOWASP:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces request cancellation via AbortController across the search flow (components and hooks), extracts debounce timing to a constant, removes unnecessary event data handling from MultiSearch, and refactors error handling to return safe defaults instead of throwing, addressing issue Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 7 files
Confidence score: 2/5
- Search behavior risk:
frontend/src/server/fetchAlgoliaData.tsremoves theidx_is_active:truefacet and drops the early return whenIDX_URLis unset, which can expose inactive results and triggerfetch(undefined)at runtime. - Aborted searches may still update UI state because
fetchAlgoliaDataswallowsAbortError, soMultiSearchcan apply stale suggestions despite aborting. - Given multiple high-severity, user-facing search and runtime risks, the merge risk is elevated.
- Pay close attention to
frontend/src/server/fetchAlgoliaData.ts,frontend/src/components/MultiSearch.tsx- search filtering/abort handling and potential runtime fetch errors.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/server/fetchAlgoliaData.ts">
<violation number="1" location="frontend/src/server/fetchAlgoliaData.ts:10">
P1: The `idx_is_active:true` facet filter for `projects` and `chapters` indexes was removed. This filter ensured that only active projects/chapters appeared in search results. No callers pass this filter explicitly, so inactive items will now show up in search results — a functional regression unrelated to the debounce fix.</violation>
<violation number="2" location="frontend/src/server/fetchAlgoliaData.ts:16">
P1: Missing early return when `IDX_URL` is not set. The `console.warn` replaced the old `throw new Error(...)` but no `return` statement was added. This causes the code to fall through and call `fetch(undefined)`, which will always fail. The error is caught and swallowed, but the intent is clearly to bail out here.
Add a `return` with empty results after the warning.</violation>
<violation number="3" location="frontend/src/server/fetchAlgoliaData.ts:38">
P2: Misleading error message: `'Algolia disabled locally'` is logged for any non-OK response (500, 403, etc.), not just when Algolia is unconfigured. This will confuse debugging in production. Consider including the status code and a more accurate message, e.g., `Algolia search failed with status ${response.status}`.</violation>
</file>
<file name="frontend/__tests__/unit/components/SearchValidation.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/SearchValidation.test.tsx:46">
P2: Missing `await` on `act()` — advancing timers past the debounce threshold triggers the async `triggerSearch` callback, which performs async fetch calls and state updates. Without `await`, subsequent assertions may run before these operations complete, causing flaky behavior. This is done correctly in the debounce test (line 50) but not here.</violation>
</file>
<file name="frontend/package.json">
<violation number="1" location="frontend/package.json:121">
P2: The `engines.node` constraint should be `^20.9.0` instead of `^20.0.0`. According to the Next.js 16 docs, the minimum supported Node.js version is **20.9.0**. The current `^20.0.0` allows Node 20.0–20.8, which Next.js 16 does not support.</violation>
</file>
<file name="frontend/src/components/MultiSearch.tsx">
<violation number="1" location="frontend/src/components/MultiSearch.tsx:82">
P2: Abort does not actually prevent stale state updates. `fetchAlgoliaData` catches `AbortError` internally and returns empty results instead of re-throwing, so the aborted invocation still reaches `setSuggestions`/`setShowSuggestions`. The `AbortError` catch in this component is dead code.
To fix, either:
1. Re-throw `AbortError` in `fetchAlgoliaData` so it propagates to the component's catch block, or
2. Check `signal.aborted` after `Promise.all` resolves before updating state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/hooks/useSearchPage.ts (1)
107-112:⚠️ Potential issue | 🟠 MajorDead code: the
elsebranch is unreachable afterfetchAlgoliaDatachanges.
fetchAlgoliaDatanow always returns{ hits: [], totalPages: 0 }on any error (including network failures and non-OK responses), never throwing. This means'hits' in responseis alwaystrue, making theelsebranch dead code. More importantly, errors are silently swallowed —handleAppErrorwill never be called, so users see empty results with no error indication.Consider either:
- Restoring error-throwing behavior in
fetchAlgoliaDatafor non-abort errors so this catch block can display toasts, or- Having
fetchAlgoliaDatareturn a discriminated union (e.g.,{ ok: false, error }) so callers can distinguish errors from empty results.
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/components/SearchValidation.test.tsx`:
- Around line 124-128: The test assertion uses queryByRole('listbox') which
never matches because MultiSearch renders suggestions as a <ul> (role "list");
update the test in SearchValidation.test.tsx to assert against
queryByRole('list') instead of 'listbox' (so change the call near the expect
that currently checks not.toBeInTheDocument()), keeping the consoleSpy
assertions intact; this ensures the test actually verifies the absence of the
suggestions list for the MultiSearch component.
In `@frontend/package.json`:
- Line 121: The package.json engines field was changed to "node": "^20.0.0",
which unintentionally restricts Node versions to 20.x; either restore or update
the engine to the intended supported LTS (e.g., "^24.0.0" or "^22.0.0") or
explicitly document the reason for downgrading in the PR description; update the
"node" entry in package.json accordingly and add a short note in the PR body
explaining why you chose that specific range to avoid surprise in unrelated
changes (refer to the "node" engines key in package.json to locate the change).
In `@frontend/src/components/MultiSearch.tsx`:
- Around line 55-59: The early-return condition in MultiSearch.tsx currently
checks the raw query length and thus allows whitespace-only queries to proceed;
update the check to use query.trim() (e.g., if (!query.trim().length) or if
(query.trim() === '') ) so whitespace-only input triggers setSuggestions([]),
setShowSuggestions(false) and returns early; update the condition around
setSuggestions and setShowSuggestions to use query.trim() to match the GA guard
at the top.
- Around line 38-90: The debounce in triggerSearch (the useMemo that creates the
debounced function) is being recreated whenever the indexes array reference
changes; ensure the indexes prop passed into MultiSearchBar is stable so
triggerSearch isn't reset on every parent render — either memoize the array in
the parent (e.g., useMemo(() => ['chapters','organizations','projects','users'],
[])) or provide a module-level constant and pass that reference; keep the
dependency in MultiSearch's useMemo as indexes (do not change it to omit or
stringify) and update the parent where MultiSearchBar is invoked to pass the
memoized/stable indexes reference.
In `@frontend/src/server/fetchAlgoliaData.ts`:
- Around line 14-17: In fetchAlgoliaData, when IDX_URL is falsy the code only
logs a warning but still proceeds to call fetch(IDX_URL); modify the function
(fetchAlgoliaData) to return early immediately after the console.warn when
IDX_URL is not set (e.g., return an empty result or Promise.resolve([])
consistent with existing return shape) so no fetch is attempted with an
undefined URL and no spurious TypeError is thrown.
🧹 Nitpick comments (4)
frontend/src/utils/constants.ts (1)
36-56: Inconsistent indentation in footerIcons array.The array items switched from 2-space to 4-space indentation, while the rest of the file uses 2-space. If this wasn't intentional (e.g., auto-formatter), consider reverting to keep the file consistent.
frontend/src/server/fetchAlgoliaData.ts (1)
36-40: Misleading warning message on non-OK responses."Algolia disabled locally" is not accurate for a non-OK HTTP status — this could occur in any environment (e.g., server error, auth failure). Consider a more descriptive message like
Algolia request failed with status ${response.status}.✏️ Suggested fix
if (!response.ok) { // eslint-disable-next-line no-console - console.warn('Algolia disabled locally') + console.warn(`Algolia request failed: ${response.status} ${response.statusText}`) return { hits: [], totalPages: 0 } }frontend/src/components/MultiSearch.tsx (1)
238-251: Helper functionsgetHitName/getHitKeycould be extracted outside the component.These are pure functions that don't depend on component state or props. Defining them outside the component avoids unnecessary re-creation on each render and improves readability.
frontend/src/hooks/useSearchPage.ts (1)
128-128: RemovepageTitlefrom the dependency array.
pageTitleis not used inside the effect body (lines 81-128), so including it in the dependency array causes unnecessary re-fetches wheneverpageTitlechanges. Remove it to avoid unintended effect re-runs.♻️ Proposed fix
- }, [currentPage, searchQuery, order, sortBy, hitsPerPage, indexName, pageTitle]) + }, [currentPage, searchQuery, order, sortBy, hitsPerPage, indexName])
|
|
||
| expect(screen.queryByRole('listbox')).not.toBeInTheDocument() | ||
|
|
||
| expect(consoleSpy).toHaveBeenCalled() | ||
| consoleSpy.mockRestore() |
There was a problem hiding this comment.
queryByRole('listbox') never matches — the assertion is vacuous.
The MultiSearch component renders suggestions as <ul> elements (implicit ARIA role "list", not "listbox"). So queryByRole('listbox') returns null regardless of whether suggestions are visible, making this assertion always pass and not actually testing error handling.
Use "list" to match the <ul>, or add role="listbox" to the suggestions container in the component.
🐛 Proposed fix (test side)
- expect(screen.queryByRole('listbox')).not.toBeInTheDocument()
+ expect(screen.queryByRole('list')).not.toBeInTheDocument()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(screen.queryByRole('listbox')).not.toBeInTheDocument() | |
| expect(consoleSpy).toHaveBeenCalled() | |
| consoleSpy.mockRestore() | |
| expect(screen.queryByRole('list')).not.toBeInTheDocument() | |
| expect(consoleSpy).toHaveBeenCalled() | |
| consoleSpy.mockRestore() |
🤖 Prompt for AI Agents
In `@frontend/__tests__/unit/components/SearchValidation.test.tsx` around lines
124 - 128, The test assertion uses queryByRole('listbox') which never matches
because MultiSearch renders suggestions as a <ul> (role "list"); update the test
in SearchValidation.test.tsx to assert against queryByRole('list') instead of
'listbox' (so change the call near the expect that currently checks
not.toBeInTheDocument()), keeping the consoleSpy assertions intact; this ensures
the test actually verifies the absence of the suggestions list for the
MultiSearch component.
frontend/package.json
Outdated
| }, | ||
| "engines": { | ||
| "node": "^24.0.0" | ||
| "node": "^20.0.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js 20 LTS end of life date 2025 2026
💡 Result:
Node.js 20.x (Iron) LTS reaches End-of-Life on April 30, 2026 (that’s when security fixes end). It entered Maintenance LTS on October 22, 2024, so throughout 2025 and early 2026 it’s in maintenance until EOL. [1], [2]
Sources: Node.js release schedule (Node.js Release WG) [1] and Node.js official releases page [2].
🌐 Web query:
Node.js 24 release date existence current version
💡 Result:
- Does Node.js 24 exist? Yes — the Node.js 24.x line (“Krypton”) is an official release branch. [1]
- Node.js 24 initial release date: May 6, 2025 (v24.0.0). [1]
- Current versions (as of Feb 11, 2026):
- Latest LTS: v24.13.0 [2]
- Latest Current (non‑LTS): v25.6.0 [3]
- Most recent update within the 24.x line shown in the official archive: last updated Jan 12, 2026. [4]
Sources: [1] [2] [3] [4]
Node engine downgrade from ^24.0.0 to ^20.0.0 — clarify intent.
This restricts the engine to Node 20.x (^20.0.0 = >=20.0.0 <21.0.0). Node 20 is currently in maintenance mode and reaches end-of-life on April 30, 2026, while Node 24 is the current LTS. If the intent is to widen compatibility, consider ^22.0.0 (Node 22 LTS) or keep ^24.0.0. If this downgrade fixes an incorrect earlier constraint, clarify in the PR description since this change is unrelated to the search debounce objective.
🤖 Prompt for AI Agents
In `@frontend/package.json` at line 121, The package.json engines field was
changed to "node": "^20.0.0", which unintentionally restricts Node versions to
20.x; either restore or update the engine to the intended supported LTS (e.g.,
"^24.0.0" or "^22.0.0") or explicitly document the reason for downgrading in the
PR description; update the "node" entry in package.json accordingly and add a
short note in the PR body explaining why you chose that specific range to avoid
surprise in unrelated changes (refer to the "node" engines key in package.json
to locate the change).
| const triggerSearch = useMemo( | ||
| () => | ||
| debounce(async (query: string) => { | ||
| if (query && query.trim() !== '') { | ||
| sendGAEvent({ | ||
| event: 'homepageSearch', | ||
| path: globalThis.location.pathname, | ||
| value: query, | ||
| }) | ||
| } | ||
| if (query.length > 0) { | ||
| try { | ||
| activeRequest.current?.abort() | ||
|
|
||
| activeRequest.current = new AbortController() | ||
| const signal = activeRequest.current.signal | ||
|
|
||
| if (query && query.trim() !== '') { | ||
| sendGAEvent({ | ||
| event: 'homepageSearch', | ||
| path: globalThis.location.pathname, | ||
| value: query, | ||
| }) | ||
| } | ||
|
|
||
| if (!query.length) { | ||
| setSuggestions([]) | ||
| setShowSuggestions(false) | ||
| return | ||
| } | ||
|
|
||
| const results = await Promise.all( | ||
| indexes.map(async (index) => { | ||
| const data = await fetchAlgoliaData(index, query, pageCount, suggestionCount) | ||
| const data = await fetchAlgoliaData( | ||
| index, | ||
| query, | ||
| pageCount, | ||
| suggestionCount, | ||
| [], | ||
| signal | ||
| ) | ||
|
|
||
| return { | ||
| indexName: index, | ||
| hits: data.hits as Chapter[] | Event[] | Organization[] | Project[] | User[], | ||
| hits: data.hits, | ||
| totalPages: data.totalPages || 0, | ||
| } | ||
| }) | ||
| ) | ||
| const filteredEvents = | ||
| eventData?.filter((event) => event.name.toLowerCase().includes(query.toLowerCase())) || | ||
| [] | ||
|
|
||
| if (filteredEvents.length > 0) { | ||
| results.push({ | ||
| indexName: 'events', | ||
| hits: filteredEvents.slice(0, suggestionCount), | ||
| totalPages: 1, | ||
| }) | ||
| } | ||
| setSuggestions(results.filter((result) => result.hits.length > 0) as Suggestion[]) | ||
| setSuggestions(results.filter((r) => r.hits.length > 0) as Suggestion[]) | ||
| setShowSuggestions(true) | ||
| } else { | ||
| setSuggestions([]) | ||
| setShowSuggestions(false) | ||
| } catch (error: unknown) { | ||
| if ((error as Error).name !== 'AbortError') { | ||
| // eslint-disable-next-line no-console | ||
| console.error(error) | ||
| } | ||
| } | ||
| }, 300), | ||
| [eventData, indexes] | ||
| }, DEBOUNCE_TIME), | ||
| [indexes] | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find usages of MultiSearch or MultiSearchBar component
rg -n 'MultiSearch' --type=tsx --type=ts -C3 -g '!**/MultiSearch.tsx' -g '!**test**' | head -100Repository: OWASP/Nest
Length of output: 81
🏁 Script executed:
#!/bin/bash
# Also check the MultiSearch.tsx file itself to understand the component signature
cat -n frontend/src/components/MultiSearch.tsx | head -40Repository: OWASP/Nest
Length of output: 1922
🏁 Script executed:
#!/bin/bash
# Search for where indexes array is defined or constructed for MultiSearch
rg -n "indexes\s*=" --type=tsx --type=ts -C2 -g '!**test**' | head -150Repository: OWASP/Nest
Length of output: 81
🏁 Script executed:
#!/bin/bash
# Find where MultiSearchBar is imported or used
rg 'MultiSearchBar' -C5 -g '!node_modules' | head -150Repository: OWASP/Nest
Length of output: 11526
🏁 Script executed:
#!/bin/bash
# Check the types file to understand MultiSearchBarProps
rg -n 'MultiSearchBarProps' -C10 -g '!node_modules' | head -100Repository: OWASP/Nest
Length of output: 3236
🏁 Script executed:
#!/bin/bash
# Look for any inline array passing to MultiSearchBar or indexes prop
rg 'indexes\s*=\s*\{?\[' -C3 -g '!node_modules' | head -100Repository: OWASP/Nest
Length of output: 7049
🏁 Script executed:
#!/bin/bash
# Check the Home/page.tsx component to see if indexes is memoized
cat -n frontend/src/app/page.tsx | head -150Repository: OWASP/Nest
Length of output: 5811
🏁 Script executed:
#!/bin/bash
# Check if page.tsx has any useMemo, useCallback, or React.memo wrapping
grep -n "useMemo\|useCallback\|React.memo\|const.*=" frontend/src/app/page.tsx | head -50Repository: OWASP/Nest
Length of output: 463
🏁 Script executed:
#!/bin/bash
# Get the full context around the MultiSearchBar usage
sed -n '1,200p' frontend/src/app/page.tsx | tail -80Repository: OWASP/Nest
Length of output: 3655
useMemo dependency on indexes — debounce resets if array reference changes.
In frontend/src/app/page.tsx, the MultiSearchBar is called with an inline array literal: indexes={['chapters', 'organizations', 'projects', 'users']}. Since the parent component is not memoized and re-renders frequently (on GraphQL data updates, state changes), this creates a new array reference on each render. The useMemo dependency on indexes causes the debounced function to be recreated and the debounce timer to reset on every parent render, defeating the debouncing mechanism.
Memoize the indexes prop in the parent component using useMemo, or accept a stable reference from configuration/constants.
🤖 Prompt for AI Agents
In `@frontend/src/components/MultiSearch.tsx` around lines 38 - 90, The debounce
in triggerSearch (the useMemo that creates the debounced function) is being
recreated whenever the indexes array reference changes; ensure the indexes prop
passed into MultiSearchBar is stable so triggerSearch isn't reset on every
parent render — either memoize the array in the parent (e.g., useMemo(() =>
['chapters','organizations','projects','users'], [])) or provide a module-level
constant and pass that reference; keep the dependency in MultiSearch's useMemo
as indexes (do not change it to omit or stringify) and update the parent where
MultiSearchBar is invoked to pass the memoized/stable indexes reference.
| if (!query.length) { | ||
| setSuggestions([]) | ||
| setShowSuggestions(false) | ||
| return | ||
| } |
There was a problem hiding this comment.
Whitespace-only queries bypass the early return and trigger fetches.
Line 55 checks !query.length, but a whitespace-only string like " " has length > 0 and will proceed to fetch. This is inconsistent with the GA guard on line 47 that checks query.trim() !== ''. Consider using trim() for consistency:
🐛 Proposed fix
- if (!query.length) {
+ if (!query.trim().length) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!query.length) { | |
| setSuggestions([]) | |
| setShowSuggestions(false) | |
| return | |
| } | |
| if (!query.trim().length) { | |
| setSuggestions([]) | |
| setShowSuggestions(false) | |
| return | |
| } |
🤖 Prompt for AI Agents
In `@frontend/src/components/MultiSearch.tsx` around lines 55 - 59, The
early-return condition in MultiSearch.tsx currently checks the raw query length
and thus allows whitespace-only queries to proceed; update the check to use
query.trim() (e.g., if (!query.trim().length) or if (query.trim() === '') ) so
whitespace-only input triggers setSuggestions([]), setShowSuggestions(false) and
returns early; update the condition around setSuggestions and setShowSuggestions
to use query.trim() to match the GA guard at the top.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/server/fetchAlgoliaData.ts">
<violation number="1" location="frontend/src/server/fetchAlgoliaData.ts:15">
P1: Mutating the caller's `facetFilters` array via `.push()`. This will accumulate duplicate `'idx_is_active:true'` entries if the same array is passed across multiple calls (very likely with the debounce pattern). Use a local copy instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/MultiSearch.tsx`:
- Around line 247-253: The getHitKey function can return the literal 'unknown'
for multiple hits causing duplicate React keys; update getHitKey to prefer
'objectID' first (Algolia guarantees it), then fall back to 'key', 'login',
'url', and finally return a unique fallback that includes the list index (or
another unique per-item value) so keys are always unique; change references to
getHitKey where used in rendering to pass the item index if needed so the
function can append it as a last-resort uniqueness suffix.
🧹 Nitpick comments (3)
frontend/src/server/fetchAlgoliaData.ts (2)
10-16: Mutating the caller'sfacetFiltersarray.
facetFilters.push(...)on Line 15 mutates the array passed by the caller. While current call sites pass fresh[]literals, any future caller sharing a reference will silently accumulate stale filters across calls. Prefer creating a local copy.♻️ Proposed fix
- facetFilters: string[] = [], + facetFilters: readonly string[] = [], signal?: AbortSignal ): Promise<AlgoliaResponse<T>> => { try { + const filters = [...facetFilters] if (['projects', 'chapters'].includes(indexName)) { - facetFilters.push('idx_is_active:true') + filters.push('idx_is_active:true') }Then use
filtersinstead offacetFiltersin thebodyon Line 32.
52-59: Misleading catch-all error message.
'Algolia disabled locally:'is only accurate whenIDX_URLis unset (already handled above). At this point the error could be a network failure, JSON parse error, etc. A more accurate message avoids confusion during debugging.✏️ Suggested wording
- console.warn('Algolia disabled locally:', error) + console.warn('Algolia search error:', error)frontend/src/components/MultiSearch.tsx (1)
47-59: GA event fires before the empty-query guard, causing analytics noise for whitespace-only input.Lines 47-53 send a GA event for any truthy trimmed query, but Line 55 only checks
!query.length. If the whitespace fix above is applied, this ordering becomes fine. However, even today the GA event fires before the decision to skip the search — consider moving GA below the guard so analytics only records queries that actually execute.♻️ Reorder GA after the guard
+ if (!query.trim().length) { + setSuggestions([]) + setShowSuggestions(false) + return + } + if (query && query.trim() !== '') { sendGAEvent({ event: 'homepageSearch', path: globalThis.location.pathname, value: query, }) } - - if (!query.length) { - setSuggestions([]) - setShowSuggestions(false) - return - }
| const getHitKey = (hit: Chapter | Project | User | Event | Organization) => { | ||
| if ('key' in hit && hit.key) return hit.key | ||
| if ('login' in hit && hit.login) return hit.login | ||
| if ('url' in hit && hit.url) return hit.url | ||
| if ('objectID' in hit && hit.objectID) return hit.objectID | ||
| return 'unknown' | ||
| } |
There was a problem hiding this comment.
getHitKey fallback 'unknown' can produce duplicate React keys.
If two hits in the same index lack key, login, url, and objectID, they share the key 'unknown', causing React reconciliation issues. Consider incorporating the array index as a last-resort fallback, or using objectID first (Algolia guarantees it).
🐛 Proposed fix — use objectID as primary key (Algolia always provides it)
const getHitKey = (hit: Chapter | Project | User | Event | Organization) => {
+ if ('objectID' in hit && hit.objectID) return hit.objectID
if ('key' in hit && hit.key) return hit.key
if ('login' in hit && hit.login) return hit.login
if ('url' in hit && hit.url) return hit.url
- if ('objectID' in hit && hit.objectID) return hit.objectID
return 'unknown'
}🤖 Prompt for AI Agents
In `@frontend/src/components/MultiSearch.tsx` around lines 247 - 253, The
getHitKey function can return the literal 'unknown' for multiple hits causing
duplicate React keys; update getHitKey to prefer 'objectID' first (Algolia
guarantees it), then fall back to 'key', 'login', 'url', and finally return a
unique fallback that includes the list index (or another unique per-item value)
so keys are always unique; change references to getHitKey where used in
rendering to pass the item index if needed so the function can append it as a
last-resort uniqueness suffix.
Shubb07
force-pushed
the
feature/search-debounce-cancellation
branch
from
ddf58d4 to
e190327
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/components/SearchValidation.test.tsx`:
- Around line 27-33: The test's defaultProps includes an obsolete prop
`eventData` which was removed from the component props interface; remove
`eventData` from the `defaultProps` object in
frontend/__tests__/unit/components/SearchValidation.test.tsx so the shape
matches MultiSearchBarProps used by the component (see MultiSearch.tsx props
definition around the interface and the `defaultProps` variable) to avoid the
TypeScript property-does-not-exist error.
a470d52 to
fb9a3c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/apps/slack/apps.py`:
- Line 23: The Slack app currently sets token_verification_enabled=False and
only guards for the literal string "None", which allows empty or placeholder
tokens to bypass startup verification; update the sanity check and decision
logic around token_verification_enabled in apps.py: change the guard that checks
the token value (currently comparing to "None") to also reject empty strings and
common placeholders (e.g., "", "none", "placeholder", "xoxb-REPLACE-ME") and
then set token_verification_enabled to True for non-placeholder tokens (or
revert to the default verification behavior), or alternatively move this
local-dev override into a dedicated dev config with clear comments so production
keeps verification enabled; reference the token_verification_enabled flag and
the existing token guard in apps.py when making the changes.
In `@frontend/src/components/MultiSearch.tsx`:
- Around line 240-245: The getHitName function can return undefined for
Organization hits that lack both name and login; update getHitName (used for
Chapter|Project|User|Event|Organization) to ensure it always returns a string by
adding a fallback like 'Unnamed' (or an appropriate placeholder) when neither
hit.name nor hit.login is present, and adjust the function signature/return type
if needed to reflect a non-optional string.
backend/apps/slack/apps.py
Outdated
| App( | ||
| signing_secret=settings.SLACK_SIGNING_SECRET, | ||
| token=settings.SLACK_BOT_TOKEN, | ||
| token_verification_enabled=False, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
slack-bolt token_verification_enabled parameter slack-sdk python
💡 Result:
In Bolt for Python (slack_bolt.App), token_verification_enabled is a boolean constructor parameter (default True) that controls whether Bolt will eagerly validate the provided token by calling Slack Web API auth.test during app initialization. (docs.slack.dev)
- When
True: Bolt callsclient.auth_test(token=self._token)to verify the token up front. (docs.slack.dev) - When
False: Bolt skips that startup token validation (useful to avoid a network call at boot, or when tokens aren’t available until later). (docs.slack.dev)
Example:
from slack_bolt import App
app = App(
token="xoxb-***",
signing_secret="***",
token_verification_enabled=False,
)Note: this is unrelated to Slack’s deprecated verification token mechanism (replaced by signing secrets for request verification). (api.slack.com)
Citations:
- 1: https://docs.slack.dev/tools/bolt-python/reference/?utm_source=openai
- 2: https://docs.slack.dev/tools/bolt-python/reference/app/app.html?utm_source=openai
- 3: https://docs.slack.dev/tools/bolt-python/reference/app/app.html?utm_source=openai
- 4: https://api.slack.com/docs/verifying-requests-from-slack?utm_source=openai
🏁 Script executed:
# First, let me examine the actual code file to see the context around line 23
cat -n backend/apps/slack/apps.py | head -40Repository: OWASP/Nest
Length of output: 1336
Confirm whether disabling token verification is intentional for local Docker support.
The token_verification_enabled=False parameter is a documented feature in slack-bolt 1.22.0 that skips the auth.test call at startup, allowing the app to initialize even with placeholder credentials. This is useful for local Docker environments where real Slack tokens aren't configured.
However, this creates a trade-off: invalid tokens won't be caught until runtime when API calls are made. The guard on line 25 only checks for the literal string "None", so dummy or invalid token values that aren't "None" will pass the guard and proceed without startup verification.
If this is intentional to support local development, clarify the decision and consider documenting it. Alternatively, widen the guard to check for empty strings or known placeholder values, so you can keep token verification enabled for production.
Additionally, this change appears unrelated to the PR's search debounce/cancellation objective — consider moving it to a separate PR for infrastructure/local development improvements.
🤖 Prompt for AI Agents
In `@backend/apps/slack/apps.py` at line 23, The Slack app currently sets
token_verification_enabled=False and only guards for the literal string "None",
which allows empty or placeholder tokens to bypass startup verification; update
the sanity check and decision logic around token_verification_enabled in
apps.py: change the guard that checks the token value (currently comparing to
"None") to also reject empty strings and common placeholders (e.g., "", "none",
"placeholder", "xoxb-REPLACE-ME") and then set token_verification_enabled to
True for non-placeholder tokens (or revert to the default verification
behavior), or alternatively move this local-dev override into a dedicated dev
config with clear comments so production keeps verification enabled; reference
the token_verification_enabled flag and the existing token guard in apps.py when
making the changes.
| const getHitName = (hit: Chapter | Project | User | Event | Organization) => { | ||
| if ('login' in hit) { | ||
| return hit.name || hit.login | ||
| } | ||
| return hit.name | ||
| } |
There was a problem hiding this comment.
getHitName may return undefined for organizations without name or login.
If an Organization hit has neither name nor login, getHitName returns undefined. This is an edge case — Algolia data should normally have one of these — but a fallback like 'Unnamed' would be safer.
🤖 Prompt for AI Agents
In `@frontend/src/components/MultiSearch.tsx` around lines 240 - 245, The
getHitName function can return undefined for Organization hits that lack both
name and login; update getHitName (used for
Chapter|Project|User|Event|Organization) to ensure it always returns a string by
adding a fallback like 'Unnamed' (or an appropriate placeholder) when neither
hit.name nor hit.login is present, and adjust the function signature/return type
if needed to reflect a non-optional string.
|
Hi @arkid15r , Closing this PR to clean up the changes and reduce unrelated diffs. |
Shubb07
force-pushed
the
feature/search-debounce-cancellation
branch
from
fb9a3c7 to
9479366
Compare
|
Hi @arkid15r , I have updated branch with debounce + AbortController cancellation + lint fixes. Ready for re-review. Kindly approve workflows. |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
I haven't checked if this works -- your code won't pass code quality checks and I don't see any tests added/updated.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/hooks/useSearchPage.ts">
<violation number="1" location="frontend/src/hooks/useSearchPage.ts:54">
P2: updateUrl skips clearing params like sortBy/order when their values reset to empty strings, leaving stale query parameters in the URL.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/hooks/useSearchPage.ts">
<violation number="1" location="frontend/src/hooks/useSearchPage.ts:59">
P2: `updateUrl` deletes any param whose value is `1`/`'1'`, so a valid search query of "1" is removed from the URL. This is a regression from the previous behavior which only treated `'1'` as empty for `page`, and it breaks URL/state consistency on refresh or navigation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Added debounced input handling in MultiSearch - Implemented AbortController-based request cancellation - Prevented stale responses from updating UI - Improved debounce handling inside useSearchPage - Fixed URL parameter cleanup logic - Updated MultiSearch component logic - Adjusted and fixed related unit tests - Resolved merge conflicts
- Added debounced input handling in MultiSearch - Implemented AbortController-based request cancellation - Prevented stale responses from updating UI - Improved debounce handling inside useSearchPage - Fixed URL parameter cleanup logic - Updated MultiSearch component logic - Adjusted and fixed related unit tests - Resolved merge conflicts test(search): fix CI failures related to AbortController changes fix(search): clear stale query params when resetting filters fix(useSearchPage): restrict page=1 cleanup to page param only (avoid removing q=1) Resolve merge conflict in MultiSearch.test.tsx
…llation - Implement debounced search input - Cancel overlapping requests using AbortController - Prevent stale responses from updating UI - Improve URL query parameter handling - Fix and update related unit and e2e tests
There was a problem hiding this comment.
8 issues found across 41 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/__tests__/unit/components/HealthMetrics.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/HealthMetrics.test.tsx:139">
P3: Test name claims fallback-to-zero behavior, but the assertion only checks a static title render, so regressions in fallback values would still pass.</violation>
</file>
<file name="frontend/jest.config.ts">
<violation number="1" location="frontend/jest.config.ts:29">
P2: Global branch coverage gate was reduced from 90 to 89, weakening CI quality enforcement across the frontend suite.</violation>
</file>
<file name="frontend/src/utils/dateFormatter.ts">
<violation number="1" location="frontend/src/utils/dateFormatter.ts:7">
P2: Date normalization was added only to `formatDate`, leaving `formatDateRange`/`formatDateForInput` to parse string dates directly, which can yield different day results for slash-formatted inputs. This introduces inconsistent behavior between helpers when given the same string format.</violation>
</file>
<file name="frontend/src/components/MultiSearch.tsx">
<violation number="1" location="frontend/src/components/MultiSearch.tsx:122">
P2: Keyboard highlight state can become stale across result updates/hidden dropdown, leading to out-of-bounds access or hidden-item selection on Enter.</violation>
</file>
<file name="frontend/src/hooks/useSearchPage.ts">
<violation number="1" location="frontend/src/hooks/useSearchPage.ts:56">
P2: `updateUrl` now deletes any param whose value is 1/'1', so a legitimate search query like `q=1` is removed from the URL. This breaks sharing/back-forward state for that query and is a regression from the previous `page`-only logic.</violation>
</file>
<file name="frontend/package.json">
<violation number="1" location="frontend/package.json:16">
P2: cross-env scripts use single-quoted NODE_OPTIONS, which is not portable on Windows cmd and can include the quotes in the variable value. Use double quotes (escaped in JSON) for cross-platform compatibility.</violation>
</file>
<file name="frontend/jest.setup.ts">
<violation number="1" location="frontend/jest.setup.ts:135">
P1: Global test setup now silences `console.error` for all tests, which can hide real runtime/test failures and reduce suite reliability.</violation>
</file>
<file name="frontend/__tests__/unit/pages/EditModule.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/EditModule.test.tsx:276">
P2: Test titled for missing optional fields no longer supplies null/absent values, so it no longer verifies null-handling behavior and can miss regressions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "lint:check": "eslint . --config eslint.config.mjs --max-warnings=0", | ||
| "start": "next start", | ||
| "test:a11y": "NODE_OPTIONS='--experimental-vm-modules --no-warnings=DEP0040' jest __tests__/a11y/ --coverage=false", | ||
| "test:a11y": "cross-env NODE_OPTIONS='--experimental-vm-modules --no-warnings=DEP0040' jest __tests__/a11y/ --coverage=false --forceExit --maxWorkers=2", |
There was a problem hiding this comment.
P2: cross-env scripts use single-quoted NODE_OPTIONS, which is not portable on Windows cmd and can include the quotes in the variable value. Use double quotes (escaped in JSON) for cross-platform compatibility.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/package.json, line 16:
<comment>cross-env scripts use single-quoted NODE_OPTIONS, which is not portable on Windows cmd and can include the quotes in the variable value. Use double quotes (escaped in JSON) for cross-platform compatibility.</comment>
<file context>
@@ -13,9 +13,9 @@
"lint:check": "eslint . --config eslint.config.mjs --max-warnings=0",
"start": "next start",
- "test:a11y": "NODE_OPTIONS='--experimental-vm-modules --no-warnings=DEP0040' jest __tests__/a11y/ --coverage=false",
+ "test:a11y": "cross-env NODE_OPTIONS='--experimental-vm-modules --no-warnings=DEP0040' jest __tests__/a11y/ --coverage=false --forceExit --maxWorkers=2",
"test:e2e": "npx playwright test",
- "test:unit": "tsc --noEmit && NODE_OPTIONS='--experimental-vm-modules --no-warnings=DEP0040' jest"
</file context>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/__tests__/a11y/pages/ApiKeysPage.a11y.test.tsx`:
- Around line 82-86: Replace the generic role lookup for the revoke button with
a named query to make the test explicit and robust: in the test where you use
within(row).getByRole('button') to get revokeButton, instead query
within(row).getByRole('button', { name: /Revoke API Key/i }) (or equivalent
case-insensitive exact string) so the test targets the button by its aria-label
"Revoke API Key" and remains stable if other buttons are added; then continue to
fireEvent.click(revokeButton) inside the existing act wrapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ebe3de4-97d3-4e65-af00-dcf807b14cd0
📒 Files selected for processing (2)
frontend/__tests__/a11y/pages/ApiKeysPage.a11y.test.tsxfrontend/__tests__/e2e/pages/About.spec.ts
…, and security scan dependencies
There was a problem hiding this comment.
3 issues found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/__tests__/unit/components/MultiSearch.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/MultiSearch.test.tsx:542">
P2: Tests bypass real debounce timing and do not validate the PR’s core request-throttling/cancellation behavior, leaving the regression unprotected.</violation>
<violation number="2" location="frontend/__tests__/unit/components/MultiSearch.test.tsx:545">
P2: Keyboard-navigation test creates duplicate hit keys, while the component uses hit key as React list key, making the test data unreliable.</violation>
</file>
<file name="frontend/package.json">
<violation number="1" location="frontend/package.json:130">
P2: This PR mixes unrelated bulk dependency override changes into a search behavior fix, increasing maintenance and regression risk.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| it('navigates up through suggestions using ArrowUp', async () => { | ||
| mockFetchAlgoliaData.mockResolvedValue({ | ||
| hits: [mockChapter, mockUser], | ||
| hits: [mockChapter, { ...mockChapter, name: 'Another Chapter', objectID: 'chap2' }], |
There was a problem hiding this comment.
P2: Keyboard-navigation test creates duplicate hit keys, while the component uses hit key as React list key, making the test data unreliable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/__tests__/unit/components/MultiSearch.test.tsx, line 545:
<comment>Keyboard-navigation test creates duplicate hit keys, while the component uses hit key as React list key, making the test data unreliable.</comment>
<file context>
@@ -538,4 +538,103 @@ describe('Rendering', () => {
+ describe('Keyboard Navigation', () => {
+ it('navigates up through suggestions using ArrowUp', async () => {
+ mockFetchAlgoliaData.mockResolvedValue({
+ hits: [mockChapter, { ...mockChapter, name: 'Another Chapter', objectID: 'chap2' }],
+ totalPages: 1,
+ })
</file context>
| hits: [mockChapter, { ...mockChapter, name: 'Another Chapter', objectID: 'chap2' }], | |
| hits: [mockChapter, { ...mockChapter, key: 'another-chapter', name: 'Another Chapter', objectID: 'chap2' }], |
| }) | ||
| }) | ||
|
|
||
| describe('Keyboard Navigation', () => { |
There was a problem hiding this comment.
P2: Tests bypass real debounce timing and do not validate the PR’s core request-throttling/cancellation behavior, leaving the regression unprotected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/__tests__/unit/components/MultiSearch.test.tsx, line 542:
<comment>Tests bypass real debounce timing and do not validate the PR’s core request-throttling/cancellation behavior, leaving the regression unprotected.</comment>
<file context>
@@ -538,4 +538,103 @@ describe('Rendering', () => {
})
})
+
+ describe('Keyboard Navigation', () => {
+ it('navigates up through suggestions using ArrowUp', async () => {
+ mockFetchAlgoliaData.mockResolvedValue({
</file context>
| @@ -13,9 +13,9 @@ | |||
| "lint": "eslint . --config eslint.config.mjs --fix --max-warnings=0", | |||
There was a problem hiding this comment.
P2: This PR mixes unrelated bulk dependency override changes into a search behavior fix, increasing maintenance and regression risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/package.json, line 130:
<comment>This PR mixes unrelated bulk dependency override changes into a search behavior fix, increasing maintenance and regression risk.</comment>
<file context>
@@ -126,7 +126,22 @@
"glob": "^11.1.0",
- "test-exclude": "^7.0.1"
+ "test-exclude": "^7.0.1",
+ "minimatch@<3.1.4": "^3.1.4",
+ "minimatch@>=4.0.0 <4.2.5": "^4.2.5",
+ "minimatch@>=5.0.0 <5.1.8": "^5.1.8",
</file context>
|
2 participants
Proposed change
Resolves #3439
Typing in the search bar was triggering a request on every keystroke, which caused
multiple parallel calls and sometimes stale results showing in the UI.
This change adds a small debounce and cancels previous requests so only the latest
query runs.
##Changes:
##Checklist
make check-testlocally: all warnings addressed, tests passed