refactor: query handling#402
Merged
Merged
Conversation
yannickmonney
force-pushed
the
feat/refactor-query-handling
branch
from
c7888fc to
c341920
Compare
📝 WalkthroughWalkthroughThis PR introduces performance optimizations and refactors across the platform. The frontend changes add memoization (useMemo/useCallback) to React components and hooks to reduce unnecessary re-renders, improve key stability in list renderings, and optimize callback references. Backend changes include consolidating query construction into helper functions that determine which database indexes to use based on provided filters, enabling more selective index selection. The pagination system removes deprecated offset-based pagination in favor of cursor-based pagination. Day.js locale loading is refactored from eager imports to lazy on-demand loading. RAG service document processing is made concurrent using asyncio.gather, and connection handling is centralized. Various utilities are consolidated (formatBytes, import locations). Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
services/platform/app/features/approvals/components/approvals-client.tsx (1)
425-438: 🧹 Nitpick | 🔵 TrivialGood improvement — stable keys from product identifiers.
Minor caveat: if two products in the same list share the same
productId(or both lack it and share the sameid), you'll get duplicate React keys. TheString(index)fallback only kicks in when both fields are empty. Consider appending the index as a disambiguator:Suggested tweak
- const id = - safeGetString(p, 'productId', '') || - safeGetString(p, 'id', '') || - String(index); + const id = + safeGetString(p, 'productId', '') || + safeGetString(p, 'id', '') || + String(index); + const key = `${id}-${index}`;Then use
key={key}on Line 438.services/platform/convex/threads/list_threads.ts (1)
23-30:⚠️ Potential issue | 🟡 MinorHardcoded page size silently drops threads beyond 100.
If a user accumulates more than 100 threads, only the most recent 100 are ever considered. This is likely pre-existing, but worth noting: there's no follow-up cursor fetch, so threads beyond the first page are invisible. Consider either documenting this as intentional or paginating until exhaustion if completeness matters.
services/platform/app/features/settings/integrations/components/integrations-client.tsx (2)
160-169: 🛠️ Refactor suggestion | 🟠 MajorInline
onClickduplicates and shadowshandleManageClickfor most integration IDs.The inline handler on the manage
Button(Lines 392-401) explicitly handles'circuly','shopify', and'protel', falling through tohandleManageClickonly in theelsebranch. SincehandleManageClickalso handles'protel'(Line 162), that case is dead code — it can never be reached from the current call site.Consider consolidating: either extend
handleManageClickto cover all IDs and use it directly as theonClick, or remove the unreachable'protel'case fromhandleManageClick.♻️ Option: consolidate into handleManageClick
const handleManageClick = useCallback((integrationId: string) => { switch (integrationId) { + case 'shopify': + setShopifyDialogOpen(true); + break; + case 'circuly': + setCirculyDialogOpen(true); + break; case 'protel': setProtelDialogOpen(true); break; case 'email': setEmailDialogOpen(true); break; } }, []);Then simplify the button:
<Button variant="link" size="sm" - onClick={() => { - if (integration.id === 'circuly') { - setCirculyDialogOpen(true); - } else if (integration.id === 'shopify') { - setShopifyDialogOpen(true); - } else if (integration.id === 'protel') { - setProtelDialogOpen(true); - } else { - handleManageClick(integration.id); - } - }} + onClick={() => handleManageClick(integration.id)} className="flex items-center gap-1 text-sm h-6" >Also applies to: 389-401
171-214:⚠️ Potential issue | 🟠 MajorShopify create path may leave an orphaned integration on
testConnectionfailure.If
createIntegrationsucceeds (Line 207) buttestConnectionfails (Lines 211-212), the thrown error propagates to the caller, but the newly created integration record is left behind with no cleanup or rollback. The update path mitigates this somewhat by markingstatus: 'testing', isActive: false(Lines 190-191), but the create path has no such safeguard — the record's default status is whatevercreateIntegrationassigns.Consider either:
- Setting an explicit
status: 'testing', isActive: falseon the create payload so the integration isn't treated as live if the test fails, or- Deleting the integration in a catch block if
testConnectionfails after creation.services/rag/app/services/cognee/service.py (1)
919-919: 🧹 Nitpick | 🔵 TrivialDead conditional —
datasetsis always truthy here.Line 891-893 already returns early when
not datasets, so theif datasets:guard on Line 919 is always true. This is a leftover from the removed global-search branch.Suggested cleanup
- if datasets: - async def search_dataset(ds: str) -> tuple[str, list[Any] | Exception]: + async def search_dataset(ds: str) -> tuple[str, list[Any] | Exception]:(and de-indent the entire block one level)
services/platform/app/features/conversations/components/conversations-list.tsx (1)
50-77: 🛠️ Refactor suggestion | 🟠 MajorDuplicated HTML sanitization logic in
getLastMessagePreview.The content-cleaning pipeline (strip
<style>/<script>, insert spaces at block boundaries,striptags,decode, whitespace collapse) is copy-pasted in two branches (lines 51-77 and 81-107). Extract a helper to eliminate duplication.Proposed fix
+const sanitizeMessageContent = (content: string): string => { + content = content.replace(/<style[^>]*>[\s\S]*?<\/style>/gi, ''); + content = content.replace(/<script[^>]*>[\s\S]*?<\/script>/gi, ''); + content = content + .replace(/<br\s*\/?>(?=\S)/gi, ' ') + .replace( + /<\/(p|div|li|h[1-6]|section|article|header|footer|tr|td|th)>/gi, + ' ', + ); + content = striptags(content).trim(); + content = decode(content); + return content.replace(/\s+/g, ' ').trim(); +}; + const getLastMessagePreview = (conversation: Conversation): string => { // ... (find displayable message logic) ... - // duplicated sanitization + return sanitizeMessageContent(displayableMessage.content); // ... };Also applies to: 80-107
services/platform/convex/audit_logs/helpers.ts (1)
358-399:⚠️ Potential issue | 🔴 CriticalMissing in-memory fallback filters for
actorId,category, andresourceType.When
buildAuditLogQueryselects an index based on one filter (e.g.,category + startDate), other filter fields likeactorIdorresourceTypeare silently ignored because there are no corresponding in-loop checks. For example, if a caller passes{ category: 'auth', startDate: 123, actorId: 'user1' }, the index handlescategoryandstartDate, butactorIdis never checked—returning logs from all actors.The other refactored modules (e.g.,
query_conversations.ts,query_products.ts) use explicitneedsXFilter = !('x' in indexedFields) && args.x !== undefinedguards to apply in-memory filters for non-indexed fields. This file should do the same.🐛 Proposed fix — add missing in-loop filters
for await (const log of query.order('desc')) { if (!foundCursor) { if (String(log._id) === startCursor) { foundCursor = true; } continue; } if (filter.endDate && log.timestamp > filter.endDate) { continue; } if (!startDateHandledByIndex && filter.startDate && log.timestamp < filter.startDate) { break; } + if (!('actorId' in indexedFields) && filter.actorId && log.actorId !== filter.actorId) { + continue; + } + + if (!('category' in indexedFields) && filter.category && log.category !== filter.category) { + continue; + } + + if (!('resourceType' in indexedFields) && filter.resourceType && log.resourceType !== filter.resourceType) { + continue; + } + if (filter.status && log.status !== filter.status) { continue; }services/platform/convex/products/get_products.ts (1)
231-242: 🧹 Nitpick | 🔵 Trivial
'category' in indexedFieldscheck is always false here — consider simplifying.
buildIterationQueryat line 231 is called with{ organizationId, status }(nocategory), soindexedFieldswill never contain'category'. The guard on line 238 therefore always evaluates its first condition totrue, making it dead logic. This is harmless but somewhat misleading — a reader might think the category index could be in play here.Suggested simplification
- if (!('category' in indexedFields) && category !== undefined && product.category !== category) continue; + if (category !== undefined && product.category !== category) continue;
🤖 Fix all issues with AI agents
In `@services/platform/app/features/automations/components/automation-edge.tsx`:
- Around line 75-98: The memo currently depends on data?.labelOffset object
which can be recreated each render and defeat useMemo; update the dependency
array in the useMemo that computes labelX/labelY to depend on the primitive
fields data?.labelOffset?.x and data?.labelOffset?.y (and keep
data?.labelPosition and data?.isBackwardConnection) and ensure you reference
those same primitives when reading the offset (data?.labelOffset?.x / ?.y) in
the computation so labelX/labelY only recompute when actual offset values
change.
In
`@services/platform/app/features/automations/components/automation-sidepanel.tsx`:
- Around line 386-387: The list item keys in the errors map (errors.map(...)
with <li key={error}>) risk collisions because error messages may repeat; update
the key to a composite unique value such as `${error}-${index}` or
`${type}-${index}` by using the map callback index (e.g., errors.map((error,
index) => ...)) and apply the composite key on the <li> to eliminate React key
duplication (also apply the same change for the similar mapping at the other
occurrence around lines 400-401).
In `@services/platform/app/features/automations/components/automation-tester.tsx`:
- Around line 208-209: The list rendering in AutomationTester
(automation-tester.tsx) uses the error/warning string itself as the React key in
dryRunResult.errors.map and dryRunResult.warnings.map which can create duplicate
keys; change the mapping to use a stable unique key (e.g., include the array
index or combine the message and index like `${err}-${i}`) or derive a unique id
before rendering so each <li> has a guaranteed-unique key instead of the raw
message.
In `@services/platform/app/features/chat/components/chat-search-dialog.tsx`:
- Around line 85-105: The handleKeyDown callback is being re-created on every
ArrowUp/ArrowDown press because selectedIndex is in its dependency array; make
selectedIndex stable by moving its current value into a ref and reading that ref
for the Enter branch. Create a selectedIndexRef that you update whenever
setSelectedIndex runs (or via useEffect syncing selectedIndex ->
selectedIndexRef), remove selectedIndex from handleKeyDown's dependency array,
and use selectedIndexRef.current when building params for navigate({ to:
'/dashboard/$id/chat/$threadId', params: { id: organizationId, threadId:
chats[selectedIndexRef.current]._id } }) while keeping setSelectedIndex((i) =>
...) for arrow keys and retaining navigate, chats, organizationId, and
onOpenChange in the dependency array.
In
`@services/platform/app/features/conversations/components/conversations-list.tsx`:
- Around line 229-238: Replace the clickable <div role="button"> with a semantic
<button> element in the conversations list item: remove the manual keyboard
handling (drop onKeyDown/handleKeyDown) and keep onClick={handleClick}, tabIndex
and ARIA can be simplified (retain aria-pressed if needed) and preserve
className using cn and isSelected; update the component rendering that currently
uses the div wrapper (where cn(..., isSelected && 'bg-muted') is applied) to
render a <button> so native button behavior replaces the custom keyboard logic.
- Around line 335-347: The ConversationRow memo is being defeated because t and
tDialogs are unstable references each render; stabilize them by memoizing the
translation values before passing to ConversationRow (e.g., capture useT results
and wrap in useMemo to produce stable t and tDialogs references) or move
translation lookups into a stable context/hook so ConversationRow receives
stable props; ensure you keep formatDateSmart as-is (already useCallback) and
update the conversations.map call to pass the memoized t and tDialogs to
ConversationRow.
In
`@services/platform/app/features/settings/integrations/components/integrations-client.tsx`:
- Line 303: Replace the hardcoded English error thrown at the throw new
Error('Organization ID is required') site with the localized string used
elsewhere: throw new Error(t('integrations.errors.organizationRequired')).
Ensure the same i18n translator (t) used in the other handlers (the calls at
lines where t('integrations.errors.organizationRequired') is already used) is in
scope for this module so the replacement compiles and provides the localized
message.
- Around line 306-315: The Protel update branch drops sqlConnectionConfig
changes — when protelIntegration exists the updateIntegration call only sends
basicAuth, status and isActive; modify that call to include sqlConnectionConfig
populated from the same form values used in the create path (e.g., data.server,
data.port, data.database, etc.) so edits to server/port/database are persisted;
locate the updateIntegration invocation near the protelIntegration check and
mirror the sqlConnectionConfig shape used in the createIntegration/create path
to ensure parity.
In `@services/platform/app/hooks/use-copy.ts`:
- Line 112: The dependency array for the memoized/stable copy function still
includes onSuccess and onError which defeats the stabilization; change onSuccess
and onError to refs (like toast and tCommon) or wrap them with useEffectEvent so
copy no longer depends on their identity, then replace direct usages of
onSuccess/onError inside the copy function with theRef.current (or the
useEffectEvent handler) to keep copy referentially stable; update the dependency
array [copiedDuration, showErrorToast] accordingly and/or document callers must
memoize their callbacks.
In `@services/platform/app/hooks/use-locale.ts`:
- Around line 14-17: The current fallback builds `${base}-US` for any
browserLocale which yields odd results for non-English languages; update the
logic in the useLocale hook so you only construct and validate a `${base}-US`
fallback when the base language is English (base === 'en'); otherwise skip that
intermediate fallback and return the configured defaultLocale; adjust the branch
around browserLocale, isValidLocale and defaultLocale accordingly to ensure
non-English bases fall through to defaultLocale.
- Around line 26-30: The hook currently calls loadDayjsLocale(detected) without
awaiting it so Day.js locale may not be ready on first render; change the
initialization to not fire-and-forget but instead load the locale in an effect
and update state after the import resolves. Concretely, remove the async
side-effect from the useState initializer (keep detectLocale but don't call
loadDayjsLocale there), then add a useEffect that calls
loadDayjsLocale(detected).then(() => setLocaleState(detected)) (or await inside
an async IIFE) so that setLocaleState is invoked after loadDayjsLocale
completes; reference functions/identifiers: detectLocale, loadDayjsLocale,
useState, setLocaleState, and applyLocale (format.ts) to ensure re-render occurs
when Day.js locale is ready.
In `@services/platform/app/hooks/use-resize-observer.ts`:
- Around line 32-33: The current pattern writes to callbackRef.current during
render (callbackRef and its assignment in useResizeObserver), which is a
render-side effect; replace the manual ref plumbing by using React's
useEffectEvent (when available) to create a stable event callback that always
reads the latest closure instead of updating callbackRef during render—i.e.,
remove the callbackRef assignment and wrap the incoming callback with
useEffectEvent inside useResizeObserver so the returned/stable function can be
used where the observer expects a stable handler.
In `@services/platform/app/hooks/use-sync-status.ts`:
- Around line 38-51: The useMemo in the useSyncStatus hook is ineffective
because getSyncState() (from sync-manager.ts) returns a new object every update,
so state reference changes always; fix by removing the useMemo and return the
derived object directly from useSyncExternalStore (i.e., stop memoizing in
use-sync-status.ts), or alternatively change getSyncState()/updateSyncState() in
sync-manager.ts to preserve referential stability (memoize snapshots or avoid
creating a new object when values haven’t changed) so the state dependency in
useSyncStatus remains stable; update either useSyncStatus (remove useMemo) or
sync-manager (make getSyncState stable) and ensure symbols referenced are
useSyncExternalStore, getSyncState, updateSyncState, and useSyncStatus.
In `@services/platform/convex/conversations/query_conversation_messages.ts`:
- Line 33: Remove the redundant TypeScript assertion on the order call: change
the .order('asc' as const) invocation to .order('asc') inside the query builder
(look for the .order(...) call in the query_conversation_messages code path,
e.g., in the function that builds the conversation message query) so the literal
type is used directly without the unnecessary as const.
In `@services/platform/convex/customers/query_customers.ts`:
- Around line 27-95: The buildQuery function currently chooses among multiple
compound indexes; change it to always use the simple by_organizationId index
(keep ctx.db.query('customers').withIndex('by_organizationId', q =>
q.eq('organizationId', organizationId)).order('desc')) and remove the
special-case returns that use by_organizationId_and_externalId,
by_organizationId_and_status, by_organizationId_and_source, and
by_organizationId_and_locale; ensure indexedFields is set to {} as const and
implement the additional filtering downstream using the existing in-memory
filter sets (statusSet, sourceSet, localeSet, needsExternalIdFilter) so the
codebase follows the established pattern of single-index queries with in-memory
filtering.
In `@services/platform/convex/sso_providers/entra_id/team_sync.ts`:
- Around line 163-167: The loop over userMembershipsResult.page silently
continues when teamById lacks membership.teamId; instead log a debug-level
message before continuing so operators can distinguish cross-org memberships
from missing/fetched-team pagination gaps. In the block currently using "if
(!team) continue;" (iterating membership in userMembershipsResult.page), emit a
debug log via the module's logger (e.g., logger or processLogger) including
membership.teamId, membership.userId (or membership.user identifier), any
available page/cursor info from userMembershipsResult, and a short context
string like "missing team in teamById" before executing the continue. This keeps
the filter behavior but surfaces missing-team cases for investigation.
In
`@services/platform/convex/workflow_engine/helpers/scheduler/get_last_execution_time.ts`:
- Around line 37-50: Replace the duplicated query inside the Promise.all with a
call to the existing helper getLastExecutionTime: import getLastExecutionTime
into this module and inside args.wfDefinitionIds.map(...) call
getLastExecutionTime(ctx, wfDefinitionId) (or the correct signature) to obtain
the last startedAt, then return [wfDefinitionId, last] as before; this removes
the duplicated .query('wfExecutions') logic and centralizes the lookup in
getLastExecutionTime.
In
`@services/platform/convex/workflow_engine/helpers/scheduler/get_scheduled_workflows.ts`:
- Line 55: The declaration const firstStepMap = new Map() is untyped; add
explicit type parameters to firstStepMap to improve readability and type safety
(e.g., const firstStepMap: Map<string, ScheduledWorkflowStep> = new Map() or
whatever key/value types match usages in get_scheduled_workflows.ts), updating
the Map's key and value types to the concrete types used elsewhere in the file
so callers and iterations are strongly typed.
In `@services/platform/lib/utils/date/dayjs-setup.ts`:
- Around line 74-76: In loadDayjsLocale, the locale normalization uses
locale.toLowerCase().replace('_', '-') which only replaces the first underscore;
change the replacement to cover all underscores (e.g., use replaceAll('_', '-')
or locale.replace(/_/g, '-') ) so the computed key variable converts every
underscore to a hyphen before checking loadedLocales; update any tests/usage
assuming the single-replace behavior accordingly.
- Around line 85-98: The code incorrectly marks the base locale as loaded when
importing a regional locale: inside the promise.then block (references:
loadedLocales, key, base, importFn, pendingLoads, loadDayjsLocale) remove the
unconditional loadedLocales.add(base) so importing 'de-at' does not claim 'de'
is present; if the intended behavior is that a regional import should satisfy a
base-locale request, explicitly import/register the base locale as well (e.g.
call the base import via importFn or a separate import) rather than only marking
it as loaded.
In `@services/rag/app/services/cognee/cleanup.py`:
- Line 22: The _pg_connection async generator is annotated as
AsyncGenerator[object, None], losing type info; update it to
AsyncGenerator[asyncpg.Connection, None] by adding a typing.TYPE_CHECKING guard:
import TYPE_CHECKING from typing, inside if TYPE_CHECKING: import asyncpg, then
change the return annotation on _pg_connection to
AsyncGenerator[asyncpg.Connection, None] so callers get precise connection types
without a runtime import.
- Around line 35-41: The asyncpg.connect call in cleanup.py (the invocation
creating conn using parsed.hostname/port/path/username/password) can hang
indefinitely; add a timeout parameter (e.g., timeout=10 or read from config/ENV)
to the asyncpg.connect(...) call so connection attempts fail fast instead of
waiting for OS TCP timeouts; ensure the chosen timeout is applied to the same
asyncpg.connect invocation that returns conn and document/propagate the
configured value if used elsewhere.
In `@services/rag/app/services/cognee/service.py`:
- Around line 785-791: The loop currently overwrites result with each ds_result
from dataset_results, losing earlier results; change the assignment so result is
preserved (e.g., only set result if result is None or collect all ds_result
values) and ensure normalize_add_result(document_id, ...) uses the intended
value; update the code around the dataset_results loop (variables: ds_name,
ds_result, ds_chunks, result, total_chunks_created, added_datasets) and add a
brief comment explaining why you preserve the first/aggregate ds_result so
future readers know document_id handling is intentional.
- Around line 780-783: Change the asyncio.gather call that runs _process_dataset
to use return_exceptions=True so all dataset tasks run to completion and you can
handle per-task errors instead of propagating the first exception; after
awaiting dataset_results iterate over each result, treat asyncio exceptions (or
Exception instances) as failures (log them and collect failure info) and collect
successful results into the aggregated response so partial successes are
preserved; ensure vision_temp_file is only deleted after gather completes and
after processing all dataset_results (so no background _process_dataset can
still be using the file while the finally block removes it) and reference
_process_dataset, dataset_results, vision_temp_file and cognee.add when making
these changes.
This was referenced Feb 10, 2026
This was referenced Feb 17, 2026
yannickmonney
added a commit
that referenced
this pull request
Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Performance Improvements
New Features
Bug Fixes